Skip to content

fix(survey): unify explicit-vcov_type contract under replicate designs (warn + hc1 remap)#654

Open
igerber wants to merge 1 commit into
mainfrom
fix/replicate-vcov-type-warn
Open

fix(survey): unify explicit-vcov_type contract under replicate designs (warn + hc1 remap)#654
igerber wants to merge 1 commit into
mainfrom
fix/replicate-vcov-type-warn

Conversation

@igerber

@igerber igerber commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • Resolves the TWFE replicate-hc2 Heavy TODO row by investigation rather than implementation: with uses_replicate_variance the analytical sandwich is replaced wholesale — the per-replicate refits return point estimates only, which are identical across vcov families (FWL: the full-dummy and within-transformed fits give the same coefficients) — so the proposed per-replicate full-dummy HC2 refit is a costly no-op that cannot change the replicate variance. Verified empirically before changing anything: DiD hc2+replicate already produced bit-identical output to hc1+replicate, silently.
  • The twins were inconsistent: TwoWayFixedEffects(vcov_type="hc2"/"hc2_bm") raised NotImplementedError while DifferenceInDifferences silently ignored the kwarg. All three estimators (incl. MultiPeriodDiD) now share one contract via a base-class helper: explicit non-hc1 analytical vcov_type (hc2/hc2_bm/classical) warns ("has no effect with replicate-weight survey designs…") and the discarded base fit remaps to hc1 — avoiding wasted CR2-BM work, one-way-only validator rejections, and the TWFE full-dummy auto-route (which does not compose with per-replicate re-demeaning). Explicit hc1 — the old error message's own workaround guidance — stays silent. conley is excluded: it carries its own survey-design support contract (TSL stratified-Conley sandwich; dedicated validators), which keeps firing unchanged (local-review P1, locked by a contract test).
  • Tests lock warn + bit-identity-to-hc1 on all three estimators plus the silent-default, silent-explicit-hc1, and conley-contract cases; the old TWFE raise test is rewritten to the new contract. REGISTRY survey-section Note documents the decision and the rejected alternative.

Methodology references (required if estimator / math changes)

  • Method name(s): Replicate-weight variance (Wolter 2007; BRR/Fay/JK1/JKn/SDR) — no variance formula changes; user-contract unification only (numbers under every affected request are bit-identical to the hc1 request)
  • Paper / source link(s): REGISTRY.md § Survey Data Support (new Note: "vcov_type has no effect under replicate variance")
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_replicate_weight_expansion.py::TestReplicateVcovTypeWarn (4 tests), tests/test_estimators_vcov_type.py (raise test → warn+identical, both vcov params). Impacted suites green: 522 tests (replicate expansion, vcov-type, estimators, survey, survey_phase6).
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Overall Assessment

⚠️ Needs changes — one unmitigated P1.

Executive Summary

  • The new registry note documents the intended replicate-weight contract, so the warn/remap approach is methodologically acceptable in principle.
  • TwoWayFixedEffects applies the remap before its full-dummy route, but DifferenceInDifferences and MultiPeriodDiD still branch on the original self.vcov_type for absorb -> fixed_effects.
  • That means explicit hc2/hc2_bm can still affect absorbed-FE replicate fits, despite the warning saying it has no effect.
  • REGISTRY.md now contains contradictory TWFE replicate-weight guidance.
  • I could not run tests: python -m pytest ... failed because pytest is not installed.

Methodology

Finding 1 — P1

Location: diff_diff/estimators.py:L356-L357, diff_diff/estimators.py:L415-L419, diff_diff/estimators.py:L580-L584, diff_diff/estimators.py:L1595-L1597, diff_diff/estimators.py:L1653-L1656, diff_diff/estimators.py:L1851-L1855

Impact: The new helper says explicit non-hc1 vcov_type “cannot influence any reported number” under replicate variance, but DiD/MPD absorbed-FE paths still use self.vcov_type to trigger the HC2/HC2-BM full-dummy auto-route before using the remapped _fit_vcov_type="hc1". This can change the result surface versus an explicit hc1 request and can change replicate validity because the fixed-dummy compute_replicate_vcov path requires all exposed coefficients to be finite, while the absorbed refit path works on the reduced identified design.

Concrete fix: Compute an effective vcov family before any vcov-dependent routing, e.g. route_vcov_type = "hc1" if _replicate_vcov_remap else self.vcov_type, and use that for the absorb and ... in ("hc2", "hc2_bm") checks in both DiD and MPD. Add tests for DiD and MPD with absorb=..., replicate weights, and vcov_type in {"classical", "hc2", "hc2_bm"} comparing ATT, SE, vcov shape/ATT variance, and no unintended full-dummy surface change against explicit hc1.

Code Quality

Finding 2 — P3

Location: diff_diff/estimators.py:L357-L358, diff_diff/estimators.py:L1596-L1597, diff_diff/twfe.py:L239-L256

Impact: The replicate-vcov warning can fire before the wild_bootstrap incompatibility error. The warning says the fit is “proceeding with replicate variance,” but the call then raises.

Concrete fix: Move the wild_bootstrap replicate-design rejection before _warn_replicate_vcov_ignored() in all three fit paths.

Performance

No separate performance blocker. The P1 also has a performance side effect: absorbed DiD/MPD replicate fits may still build full-dummy designs under an effective hc1 remap.

Maintainability

No additional maintainability findings beyond the P1 routing split between self.vcov_type and _fit_vcov_type.

Tech Debt

No untracked blocking tech debt found. Removing the old TODO row is acceptable if the implementation and registry are made internally consistent.

Security

No security issues found in the changed files.

Documentation/Tests

Finding 3 — P2

Location: docs/methodology/REGISTRY.md:L3404, docs/methodology/REGISTRY.md:L4793

Impact: The registry now contradicts itself. The new replicate-variance note says TWFE hc2/hc2_bm warns and remaps, while the older TWFE HC2/HC2-BM section still says replicate-weight designs are blocked with NotImplementedError.

Concrete fix: Update the older TWFE paragraph to reference the new replicate-weight note and remove the stale NotImplementedError/workaround language.

Finding 4 — P2

Location: tests/test_replicate_weight_expansion.py:L535-L584, tests/test_estimators_vcov_type.py:L973-L1017

Impact: Tests cover TWFE hc2/hc2_bm, but DiD/MPD only cover hc2 and do not cover classical, hc2_bm, or the absorbed-FE interaction where the remap currently leaks.

Concrete fix: Parametrize the replicate-vcov tests across classical, hc2, and hc2_bm for all three estimators, and add absorbed-FE DiD/MPD cases that assert equivalence to explicit hc1.

Path to Approval

  1. Use the remapped effective vcov value for DiD and MPD absorbed-FE routing, not self.vcov_type.
  2. Add absorbed-FE replicate tests for DiD and MPD comparing explicit non-hc1 vcov requests to explicit hc1.
  3. Update the stale TWFE methodology registry paragraph at docs/methodology/REGISTRY.md:L3404.
  4. Move the wild-bootstrap replicate rejection before the new warning helper, or otherwise avoid warning on a fit that will raise.

…s (warn + hc1 remap)

With replicate variance the analytical sandwich is replaced wholesale:
per-replicate refits return point estimates only, identical across vcov
families (FWL: full-dummy and within-transformed fits give the same
coefficients), so the requested vcov family cannot influence any
reported number. TwoWayFixedEffects(vcov_type='hc2'/'hc2_bm') raised
NotImplementedError while DifferenceInDifferences silently ignored the
kwarg — inconsistent twins. All three (incl. MultiPeriodDiD) now share
one contract via a base-class helper: explicit non-hc1 vcov_type warns
and the (discarded) base fit remaps to hc1 — avoiding wasted CR2-BM
work, one-way validator rejections, and the TWFE full-dummy auto-route
(which does not compose with per-replicate re-demeaning). Explicit hc1
(the old error's own workaround guidance) stays silent. The per-
replicate full-dummy HC2 implementation the TODO row proposed was
investigated and rejected as a costly no-op: it cannot change the
replicate variance. Tests lock warn + bit-identity-to-hc1 on all three
estimators; REGISTRY survey-section Note documents the decision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@igerber igerber force-pushed the fix/replicate-vcov-type-warn branch from fe4446d to b399219 Compare July 8, 2026 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant