Skip to content

Commit 23d63f5

Browse files
igerberclaude
andcommitted
Address Codex review: MPD+survey reject, NaN unit guard, MPD coords guard
- Conley + survey_design front-door reject on MultiPeriodDiD.fit() so the user gets NotImplementedError instead of silent BRR/TSL SEs. The previous bypass: MPD passed return_vcov=not _use_survey_vcov to solve_ols, so the conley+weights guard inside _compute_robust_vcov_numpy never fired, and compute_survey_vcov silently overwrote the vcov. - solve_ols also gains a top-level Conley-only validator that catches bypass cases for direct compute_robust_vcov callers. Scoped to Conley only — hc2_bm + replicate-weight silently routing to BRR is a long-standing intentional contract preserved by other tests. - _validate_conley_kwargs now rejects NaN / pd.NA in conley_unit. The prior code accepted them and np.unique + boolean mask silently dropped those rows from the per-unit serial HAC sum. - MultiPeriodDiD.fit() adds an explicit conley_coords / conley_cutoff_km None-guard so missing kwargs raise ValueError instead of a raw TypeError on `self.conley_coords[0]`. - TwoWayFixedEffects class docstring updated from Phase 1 "Conley rejected" / "Phase 2 will add Driscoll-Kraay product kernel" to the shipped block-decomposed contract. - Regression tests in tests/test_conley_vcov.py: * TestConleyValidatorHelpers: NaN-float and pd.NA-object unit cases * TestConleyEstimatorIntegration: MPD + survey_design (pweight TSL and stratified PSU) raises; MPD missing conley_coords raises ValueError. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 351332e commit 23d63f5

5 files changed

Lines changed: 212 additions & 14 deletions

File tree

diff_diff/conley.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,25 @@ def _validate_conley_kwargs(
230230
raise ValueError(
231231
f"conley_time must be a 1-D array of length {n}; got shape {time_arr.shape}."
232232
)
233-
if not np.isfinite(time_arr).all():
233+
# Use pandas.isna for object/categorical dtypes; np.isfinite catches
234+
# NaN/inf for numeric dtypes only.
235+
import pandas as _pd
236+
237+
if _pd.isna(time_arr).any():
238+
raise ValueError("conley_time contains NaN or missing values.")
239+
if time_arr.dtype.kind in "fc" and not np.isfinite(time_arr).all():
234240
raise ValueError("conley_time contains NaN or inf values.")
235241
unit_arr = np.asarray(unit)
236242
if unit_arr.ndim != 1 or unit_arr.shape[0] != n:
237243
raise ValueError(
238244
f"conley_unit must be a 1-D array of length {n}; got shape {unit_arr.shape}."
239245
)
246+
# conley_unit may be any hashable (int, string, categorical). Use
247+
# pandas.isna for cross-dtype NA detection. NaN unit IDs would silently
248+
# drop those rows from the per-unit serial HAC sum at
249+
# `np.unique(unit_arr) + mask_u = unit_arr == u_val`.
250+
if _pd.isna(unit_arr).any():
251+
raise ValueError("conley_unit contains NaN or missing values.")
240252
if not (isinstance(lag_cutoff, (int, np.integer)) and int(lag_cutoff) >= 0):
241253
raise ValueError(
242254
f"conley_lag_cutoff must be a non-negative integer; got {lag_cutoff!r}."

diff_diff/estimators.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1380,6 +1380,12 @@ def fit( # type: ignore[override]
13801380
# array extraction (and conley_coords resolution from column names)
13811381
# happens just below at the solve_ols call.
13821382
if self.vcov_type == "conley":
1383+
if self.conley_coords is None or self.conley_cutoff_km is None:
1384+
raise ValueError(
1385+
"MultiPeriodDiD(vcov_type='conley') requires "
1386+
"conley_coords=(lat_col, lon_col) and conley_cutoff_km "
1387+
"on the constructor."
1388+
)
13831389
if unit is None:
13841390
raise ValueError(
13851391
"MultiPeriodDiD(vcov_type='conley') requires unit= at "
@@ -1393,6 +1399,15 @@ def fit( # type: ignore[override]
13931399
"within-period only, no serial component). See R "
13941400
"conleyreg's `lag_cutoff` argument for the convention."
13951401
)
1402+
if survey_design is not None:
1403+
raise NotImplementedError(
1404+
"MultiPeriodDiD(vcov_type='conley', survey_design=...) "
1405+
"is not supported: Conley + survey weights / replicate "
1406+
"vcov is deferred to a follow-up PR (Bertanha-Imbens 2014 "
1407+
"territory). Use vcov_type='hc1' for survey-aware "
1408+
"cluster-robust without spatial HAC, or drop survey_design= "
1409+
"for panel Conley."
1410+
)
13961411
# Pre-compute non_ref_periods (needed for absorb demeaning)
13971412
non_ref_periods = [p for p in all_periods if p != reference_period]
13981413

diff_diff/linalg.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,19 @@ def solve_ols(
660660
"Clean your data or set check_finite=False to skip this check."
661661
)
662662

663+
# Front-door Conley-specific validation. Runs BEFORE the routing/backend
664+
# branching so `return_vcov=False` cannot bypass the conley+weights /
665+
# conley+cluster_ids guards. Conley needs this because survey-replicate
666+
# paths in MultiPeriodDiD pass `return_vcov=False` + `weights=survey_w`
667+
# and would otherwise silently return survey SEs under a Conley request.
668+
# The full `_validate_vcov_args` is NOT called here because other vcov
669+
# types (e.g. `hc2_bm` + replicate weights) intentionally fall through
670+
# to the survey-vcov path with the analytical validator skipped — that
671+
# legacy contract is preserved.
672+
if vcov_type == "conley":
673+
if cluster_ids is not None or weights is not None:
674+
_validate_vcov_args(vcov_type, cluster_ids, weights)
675+
663676
# WLS transformation: apply sqrt(w) scaling to X and y
664677
# This happens BEFORE routing to Rust or NumPy backends — they receive
665678
# pre-transformed X_w, y_w and solve standard OLS.

diff_diff/twfe.py

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -67,19 +67,24 @@ class TwoWayFixedEffects(DifferenceInDifferences):
6767
``TODO.md`` under Methodology/Correctness; also documented in
6868
``docs/methodology/REGISTRY.md``.
6969
70-
**Conley spatial-HAC (``vcov_type="conley"``) is rejected at fit-time
71-
in Phase 1.** TwoWayFixedEffects is intrinsically a multi-period panel
72-
estimator and Phase 1's cross-sectional Conley does not handle the
73-
time dimension — applying it over (unit, time) rows would treat same-
74-
unit cross-time pairs as ``d_ij = 0 → K = 1``, mishandling the space-
75-
time HAC. The supported Phase 1 path for Conley is direct
76-
``compute_robust_vcov`` / ``LinearRegression`` on a single-period
77-
regression. The ``conley_*`` kwargs are inherited from
78-
``DifferenceInDifferences.__init__`` for sklearn-style API symmetry
79-
(``get_params`` / ``set_params`` round-trip), but
80-
``TwoWayFixedEffects(vcov_type="conley").fit(...)`` raises
81-
``NotImplementedError``. Phase 2 will add the space-time product kernel
82-
(Driscoll-Kraay) and lift the rejection.
70+
**Conley spatial-HAC (``vcov_type="conley"``) is supported via the
71+
block-decomposed panel sandwich (matches R ``conleyreg`` with
72+
``lag_cutoff > 0``).** Pass ``conley_coords=(lat_col, lon_col)``,
73+
``conley_cutoff_km=<float>``, and ``conley_lag_cutoff=<int>`` on the
74+
constructor; the ``time`` / ``unit`` arrays are auto-derived from the
75+
estimator's ``time`` and ``unit`` column-name arguments at fit-time.
76+
The sandwich sums within-period spatial pairs plus within-unit
77+
Bartlett serial pairs (lag=0 excluded to avoid double-counting); this
78+
is NOT a multiplicative product kernel. FWL composability: the
79+
within-transformed scores ``S = X_demeaned * residuals_demeaned`` form
80+
the same meat as the full-dummy-expansion design. The temporal kernel
81+
is hardcoded Bartlett regardless of ``conley_kernel`` (matches
82+
``conleyreg::time_dist``). Restrictions:
83+
``inference="wild_bootstrap"`` + Conley raises (incompatible inference
84+
modes); explicit ``cluster=...`` + Conley raises (combined spatial-
85+
cluster kernel deferred to a follow-up PR; auto-cluster on the Conley
86+
path is silently dropped); ``survey_design=`` + Conley raises (Phase 5
87+
follow-up).
8388
8489
Warning: TWFE can be biased with staggered treatment timing
8590
and heterogeneous treatment effects. Consider using

tests/test_conley_vcov.py

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,43 @@ def test_panel_time_nan_raises(self):
433433
lag_cutoff=1,
434434
)
435435

436+
def test_panel_unit_nan_float_raises(self):
437+
"""NaN unit IDs would silently drop those rows from the per-unit
438+
serial HAC sum at `np.unique(unit_arr) + mask_u = unit_arr == u_val`.
439+
Closes Codex P1.
440+
"""
441+
n = 4
442+
unit = np.array([1.0, 2.0, np.nan, 3.0])
443+
with pytest.raises(ValueError, match="conley_unit contains NaN"):
444+
_validate_conley_kwargs(
445+
coords=np.zeros((n, 2)),
446+
cutoff=100.0,
447+
metric="euclidean",
448+
kernel="bartlett",
449+
n=n,
450+
time=np.array([1.0, 2.0, 1.0, 2.0]),
451+
unit=unit,
452+
lag_cutoff=1,
453+
)
454+
455+
def test_panel_unit_pd_na_object_raises(self):
456+
"""Object-dtype unit IDs (mixed string + pd.NA) must also raise."""
457+
import pandas as pd
458+
459+
n = 4
460+
unit = np.array(["A", "B", pd.NA, "C"], dtype=object)
461+
with pytest.raises(ValueError, match="conley_unit contains NaN"):
462+
_validate_conley_kwargs(
463+
coords=np.zeros((n, 2)),
464+
cutoff=100.0,
465+
metric="euclidean",
466+
kernel="bartlett",
467+
n=n,
468+
time=np.array([1.0, 2.0, 1.0, 2.0]),
469+
unit=unit,
470+
lag_cutoff=1,
471+
)
472+
436473

437474
# ---------------------------------------------------------------------------
438475
# TestConleyDirectHelper — _compute_conley_vcov correctness
@@ -1062,6 +1099,122 @@ def test_multi_period_did_conley_missing_lag_cutoff_raises(self):
10621099
reference_period=1,
10631100
)
10641101

1102+
def test_multi_period_did_conley_with_survey_design_raises(self):
1103+
"""MPD + vcov_type='conley' + survey_design raises NotImplementedError.
1104+
1105+
Closes Codex P0: previously, MPD passed return_vcov=False to solve_ols
1106+
when _use_survey_vcov=True, bypassing the conley + weights guard, and
1107+
then overwrote vcov with compute_survey_vcov — silently returning
1108+
survey SEs under a Conley request.
1109+
"""
1110+
import pandas as pd
1111+
1112+
from diff_diff import MultiPeriodDiD
1113+
from diff_diff.survey import SurveyDesign
1114+
1115+
rng = np.random.default_rng(seed=29)
1116+
n_units = 24
1117+
rows = []
1118+
for u in range(n_units):
1119+
lat = rng.uniform(-30, 30)
1120+
lon = rng.uniform(-100, 100)
1121+
for t in range(3):
1122+
rows.append(
1123+
{
1124+
"unit": u,
1125+
"time": t,
1126+
"y": rng.normal(),
1127+
"treated": int(u < 12),
1128+
"lat": lat,
1129+
"lon": lon,
1130+
"weight": 1.0 + 0.1 * rng.random(),
1131+
"stratum": u % 4,
1132+
"psu": u // 6,
1133+
}
1134+
)
1135+
df_mp = pd.DataFrame(rows)
1136+
# Pure pweight (no PSU / strata) — would route through analytical conley
1137+
# path; the guard must fire before solve_ols.
1138+
sd_tsl = SurveyDesign(weights="weight", weight_type="pweight")
1139+
with pytest.raises(NotImplementedError, match="survey_design"):
1140+
MultiPeriodDiD(
1141+
vcov_type="conley",
1142+
conley_coords=("lat", "lon"),
1143+
conley_cutoff_km=2000.0,
1144+
conley_lag_cutoff=1,
1145+
).fit(
1146+
df_mp,
1147+
outcome="y",
1148+
treatment="treated",
1149+
time="time",
1150+
post_periods=[2],
1151+
unit="unit",
1152+
reference_period=1,
1153+
survey_design=sd_tsl,
1154+
)
1155+
# Stratified PSU survey design — would route through Taylor TSL path
1156+
# and was the canonical bypass case the codex reviewer flagged.
1157+
sd_psu = SurveyDesign(
1158+
weights="weight",
1159+
strata="stratum",
1160+
psu="psu",
1161+
weight_type="pweight",
1162+
nest=True,
1163+
)
1164+
with pytest.raises(NotImplementedError, match="survey_design"):
1165+
MultiPeriodDiD(
1166+
vcov_type="conley",
1167+
conley_coords=("lat", "lon"),
1168+
conley_cutoff_km=2000.0,
1169+
conley_lag_cutoff=1,
1170+
).fit(
1171+
df_mp,
1172+
outcome="y",
1173+
treatment="treated",
1174+
time="time",
1175+
post_periods=[2],
1176+
unit="unit",
1177+
reference_period=1,
1178+
survey_design=sd_psu,
1179+
)
1180+
1181+
def test_multi_period_did_conley_missing_coords_raises(self):
1182+
"""MPD + vcov_type='conley' without conley_coords raises a clean
1183+
ValueError instead of a raw TypeError on `self.conley_coords[0]`.
1184+
Closes Codex P2 #1.
1185+
"""
1186+
import pandas as pd
1187+
1188+
from diff_diff import MultiPeriodDiD
1189+
1190+
rng = np.random.default_rng(seed=31)
1191+
n_units = 10
1192+
rows = []
1193+
for u in range(n_units):
1194+
for t in range(2):
1195+
rows.append(
1196+
{
1197+
"unit": u,
1198+
"time": t,
1199+
"y": rng.normal(),
1200+
"treated": int(u < 5),
1201+
}
1202+
)
1203+
df_mp = pd.DataFrame(rows)
1204+
with pytest.raises(ValueError, match="conley_coords.*conley_cutoff_km"):
1205+
MultiPeriodDiD(
1206+
vcov_type="conley",
1207+
conley_lag_cutoff=1,
1208+
).fit(
1209+
df_mp,
1210+
outcome="y",
1211+
treatment="treated",
1212+
time="time",
1213+
post_periods=[1],
1214+
unit="unit",
1215+
reference_period=0,
1216+
)
1217+
10651218

10661219
class TestConleyTWFE:
10671220
"""TwoWayFixedEffects + vcov_type='conley' uses the Phase 2 block-decomposed

0 commit comments

Comments
 (0)