Skip to content

Commit d103eec

Browse files
igerberclaude
andcommitted
Address CI AI review: NaN first_treat + StaggeredTripleDifference coverage
Two new P1 findings from CI AI re-review: 1. ContinuousDiD.fit() still accepted NaN `first_treat` values. NaN survives preprocessing but satisfies neither the treated (g > 0) nor never-treated (g == 0) mask, so affected units were silently excluded from both pools — same silent-failure shape as the already-rejected `first_treat < 0`. Reject NaN explicitly with ValueError + row count. 2. `StaggeredTripleDifference.fit()` silently recoded `first_treat=np.inf -> 0` via `.replace([np.inf, float("inf")], 0)`. Its sibling `staggered.py:1508-1519` already surfaces this with a UserWarning; this PR mirrors that contract so the estimator no longer shifts units between treated and never-treated pools without signaling. REGISTRY gets a matching **Note:** under the StaggeredTripleDifference section. Regression tests: - test_nan_first_treat_raises_with_row_count (ContinuousDiD) on a 4-unit x 3-period panel with 3 NaN rows. - test_inf_first_treat_works (StaggeredTripleDifference) upgraded from silent-success to `pytest.warns(UserWarning, match=row-count)`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2ace61d commit d103eec

5 files changed

Lines changed: 64 additions & 3 deletions

File tree

diff_diff/continuous_did.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,19 @@ def fit(
235235
# existing `.replace([np.inf, float("inf")], 0)` semantics on the
236236
# next line).
237237
first_treat_vals = df[first_treat].values
238+
# Reject NaN first_treat explicitly. NaN survives preprocessing but
239+
# satisfies neither the treated (g > 0) nor never-treated (g == 0)
240+
# mask, so affected units would be silently excluded from the
241+
# estimator (same silent-failure shape as `first_treat < 0`).
242+
nan_mask = pd.isna(df[first_treat])
243+
n_nan_first_treat = int(nan_mask.sum())
244+
if n_nan_first_treat > 0:
245+
raise ValueError(
246+
f"{n_nan_first_treat} row(s) have NaN '{first_treat}' "
247+
f"values. Valid values are 0 (never-treated) or a positive "
248+
f"treatment period; such units would otherwise be silently "
249+
f"excluded from both treated and control pools."
250+
)
238251
inf_mask = np.isposinf(first_treat_vals)
239252
n_inf_first_treat = int(inf_mask.sum())
240253
if n_inf_first_treat > 0:

diff_diff/staggered_triple_diff.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,19 @@ def fit(
284284

285285
if first_treat != "first_treat":
286286
df["first_treat"] = df[first_treat]
287+
# Surface the inf → 0 recategorization the same way StaggeredDiD does
288+
# (see `staggered.py:1508-1519`). Silently recoding inf would shift
289+
# units between treated and never-treated pools with no signal
290+
# (axis-E silent coercion under the Phase 2 audit).
291+
_inf_mask = np.isposinf(df["first_treat"].values)
292+
if _inf_mask.any():
293+
n_inf_rows = int(_inf_mask.sum())
294+
warnings.warn(
295+
f"{n_inf_rows} row(s) have first_treat=inf; recoding to 0 "
296+
f"(never-treated). Use first_treat=0 to suppress this warning.",
297+
UserWarning,
298+
stacklevel=2,
299+
)
287300
df["first_treat"] = df["first_treat"].replace([np.inf, float("inf")], 0)
288301

289302
precomputed = self._precompute_structures(

docs/methodology/REGISTRY.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1692,6 +1692,7 @@ Balanced panel. Key variables:
16921692
- `Q_i` (`eligibility`): binary, time-invariant eligibility indicator
16931693
- Treatment: `D_{i,t} = 1{t >= S_i AND Q_i = 1}` (absorbing)
16941694
- Covariates `X_i`: time-invariant (first observation per unit used)
1695+
- **Note:** `first_treat=inf` (R-style never-enabled marker) is accepted and normalized to `0` internally. The recoding now emits a `UserWarning` reporting the affected row count so the reclassification is not silent (axis-E silent coercion under the Phase 2 audit, mirroring the StaggeredDiD behavior). Pass `first_treat=0` directly to avoid the warning.
16951696

16961697
*Estimator equation (Equation 4.1 in paper, as implemented):*
16971698

tests/test_continuous_did.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -751,6 +751,29 @@ def test_negative_first_treat_raises_with_row_count(self):
751751
):
752752
est.fit(data, "outcome", "unit", "period", "first_treat", "dose")
753753

754+
def test_nan_first_treat_raises_with_row_count(self):
755+
"""NaN `first_treat` must raise ValueError with the row count. Without
756+
this guard, NaN rows survive preprocessing but match neither the
757+
treated (g > 0) nor never-treated (g == 0) mask, so the affected
758+
units would be silently excluded."""
759+
rows = []
760+
for unit in range(4):
761+
# Unit 0 has NaN first_treat across all 3 periods (3 NaN rows).
762+
ft = np.nan if unit == 0 else 0.0
763+
for t in range(1, 4):
764+
rows.append({
765+
"unit": unit, "period": t, "outcome": float(unit + t),
766+
"first_treat": ft, "dose": 0.0,
767+
})
768+
data = pd.DataFrame(rows)
769+
est = ContinuousDiD()
770+
771+
with pytest.raises(
772+
ValueError,
773+
match=r"3 row\(s\) have NaN 'first_treat' values",
774+
):
775+
est.fit(data, "outcome", "unit", "period", "first_treat", "dose")
776+
754777
def test_positive_inf_warning_silent_when_no_inf(self):
755778
"""+inf warning is gated on +inf rows only; panels with only valid
756779
non-negative values (including just 0 and positive periods) must

tests/test_staggered_triple_diff.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -397,12 +397,23 @@ def test_missing_column_raises(self, simple_data):
397397
est.fit(simple_data, "outcome", "unit", "period", "nonexistent", "eligibility")
398398

399399
def test_inf_first_treat_works(self):
400-
"""Never-enabled units encoded as inf should work."""
400+
"""Never-enabled units encoded as inf should be recoded to 0, and the
401+
recoding must surface a UserWarning with the affected row count
402+
(axis-E silent coercion, mirroring the StaggeredDiD behavior)."""
401403
data = generate_staggered_ddd_data(n_units=100, seed=33)
402404
data["first_treat"] = data["first_treat"].astype(float)
403-
data.loc[data["first_treat"] == 0, "first_treat"] = np.inf
405+
inf_mask = data["first_treat"] == 0
406+
n_inf_rows = int(inf_mask.sum())
407+
data.loc[inf_mask, "first_treat"] = np.inf
404408
est = StaggeredTripleDifference()
405-
res = est.fit(data, "outcome", "unit", "period", "first_treat", "eligibility")
409+
410+
with pytest.warns(
411+
UserWarning,
412+
match=rf"{n_inf_rows} row\(s\) have first_treat=inf; recoding to 0",
413+
):
414+
res = est.fit(
415+
data, "outcome", "unit", "period", "first_treat", "eligibility"
416+
)
406417
assert np.isfinite(res.overall_att)
407418

408419
def test_survey_design_invalid_type_raises(self, simple_data):

0 commit comments

Comments
 (0)