Skip to content

Commit 0bcda79

Browse files
igerberclaude
andcommitted
Address PR #365 R7 P3: gate _loo_unit_ids on unit-granularity; refresh harness docstring
P3 (Maintainability — survey jackknife still populated _loo_unit_ids): ``fit()`` was unconditionally setting ``_loo_unit_ids`` / ``_loo_roles`` on every jackknife fit, including full-design survey fits where the underlying replicates are PSU-level and ``get_loo_effects_df()`` now raises ``NotImplementedError``. Internal / canned guidance keyed off ``_loo_unit_ids is not None`` as the availability check (e.g., ``practitioner.py``) would still call the accessor on a survey fit and hit the new raise. Fix: only populate ``_loo_unit_ids`` / ``_loo_roles`` when ``_loo_granularity == "unit"``; leave them ``None`` on the PSU path so ``_loo_unit_ids is not None`` correctly reports availability. ``_loo_granularity`` is the authoritative accessor gate; the legacy ``_loo_unit_ids`` sentinel now agrees with it. P3 (Documentation — harness docstring stale): ``coverage_sdid.py::_fit_one`` docstring said "fit() routes [survey designs] through the bootstrap survey path (PR #352) when method=='bootstrap'" — stale after the placebo + jackknife full- design paths landed. Rewrote to describe the three method-specific survey variance paths (weighted-FW + Rao-Wu bootstrap; stratified- permutation + weighted-FW placebo; PSU-LOO + stratum-aggregation jackknife) and mention the Case B-D ValueError failure modes alongside NotImplementedError. Verification: 94 passed (no behavior change on the gating fix — it's a state-gating tightening, not a correctness change). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6d27a57 commit 0bcda79

2 files changed

Lines changed: 20 additions & 6 deletions

File tree

benchmarks/python/coverage_sdid.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -297,10 +297,14 @@ def _fit_one(
297297
"""Fit SDID and return (att, se, p_value); (None, None, None) on failure.
298298
299299
For survey DGPs the harness passes a SurveyDesign via ``survey_design``;
300-
fit() routes it through the bootstrap survey path (PR #352) when
301-
method=='bootstrap'. The DGP's ``survey_design_factory`` declares which
302-
methods are supported, so the caller skips unsupported methods entirely
303-
rather than catching the resulting NotImplementedError here.
300+
``fit()`` routes strata/PSU/FPC designs through the method-specific
301+
survey variance path — bootstrap (PR #355 weighted-FW + Rao-Wu),
302+
placebo (stratified permutation + weighted-FW), or jackknife (PSU-
303+
level LOO with stratum aggregation). The DGP's
304+
``survey_design_factory`` declares which methods are supported on
305+
that specific DGP, so the caller skips unsupported methods entirely
306+
rather than catching the resulting NotImplementedError / Case B-D
307+
ValueError here.
304308
"""
305309
try:
306310
with warnings.catch_warnings():

diff_diff/synthetic_did.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,8 +1128,6 @@ def fit( # type: ignore[override]
11281128
treated_post_trajectory=treated_post_trajectory,
11291129
time_weights_array=time_weights,
11301130
)
1131-
self.results_._loo_unit_ids = loo_unit_ids
1132-
self.results_._loo_roles = loo_roles
11331131
# Explicit LOO granularity flag for ``get_loo_effects_df``. The
11341132
# non-survey and pweight-only jackknife paths run unit-level LOO
11351133
# (one estimate per unit, matching ``control_unit_ids +
@@ -1143,6 +1141,18 @@ def fit( # type: ignore[override]
11431141
)
11441142
else:
11451143
self.results_._loo_granularity = None
1144+
# Only populate unit-level LOO bookkeeping when the granularity
1145+
# is actually unit-level (R7 P3). Leaving ``_loo_unit_ids`` /
1146+
# ``_loo_roles`` populated on the PSU path would cause
1147+
# ``_loo_unit_ids is not None`` availability checks (e.g.,
1148+
# ``practitioner.py`` / canned guidance) to call
1149+
# ``get_loo_effects_df()`` and hit ``NotImplementedError``.
1150+
if self.results_._loo_granularity == "unit":
1151+
self.results_._loo_unit_ids = loo_unit_ids
1152+
self.results_._loo_roles = loo_roles
1153+
else:
1154+
self.results_._loo_unit_ids = None
1155+
self.results_._loo_roles = None
11461156
self.results_._fit_snapshot = fit_snapshot
11471157

11481158
self._unit_weights = unit_weights

0 commit comments

Comments
 (0)