Skip to content

Commit e59840d

Browse files
igerberclaude
andcommitted
Fix CI review Round 3: L_max=1 bootstrap sync, DID_1 label, docs alignment
P1: Sync overall_* from event_study_effects[1] AFTER bootstrap propagation so bootstrap SE/p/CI flow to top-level surface for L_max=1. P1: Label overall estimand as "DID_1" (not "DID_M") when L_max=1 in __repr__. Update REGISTRY and results docstrings to document L_max=1 as per-group DID_1 path. P2: Add binary + non-binary L_max=1 bootstrap regressions asserting overall_* == event_study_effects[1]. P3: Update TODO.md, REGISTRY L_max >= 2 references to L_max >= 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 966a00d commit e59840d

5 files changed

Lines changed: 52 additions & 6 deletions

File tree

TODO.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ Deferred items from PR reviews that were not addressed before merge.
5656

5757
| Issue | Location | PR | Priority |
5858
|-------|----------|----|----------|
59-
| dCDH: Phase 1 per-period placebo DID_M^pl has NaN SE (no IF derivation for the per-period aggregation path). Multi-horizon placebos (L_max >= 2) have valid SE. | `chaisemartin_dhaultfoeuille.py` | #294 | Low |
59+
| dCDH: Phase 1 per-period placebo DID_M^pl has NaN SE (no IF derivation for the per-period aggregation path). Multi-horizon placebos (L_max >= 1) have valid SE. | `chaisemartin_dhaultfoeuille.py` | #294 | Low |
6060
| dCDH: Parity test SE/CI assertions only cover pure-direction scenarios; mixed-direction SE comparison is structurally apples-to-oranges (cell-count vs obs-count weighting). | `test_chaisemartin_dhaultfoeuille_parity.py` | #294 | Low |
6161
| CallawaySantAnna: consider materializing NaN entries for non-estimable (g,t) cells in group_time_effects dict (currently omitted with consolidated warning); would require updating downstream consumers (event study, balance_e, aggregation) | `staggered.py` | #256 | Low |
6262
| ImputationDiD dense `(A0'A0).toarray()` scales O((U+T+K)^2), OOM risk on large panels | `imputation.py` | #141 | Medium (deferred — only triggers when sparse solver fails) |

diff_diff/chaisemartin_dhaultfoeuille.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,6 +1649,21 @@ def fit(
16491649
eff + crit * se,
16501650
)
16511651

1652+
# When L_max >= 1 and the per-group path is active, sync
1653+
# overall_* from event_study_effects[1] AFTER bootstrap propagation
1654+
# so that bootstrap SE/p/CI flow to the top-level surface.
1655+
if (
1656+
L_max is not None
1657+
and L_max >= 1
1658+
and 1 in event_study_effects
1659+
):
1660+
es1 = event_study_effects[1]
1661+
overall_att = es1["effect"]
1662+
overall_se = es1["se"]
1663+
overall_t = es1["t_stat"]
1664+
overall_p = es1["p_value"]
1665+
overall_ci = es1["conf_int"]
1666+
16521667
# Phase 2: override overall_att with cost-benefit delta when L_max > 1
16531668
effective_overall_att = overall_att
16541669
effective_overall_se = overall_se

diff_diff/chaisemartin_dhaultfoeuille_results.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -303,9 +303,11 @@ class ChaisemartinDHaultfoeuilleResults:
303303
Significance level used for confidence intervals.
304304
event_study_effects : dict, optional
305305
Populated with horizon ``1`` when ``L_max=None``, or horizons
306-
``1..L_max`` when ``L_max >= 2``.
306+
``1..L_max`` when ``L_max >= 1``. When ``L_max >= 1``, uses the
307+
per-group ``DID_{g,l}`` path; when ``L_max=None``, uses the
308+
per-period ``DID_M`` path.
307309
normalized_effects : dict, optional
308-
Normalized estimator ``DID^n_l``. Populated when ``L_max >= 2``.
310+
Normalized estimator ``DID^n_l``. Populated when ``L_max >= 1``.
309311
cost_benefit_delta : dict, optional
310312
Cost-benefit aggregate ``delta``. Populated when ``L_max >= 2``.
311313
sup_t_bands : dict, optional
@@ -410,7 +412,12 @@ class ChaisemartinDHaultfoeuilleResults:
410412
def __repr__(self) -> str:
411413
"""Concise string representation."""
412414
sig = _get_significance_stars(self.overall_p_value)
413-
label = "delta" if self.L_max is not None and self.L_max >= 2 else "DID_M"
415+
if self.L_max is not None and self.L_max >= 2:
416+
label = "delta"
417+
elif self.L_max is not None and self.L_max == 1:
418+
label = "DID_1"
419+
else:
420+
label = "DID_M"
414421
return (
415422
f"ChaisemartinDHaultfoeuilleResults("
416423
f"{label}={self.overall_att:.4f}{sig}, "

docs/methodology/REGISTRY.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -517,7 +517,7 @@ DID_M^pl = (1/N_S^pl) * sum_{t>=3} (
517517

518518
*Phase 2: Multi-horizon event study (Equation 3 and 5 of the dynamic companion paper):*
519519

520-
When `L_max >= 2`, the estimator computes the per-group building block `DID_{g,l}` and the aggregate `DID_l` for each horizon:
520+
When `L_max >= 1`, the estimator computes the per-group building block `DID_{g,l}` and the aggregate `DID_l` for each horizon. When `L_max=1`, `overall_att` holds `DID_1` (the per-group estimand, not the per-period `DID_M`). When `L_max >= 2`, `overall_att` holds the cost-benefit delta. When `L_max=None`, the per-period `DID_M` path is used:
521521

522522
```
523523
DID_{g,l} = Y_{g, F_g-1+l} - Y_{g, F_g-1}

tests/test_chaisemartin_dhaultfoeuille.py

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1820,6 +1820,24 @@ def test_L_max_none_preserves_phase1_behavior(self, data):
18201820
assert r.sup_t_bands is None
18211821
assert r.placebo_event_study is None
18221822

1823+
def test_L_max_1_bootstrap_overall_matches_es1(self, data, ci_params):
1824+
"""With L_max=1 + bootstrap, overall_* must match event_study_effects[1]."""
1825+
n_boot = ci_params.bootstrap(99)
1826+
est = ChaisemartinDHaultfoeuille(
1827+
placebo=False, twfe_diagnostic=False, n_bootstrap=n_boot, seed=42
1828+
)
1829+
with warnings.catch_warnings():
1830+
warnings.simplefilter("ignore")
1831+
r = est.fit(
1832+
data, outcome="outcome", group="group", time="period",
1833+
treatment="treatment", L_max=1,
1834+
)
1835+
es1 = r.event_study_effects[1]
1836+
assert r.overall_att == es1["effect"]
1837+
assert r.overall_se == es1["se"]
1838+
assert r.overall_p_value == es1["p_value"]
1839+
assert r.overall_conf_int == es1["conf_int"]
1840+
18231841
def test_L_max_1_uses_per_group_path(self, data):
18241842
"""L_max=1 uses the per-group DID_{g,1} path (same as L_max >= 2
18251843
uses for l=1). This is a different estimand from the per-period
@@ -2370,7 +2388,8 @@ def test_mixed_binary_nonbinary_panel_lmax1(self):
23702388
assert r.overall_att == r.event_study_effects[1]["effect"]
23712389

23722390
def test_nonbinary_bootstrap(self, ci_params):
2373-
"""Non-binary panel with bootstrap should produce finite event study SEs."""
2391+
"""Non-binary panel with bootstrap: finite event study SEs AND
2392+
top-level overall_* matches event_study_effects[1]."""
23742393
np.random.seed(66)
23752394
n_boot = ci_params.bootstrap(99)
23762395
rows = []
@@ -2397,6 +2416,11 @@ def test_nonbinary_bootstrap(self, ci_params):
23972416
assert r.bootstrap_results.event_study_ses is not None
23982417
assert 1 in r.bootstrap_results.event_study_ses
23992418
assert np.isfinite(r.bootstrap_results.event_study_ses[1])
2419+
# Top-level overall_* must match event_study_effects[1]
2420+
es1 = r.event_study_effects[1]
2421+
assert r.overall_att == es1["effect"]
2422+
assert r.overall_se == es1["se"]
2423+
assert r.overall_p_value == es1["p_value"]
24002424

24012425
def test_twfe_diagnostic_skipped_nonbinary(self):
24022426
"""TWFE diagnostic should be skipped (with warning) for non-binary."""

0 commit comments

Comments
 (0)