Skip to content

Commit 3f7e72c

Browse files
igerberclaude
andcommitted
Fix CI review Round 3: validation guards for het/design2 interactions
P1: heterogeneity without L_max now raises ValueError (was silent no-op). P1: heterogeneity rejects trends_linear and trends_nonparam (would produce coefficients for wrong estimand since het test uses raw level changes). P1: design2=True with drop_larger_lower=True now raises ValueError (the 2-switch groups Design-2 needs are dropped by the default filter). P3: NaN overall row under trends_linear+L_max>=2 now labeled as "DID^{fd}_l (see linear_trends_effects)" instead of "Cost-Benefit Delta". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 6f1c99f commit 3f7e72c

3 files changed

Lines changed: 74 additions & 2 deletions

File tree

diff_diff/chaisemartin_dhaultfoeuille.py

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,16 @@ def fit(
601601
"CallawaySantAnna which supports survey_design."
602602
)
603603

604+
# Design-2 precondition: requires drop_larger_lower=False
605+
if design2 and self.drop_larger_lower:
606+
raise ValueError(
607+
"design2=True requires drop_larger_lower=False because "
608+
"Design-2 groups have exactly 2 treatment changes (join "
609+
"then leave), which are dropped by the default "
610+
"drop_larger_lower=True filter. Construct the estimator "
611+
"with ChaisemartinDHaultfoeuille(drop_larger_lower=False)."
612+
)
613+
604614
# ------------------------------------------------------------------
605615
# Step 4-5: Validate input + aggregate to (g, t) cells via the
606616
# shared helper used by both fit() and twowayfeweights(). The
@@ -2132,7 +2142,12 @@ def fit(
21322142
# Heterogeneity testing (Web Appendix Section 1.5, Lemma 7)
21332143
# ------------------------------------------------------------------
21342144
heterogeneity_effects: Optional[Dict[int, Dict[str, Any]]] = None
2135-
if heterogeneity is not None and L_max is not None and L_max >= 1:
2145+
if heterogeneity is not None:
2146+
if L_max is None:
2147+
raise ValueError(
2148+
"heterogeneity testing requires L_max >= 1. Set L_max "
2149+
"to use the per-group DID_{g,l} path."
2150+
)
21362151
het_col = str(heterogeneity)
21372152
if het_col not in data.columns:
21382153
raise ValueError(
@@ -2146,6 +2161,20 @@ def fit(
21462161
"R's did_multiplegt_dyn disallows predict_het with "
21472162
"controls; remove one of the two options."
21482163
)
2164+
if _is_trends_linear:
2165+
raise ValueError(
2166+
"heterogeneity cannot be combined with trends_linear. "
2167+
"The heterogeneity test operates on level outcome "
2168+
"changes but trends_linear uses second-differenced "
2169+
"outcomes; the results would be inconsistent."
2170+
)
2171+
if trends_nonparam is not None:
2172+
raise ValueError(
2173+
"heterogeneity cannot be combined with trends_nonparam. "
2174+
"The heterogeneity test does not thread state-set "
2175+
"control-pool restrictions; the results would be "
2176+
"inconsistent with the fitted estimator."
2177+
)
21492178
# Extract per-group covariate (must be time-invariant)
21502179
het_per_group = data.groupby(group)[het_col].nunique()
21512180
het_varying = het_per_group[het_per_group > 1]

diff_diff/chaisemartin_dhaultfoeuille_results.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,11 @@ def _estimand_label(self) -> str:
423423
has_controls = self.covariate_residuals is not None
424424
has_trends = self.linear_trends_effects is not None
425425

426+
# When trends_linear + L_max>=2, overall is NaN (no aggregate).
427+
# Label reflects that per-horizon effects are in linear_trends_effects.
428+
if has_trends and self.L_max is not None and self.L_max >= 2:
429+
return "DID^{fd}_l (see linear_trends_effects)"
430+
426431
if self.L_max is not None and self.L_max >= 2:
427432
base = "delta"
428433
elif self.L_max is not None and self.L_max == 1:

tests/test_chaisemartin_dhaultfoeuille.py

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2772,6 +2772,34 @@ def test_heterogeneity_rejects_controls(self):
27722772
L_max=1, heterogeneity="het_x", controls=["X1"],
27732773
)
27742774

2775+
def test_heterogeneity_requires_lmax(self):
2776+
"""heterogeneity without L_max raises ValueError."""
2777+
df = self._make_panel_with_het()
2778+
with pytest.raises(ValueError, match="requires L_max >= 1"):
2779+
ChaisemartinDHaultfoeuille(seed=1).fit(
2780+
df, "outcome", "group", "period", "treatment",
2781+
heterogeneity="het_x",
2782+
)
2783+
2784+
def test_heterogeneity_rejects_trends_linear(self):
2785+
"""heterogeneity + trends_linear raises ValueError."""
2786+
df = self._make_panel_with_het()
2787+
with pytest.raises(ValueError, match="cannot be combined with trends_linear"):
2788+
ChaisemartinDHaultfoeuille(seed=1).fit(
2789+
df, "outcome", "group", "period", "treatment",
2790+
L_max=2, heterogeneity="het_x", trends_linear=True,
2791+
)
2792+
2793+
def test_heterogeneity_rejects_trends_nonparam(self):
2794+
"""heterogeneity + trends_nonparam raises ValueError."""
2795+
df = self._make_panel_with_het()
2796+
df["state"] = df["group"] % 3
2797+
with pytest.raises(ValueError, match="cannot be combined with trends_nonparam"):
2798+
ChaisemartinDHaultfoeuille(seed=1).fit(
2799+
df, "outcome", "group", "period", "treatment",
2800+
L_max=1, heterogeneity="het_x", trends_nonparam="state",
2801+
)
2802+
27752803

27762804
class TestDesign2:
27772805
"""Design-2 switch-in/switch-out separation (ROADMAP item 3e)."""
@@ -2822,7 +2850,8 @@ def test_design2_no_eligible(self):
28222850
y = 10 + 2 * t + 5 * d + rng.normal(0, 0.5)
28232851
rows.append({"group": g, "period": t, "treatment": d, "outcome": y})
28242852
df = pd.DataFrame(rows)
2825-
r = ChaisemartinDHaultfoeuille(seed=1).fit(
2853+
# drop_larger_lower=False required for design2=True
2854+
r = ChaisemartinDHaultfoeuille(seed=1, drop_larger_lower=False).fit(
28262855
df, "outcome", "group", "period", "treatment",
28272856
L_max=1, design2=True,
28282857
)
@@ -2836,6 +2865,15 @@ def test_design2_disabled_by_default(self):
28362865
)
28372866
assert r.design2_effects is None
28382867

2868+
def test_design2_rejects_drop_larger_lower(self):
2869+
"""design2=True with default drop_larger_lower=True raises ValueError."""
2870+
df = self._make_join_then_leave_panel()
2871+
with pytest.raises(ValueError, match="drop_larger_lower=False"):
2872+
ChaisemartinDHaultfoeuille(seed=1).fit(
2873+
df, "outcome", "group", "period", "treatment",
2874+
L_max=1, design2=True,
2875+
)
2876+
28392877

28402878
class TestNonBinaryTreatment:
28412879
"""Non-binary treatment support (ROADMAP item 3f)."""

0 commit comments

Comments
 (0)