diff --git a/CHANGELOG.md b/CHANGELOG.md index 0692f621..121fafb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **`n_bootstrap` type guards aligned onto `utils.validate_n_bootstrap`** + for the estimators the M-081 sweep deliberately left out — + `HeterogeneousAdoptionDiD`, `ChaisemartinDHaultfoeuille`, `TROP`, + `SyntheticDiD` (jackknife lane included), plus the two HAD pretest + helpers (`stute_test`, `stute_joint_pretest`): previously-accepted + type-blind values now raise the shared message — `True` (silently ran as + 1 replicate on HAD/dCDH), floats like `2.5` (passed the `>= 2` floors), + and bool/negative under SyntheticDiD's jackknife floor exemption. The + estimator-specific floors are unchanged and keep their own messages for + non-negative sub-floor integers (TROP/SDiD `n_bootstrap=1`, HAD `0`); + NEGATIVE values now surface the shared validator's message instead of + each estimator's former wording. +- **`honest_did` inference-df resolution consolidated** onto the shared + `aggregation.resolve_inference_df()` (three duplicated precedence blocks + removed). Same precedence; `HonestDiDResults.df_survey` is now + float-typed (`31.0` where it was `31`), and a fractional `df_inference` + is preserved instead of truncated. + ### Fixed - **Per-row event-study df provenance (M-092 completion)** for the four remaining holes — `EfficientDiD`, `ImputationDiD`, `ContinuousDiD`, and diff --git a/TODO.md b/TODO.md index 0fb4b50e..00502909 100644 --- a/TODO.md +++ b/TODO.md @@ -54,7 +54,6 @@ Related tracking surfaces: | `WooldridgeDiD` DROPS the observations of a cohort with no supported pre-period before `g - anticipation` ([M-123]) rather than identifying it. Excluding the rows is correct given `g-1` normalization -- leaving them in silently loads the cohort's effect onto the time FE -- but dropping a cohort a user supplied is a lossy last resort. **Route (b) is now SETTLED NEGATIVELY and is not the answer:** the paper's no-never-treated last-cohort normalization shipped (W2025 Sec 5.4, per-period comparison support), and it does NOT identify these cohorts -- `wooldridge-2025-review.md:477` is explicit that in the final period the last cohort's ATT is unidentified, and the implementation still excludes any cohort whose reference is `None`. **Route (a) remains open:** an explicit user-supplied reference period per cohort -- W2025 Section 6.1 says any pre-treatment period may serve and the pre-trend `t`-test is invariant to the choice, so a cohort with ANY supported pre-period is a candidate even when `g-1` is missing. If route (a) also fails to identify the cohort, convert this row into a REGISTRY Note recording exclusion as the deliberate final answer. | `diff_diff/wooldridge.py`, `docs/methodology/REGISTRY.md` | #724 | Heavy | Medium | | `WooldridgeDiD` REFUSES a panel whose units split into disconnected support groups within a cohort, rather than estimating what IS identified. The connectivity guard (REGISTRY *within-cohort support connectivity*) correctly detects that a closed component's cells are collinear with the unit FE — previously QR dropped one silently and the overall ATT averaged an incomplete set (issue #724's failure mode via unit support). **Refusing is the safe answer, not the complete one.** The connected component containing the reference is still fully identified, so the estimable resolution is either (a) estimate the connected component and report the disconnected units as excluded, with the estimand restated (a sub-population of units, so it needs a REGISTRY definition and interacts with the survey-domain row above), or (b) per-component references, if a component with its own pre-period can carry its own normalization — needs a methodology decision, since components then are not comparable on one baseline. Gate with the split-support fixture in `TestWithinCohortSupportConnectivity`. | `diff_diff/wooldridge.py` | #724-codex-R7 | Heavy | Medium | | `WooldridgeDiD` fully resolves the `SurveyDesign` TWICE on every supported survey fit. The pre-exclusion validation pass (added so invalid metadata cannot hide in rows that cohort exclusion deletes) calls `survey_design.resolve(sample)`, and each fitter then calls `_resolve_survey_for_wooldridge` -> `_resolve_survey_for_fit` on the same frame, repeating weight normalization, strata/PSU/FPC validation and design-array construction. Any fit that REACHES the second resolve has an unchanged sample (survey + unidentified-cohort exclusion raises first), so the first result is reusable: capture the `_resolve_survey_for_fit` 4-tuple early and thread it into the three fitters as an optional `pre_resolved`. **Caveat that makes this non-trivial:** `sample = sample.reset_index(drop=True)` runs BETWEEN the two calls, so the reused object must be verified index-independent (resolution extracts positional numpy arrays, but `_inject_cluster_as_psu` and the metadata recompute need checking), and the early call must stop suppressing warnings or the user loses the weight-normalization notice. Gate with a survey fit asserting one normalization warning and byte-identical SEs. | `diff_diff/wooldridge.py` | #724-codex-R10 | Mid | Low | -| Consolidate the inference-df precedence duplicated across `honest_did.py` (3 copies at ~L655/L836/L1004) onto the shared `resolve_inference_df()` helper added in `diff_diff/aggregation.py`. The copies are correct today; the risk is drift if the survey/replicate precedence changes in one place only. (Adjacent but distinct from PR C's `utils.resolve_tail_df`: that is the FIT-TIME `df_convention` fallback resolver, this is a post-fit results READER.) | `diff_diff/honest_did.py` | #726 | Quick | Low | | `ContinuousDiD` CGBS-2024 remaining extensions (earlier phases — `covariates=` reg/dr, `treatment_type="discrete"`, single-cohort `control_group="lowest_dose"` with estimand `ATT(d)−ATT(d_L)` — are already supported; see REGISTRY Note #7). Remaining (all deferred `NotImplementedError`, documented): `estimation_method="ipw"` on the dose curve (scalar-adjustment / degenerate); `covariates=` × `survey_design=` (weighted OR + weighted nuisance IF); multi-cohort **heterogeneous-support** discrete aggregation (support-aware: average each dose only over the cohorts that observe it); **multi-cohort `lowest_dose`** (within-cohort `d_L` reference + support-aware cross-cohort aggregation); and **`covariates=` × `lowest_dose`** (conditional-PT-relative-to-`d_L` estimand). Single-cohort / 2-period / shared-support multi-cohort are supported. | `continuous_did.py` | CGBS-2024 | Heavy | Low | | `WooldridgeDiD` does not apply the W2025 Sec 5.4 `D_{G_max} x X` covariate normalization, and three sibling covariate rank deficiencies are pre-existing. Measured with the period range pinned and only the never-treated units toggled: (1) time-invariant `exovar` is absorbed by the unit FE, 4 of 26 columns, IDENTICALLY with and without never-treated units; (2) `xgvar`'s cell x covariate block, 19 of 41, identical on both panels; (3) `xtvar` under `demean_covariates=False` does exhibit the `sum_g D_g x = x` dependency that the default demeaning removes; (4) the newly-reachable case -- time-VARYING data passed through `exovar`, which its own docstring reserves for time-invariant covariates -- where the paper's `dT_i` rule would give a deterministic `D_{G_max} x X` drop instead of QR's arbitrary pick (coefficients unaffected, `1.35e-14`; `rank_deficient_action="error"` raises). REGISTRY's narrowed Sec 5.4 note cross-references this row. **Trap for whoever takes it:** `xtvar` under the DEFAULT `demean_covariates=True` is FULL RANK -- the raw block carries demeaned values while `D_g x X` carries raw ones -- and forcing the drop there moves `overall_att` 1.11903 -> 1.46269. Pinned as-is by `TestComparisonSupportFiltering::test_cells_derived_groups_did_not_leak_into_the_design`. | `diff_diff/wooldridge.py` | #729-followup | Heavy | Medium | | `WooldridgeDiD.n_control_units` counts never-treated UNITS on `control_group="never_treated"` regardless of method, but on the nonlinear paths (`logit`/`poisson`) treated units' pre-treatment rows ARE the identifying comparison -- only the OLS path absorbs them into their own cells. So the reported count under-states the comparison pool exactly where the REGISTRY control-pool asymmetry note applies. Widen to `not_yet_treated or (never_treated and method != "ols")`, or document the count as never-treated-units-by-definition. Behavior is PRE-EXISTING; documented for now in the REGISTRY control-pool Note rather than changed, because widening moves a public results field and wants its own ledger row and test matrix. | `diff_diff/wooldridge.py` | #729-followup | Mid | Low | @@ -82,7 +81,6 @@ generic sparse-FE, QR+SVD rank-detection redundancy, `check_finite` bypass — m | Replicate Chang (2020) §4's own RCS simulation DGPs (pp. 17-21, "fully specified" per the paper review) as recovery/coverage fixtures for the `DMLDiD(panel=False)` lane — the shipped tests use a library-authored RCS design (documented in the REGISTRY checklist caveat); needs the paper PDF to extract the parameterization | `tests/test_methodology_dml_did.py`, `docs/methodology/papers/chang-2020-review.md` | DML PR-B2 | Mid | Low | | Optional scheduled end-to-end execution gate for the MMM tutorials (29/30): a cron-only workflow (or extension of `mmm-interop.yml`) that executes both notebooks in isolated exact-pin environments, so a stale/invalid committed posterior cannot stay green indefinitely - today the hybrid posture (deliberate: notebooks execute locally with committed outputs; CI smoke-tests the exporters without sampling; drift tests pin source + committed-output needles) leaves the MCMC claims un-re-executed in CI | `.github/workflows/mmm-interop.yml`, `docs/tutorials/29_mmm_calibration_pymc.ipynb`, `docs/tutorials/30_mmm_calibration_meridian.ipynb` | mmm-interop | Mid | Low | | Committed `fixest::feols` event-study golden for TWFE `event_study=True` (within + pooled specs, unbalanced + covariate panels, matched CR1 cluster convention, per-period effects + vcov block) - the in-suite gates are shared-core cross-checks (TWFE-within == MPD-absorb, pooled == MPD bit-exact), so a defect common to the shared core would pass; the live-R harness (`benchmarks/R/benchmark_multiperiod.R`, `feols(y ~ treated * time_f \| unit)`) validated the within design in `docs/benchmarks.rst` but is not a committed regression test - follow the `fixest_did_twfe_golden.json` committed-golden pattern (pytest.skip when absent) | `tests/test_fixest_did_twfe_parity.py`, `benchmarks/R/` | 3(a) R2 | Mid | Medium | -| Type-blind `n_bootstrap` acceptance in already-validated estimators - HAD bool (`isinstance(..., int)` passes `True`, runs as 1 replicate), dCDH bool+float (its bare `< 0` check passes both `True` and `2.5`), TROP float (`2.5` passes the `>= 2` floor), SyntheticDiD float under all three variance methods + bool/negative under jackknife (its floor check is skipped there) - align these local checks with the `utils.validate_n_bootstrap` type guard (M-081 kept them out of the sweep: it scoped to previously-UNvalidated estimators only) | `diff_diff/had.py`, `diff_diff/chaisemartin_dhaultfoeuille.py`, `diff_diff/trop.py`, `diff_diff/synthetic_did.py` | 2(d) PR-B | Quick | Low | | Evaluate adding the `BaseEstimator` param surface (get_params/set_params) to the exported classes that never had it - `PowerAnalysis`, `LinearRegression`, `BusinessReport`, `DiagnosticReport`, `TWFEWeightsResult` (a NEW public surface, deliberately out of the 2(c)-i pure-refactor scope; `LinearRegression` is the one `fit`-bearing class excluded from the contract suite's roster-completeness test). | `diff_diff/linalg.py`, `diff_diff/power.py` | mixin PR | Mid | Low | | Tighten the mypy suppressions that back the enforced-zero posture: burn down `prep_dgp`'s per-module `[index]` override (needs a None-vs-array restructure that preserves the seeded RNG stream), and evaluate re-enabling the globally disabled codes (`arg-type`, `return-value`, `var-annotated`, `assignment`) one at a time — `assignment` alone hid several real annotation drifts found during the 2026-07 triage. | `pyproject.toml` `[tool.mypy]`, `diff_diff/prep_dgp.py` | lint-CI | Mid | Low | | Tracking-file contract guard test: reject NEW active deferred-work pointers at `TODO.md` (deferred rows live in `DEFERRED.md`; allowlist for historical/past-tense prose and actionable-row pointers) and assert rows cross-linking a `docs/v4-deprecations.yaml` `M-xxx` id don't restate ledger status. Origin: tracking-split local review R2. | `tests/`, `TODO.md`, `DEFERRED.md` | tracking-split | Quick | Low | diff --git a/diff_diff/chaisemartin_dhaultfoeuille.py b/diff_diff/chaisemartin_dhaultfoeuille.py index 1a24a200..c6b08bd4 100644 --- a/diff_diff/chaisemartin_dhaultfoeuille.py +++ b/diff_diff/chaisemartin_dhaultfoeuille.py @@ -51,7 +51,7 @@ DCDHBootstrapResults, ) from diff_diff.linalg import solve_ols -from diff_diff.utils import safe_inference +from diff_diff.utils import safe_inference, validate_n_bootstrap __all__ = [ "ChaisemartinDHaultfoeuille", @@ -762,8 +762,8 @@ def __init__( ) if not 0.0 < alpha < 1.0: raise ValueError(f"alpha must be in (0, 1), got {alpha}") - if n_bootstrap < 0: - raise ValueError(f"n_bootstrap must be non-negative, got {n_bootstrap}") + # Shared type guard (rejects bool/float, floor stays 0). + validate_n_bootstrap(n_bootstrap) if by_path is not None: if isinstance(by_path, bool) or not isinstance(by_path, int): raise ValueError( @@ -836,8 +836,9 @@ def _validate_invariants(self) -> None: ) if not 0.0 < self.alpha < 1.0: raise ValueError(f"alpha must be in (0, 1), got {self.alpha}") - if self.n_bootstrap < 0: - raise ValueError(f"n_bootstrap must be non-negative, got {self.n_bootstrap}") + # Shared type guard (kept aligned with __init__ so the two sites + # cannot drift). + validate_n_bootstrap(self.n_bootstrap) if self.by_path is not None: if isinstance(self.by_path, bool) or not isinstance(self.by_path, int): raise ValueError( diff --git a/diff_diff/had.py b/diff_diff/had.py index 91bb0d0e..0a7fd3bb 100644 --- a/diff_diff/had.py +++ b/diff_diff/had.py @@ -95,7 +95,7 @@ SurveyMetadata, compute_survey_metadata, ) -from diff_diff.utils import safe_inference +from diff_diff.utils import safe_inference, validate_n_bootstrap __all__ = [ "HeterogeneousAdoptionDiD", @@ -3045,9 +3045,10 @@ def _validate_constructor_args(self) -> None: f"or None." ) # Phase 4.5 B: n_bootstrap must be a positive int; seed must be - # None or a nonneg int (numpy default_rng contract). - if not isinstance(self.n_bootstrap, (int, np.integer)): - raise ValueError(f"n_bootstrap must be an int; got {type(self.n_bootstrap).__name__}.") + # None or a nonneg int (numpy default_rng contract). The shared + # type guard rejects bool (previously ran as 1 replicate) and + # floats; the >= 1 floor stays HAD-specific. + validate_n_bootstrap(self.n_bootstrap) if int(self.n_bootstrap) < 1: raise ValueError(f"n_bootstrap must be >= 1; got {self.n_bootstrap!r}.") if self.seed is not None: diff --git a/diff_diff/had_pretests.py b/diff_diff/had_pretests.py index b815e523..3c606f89 100644 --- a/diff_diff/had_pretests.py +++ b/diff_diff/had_pretests.py @@ -96,7 +96,7 @@ SurveyDesign, make_pweight_design, ) -from diff_diff.utils import _generate_mammen_weights +from diff_diff.utils import _generate_mammen_weights, validate_n_bootstrap __all__ = [ "QUGTestResults", @@ -1676,6 +1676,9 @@ def stute_test( """ if not (0.0 < alpha < 1.0): raise ValueError(f"alpha must satisfy 0 < alpha < 1, got {alpha}.") + # Shared type guard first (a float like 999.5 previously passed the + # floor and reached np.empty(n_bootstrap)). + validate_n_bootstrap(n_bootstrap) if n_bootstrap < _MIN_N_BOOTSTRAP: raise ValueError( f"n_bootstrap must be >= {_MIN_N_BOOTSTRAP} (below this the " @@ -2880,6 +2883,8 @@ def stute_joint_pretest( # Note: the actual `warn + return` happens below after horizon # labels are validated and collision-checked, so the NaN result # carries full per-horizon diagnostic keys. + # Shared type guard first (float floors through otherwise). + validate_n_bootstrap(n_bootstrap) if n_bootstrap < _MIN_N_BOOTSTRAP: raise ValueError(f"n_bootstrap must be >= {_MIN_N_BOOTSTRAP}; got " f"{n_bootstrap}.") if not isinstance(alpha, (int, float)) or not (0 < float(alpha) < 1): diff --git a/diff_diff/honest_did.py b/diff_diff/honest_did.py index 7054bc48..3466856e 100644 --- a/diff_diff/honest_did.py +++ b/diff_diff/honest_did.py @@ -26,6 +26,7 @@ from scipy import optimize from diff_diff._base import BaseEstimator +from diff_diff.aggregation import resolve_inference_df from diff_diff.results import ( MultiPeriodDiDResults, ) @@ -1273,26 +1274,16 @@ def _extract_event_study_params( # Fallback: diagonal from SEs sigma = np.diag(np.array(ses) ** 2) - # Extract inference df. Prefer ``survey_metadata.df_survey`` (the - # actual CS-internal df, which may have been tightened post-resolve - # for replicate designs) over the dedicated ``df_inference`` field. - # ``df_inference`` is the FALLBACK carrier for bare-``cluster=`` CS - # fits where ``survey_metadata`` is intentionally None (preserving - # the survey/non-survey contract for ``DiagnosticReport`` / - # ``summary()``). Reading ``df_inference`` first would silently - # overstate the denominator df on panel survey fits whose df was - # tightened during aggregation. Replicate designs with undefined - # df → sentinel 0. - df_survey = None - if hasattr(results, "survey_metadata") and results.survey_metadata is not None: - sm = results.survey_metadata - df_survey = getattr(sm, "df_survey", None) - if df_survey is None and getattr(sm, "replicate_method", None) is not None: - df_survey = 0 # undefined replicate df → NaN inference - if df_survey is None: - df_inference = getattr(results, "df_inference", None) - if df_inference is not None: - df_survey = int(df_inference) + # Extract inference df via the shared precedence helper: prefer + # ``survey_metadata.df_survey`` (the actual internal df, which may + # have been tightened post-resolve for replicate designs; undefined + # replicate df -> the 0.0 sentinel) over the ``df_inference`` + # FALLBACK carrier (bare-``cluster=`` fits where + # ``survey_metadata`` is intentionally None). Reading + # ``df_inference`` first would silently overstate the denominator + # df on panel survey fits whose df was tightened during + # aggregation. + df_survey = resolve_inference_df(results) # Return the ESTIMATED label lists - the ones beta_hat/sigma were # built from - not the declared results.pre_periods/post_periods @@ -1521,26 +1512,11 @@ def _extract_event_study_params( "or use balance_e to restrict to a balanced subset." ) - # Extract inference df. Prefer ``survey_metadata.df_survey`` - # (the actual CS-internal df, which may have been tightened - # post-resolve for replicate designs) over the dedicated - # ``df_inference`` field. ``df_inference`` is the FALLBACK - # carrier for bare-``cluster=`` CS fits where - # ``survey_metadata`` is intentionally None (preserving the - # survey/non-survey contract for ``DiagnosticReport`` / - # ``summary()``). Mirrors the MPD branch preference order - # at ``_extract_event_study_params`` so all branches agree. - # Replicate designs with undefined df → sentinel 0. - df_survey = None - if hasattr(results, "survey_metadata") and results.survey_metadata is not None: - sm = results.survey_metadata - df_survey = getattr(sm, "df_survey", None) - if df_survey is None and getattr(sm, "replicate_method", None) is not None: - df_survey = 0 # undefined replicate df → NaN inference - if df_survey is None: - df_inference = getattr(results, "df_inference", None) - if df_inference is not None: - df_survey = int(df_inference) + # Extract inference df via the shared precedence helper + # (``survey_metadata.df_survey`` -> replicate 0.0 sentinel + # -> ``df_inference`` fallback carrier) - the same order the + # MPD branch uses, so all branches agree. + df_survey = resolve_inference_df(results) return ( beta_hat, @@ -1693,26 +1669,11 @@ def _largest_consecutive_block(times, boundary_val): beta_hat = np.array(effects) sigma = np.diag(np.array(ses) ** 2) - # Extract inference df. Prefer ``survey_metadata.df_survey`` - # (the actual CS-internal df, which may have been tightened - # post-resolve for replicate designs) over the dedicated - # ``df_inference`` field. ``df_inference`` is the FALLBACK - # carrier for bare-``cluster=`` CS fits where - # ``survey_metadata`` is intentionally None (preserving the - # survey/non-survey contract for ``DiagnosticReport`` / - # ``summary()``). Mirrors the MPD branch preference order - # at ``_extract_event_study_params`` so all branches agree. - # Replicate designs with undefined df → sentinel 0. - df_survey = None - if hasattr(results, "survey_metadata") and results.survey_metadata is not None: - sm = results.survey_metadata - df_survey = getattr(sm, "df_survey", None) - if df_survey is None and getattr(sm, "replicate_method", None) is not None: - df_survey = 0 # undefined replicate df → NaN inference - if df_survey is None: - df_inference = getattr(results, "df_inference", None) - if df_inference is not None: - df_survey = int(df_inference) + # Extract inference df via the shared precedence helper + # (``survey_metadata.df_survey`` -> replicate 0.0 sentinel + # -> ``df_inference`` fallback carrier) - the same order the + # MPD branch uses, so all branches agree. + df_survey = resolve_inference_df(results) return ( beta_hat, diff --git a/diff_diff/results_base.py b/diff_diff/results_base.py index 18280399..7b83f51d 100644 --- a/diff_diff/results_base.py +++ b/diff_diff/results_base.py @@ -828,15 +828,15 @@ def _validate_vcov_subblock( def _resolve_scalar_df_survey(results: Any) -> Optional[float]: """Resolve the producer's SCALAR inference df for container provenance. - Mirrors the fit-time consumers' preference order (honest_did): - ``survey_metadata.df_survey`` where present, with a replicate design - whose df is undefined mapping to the ``0.0`` sentinel (fails closed to - NaN critical values downstream); else the bare-``cluster=`` - ``df_inference`` carrier; else None. Deliberate local sibling of - ``aggregation.resolve_inference_df``: this module cannot import it - (aggregation.py imports results_base - the dependency is one-way); - folding the copies together is tracked in TODO.md (df-resolution / - adapter-naming consolidation row). + Mirrors ``aggregation.resolve_inference_df`` (the shared preference + order honest_did now consumes directly): ``survey_metadata.df_survey`` + where present, with a replicate design whose df is undefined mapping to + the ``0.0`` sentinel (fails closed to NaN critical values downstream); + else the bare-``cluster=`` ``df_inference`` carrier; else None. + Deliberate local sibling: this module cannot import it (aggregation.py + imports results_base - the dependency is one-way); folding the two + together is tracked in TODO.md (df-resolution / adapter-naming + consolidation row). """ sm = getattr(results, "survey_metadata", None) if sm is not None: diff --git a/diff_diff/synthetic_did.py b/diff_diff/synthetic_did.py index 3f2df374..56146c7e 100644 --- a/diff_diff/synthetic_did.py +++ b/diff_diff/synthetic_did.py @@ -23,6 +23,7 @@ compute_time_weights_survey, safe_inference, validate_binary, + validate_n_bootstrap, ) @@ -266,6 +267,10 @@ def _validate_config(self) -> None: f"variance_method must be one of {self._VALID_VARIANCE_METHODS}, " f"got '{self.variance_method}'" ) + # Shared type guard UNCONDITIONALLY (bool/negative previously + # slipped through under jackknife, whose floor exemption below is + # about the >= 2 minimum only). + validate_n_bootstrap(self.n_bootstrap) if self.n_bootstrap < 2 and self.variance_method != "jackknife": raise ValueError( f"n_bootstrap must be >= 2 (got {self.n_bootstrap}). At least 2 " diff --git a/diff_diff/trop.py b/diff_diff/trop.py index d085baf2..0eda8524 100644 --- a/diff_diff/trop.py +++ b/diff_diff/trop.py @@ -42,7 +42,7 @@ TROPResults, _PrecomputedStructures, ) -from diff_diff.utils import safe_inference, warn_if_not_converged +from diff_diff.utils import safe_inference, validate_n_bootstrap, warn_if_not_converged class TROP(TROPLocalMixin, TROPGlobalMixin, BaseEstimator): @@ -182,6 +182,9 @@ def __init__( self.lambda_unit_grid = lambda_unit_grid or [0.0, 0.1, 0.5, 1.0, 2.0, 5.0] self.lambda_nn_grid = lambda_nn_grid or [0.0, 0.01, 0.1, 1.0, 10.0] + # Shared type guard first (rejects bool/float), then the + # TROP-specific floor. + validate_n_bootstrap(n_bootstrap) if n_bootstrap < 2: raise ValueError( "n_bootstrap must be >= 2 for TROP (bootstrap variance " diff --git a/docs/v4-deprecations.yaml b/docs/v4-deprecations.yaml index 8741485d..1a33518b 100644 --- a/docs/v4-deprecations.yaml +++ b/docs/v4-deprecations.yaml @@ -1037,9 +1037,9 @@ rows: removed_in: null status: done phase: 2 - code_refs: [diff_diff/estimators.py, diff_diff/synthetic_did.py, diff_diff/changes_in_changes.py, diff_diff/utils.py, diff_diff/staggered.py, diff_diff/sun_abraham.py, diff_diff/efficient_did.py, diff_diff/imputation.py, diff_diff/two_stage.py, diff_diff/wooldridge.py, diff_diff/continuous_did.py, diff_diff/staggered_triple_diff.py] + code_refs: [diff_diff/estimators.py, diff_diff/synthetic_did.py, diff_diff/changes_in_changes.py, diff_diff/utils.py, diff_diff/staggered.py, diff_diff/sun_abraham.py, diff_diff/efficient_did.py, diff_diff/imputation.py, diff_diff/two_stage.py, diff_diff/wooldridge.py, diff_diff/continuous_did.py, diff_diff/staggered_triple_diff.py, diff_diff/had.py, diff_diff/had_pretests.py, diff_diff/trop.py, diff_diff/chaisemartin_dhaultfoeuille.py] test_ref: tests/test_v4_inference_policy.py - notes: "Semantic unification only: n_bootstrap=0 = bootstrap off wherever an analytical path exists; per-estimator counts stay tuned (999 light / 200 compute-heavy). No numeric default changes. introduced_in gates the 3.9 cut (pattern of [M-091]/[M-092]): the unification is a Phase 2 obligation and must not slip past the bump unflipped. deprecated_in stays null deliberately - a flip version would arm the early-flip guard against the Phase 2 PR, which lands while __version__ is still 3.8.x. SHIPPED in 2(d) PR-B: a shared utils.validate_n_bootstrap (promoted verbatim from ChangesInChanges' local validator; non-negative int, accepts numpy integers, rejects bool/None/float/negative) is applied at __init__ across the previously-unvalidated roster - DifferenceInDifferences (MultiPeriodDiD/TwoWayFixedEffects inherit), CallawaySantAnna, SunAbraham, EfficientDiD, ImputationDiD, TwoStageDiD, WooldridgeDiD, ContinuousDiD, StaggeredTripleDifference - with CiC/QDiD re-pointed to the shared helper (byte-identical message). SCOPE of 0=off: 0 stays legal at construction and still means bootstrap off on every > 0-gated analytical lane (the eight zero-default sweep classes); under DiD/TWFE inference='wild_bootstrap', where 0 NEVER meant off (the routing consults only the selector - it ran WCR with zero draws and returned a wild-labeled all-NaN inference tuple), 0 and 1 now raise at fit per [M-096]. EXCEPTION: HeterogeneousAdoptionDiD has an analytical pointwise path but deliberately floors n_bootstrap >= 1 (its n_bootstrap powers ONLY the optional sup-t band, whose off-switch is fit(cband=False); n_bootstrap=0 would be a second, ambiguous off-switch) - the 0=off clause does not apply to HAD and no HAD behavior changed. Already-validated estimators (SyntheticDiD >= 2 unless jackknife, TROP >= 2, HAD >= 1, dCDH non-negative) keep their own checks; their type-blind holes are a TODO.md row, not this PR." + notes: "Semantic unification only: n_bootstrap=0 = bootstrap off wherever an analytical path exists; per-estimator counts stay tuned (999 light / 200 compute-heavy). No numeric default changes. introduced_in gates the 3.9 cut (pattern of [M-091]/[M-092]): the unification is a Phase 2 obligation and must not slip past the bump unflipped. deprecated_in stays null deliberately - a flip version would arm the early-flip guard against the Phase 2 PR, which lands while __version__ is still 3.8.x. SHIPPED in 2(d) PR-B: a shared utils.validate_n_bootstrap (promoted verbatim from ChangesInChanges' local validator; non-negative int, accepts numpy integers, rejects bool/None/float/negative) is applied at __init__ across the previously-unvalidated roster - DifferenceInDifferences (MultiPeriodDiD/TwoWayFixedEffects inherit), CallawaySantAnna, SunAbraham, EfficientDiD, ImputationDiD, TwoStageDiD, WooldridgeDiD, ContinuousDiD, StaggeredTripleDifference - with CiC/QDiD re-pointed to the shared helper (byte-identical message). SCOPE of 0=off: 0 stays legal at construction and still means bootstrap off on every > 0-gated analytical lane (the eight zero-default sweep classes); under DiD/TWFE inference='wild_bootstrap', where 0 NEVER meant off (the routing consults only the selector - it ran WCR with zero draws and returned a wild-labeled all-NaN inference tuple), 0 and 1 now raise at fit per [M-096]. EXCEPTION: HeterogeneousAdoptionDiD has an analytical pointwise path but deliberately floors n_bootstrap >= 1 (its n_bootstrap powers ONLY the optional sup-t band, whose off-switch is fit(cband=False); n_bootstrap=0 would be a second, ambiguous off-switch) - the 0=off clause does not apply to HAD (in the 2(d) PR-B cut no HAD behavior changed; the later type-guard alignment below made HAD reject bool n_bootstrap, previously run as 1 replicate). RESOLVED (post-3.11 type-guard alignment): the already-validated estimators (SyntheticDiD >= 2 unless jackknife, TROP >= 2, HAD >= 1, dCDH non-negative) formerly kept type-blind local checks (bool/float slipped through; the jackknife lane skipped even the floor); validate_n_bootstrap now runs BEFORE each retained floor at all of them, plus the two had_pretests.py floor sites - floors unchanged, with estimator-specific messages retained for non-negative sub-floor integers while NEGATIVE values now raise the shared validator's message; the former TODO.md row is closed." - id: M-096 kind: behavior group: policy-wild-bootstrap diff --git a/tests/test_had_pretests.py b/tests/test_had_pretests.py index 71d85908..90f04671 100644 --- a/tests/test_had_pretests.py +++ b/tests/test_had_pretests.py @@ -358,6 +358,14 @@ def test_n_bootstrap_below_99_raises(self): with pytest.raises(ValueError, match=r"n_bootstrap must be >= 99"): stute_test(d, dy, n_bootstrap=50) + @pytest.mark.parametrize("bad", [999.5, True, None], ids=repr) + def test_n_bootstrap_type_blind_values_raise(self, bad): + """Shared type guard: a float like 999.5 previously passed the >= 99 + floor and reached np.empty(n_bootstrap); bool/None likewise.""" + d, dy = _linear_dgp(G=50) + with pytest.raises(ValueError, match="n_bootstrap must be a non-negative integer"): + stute_test(d, dy, n_bootstrap=bad) + def test_exact_linear_returns_p1_not_nan(self): """P1 fix: exact linear fit (eps=0) must fail-to-reject with p=1, NOT return NaN. Assumption 8 holds exactly in this case.""" @@ -1347,6 +1355,28 @@ def test_k1_parity_with_single_horizon_stute(self): # one eta vector per iteration, single draws one - same shape); # the statistic being bit-identical is the critical check. + @pytest.mark.parametrize("bad", [999.5, True, None], ids=repr) + def test_joint_n_bootstrap_type_blind_values_raise(self, bad): + """Shared type guard on the joint helper's n_bootstrap (a float + previously passed the >= 99 floor and reached range(n_bootstrap)).""" + rng = np.random.default_rng(42) + G = 60 + d = rng.uniform(0.0, 1.0, G) + dy = 0.3 * d + rng.normal(0.0, 0.2, G) + x = np.column_stack([np.ones(G), d]) + beta = np.linalg.solve(x.T @ x, x.T @ dy) + fitted = x @ beta + with pytest.raises(ValueError, match="n_bootstrap must be a non-negative integer"): + stute_joint_pretest( + residuals_by_horizon={"only": dy - fitted}, + fitted_by_horizon={"only": fitted}, + doses=d, + design_matrix=x, + n_bootstrap=bad, + seed=123, + null_form="linearity", + ) + def test_linear_dgp_fails_to_reject(self): """Linear DGP across all horizons: joint test should not reject.""" rng = np.random.default_rng(2) diff --git a/tests/test_honest_did.py b/tests/test_honest_did.py index b8d71706..35d89587 100644 --- a/tests/test_honest_did.py +++ b/tests/test_honest_did.py @@ -1256,6 +1256,12 @@ def test_df_survey_extracted_from_cs_results(self): assert h_result.df_survey is not None assert h_result.df_survey > 0 assert h_result.survey_metadata is not None + # Single-source oracle: the CS branch resolves via the shared helper + # (aggregation.resolve_inference_df) - value-equal, float-typed. + from diff_diff.aggregation import resolve_inference_df + + assert h_result.df_survey == resolve_inference_df(cs_result) + assert isinstance(h_result.df_survey, float) def test_event_study_vcov_computed(self): """CallawaySantAnna event_study_vcov is computed and used by HonestDiD.""" @@ -1477,6 +1483,43 @@ def test_dcdh_extraction(self): assert all(t < 0 for t in pre_t) assert all(t > 0 for t in post_t) assert df_s is None # non-survey fixture → df_survey is None + # Single-source oracle: the dCDH branch resolves via the shared + # helper (both None on this non-survey fixture). + from diff_diff.aggregation import resolve_inference_df + + assert df_s == resolve_inference_df(results) + + def test_dcdh_survey_df_matches_shared_resolver(self): + """Survey dCDH fit: the extracted df equals the shared resolver's + value (float-typed), pinning the MPD/CS/dCDH single-source + consolidation on a branch with a FINITE df.""" + import warnings + + from diff_diff import ChaisemartinDHaultfoeuille, SurveyDesign + from diff_diff.aggregation import resolve_inference_df + from diff_diff.prep import generate_reversible_did_data + + data = generate_reversible_did_data(n_groups=40, n_periods=6, seed=42) + units = data["group"].unique() + wmap = {u: 1.0 + 0.3 * (i % 3) for i, u in enumerate(units)} + pmap = {u: i // 5 for i, u in enumerate(units)} + data = data.assign(weight=data["group"].map(wmap), psu=data["group"].map(pmap)) + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + res = ChaisemartinDHaultfoeuille(n_bootstrap=49, seed=7).fit( + data, + "outcome", + "group", + "period", + "treatment", + L_max=2, + survey_design=SurveyDesign(weights="weight", psu="psu"), + ) + *_, df_s = _extract_event_study_params(res) + expected = resolve_inference_df(res) + assert df_s == expected + if df_s is not None: + assert isinstance(df_s, float) def test_dcdh_no_placebos_raises(self): """dCDH results without placebos raise ValueError.""" diff --git a/tests/test_v4_inference_policy.py b/tests/test_v4_inference_policy.py index 4ddf54ec..82117f71 100644 --- a/tests/test_v4_inference_policy.py +++ b/tests/test_v4_inference_policy.py @@ -34,17 +34,21 @@ import diff_diff from diff_diff import ( + TROP, CallawaySantAnna, + ChaisemartinDHaultfoeuille, ChangesInChanges, ContinuousDiD, DifferenceInDifferences, DMLDiD, EfficientDiD, + HeterogeneousAdoptionDiD, ImputationDiD, MultiPeriodDiD, QDiD, StaggeredTripleDifference, SunAbraham, + SyntheticDiD, TripleDifference, TwoStageDiD, TwoWayFixedEffects, @@ -102,6 +106,9 @@ def _floor_msg(n: int) -> str: TripleDifference, ChangesInChanges, QDiD, + # Type-guard alignment (post-M-081 follow-up): dCDH's floor is 0, so it + # satisfies the full roster contract (zero legal at construction). + ChaisemartinDHaultfoeuille, ] SELECTOR_CLASSES = [DifferenceInDifferences, MultiPeriodDiD, TwoWayFixedEffects] @@ -439,3 +446,52 @@ def test_inference_exposed_by_exactly_the_wcr_roster(self): "bootstrap IS its inference method must keep n_bootstrap as " "documented domain vocabulary instead (v4-design section 7)." ) + + +# =========================================================================== +# Type-guard alignment for the floor-carrying estimators (post-M-081 +# follow-up): TROP (>= 2), SyntheticDiD (>= 2 unless jackknife), and HAD +# (>= 1) keep their floors, but the shared type guard now runs FIRST, so +# bool/float/None/negative raise the shared message before any floor check. +# They cannot join VALIDATED_CLASSES (test_zero_stays_legal_at_construction +# conflicts with their floors). +# =========================================================================== + +FLOOR_CLASSES = [ + (TROP, {}), + (SyntheticDiD, {}), + (SyntheticDiD, {"variance_method": "jackknife"}), + (HeterogeneousAdoptionDiD, {}), +] + + +class TestFloorEstimatorTypeGuards: + @pytest.mark.parametrize( + "cls, extra", FLOOR_CLASSES, ids=lambda v: getattr(v, "__name__", str(v)) + ) + @pytest.mark.parametrize("bad", [1.5, True, None, -3], ids=repr) + def test_bad_type_raises_shared_message_at_init(self, cls, extra, bad): + with pytest.raises(ValueError, match=re.escape(N_BOOTSTRAP_MSG_PREFIX)): + cls(n_bootstrap=bad, **extra) + + @pytest.mark.parametrize( + "cls, extra", FLOOR_CLASSES, ids=lambda v: getattr(v, "__name__", str(v)) + ) + def test_set_params_rejects_and_rolls_back(self, cls, extra): + est = cls(**extra) + before = est.get_params() + with pytest.raises(ValueError, match=re.escape(N_BOOTSTRAP_MSG_PREFIX)): + est.set_params(n_bootstrap=2.5) + assert est.get_params() == before + + def test_floors_and_carveouts_intact(self): + # Floors keep their own messages after the type guard. + with pytest.raises(ValueError, match="n_bootstrap must be >= 2 for TROP"): + TROP(n_bootstrap=1) + with pytest.raises(ValueError, match=r"n_bootstrap must be >= 2 \(got 1\)"): + SyntheticDiD(n_bootstrap=1) + with pytest.raises(ValueError, match="n_bootstrap must be >= 1"): + HeterogeneousAdoptionDiD(n_bootstrap=0) + # The jackknife floor exemption survives (valid int 1 accepted). + est = SyntheticDiD(variance_method="jackknife", n_bootstrap=1) + assert est.get_params()["n_bootstrap"] == 1