refactor(utils): shared FE-dummy design build across DiD/MPD/TWFE

[igerber]

Summary

• Closes the TWFE dummy-build dedup TODO row: the drop-first pd.get_dummies design construction existed as three inline copies — the DifferenceInDifferences and MultiPeriodDiD fixed_effects= loops and the TwoWayFixedEffects HC2/HC2-BM full-dummy path — whose FE naming / dtype / column-order conventions could drift independently. All three now delegate to one diff_diff.utils.build_fe_dummy_blocks helper whose names match fe_dummy_names (the reserved-name collision guard) by construction. The row offered a bigger alternative (delegate TWFE's path to DiD's branch entirely); the shared-helper option was chosen for the smaller blast radius.
• Bit-identical: A/B against a pristine origin/main worktree (backend pinned) on DiD with a non-default-order Categorical FE + covariates, TWFE hc2 + hc2_bm, and MultiPeriodDiD multi-FE including the fe == time skip — every ATT/SE/coefficient-dict/vcov-sum identical. Side benefit: the DiD/MPD paths drop the per-column np.column_stack accumulation (O(k²) copies) for one block stack.
• Contract tests lock names==fe_dummy_names (plain / Categorical-with-non-default-order / numeric FEs) and values==get_dummies with float64 dtype. Impacted suites green (416: estimators, vcov-type, utils).

Methodology references (required if estimator / math changes)

• Method name(s): N/A — design-construction refactor, bit-identical outputs
• Paper / source link(s): N/A
• Any intentional deviations from the source (and why): None

Validation

• Tests added/updated: tests/test_utils.py::TestBuildFeDummyBlocks (2 contract tests).
• Backtest / simulation / notebook evidence (if applicable): A/B bit-identity vs origin/main (described above).

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

PR Review

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected methods: DifferenceInDifferences(fixed_effects=...), MultiPeriodDiD(fixed_effects=...), and TwoWayFixedEffects(vcov_type in {"hc2", "hc2_bm"}).
• Methodology registry alignment looks intact: the refactor preserves the documented full-dummy FE design and pd.get_dummies(..., drop_first=True) naming contract.
• No inference, weighting, variance/SE, identification, or default behavior defect found.
• One minor code-quality hardening item: validate prefixes length in the new helper.
• I could not run tests locally because this sandbox lacks pytest and numpy.

Methodology

No findings.

The changed paths still build the documented full-dummy FE design and preserve the registry’s dummy-name collision contract: diff_diff/utils.py:L185-L217, diff_diff/utils.py:L220-L258, diff_diff/estimators.py:L526-L532, diff_diff/estimators.py:L1764-L1769, diff_diff/twfe.py:L355-L374.

Code Quality

• Severity: P2
• Impact: build_fe_dummy_blocks() uses zip(fe_cols, prefixes or fe_cols), so a future caller passing a shorter non-empty prefixes list would silently omit trailing fixed-effect columns. Current PR callers pass either None or a matching two-prefix list, so I do not see a current estimator correctness issue.
• Concrete fix: add if prefixes is not None and len(prefixes) != len(fe_cols): raise ValueError(...) before the loop, plus a small utility test. Location: diff_diff/utils.py:L220-L258.

Performance

No findings.

The DiD/MPD change removes repeated per-column np.column_stack growth on the refactored path.

Maintainability

No findings beyond the P2 helper validation item above.

Tech Debt

No findings.

The removed TODO row corresponds to the refactor implemented here: TODO.md:L29-L34.

Security

No findings.

No secrets, file IO, network calls, or unsafe execution surfaces are introduced.

Documentation/Tests

No blocking findings.

The new utility tests cover name parity with fe_dummy_names() and value parity with pd.get_dummies(): tests/test_utils.py:L2084-L2118. I was unable to execute them in this environment because pytest and numpy are not installed.

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 30e8b741eb4fb899dfd8f3fa3940fccb5e0ea826

---

PR Review

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected methods: DifferenceInDifferences(fixed_effects=...), MultiPeriodDiD(fixed_effects=...), and TwoWayFixedEffects(vcov_type in {"hc2", "hc2_bm"}).
• Methodology alignment looks intact: the refactor preserves the documented full-dummy/drop-first FE design and the fe_dummy_names() collision-guard contract.
• No changes to weighting, variance/SE formulas, identification assumptions, inference computation, or defaults were found.
• Prior P2 helper-hardening item remains: prefixes length is not validated in build_fe_dummy_blocks().
• I could not run the focused tests because pytest is not installed in this environment.

Methodology

No findings.

The registry requires fixed-effect dummy names to match pd.get_dummies(..., drop_first=True) exactly for the coefficient collision guard, including categorical ordering: docs/methodology/REGISTRY.md:L82. The new helper implements that same call and dtype materialization: diff_diff/utils.py:L220-L259, and the changed estimator paths consume it without changing the design contract: diff_diff/estimators.py:L531-L538, diff_diff/estimators.py:L1815-L1820, diff_diff/twfe.py:L352-L370.

The HC2/HC2-BM registry notes require full-dummy FE designs because FWL does not preserve leverage/hat-matrix quantities; the TWFE and DiD/MPD full-dummy routing remains in place: docs/methodology/REGISTRY.md:L3402-L3405. MPD’s documented redundant time-FE skip is also preserved: diff_diff/estimators.py:L1803-L1820.

Code Quality

• Severity: P2
• Impact: build_fe_dummy_blocks() still uses zip(fe_cols, prefixes or fe_cols), so a future caller passing a shorter non-empty prefixes list would silently skip trailing FE columns. Current PR call sites are safe: DiD/MPD pass no prefixes, and TWFE passes exactly two prefixes for [unit, time].
• Concrete fix: add if prefixes is not None and len(prefixes) != len(fe_cols): raise ValueError(...) before the loop, plus a utility test for mismatched lengths. Location: diff_diff/utils.py:L220-L259.

Performance

No findings.

The DiD/MPD paths replace repeated per-dummy np.column_stack growth with one block stack, which is a reasonable performance improvement without changing the design matrix semantics.

Maintainability

No findings beyond the P2 helper validation item above.

The shared helper reduces drift risk across three formerly duplicated dummy-build paths.

Tech Debt

No findings.

The removed TODO row corresponds to the implemented shared-helper refactor; the remaining actionable methodology rows are unrelated: TODO.md:L29-L33.

Security

No findings.

The diff does not introduce file IO, network access, shell execution, credential handling, or secret-like content.

Documentation/Tests

No blocking findings.

The added tests cover helper-name parity with fe_dummy_names() and value parity with pd.get_dummies(..., drop_first=True): tests/test_utils.py:L2084-L2118. Verification note: PYTHONDONTWRITEBYTECODE=1 pytest -q tests/test_utils.py::TestBuildFeDummyBlocks -p no:cacheprovider could not run because pytest is not installed.