Skip to content

Commit 7d5ecd6

Browse files
igerberclaude
andcommitted
PR #458 R2: move auto-route ahead of multi-absorb-survey guard
R2 review flagged that REGISTRY/CHANGELOG documented `DiD(absorb=..., vcov_type in {hc2,hc2_bm})` as SUPPORTED, but the legacy `len(absorb) > 1 + survey_weights` guard at estimators.py:347 fired BEFORE the auto-route, so weighted multi-absorb fits still raised. The guard's rationale ("single-pass demeaning isn't the correct weighted FWL projection for N>1 absorbed dimensions") doesn't apply when we're auto-routing to fixed_effects= — the fixed_effects= path builds the full-dummy design and solves WLS directly with no within-transform. Reorder: move the auto-route block above the multi-absorb-survey guard. The guard now only fires when absorb was NOT consumed by the auto-route (i.e., hc1/classical/conley/etc. — paths that still demean). Adds `test_absorb_hc2_bm_survey_multi_absorb_auto_routes` to pin the new placement against silent regression. The existing `test_survey.py` multi-absorb-survey rejection tests continue to pass (they use the default vcov_type=hc1 path which still hits the guard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d883546 commit 7d5ecd6

2 files changed

Lines changed: 72 additions & 19 deletions

File tree

diff_diff/estimators.py

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -344,25 +344,6 @@ def fit(
344344
n_treated_raw = int(np.sum(data[treatment].values.astype(float)))
345345
n_control_raw = len(data) - n_treated_raw
346346

347-
# Reject multi-absorb with survey weights (single-pass demeaning is
348-
# not the correct weighted FWL projection for N > 1 dimensions)
349-
if absorb and len(absorb) > 1 and survey_weights is not None:
350-
raise ValueError(
351-
f"Multiple absorbed fixed effects (absorb={absorb}) with survey "
352-
"weights is not supported. Single-pass sequential demeaning is not "
353-
"the correct weighted FWL projection for multiple absorbed dimensions. "
354-
"Use absorb with a single variable, or use fixed_effects= instead."
355-
)
356-
357-
if absorb and fixed_effects:
358-
raise ValueError(
359-
"Cannot use both absorb and fixed_effects. "
360-
"The absorb within-transformation does not residualize "
361-
"fixed_effects dummies, violating the FWL theorem. "
362-
"Use absorb alone (for high-dimensional FE) "
363-
"or fixed_effects alone (for low-dimensional FE)."
364-
)
365-
366347
# Auto-route absorb → fixed_effects when vcov_type needs the FULL FE
367348
# hat matrix. HC2 leverage and CR2 Bell-McCaffrey DOF both depend on
368349
# the full-design hat; FWL preserves coefficients and residuals but
@@ -379,12 +360,42 @@ def fit(
379360
# Note: the user-facing `result.coefficients` under this auto-route
380361
# will include the FE-dummy entries (matching the fixed_effects= path),
381362
# not the slope-only view that a plain `absorb=` returns.
363+
#
364+
# Placement: this auto-route runs BEFORE the legacy multi-absorb +
365+
# survey-weights guard because that guard's rationale ("single-pass
366+
# demeaning is not the correct weighted FWL projection for N > 1
367+
# dimensions") doesn't apply when we're about to swap absorb for
368+
# fixed_effects: the fixed_effects= path builds the full-dummy design
369+
# and solves WLS directly, with no within-transform step. R2 review
370+
# surfaced the scope mismatch (REGISTRY/CHANGELOG said "SUPPORTED" but
371+
# the survey guard fired first on weighted multi-absorb fits).
382372
if absorb and self.vcov_type in ("hc2", "hc2_bm"):
383373
fixed_effects = list(fixed_effects or []) + list(absorb)
384374
absorb = None
385375
absorbed_vars = []
386376
n_absorbed_effects = 0
387377

378+
# Reject multi-absorb with survey weights (single-pass demeaning is
379+
# not the correct weighted FWL projection for N > 1 dimensions). Only
380+
# fires when absorb is still set — i.e., the auto-route above didn't
381+
# consume it.
382+
if absorb and len(absorb) > 1 and survey_weights is not None:
383+
raise ValueError(
384+
f"Multiple absorbed fixed effects (absorb={absorb}) with survey "
385+
"weights is not supported. Single-pass sequential demeaning is not "
386+
"the correct weighted FWL projection for multiple absorbed dimensions. "
387+
"Use absorb with a single variable, or use fixed_effects= instead."
388+
)
389+
390+
if absorb and fixed_effects:
391+
raise ValueError(
392+
"Cannot use both absorb and fixed_effects. "
393+
"The absorb within-transformation does not residualize "
394+
"fixed_effects dummies, violating the FWL theorem. "
395+
"Use absorb alone (for high-dimensional FE) "
396+
"or fixed_effects alone (for low-dimensional FE)."
397+
)
398+
388399
# Validate vcov_type="conley" wire-up. DiD.fit() accepts `unit`
389400
# as a fit-time arg (NOT on __init__) because cluster/unit
390401
# semantics on DiD are opt-in rather than auto-derived (unlike

tests/test_estimators_vcov_type.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,6 +1095,48 @@ def test_absorb_hc2_bm_clustered_matches_clubsandwich(self):
10951095
np.testing.assert_allclose(res.se, expected_se_slope, atol=1e-10)
10961096
np.testing.assert_allclose(res.att, float(d["coef"][treat_post_idx]), atol=1e-10)
10971097

1098+
def test_absorb_hc2_bm_survey_multi_absorb_auto_routes(self):
1099+
"""Survey-weighted multi-absorb + HC2-BM should auto-route, not reject.
1100+
1101+
The legacy guard at `estimators.py` rejects `survey_design` paired with
1102+
`len(absorb) > 1` because single-pass demeaning is not the correct
1103+
weighted FWL projection for multiple absorbed dimensions. But when the
1104+
auto-route fires (hc2/hc2_bm), absorb is swapped for fixed_effects=
1105+
BEFORE the survey guard sees it, so the demeaning rationale doesn't
1106+
apply. R2 review caught the scope mismatch: REGISTRY said "SUPPORTED"
1107+
but the survey guard fired first on weighted multi-absorb. This test
1108+
pins the new placement.
1109+
"""
1110+
from diff_diff import SurveyDesign
1111+
1112+
d = self._load_golden()
1113+
rng = np.random.default_rng(20260420)
1114+
n = len(d["y"])
1115+
data = pd.DataFrame(
1116+
{
1117+
"unit": d["unit"],
1118+
"period": d["period"],
1119+
"treated": d["treated"],
1120+
"post": d["post"],
1121+
"y": d["y"],
1122+
"weight": rng.uniform(0.5, 2.0, size=n),
1123+
}
1124+
)
1125+
sd = SurveyDesign(weights="weight", weight_type="aweight")
1126+
# Multi-absorb (`unit` + `period`) + survey-weighted + hc2_bm: should
1127+
# auto-route to fixed_effects= and succeed.
1128+
res = DifferenceInDifferences(vcov_type="hc2_bm").fit(
1129+
data,
1130+
outcome="y",
1131+
treatment="treated",
1132+
time="post",
1133+
absorb=["unit", "period"],
1134+
unit="unit",
1135+
survey_design=sd,
1136+
)
1137+
assert np.isfinite(res.att)
1138+
assert np.isfinite(res.se)
1139+
10981140
def test_absorb_hc2_bm_df_sensitive_inference(self):
10991141
"""Bell-McCaffrey Satterthwaite DOF must propagate to `p_value` / `conf_int`.
11001142

0 commit comments

Comments
 (0)