Skip to content

Commit ee6ad2f

Browse files
igerberclaude
andcommitted
Fix CI review R1: boundary-horizon validation, placebo=False guard, docstrings
P0: _largest_consecutive_block now raises ValueError when boundary horizon (-1 or +1) is missing after finite-SE filtering instead of silently returning the full list (would produce wrong HonestDiD bounds). P1: honest_did=True now rejects placebo=False early instead of silently returning honest_did_results=None with no warning. P2: Added 3 regression tests (boundary -1 missing, boundary +1 missing, placebo=False + honest_did). P3: Updated docstrings in honest_did.py (6 locations) and docs/llms.txt to include ChaisemartinDHaultfoeuilleResults alongside MultiPeriodDiD/CS. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent b732007 commit ee6ad2f

5 files changed

Lines changed: 56 additions & 9 deletions

File tree

diff_diff/chaisemartin_dhaultfoeuille.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -957,6 +957,12 @@ def fit(
957957
"Set L_max to compute DID^{pl}_l placebos that HonestDiD uses as "
958958
"pre-period coefficients."
959959
)
960+
if honest_did and not self.placebo:
961+
raise ValueError(
962+
"honest_did=True requires placebo computation. The estimator was "
963+
"constructed with placebo=False. Use "
964+
"ChaisemartinDHaultfoeuille(placebo=True) (the default)."
965+
)
960966

961967
# Pivot to (group x time) matrices for vectorized computations
962968
d_pivot = cell.pivot(index=group, columns=time, values="d_gt").reindex(

diff_diff/honest_did.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ def _extract_event_study_params(
559559
560560
Parameters
561561
----------
562-
results : MultiPeriodDiDResults or CallawaySantAnnaResults
562+
results : MultiPeriodDiDResults, CallawaySantAnnaResults, or ChaisemartinDHaultfoeuilleResults
563563
Estimation results with event study structure.
564564
565565
Returns
@@ -886,8 +886,14 @@ def _largest_consecutive_block(times, boundary_val):
886886
if not times:
887887
return []
888888
if boundary_val not in times:
889-
# No boundary value - take the block closest to it
890-
return times
889+
raise ValueError(
890+
f"HonestDiD requires horizon {boundary_val} in "
891+
f"the dCDH "
892+
f"{'placebo' if boundary_val < 0 else 'event study'}"
893+
f" surface, but it was removed by finite-SE "
894+
f"filtering. Retained horizons: {times}. Ensure "
895+
f"horizon {boundary_val} has a finite SE."
896+
)
891897
# Expand outward from boundary_val
892898
block = [boundary_val]
893899
idx = times.index(boundary_val)
@@ -2197,7 +2203,7 @@ def fit(
21972203
21982204
Parameters
21992205
----------
2200-
results : MultiPeriodDiDResults or CallawaySantAnnaResults
2206+
results : MultiPeriodDiDResults, CallawaySantAnnaResults, or ChaisemartinDHaultfoeuilleResults
22012207
Results from event study estimation.
22022208
M : float, optional
22032209
Override the M parameter for this fit.
@@ -2515,7 +2521,7 @@ def sensitivity_analysis(
25152521
25162522
Parameters
25172523
----------
2518-
results : MultiPeriodDiDResults or CallawaySantAnnaResults
2524+
results : MultiPeriodDiDResults, CallawaySantAnnaResults, or ChaisemartinDHaultfoeuilleResults
25192525
Results from event study estimation.
25202526
M_grid : list of float, optional
25212527
Grid of M values to evaluate. If None, uses default grid
@@ -2614,7 +2620,7 @@ def breakdown_value(
26142620
26152621
Parameters
26162622
----------
2617-
results : MultiPeriodDiDResults or CallawaySantAnnaResults
2623+
results : MultiPeriodDiDResults, CallawaySantAnnaResults, or ChaisemartinDHaultfoeuilleResults
26182624
Results from event study estimation.
26192625
tol : float
26202626
Tolerance for binary search.
@@ -2669,7 +2675,7 @@ def compute_honest_did(
26692675
26702676
Parameters
26712677
----------
2672-
results : MultiPeriodDiDResults or CallawaySantAnnaResults
2678+
results : MultiPeriodDiDResults, CallawaySantAnnaResults, or ChaisemartinDHaultfoeuilleResults
26732679
Results from event study estimation.
26742680
method : str
26752681
Type of restriction ("smoothness", "relative_magnitude", "combined").
@@ -2705,7 +2711,7 @@ def sensitivity_plot(
27052711
27062712
Parameters
27072713
----------
2708-
results : MultiPeriodDiDResults or CallawaySantAnnaResults
2714+
results : MultiPeriodDiDResults, CallawaySantAnnaResults, or ChaisemartinDHaultfoeuilleResults
27092715
Results from event study estimation.
27102716
method : str
27112717
Type of restriction.

docs/llms.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ diagnostic steps produces unreliable results.
2020
3. **Test parallel trends** — simple 2x2: `check_parallel_trends()`, `equivalence_test_trends()`; staggered: inspect CS event-study pre-period coefficients (generic PT tests are invalid for staggered designs). Insignificant pre-trends do NOT prove PT holds.
2121
4. **Choose estimator** — staggered adoption → CS/SA/BJS (NOT plain TWFE); few treated units → SDiD; factor confounding → TROP; simple 2x2 → DiD. Run `BaconDecomposition` to diagnose TWFE bias.
2222
5. **Estimate** — `estimator.fit(data, ...)`. Always print the cluster count first and choose inference method based on the result (cluster-robust if >= 50 clusters, wild bootstrap if fewer).
23-
6. **Sensitivity analysis** — `compute_honest_did(results)` for bounds under PT violations (MultiPeriodDiD/CS only), `run_all_placebo_tests()` for 2x2 falsification, specification comparisons for staggered designs.
23+
6. **Sensitivity analysis** — `compute_honest_did(results)` for bounds under PT violations (MultiPeriodDiD, CS, or dCDH), `run_all_placebo_tests()` for 2x2 falsification, specification comparisons for staggered designs.
2424
7. **Heterogeneity** — CS: `aggregate='group'`/`'event_study'`; SA: `results.event_study_effects`/`to_dataframe(level='cohort')`; subgroup re-estimation.
2525
8. **Robustness** — compare 2-3 estimators (CS vs SA vs BJS), MUST report with and without covariates (shows whether conditioning drives identification), present pre-trends and sensitivity bounds.
2626

tests/test_chaisemartin_dhaultfoeuille.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3275,6 +3275,15 @@ def test_honest_did_requires_lmax(self):
32753275
honest_did=True,
32763276
)
32773277

3278+
def test_honest_did_rejects_placebo_false(self):
3279+
"""honest_did=True with placebo=False raises ValueError."""
3280+
df = self._make_data()
3281+
with pytest.raises(ValueError, match="placebo=False"):
3282+
ChaisemartinDHaultfoeuille(seed=1, placebo=False).fit(
3283+
df, "outcome", "group", "period", "treatment",
3284+
L_max=2, honest_did=True,
3285+
)
3286+
32783287
def test_honest_did_standalone(self):
32793288
"""compute_honest_did() on dCDH results matches honest_did=True."""
32803289
from diff_diff.honest_did import compute_honest_did

tests/test_honest_did.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1429,3 +1429,29 @@ def test_dcdh_empty_consecutive_block_raises(self):
14291429
warnings.simplefilter("ignore")
14301430
with pytest.raises(ValueError, match="No placebo horizons with finite SEs"):
14311431
compute_honest_did(results)
1432+
1433+
def test_dcdh_missing_boundary_minus1_raises(self):
1434+
"""ValueError when horizon -1 has NaN SE (boundary required)."""
1435+
import warnings
1436+
1437+
results = self._fit_dcdh()
1438+
# Corrupt only horizon -1 SE; leave -2 intact
1439+
results.placebo_event_study[-1]["se"] = float("nan")
1440+
1441+
with warnings.catch_warnings():
1442+
warnings.simplefilter("ignore")
1443+
with pytest.raises(ValueError, match="requires horizon -1"):
1444+
compute_honest_did(results)
1445+
1446+
def test_dcdh_missing_boundary_plus1_raises(self):
1447+
"""ValueError when horizon +1 has NaN SE (boundary required)."""
1448+
import warnings
1449+
1450+
results = self._fit_dcdh()
1451+
# Corrupt only horizon +1 SE
1452+
results.event_study_effects[1]["se"] = float("nan")
1453+
1454+
with warnings.catch_warnings():
1455+
warnings.simplefilter("ignore")
1456+
with pytest.raises(ValueError, match="requires horizon 1"):
1457+
compute_honest_did(results)

0 commit comments

Comments
 (0)