From 48a14698de1ac71cfb5ee8e1cbc44d3fd4103c6b Mon Sep 17 00:00:00 2001 From: igerber Date: Sat, 25 Jul 2026 13:13:02 -0400 Subject: [PATCH 1/2] docs(v4): gating-completeness amendment - mirrored renames, public-function sweep, un-rowed obligations Audits the public surface against the 4.0 spec's own contract-rename rules and closes the gaps, so the automated release gate sees every 3.9 obligation. Rename coverage. The Phase 1 inventory was derived from the ESTIMATOR ROSTER, which left two classes of surface unaudited: - RESULTS fields mirroring a renamed param. M-094 (RegressionDiscontinuityResults.treatment_col), M-095 (StackedDiDResults.clean_control), M-114 (ChaisemartinDHaultfoeuilleResults.groups -> .units, whose "groups" are unit ids). All are public dataclass fields emitted by to_dict(), so renaming the params alone would leave deprecated spellings on the serialized surface that section 5 forbids from 4.0. Section 8 gains rule 9. - Module-level public FUNCTIONS. M-097 (twowayfeweights(group=) -> unit=, which section 3.3 RETAINS past 4.0, so no wrapper removal covers it), M-098..M-112 (five _col params on each of joint_pretrends_test, joint_homogeneity_test, did_had_pretest_workflow), M-113 (trim_weights(weight_col=) -> weights=). Section 8 gains rule 10. M-115 adds the fourth `robust` site (LinearRegression): section 7 says the flag dies "everywhere it exists" while M-045..M-047 enumerated only three. M-042/M-094 target `takeup`, not `treatment`: fuzzy RD accepts non-binary dose take-up (matching R's fuzzy=, with the estimand degrading from complier-LATE to a bare local Wald ratio), and rule 7 reserves `treatment` for a 0/1 indicator. Rule 7 records the corollary. No shim had shipped, so the retarget is free. NOT violations, documented so the sweep is not re-litigated: the staggered family's group/groups and GroupTimeEffect.group name the ATT(g,t) COHORT in Callaway-Sant'Anna notation (rule 8 protects it; renaming to unit would be wrong), and TripleDifference.fit[group] is the treated-group indicator rule 3 reserves the name for. Gate coverage. Three Phase 2 obligations could not fail the release gate: M-081 (n_bootstrap semantics) gains introduced_in 3.9; new M-096 covers the inference="wild_bootstrap" selector contract; and the BaseEstimator mixin plus the R-equivalents docs table get a section 9 3.9-cut checklist since neither is rowable. Behavior rows use introduced_in rather than deprecated_in because the early-flip guard keys off deprecated_in and would fail the very PR that implements the obligation (it lands at 3.8.x) - the M-091/M-092 pattern, now written down. M-096 also records that the selector does not fail closed today: inference= is stored with no valid-value check, and DiD routes wild_bootstrap without cluster= silently to analytical (a currently pinned behavior), so a typo or a missing prerequisite quietly changes which SE/p/CI procedure runs. Repairing that is now gated Phase 2 scope. The row is a drift guard, not a uniformity mandate: estimators whose bootstrap IS their inference method run materially different procedures (CS an influence-function multiplier bootstrap; SunAbraham a unit-pairs bootstrap, Rao-Wu rescaled on survey paths; dCDH a group-level IF multiplier upgrading to Hall-Mammen PSU under strictly-coarser survey PSUs) versus DiD/TWFE's residual WCR with test-inverted CIs, so one value spelling for all would make a uniform name carry altered meaning. M-008's notes record that its telemetry soak window is the 3.9 -> 4.0 gap. Ledger 77 -> 99 rows (floor and immutable-id snapshot extended in the same diff); rows gating a 3.9 bump 45 -> 68. Every new row is `planned` - the shims land in Phase 2c. TODO.md tracks the follow-up guard test that makes this completeness mechanical rather than dependent on who runs the audit. Docs, ledger and test-count updates only - no public API or numerical behavior change. --- CHANGELOG.md | 75 ++++++++++ TODO.md | 1 + docs/v4-deprecations.yaml | 300 +++++++++++++++++++++++++++++++++++++- docs/v4-design.md | 103 ++++++++++++- tests/test_v4_matrix.py | 27 +++- 5 files changed, 489 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0caa3f3a..53c00d7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,81 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **Internal: 4.0 gating-completeness amendment to the design spec and ledger.** + An audit of the public surface against the spec's own contract-rename rules + found two renames the initial inventory missed and three Phase 2 obligations + the automated release gate could not see. (a) Two ledger rows added for + RESULTS fields that mirror already-tracked params: `M-094` + (`RegressionDiscontinuityResults.treatment_col` → `treatment`, mirroring + `M-042`) and `M-095` (`StackedDiDResults.clean_control` → `control_group`, + mirroring `M-043`). Both are public dataclass fields emitted by `to_dict()`, + so renaming the params alone would have left deprecated spellings on the + serialized surface that spec section 5 forbids from 4.0; each flips in the + same diff as its param sibling. `docs/v4-design.md` section 8 gains rule 9 + making the mirror obligation normative, so the next rename sweep greps for it + rather than rediscovering it. (b) `M-081` (`n_bootstrap` semantic + unification) gains `introduced_in: 3.9` and a new `M-096` covers the uniform + `inference="wild_bootstrap"` exposure (spec section 7) - both were real Phase + 2 obligations that no row gated, so a 3.9 cut could have shipped without + them. Both use `introduced_in` rather than `deprecated_in` deliberately: for + behavior rows the early-flip guard keys off `deprecated_in`, so a flip + version would have failed the very PR that implements the obligation (it + lands while `__version__` is still 3.8.x) - the `M-091`/`M-092` pattern. + `M-096` covers the `inference="wild_bootstrap"` selector contract, which + governs wild CLUSTER bootstrap offered as an alternative to analytical SEs - + a roster of `DifferenceInDifferences` + `TwoWayFixedEffects`. Two things the + audit established. (i) The selector does **not** fail closed today: + `inference=` is stored with no valid-value check anywhere, and DiD routes + `wild_bootstrap` without `cluster=` silently to analytical (a currently + *pinned* behavior), so a typo or a missing prerequisite quietly changes which + SE/p-value/CI procedure runs. Phase 2 must validate the accepted value set on + `__init__` and transactional `set_params` and handle the missing-cluster case + explicitly. (ii) The apparent `inference=` vs `n_bootstrap>0` split is not + drift - estimators whose bootstrap *is* their inference method run materially + different procedures (CallawaySantAnna an influence-function multiplier + bootstrap; SunAbraham a unit-pairs bootstrap, Rao-Wu rescaled on survey + paths; dCDH a group-level influence-function multiplier bootstrap upgrading + to Hall-Mammen PSU wild clustering under strictly-coarser survey PSUs) versus + DiD/TWFE's residual wild cluster bootstrap with test-inverted CIs, so one + value spelling for all of them would make a uniform name carry altered + meaning. They keep `n_bootstrap` as documented domain vocabulary, and section + 7 now records the boundary; an `inference=` selector for any of them would + name its own method and is additive post-4.0 work. + (c) The `BaseEstimator` mixin and the R-equivalents docs table are genuine + refactor/docs work that cannot be ledger rows at all, so section 9 gains a + 3.9-cut checklist alongside the existing 4.0 one. `M-008`'s notes now record + that its telemetry soak window *is* the 3.9→4.0 gap. (d) A completeness + sweep over surfaces the Phase 1 inventory never audited. That inventory was + derived from the estimator roster, so module-level public **functions** were + never checked against section 8 at all: `M-097` renames + `twowayfeweights(group=)` → `unit=` (section 3.3 **retains** that diagnostic + past 4.0, so no wrapper removal covers it), `M-098`..`M-112` rename the five + `_col` params on each of the three exported HAD pretest entry points + (`joint_pretrends_test`, `joint_homogeneity_test`, + `did_had_pretest_workflow`), `M-113` renames `trim_weights(weight_col=)` → + `weights=`, `M-114` is the dCDH results mirror (`.groups` → `.units`, whose + "groups" really are unit ids), and `M-115` adds the fourth `robust` site + (`LinearRegression`, which section 7 said dies "everywhere it exists" while + only three rows enumerated it). The same sweep found ~26 hits that are NOT + violations and are now documented as such in section 8 so they are not + re-litigated: the staggered family's `group`/`groups` name the ATT(g,t) + **cohort** in Callaway-Sant'Anna's notation (rule 8 protects it; renaming to + `unit` would be wrong), and `TripleDifference.fit[group]` is the + treated-group indicator rule 3 explicitly reserves the name for. Section 8 + gains rule 10 binding the rules to public functions, not just classes. + (e) `M-042`/`M-094`'s rename target moves from `treatment` to `takeup`: + fuzzy RD's observed-take-up column accepts non-binary dose values (matching + R's `fuzzy=`, with the estimand label degrading from complier-LATE to a bare + local Wald ratio), while section 8 rule 7 reserves `treatment` for a 0/1 + indicator and never a treatment level - so the original target would have + made a uniform name carry altered meaning. `takeup` is the spelling the RD + docstring already uses; no shim had shipped, so the retarget is free. Rule 7 + gains the corollary that a column which MAY be non-binary does not take the + `treatment` name even when it is conceptually take-up. + Ledger row count 77 → 99 (enforcement-test floor and id snapshot extended in + the same diff); rows gating a 3.9 bump 45 → 68. Docs + ledger + test-count + updates only - **no public API or numerical behavior change** (every new row + is `planned`; the shims themselves land in Phase 2c). - **Internal tooling: plan-review engine → dual-reviewer skill (no library change).** `/review-plan` + `/revise-plan` are retired for a `.claude/skills/plan-review/` skill that runs the **dual** review engine diff --git a/TODO.md b/TODO.md index 77be276a..e11efe33 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,7 @@ generic sparse-FE, QR+SVD rank-detection redundancy, `check_finite` bypass — m | 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 | | MMM interop follow-up: Meridian `roi_calibration_period` mask builder - accept the MMM's time index + channel order and emit the boolean `(n_media_times, n_media_channels)` mask so `.to_code()` scopes the prior to the experiment window automatically (today the caller passes a mask expression / `full_model_window=True`). | `diff_diff/mmm.py` | mmm-interop | Quick | Low | | MMM interop PR-B: calibration tutorial notebook (fit DiD/CS -> scope -> `to_pymc_marketing_lift_test` / `to_meridian_roi_prior`) + a `llms-practitioner.txt` Step 8 pointer to the exporters as the MMM hand-off. | `docs/tutorials/`, `diff_diff/guides/llms-practitioner.txt` | mmm-interop | Mid | Low | +| Section-8 naming-completeness guard test: sweep the public surface (module-level functions, class `__init__`/`fit` params, results dataclass fields) for contract-rename violations and fail unless each hit is either a `docs/v4-deprecations.yaml` row or an allowlist entry carrying a stated reason (seed the allowlist with the documented domain-vocabulary exemptions: the staggered family's ATT(g,t) `group`/`groups`, `TripleDifference.fit[group]`, wrapper params dying with `M-070..M-077`, and params inherited from `DifferenceInDifferences.__init__`). Today the rules are normative prose and the ledger holds only what an audit remembered — two successive manual sweeps each found surfaces the prior one missed (`M-094`/`M-095`, then the 19-row `M-097..M-115` function sweep). Should land BEFORE Phase 2c so the rename PR works from a mechanically-verified list. | `tests/`, `docs/v4-design.md` | gating-completeness amendment | Mid | Medium | | 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 | | Real-data CI canary for dataset-backed replication tests: `test_methodology_lwdid.py`'s Prop 99 / Walmart goldens skip (visibly) when loaders fall back to synthetic; add a lane or canary asserting `df.attrs["source"] == "lwdid_ssc_ancillary"` in CI so network regressions cannot silently de-gate the replication tests. Pairs with the loader-fallback repair row above. | `tests/test_methodology_lwdid.py`, `.github/workflows/` | LWDiD validation suite | Quick | Low | | `worktree-rm` safety via a tested argv helper: the prose rewrite (ask-before-remove confirmation gate, detached-HEAD reachability/rescue, tip-identity force-delete guard) was reverted to the main version because editing the prose repeatedly reintroduced shell-injection (last: sourcing a state file built from a git-derived branch name). Restore those guards in a `worktree_rm.py` that takes the name via file ingress, invokes git through argv arrays, and has a metacharacter-branch/path injection regression test — the pattern that worked for `pr_prepare.py`/`premerge_scan.py`. | `.claude/commands/worktree-rm.md`, `.claude/scripts/` | skill-audit | Heavy | Medium | diff --git a/docs/v4-deprecations.yaml b/docs/v4-deprecations.yaml index f4f52cdd..071846a1 100644 --- a/docs/v4-deprecations.yaml +++ b/docs/v4-deprecations.yaml @@ -123,7 +123,7 @@ rows: env_var: DIFF_DIFF_SOLVE_OLS_FASTPATH decision_due: "4.0" code_refs: [diff_diff/linalg.py] - notes: "DIFF_DIFF_SOLVE_OLS_FASTPATH default-ON is a v4-class change GATED on golden recapture + certification telemetry soak (TODO.md row); record go/no-go at the 4.0 cut either way." + notes: "DIFF_DIFF_SOLVE_OLS_FASTPATH default-ON is a v4-class change GATED on golden recapture + certification telemetry soak (TODO.md row); record go/no-go at the 4.0 cut either way. SOAK WINDOW = the 3.9 -> 4.0 gap: opt-in telemetry can only accumulate from a release that advertises the env var, so the 3.9 release notes are the last surface that can start it. A short gap is a legitimate outcome - 'evaluated, kept off' is terminal and needs no soak - but that is a scheduling decision to make deliberately, not to discover at the cut." # ---- The three merges + retiring classes --------------------------------- - id: M-010 @@ -478,7 +478,7 @@ rows: kind: param group: renames-col-suffix old: "diff_diff:RegressionDiscontinuity.fit[treatment_col]" - new: "diff_diff:RegressionDiscontinuity.fit[treatment]" + new: "diff_diff:RegressionDiscontinuity.fit[takeup]" introduced_in: "3.9" deprecated_in: "3.9" removed_in: "4.0" @@ -486,7 +486,20 @@ rows: phase: 2 warning: FutureWarning code_refs: [diff_diff/rdd.py] - notes: "0/1 take-up column for fuzzy RD - consistent with 'treatment' as a 0/1 indicator library-wide." + notes: "Observed take-up column for fuzzy RD. Target RETARGETED from 'treatment' to 'takeup' by the gating-completeness amendment: the column accepts NON-BINARY (dose) take-up, matching R's fuzzy= (REGISTRY 'Estimand and estimator (fuzzy RD)'; the estimand label is data-dependent - complier LATE only when values are in {0,1}, otherwise a bare local Wald ratio). Section 8 rule 7 reserves 'treatment' for a 0/1 indicator and never a treatment level, so 'treatment' would have carried altered meaning here. 'takeup' is already the library's own vocabulary for it (the RD docstring example passes treatment_col='takeup'). No shim had shipped, so the retarget costs nothing." + - id: M-094 + kind: field + group: renames-col-suffix + old: "diff_diff:RegressionDiscontinuityResults.treatment_col" + new: "diff_diff:RegressionDiscontinuityResults.takeup" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/rdd.py] + notes: "Results-side mirror of [M-042]: a public dataclass field carrying the fit param's name, emitted by to_dict(). Renaming the param alone would leave the _col spelling on the serialized surface, which spec section 5 forbids from 4.0 (canonical names only). Target is 'takeup', not 'treatment', for the reason recorded on [M-042] (non-binary take-up is accepted, so 'treatment' would violate section 8 rule 7). Flips WITH [M-042] in the same Phase 2c diff." - id: M-043 kind: param group: renames-control-group @@ -500,6 +513,19 @@ rows: warning: FutureWarning code_refs: [diff_diff/stacked_did.py] notes: "Same concept as control_group elsewhere; underscored value spellings win library-wide (spec section 8)." + - id: M-095 + kind: field + group: renames-control-group + old: "diff_diff:StackedDiDResults.clean_control" + new: "diff_diff:StackedDiDResults.control_group" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/stacked_did_results.py] + notes: "Results-side mirror of [M-043]: a public dataclass field carrying the constructor param's name, emitted by to_dict() and rendered by summary() ('Clean control:'). Renaming the param alone would leave the results metadata contradicting the param it reports. The VALUE spellings are already canonical ('not_yet_treated'); only the field name moves. Flips WITH [M-043] in the same Phase 2c diff; the summary() label follows." - id: M-044 kind: param group: renames-level @@ -961,12 +987,25 @@ rows: group: policy-n-bootstrap old: "diff_diff:DifferenceInDifferences[n_bootstrap]" new: null + introduced_in: "3.9" deprecated_in: null removed_in: null status: planned phase: 2 code_refs: [diff_diff/estimators.py, diff_diff/synthetic_did.py, diff_diff/changes_in_changes.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." + 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." + - id: M-096 + kind: behavior + group: policy-wild-bootstrap + old: "diff_diff:DifferenceInDifferences[inference]" + new: null + introduced_in: "3.9" + deprecated_in: null + removed_in: null + status: planned + phase: 2 + code_refs: [diff_diff/estimators.py, diff_diff/twfe.py] + notes: "Spec section 7 selector contract + roster guard. inference='wild_bootstrap' selects wild CLUSTER bootstrap offered as an ALTERNATIVE to analytical SEs; the SUPPORTING roster is DifferenceInDifferences + TwoWayFixedEffects. NOT a uniformity mandate over every bootstrap - each estimator whose bootstrap IS its inference method runs a materially different procedure and keeps n_bootstrap as documented domain vocabulary (section 6 precedent): CallawaySantAnna = influence-function multiplier bootstrap; SunAbraham = unit-pairs bootstrap (Rao-Wu rescaled under stratified/PSU survey designs); ChaisemartinDHaultfoeuille = group-level influence-function multiplier bootstrap, upgrading to PSU-level Hall-Mammen wild clustering under survey designs with strictly-coarser PSUs. Spelling any of those 'wild_bootstrap' would make a uniform name carry altered meaning (section 8 rule 8). MultiPeriodDiD EXPOSES the param but has no wild-bootstrap path (warns, falls back to analytical); it is removed at 4.0 [M-010] and section 4.1 replaces the fallback with a raise - so the roster pins SUPPORT, not param presence. SCOPE: the selector does not fail closed today - inference= is stored unvalidated (no valid-value check anywhere), and DiD silently routes wild_bootstrap WITHOUT cluster= to analytical (estimators.py, pinned by tests/test_wild_bootstrap.py::test_did_wild_bootstrap_requires_cluster), so a typo or a missing prerequisite silently changes the SE/p/CI procedure - a no-silent-failures violation. Phase 2 must make it fail closed: validate the accepted value set on __init__ AND transactional set_params, and handle wild_bootstrap-without-cluster explicitly (raise or warn - the implementing PR decides, but silence is not an option; flipping the pinned test is expected and its CHANGELOG entry must say so). done requires a test_ref covering: valid/invalid selector values, DiD with and without cluster, TWFE, MultiPeriodDiD's fallback, and the roster itself (a future estimator gaining WCR must adopt the selector or land its own row). Companion to [M-081] (n_bootstrap semantics). introduced_in gates the 3.9 cut for the same reason as [M-081]; behavior-at-done requires test_ref." - id: M-082 kind: param group: renames-post @@ -1015,3 +1054,256 @@ rows: warning: FutureWarning code_refs: [diff_diff/continuous_did.py] notes: "The one estimator with a design-matrix column in the constructor; moves to fit() per the sklearn split (spec section 7)." + + # ---- Completeness sweep (gating-completeness amendment) ------------------- + # The Phase 1 inventory scanned CLASS surfaces only, so module-level public + # FUNCTIONS were never audited against section 8. These rows close that gap. + # Domain-vocabulary NON-violations found by the same sweep are documented in + # spec section 8 (CS-family ATT(g,t) 'group'/'groups'; TripleDifference's + # reserved 'group'), not rowed. + - id: M-097 + kind: param + group: renames-dcdh + old: "diff_diff:twowayfeweights[group]" + new: "diff_diff:twowayfeweights[unit]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/chaisemartin_dhaultfoeuille.py] + notes: "Companion to [M-033] on the RETAINED diagnostic function (spec section 3.3 keeps twowayfeweights past 4.0 - it is not a class duplicate, so no wrapper removal mitigates this). Same meaning as dCDH's fit[group]: a unit identifier, which section 8 rule 3 reserves 'group' against. Unlike the chaisemartin_dhaultfoeuille() wrapper hit, which dies with [M-077]." + - id: M-098 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_pretrends_test[outcome_col]" + new: "diff_diff:joint_pretrends_test[outcome]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Public retained HAD diagnostic; section 8 rule 5. Mirrors the estimator-side [M-035]. The three exported HAD pretest entry points share one 5-param block ([M-098]..[M-112]) and shim together." + - id: M-099 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_pretrends_test[dose_col]" + new: "diff_diff:joint_pretrends_test[dose]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-036]; see [M-098]." + - id: M-100 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_pretrends_test[time_col]" + new: "diff_diff:joint_pretrends_test[time]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-037]; see [M-098]." + - id: M-101 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_pretrends_test[unit_col]" + new: "diff_diff:joint_pretrends_test[unit]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-038]; see [M-098]." + - id: M-102 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_pretrends_test[first_treat_col]" + new: "diff_diff:joint_pretrends_test[first_treat]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-039]; see [M-098]." + - id: M-103 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_homogeneity_test[outcome_col]" + new: "diff_diff:joint_homogeneity_test[outcome]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-035]; see [M-098]." + - id: M-104 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_homogeneity_test[dose_col]" + new: "diff_diff:joint_homogeneity_test[dose]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-036]; see [M-098]." + - id: M-105 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_homogeneity_test[time_col]" + new: "diff_diff:joint_homogeneity_test[time]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-037]; see [M-098]." + - id: M-106 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_homogeneity_test[unit_col]" + new: "diff_diff:joint_homogeneity_test[unit]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-038]; see [M-098]." + - id: M-107 + kind: param + group: renames-col-suffix + old: "diff_diff:joint_homogeneity_test[first_treat_col]" + new: "diff_diff:joint_homogeneity_test[first_treat]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-039]; see [M-098]." + - id: M-108 + kind: param + group: renames-col-suffix + old: "diff_diff:did_had_pretest_workflow[outcome_col]" + new: "diff_diff:did_had_pretest_workflow[outcome]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-035]; see [M-098]." + - id: M-109 + kind: param + group: renames-col-suffix + old: "diff_diff:did_had_pretest_workflow[dose_col]" + new: "diff_diff:did_had_pretest_workflow[dose]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-036]; see [M-098]." + - id: M-110 + kind: param + group: renames-col-suffix + old: "diff_diff:did_had_pretest_workflow[time_col]" + new: "diff_diff:did_had_pretest_workflow[time]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-037]; see [M-098]." + - id: M-111 + kind: param + group: renames-col-suffix + old: "diff_diff:did_had_pretest_workflow[unit_col]" + new: "diff_diff:did_had_pretest_workflow[unit]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-038]; see [M-098]." + - id: M-112 + kind: param + group: renames-col-suffix + old: "diff_diff:did_had_pretest_workflow[first_treat_col]" + new: "diff_diff:did_had_pretest_workflow[first_treat]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/had_pretests.py] + notes: "Mirrors [M-039]; see [M-098]." + - id: M-113 + kind: param + group: renames-col-suffix + old: "diff_diff:trim_weights[weight_col]" + new: "diff_diff:trim_weights[weights]" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/prep.py] + notes: "Public prep helper; section 8 rule 5. Target spelling is 'weights', the established library name for a weight column (5 public class surfaces use it) - NOT bare 'weight'." + - id: M-114 + kind: field + group: renames-dcdh + old: "diff_diff:ChaisemartinDHaultfoeuilleResults.groups" + new: "diff_diff:ChaisemartinDHaultfoeuilleResults.units" + introduced_in: "3.9" + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/chaisemartin_dhaultfoeuille_results.py, diff_diff/chaisemartin_dhaultfoeuille.py] + notes: "Results-side mirror of [M-033] under section 8 rule 9. dCDH's 'groups' are UNIT identifiers ('Group identifiers in the post-filter sample'), the same column fit[group] renames to unit - distinct from the CS-family 'groups', which are ATT(g,t) cohorts and stay as domain vocabulary. summary()/repr() labels follow; needs __setstate__ pickle migration per spec section 5. The n_groups_dropped_* counters are NOT renamed by this row (they count dropped units but read as diagnostics; re-audit at 4.0 if the vocabulary drifts)." + - id: M-115 + kind: param + group: renames-robust-drop + old: "diff_diff:LinearRegression[robust]" + new: null + deprecated_in: "3.9" + removed_in: "4.0" + status: planned + phase: 2 + warning: FutureWarning + code_refs: [diff_diff/linalg.py] + notes: "Fourth site of the 'robust' drop - spec section 7 says the flag dies 'everywhere it exists' but [M-045]..[M-047] enumerated only three. LinearRegression is a top-level export with its OWN __init__ (the TWFE/MultiPeriodDiD hits inherit DifferenceInDifferences.__init__ and ride [M-045]). Redundant with vcov_type, same as its siblings." diff --git a/docs/v4-design.md b/docs/v4-design.md index de2ec853..7ec4dc9a 100644 --- a/docs/v4-design.md +++ b/docs/v4-design.md @@ -423,8 +423,37 @@ domain vocabulary, not drift. analytical SEs (SyntheticControl, TROP, CiC) document their native variance methods instead of growing a dead param. - Wild bootstrap is exposed via `inference="wild_bootstrap"` uniformly where - supported (today's split between `inference=` and `n_bootstrap>0` idioms is - resolved in the Phase 2 plan for the affected estimators). + supported [M-096] - "supported" meaning the estimator offers wild CLUSTER + bootstrap as an ALTERNATIVE to analytical SEs. That roster is + DifferenceInDifferences + TwoWayFixedEffects. (MultiPeriodDiD also carries + the param but has no wild-bootstrap path - it warns and falls back to + analytical; it is removed at 4.0 [M-010] and section 4.1 replaces that + fallback with an explicit raise in the merged event-study mode, so the roster + pins estimators that SUPPORT WCR, not those that merely expose the param.) + + **The selector must fail closed** - it does not today. `inference=` is stored + without any valid-value check, and DiD routes `wild_bootstrap` WITHOUT + `cluster=` silently to analytical (pinned by + `tests/test_wild_bootstrap.py::test_did_wild_bootstrap_requires_cluster`), so + a typo or a missing prerequisite quietly changes which SE/p-value/CI + procedure runs - the no-silent-failures principle applied to inference + selection. Phase 2 validates the accepted value set on `__init__` and + transactional `set_params`, and handles wild-bootstrap-without-cluster + explicitly; silence is not an option. + + The apparent `inference=` vs `n_bootstrap>0` split is NOT drift: an estimator + whose bootstrap IS its inference method runs a different procedure - + CallawaySantAnna an influence-function multiplier bootstrap, SunAbraham a + unit-pairs bootstrap (Rao-Wu rescaled under stratified/PSU survey designs), + ChaisemartinDHaultfoeuille a group-level influence-function multiplier + bootstrap that upgrades to PSU-level Hall-Mammen wild clustering under survey + designs with strictly-coarser PSUs - against DiD/TWFE's residual wild cluster + bootstrap with test-inverted CIs and no reference t-distribution. Spelling + any of those `"wild_bootstrap"` would make a uniform name carry altered + meaning, which rule 8 of section 8 forbids; they keep `n_bootstrap` as + documented domain vocabulary on the section 6 precedent. Should one of them + later want an `inference=` selector, the value names its own method - that is + additive, post-4.0, minor-version work, not part of this program. - `n_bootstrap` semantic unification [M-081]: `0` = bootstrap off wherever an analytical path exists; bootstrap-only estimators document their positive defaults. Counts stay tuned per estimator (999 light / 200 compute-heavy) - @@ -472,10 +501,43 @@ missed.** 7. `treatment` names a 0/1 indicator (group membership or take-up), never a cohort and never a treatment level; estimators with non-binary per-period treatment (dCDH) document theirs as domain vocabulary. + Corollary: a column that MAY be non-binary does not get this name even + when it is conceptually take-up. Fuzzy RD's observed-take-up column + accepts dose values (matching R's `fuzzy=`; the estimand label degrades + from complier-LATE to a bare local Wald ratio accordingly), so [M-042] + [M-094] target `takeup` - the spelling the RD docstring already uses - + rather than `treatment`. 8. Hybrid-naming principle (locked 2026-07-12): diff-diff names where the library owns the concept; domain vocabulary where it is the field's language; an R-equivalents mapping table (`yname/tname/idname/gname` -> `outcome/time/unit/first_treat`) ships in the docs, not as param aliases. +9. **A rename carries to every surface that mirrors it.** Renaming a + constructor or `fit()` param renames any RESULTS field that echoes it back + (and the `summary()` label that prints it), in the SAME diff and on the same + shim schedule - a results field is public surface, is emitted by + `to_dict()`, and section 5 forbids deprecated names in serialized output + from 4.0. Mirrors added by amendment: [M-094] (mirrors [M-042]), [M-095] + (mirrors [M-043]), [M-114] (mirrors [M-033]). When adding a rename row, + grep the matching `*_results.py` for the old name before assuming the param + row is the whole job. +10. **These rules bind module-level PUBLIC FUNCTIONS, not just classes.** The + Phase 1 inventory was derived from the estimator roster, so exported + functions went unaudited; the completeness sweep added [M-097] (the + RETAINED `twowayfeweights` diagnostic - section 3.3 keeps it past 4.0, so + no wrapper removal covers it), [M-098]..[M-112] (the three exported HAD + pretest entry points, five `_col` params each), and [M-113] + (`trim_weights`). A function that survives 4.0 gets the same shim schedule + as a class surface; only surfaces scheduled for REMOVAL + ([M-070]..[M-077]) are exempt, because the removal moots the name. + +**Domain vocabulary that is NOT a violation** (recorded so the sweep is not +re-litigated): the staggered family's `group`/`groups` on results containers +and `GroupTimeEffect.group` name the ATT(g,t) COHORT in Callaway-Sant'Anna's +own notation - rule 8 protects exactly this, and renaming them to `unit` would +be actively wrong. `TripleDifference.fit[group]` is the treated-group 0/1 +indicator that rule 3 explicitly reserves the name for. dCDH is the one +estimator where `group` genuinely means a unit id, which is why [M-033] and +[M-114] exist and the CS-family fields have no rows. **Naming-checkpoint outcomes (2026-07-18), with losing candidates:** `event_study=` bool (over `effects=` enum, `dynamic=` bool); @@ -496,12 +558,41 @@ above; anything only one PR cares about stays in that PR's plan.** | Phase | Ships in | PRs (each: dedicated shim/removal tests + matrix flips + CHANGELOG naming flipped row ids) | |---|---|---| | 1 (this PR) | - | Spec + matrix + enforcement test + support edits | -| 2: contract foundations | 3.9 | (a) results base + unified event-study representation + to_dict completion + the Diagnostic marker base on the diagnostic result roster [M-091] (section 3.5); (b) `aggregate()` + fit(aggregate=) shims [M-020..M-027]; (c) param renames [M-030..M-047] + BaseEstimator mixin + ContinuousDiD covariates move; (d) alias introductions [M-062] [M-063] + wrapper deprecations [M-070..M-077] + n_bootstrap docs [M-081] | +| 2: contract foundations | 3.9 | (a) results base + unified event-study representation [M-092] + to_dict completion + the Diagnostic marker base on the diagnostic result roster [M-091] (section 3.5); (b) `aggregate()` + fit(aggregate=) shims [M-020..M-027]; (c) param renames [M-030..M-047] [M-084] [M-086..M-089] + their results-field mirrors [M-094] [M-095] (section 8 rule 9) + the public-function completeness sweep [M-097..M-113] (section 8 rule 10) + the dCDH results mirror [M-114] + the fourth `robust` site [M-115] + BaseEstimator mixin + ContinuousDiD covariates move; (d) alias introductions [M-062] [M-063] + wrapper deprecations [M-070..M-077] + the two inference-surface policies: `n_bootstrap` semantic unification [M-081] and the wild-cluster-bootstrap roster guard [M-096] | | 3: merges | 3.9 | (a) TWFE event-study mode [M-010] + EventStudy warn [M-060] (gates: section 4.1's equivalence/divergence/pooled-parity test triple); (b) TripleDifference facade [M-013]; (c) CiC method= [M-015] | | 4: release + soak | 3.9 cut | Migration guide written (skeleton: section 10); maintainer cuts 3.9; maint/3.8 rule active | | 5: enforcement | 4.0 | Removals [M-010..M-016, M-030..M-047 old names, M-060, M-061, M-070..M-077, M-001..M-003]; storage flips [M-050..M-058]; default policies [M-004..M-006, M-080]; warning retirement [M-007]; fastpath go/no-go [M-008]; diagnostic-family docs/roster reorganization [M-090]; docs/llms.txt/README refresh | | 6: front door | 4.1 | `event_study(data, outcome, unit, time, first_treat, estimator=...)` comparison entry point over the staggered family (sketch only; specified in its own plan) | +**3.9-cut checklist (un-rowed obligations).** `test_due_rows_are_terminal` +gates everything that HAS a row; the following Phase 2 obligations are real but +not expressible as ledger rows, so the 3.9 release PR asserts them by hand: + +1. The shared `BaseEstimator` mixin has replaced the hand-rolled + `get_params`/`set_params` copies (section 7), with `deep=` uniform and + set_params transactional per the locked rule. A pure refactor - no symbol + changes - so no row can see it. +2. The R-equivalents mapping table (section 8 rule 8) ships in the docs. +3. The migration guide exists (section 10) and its TL;DR table has a row per + breaking change, generated against the matrix rather than hand-listed. +4. `docs/v4-deprecations.yaml` and this document agree on the phase breakdown + in the table above - any PR that re-scoped a phase edited both. + +Everything else queued for 3.9 is row-gated, by one of two mechanisms. Symbol +rows that declare a `warning` gate on `deprecated_in` - the shim must have +shipped ([M-010] [M-013] [M-015], [M-020]..[M-027], [M-030]..[M-047], +[M-070]..[M-077], [M-082], +[M-084], [M-086]..[M-089], [M-094] [M-095], [M-097]..[M-115]). Rows with no +shim to assert gate +on `introduced_in` instead - the new surface must have shipped: the +introduce-only aliases [M-062] [M-063] and the `behavior`-kind policies +[M-081] [M-091] [M-092] [M-096]. That second mechanism is deliberate for +behavior rows: the early-flip guard keys off `deprecated_in`, so a flip version +would fail the very PR that implements the obligation (it lands while +`__version__` is still 3.8.x). Use `introduced_in`, not `deprecated_in`, for +any future behavior row that must ship in a release the repo has not yet +bumped to. + **4.0-cut checklist (final item):** the due-row sweep is AUTOMATED - `tests/test_v4_matrix.py::test_due_rows_are_terminal` fails any release bump whose version reaches a row's scheduled version while its status has not @@ -616,8 +707,10 @@ forever - a removed symbol resurrecting is a test failure. class row (schema-enforced). Top-level `diff_diff:Name` class/function rows and alias rows also assert `__all__` membership consistent with their status (stale `import *` entries fail). The shipped row ids are a - committed snapshot in the enforcement test (77 as of Phase 2a: Phase 1 + - the diagnostic-family amendment + the M-092/M-093 results-contract rows; + committed snapshot in the enforcement test (99 as of the + gating-completeness amendment: Phase 1 + the diagnostic-family amendment + + the M-092/M-093 results-contract rows + the M-094..M-096 amendment rows + + the M-097..M-115 completeness sweep; the snapshot extends by a new id range in the same diff that appends rows): ids are never deleted or reused, and the test fails if any snapshot id disappears. diff --git a/tests/test_v4_matrix.py b/tests/test_v4_matrix.py index 86138532..35aba6f6 100644 --- a/tests/test_v4_matrix.py +++ b/tests/test_v4_matrix.py @@ -110,17 +110,24 @@ _MD_TOKEN_RE = re.compile(r"\[(M-\d{3})\]") # Row-count floor: 75 rows from the Phase 1 spec + diagnostic-family amendment, -# plus the 2 Phase 2a results-contract rows (M-092/M-093) = 77. Ids are never -# reused and terminal rows are never deleted, so the ledger only grows - raise -# the floor when rows are added; a lower parse count means scanner/format drift -# or an illegal row deletion. -ROW_COUNT_FLOOR = 77 +# plus the 2 Phase 2a results-contract rows (M-092/M-093) = 77, plus the 3 +# gating-completeness amendment rows (M-094..M-096) = 80, plus the 19-row +# completeness sweep over public FUNCTIONS and the dCDH results mirror +# (M-097..M-115) = 99. Ids are never reused and terminal rows are never +# deleted, so the ledger only grows - raise the floor when rows are added; a +# lower parse count means scanner/format drift or an illegal row deletion. +ROW_COUNT_FLOOR = 99 # Committed snapshot of the shipped id set ("ids are never deleted or reused" # contract - a delete-one-add-one edit keeps the count above the floor but trips # this). Extend with a NEW tuple, never edit an existing one. Ranges: # (1,8)/(10,16)/(20,27)/(30,47)/(50,58)/(60,64)/(70,77)/(80,91) = Phase 1 + the -# diagnostic-family amendment; (92,93) = Phase 2a results-contract rows. +# diagnostic-family amendment; (92,93) = Phase 2a results-contract rows; +# (94,96) = the gating-completeness amendment (two results-field mirrors of +# existing param rows, plus the wild-bootstrap exposure policy); +# (97,115) = its completeness sweep over module-level public functions +# (twowayfeweights, three HAD pretest entry points, trim_weights) plus the +# dCDH results mirror and the fourth `robust` site. _INITIAL_ID_RANGES = [ (1, 8), (10, 16), @@ -131,6 +138,8 @@ (70, 77), (80, 91), (92, 93), + (94, 96), + (97, 115), ] EXPECTED_INITIAL_IDS = frozenset( f"M-{n:03d}" for lo, hi in _INITIAL_ID_RANGES for n in range(lo, hi + 1) @@ -527,10 +536,12 @@ def test_initial_ids_never_deleted(): """The shipped id set is immutable: ids are never deleted or reused (spec section 11). ROW_COUNT_FLOOR alone would let a delete-one-add-one edit pass; this snapshot cannot. - Extends as rows ship (77 as of Phase 2a: Phase 1 + diagnostic-family + M-092/M-093).""" + Extends as rows ship (99 as of the gating-completeness amendment: Phase 1 + + diagnostic-family + M-092/M-093 + M-094..M-096 + the M-097..M-115 + public-function completeness sweep).""" missing = sorted(EXPECTED_INITIAL_IDS - set(_ROW_IDS)) assert not missing, f"ledger rows deleted (ids are permanent): {missing}" - assert len(EXPECTED_INITIAL_IDS) == 77 + assert len(EXPECTED_INITIAL_IDS) == 99 def test_version_tuple_pads_to_three_components(): From 4b15d6767f24b91d7105a1aa763d49614d742624 Mon Sep 17 00:00:00 2001 From: igerber Date: Sat, 25 Jul 2026 13:29:23 -0400 Subject: [PATCH 2/2] Address CI review: fail-closed selector, consumer migration scope, stale refs P1 - M-096 contradicted its own fail-closed requirement. REGISTRY specifies WCR as inference="wild_bootstrap" (with cluster=), so the combination without a cluster is incoherent, not merely unsupported, and a warned analytical fallback still runs a procedure the caller did not request. "raise or warn" is replaced by: accepted set pinned to exactly {"analytical", "wild_bootstrap"} on __init__ and transactional set_params, and wild_bootstrap without cluster= raises ValueError. Test obligations extended to match. P1 - M-095 omitted its consumers. _reporting_helpers.py and business_report.py read results.clean_control via getattr(..., None) and branch on "never_treated"/"strict"; after removal both silently return None and fall through to the default "not_yet_treated" description, misreporting the identifying comparison group - a wrong-answer regression, not a crash. business_report.py also emits block["clean_control"] as a serialized reporting key, retired at 4.0 by the section 5 canonical-names policy, and REPORTING.md documents the field. The same audit found the estimator-side twin the review did not flag: power.py and practitioner.py read estimator.clean_control, which is M-043's surface, so that row carries its consumers too. Generalized as section 8 rule 11: a rename's scope includes every READER of the old name, not just its definition site, with the getattr-default degradation named as the specific hazard. P2 - Phase 5 table omitted the amendment's removals (M-094, M-095, M-097..M-115) and M-093. Third round in a row a phase-table sync was missed by hand, so phase-table agreement is folded into the tracked guard-test row along with rule 11 consumer coverage. P2 - Stale canonical references: section 3.4 still showed RegressionDiscontinuity.fit(treatment=) after the takeup retarget, both robust references omitted M-115, and the CHANGELOG bullet still said treatment. --- CHANGELOG.md | 10 ++++++++-- TODO.md | 2 +- docs/v4-deprecations.yaml | 10 +++++----- docs/v4-design.md | 39 ++++++++++++++++++++++++++++++--------- 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53c00d7c..38516d22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 found two renames the initial inventory missed and three Phase 2 obligations the automated release gate could not see. (a) Two ledger rows added for RESULTS fields that mirror already-tracked params: `M-094` - (`RegressionDiscontinuityResults.treatment_col` → `treatment`, mirroring + (`RegressionDiscontinuityResults.treatment_col` → `takeup`, mirroring `M-042`) and `M-095` (`StackedDiDResults.clean_control` → `control_group`, mirroring `M-043`). Both are public dataclass fields emitted by `to_dict()`, so renaming the params alone would have left deprecated spellings on the @@ -69,7 +69,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 **cohort** in Callaway-Sant'Anna's notation (rule 8 protects it; renaming to `unit` would be wrong), and `TripleDifference.fit[group]` is the treated-group indicator rule 3 explicitly reserves the name for. Section 8 - gains rule 10 binding the rules to public functions, not just classes. + gains rule 10 binding the rules to public functions, not just classes, and + rule 11 putting every READER of a renamed name in the row's scope — the + `getattr(obj, "old_name", default)` pattern degrades silently to the default + after removal, so a consumer reports the wrong thing instead of crashing. + `M-043`/`M-095` accordingly carry their reporting and power/practitioner + consumers (and the `clean_control` serialized reporting key, retired at 4.0 + by the section 5 canonical-names policy) in `code_refs`. (e) `M-042`/`M-094`'s rename target moves from `treatment` to `takeup`: fuzzy RD's observed-take-up column accepts non-binary dose values (matching R's `fuzzy=`, with the estimand label degrading from complier-LATE to a bare diff --git a/TODO.md b/TODO.md index e11efe33..31ec3697 100644 --- a/TODO.md +++ b/TODO.md @@ -45,7 +45,7 @@ generic sparse-FE, QR+SVD rank-detection redundancy, `check_finite` bypass — m | 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 | | MMM interop follow-up: Meridian `roi_calibration_period` mask builder - accept the MMM's time index + channel order and emit the boolean `(n_media_times, n_media_channels)` mask so `.to_code()` scopes the prior to the experiment window automatically (today the caller passes a mask expression / `full_model_window=True`). | `diff_diff/mmm.py` | mmm-interop | Quick | Low | | MMM interop PR-B: calibration tutorial notebook (fit DiD/CS -> scope -> `to_pymc_marketing_lift_test` / `to_meridian_roi_prior`) + a `llms-practitioner.txt` Step 8 pointer to the exporters as the MMM hand-off. | `docs/tutorials/`, `diff_diff/guides/llms-practitioner.txt` | mmm-interop | Mid | Low | -| Section-8 naming-completeness guard test: sweep the public surface (module-level functions, class `__init__`/`fit` params, results dataclass fields) for contract-rename violations and fail unless each hit is either a `docs/v4-deprecations.yaml` row or an allowlist entry carrying a stated reason (seed the allowlist with the documented domain-vocabulary exemptions: the staggered family's ATT(g,t) `group`/`groups`, `TripleDifference.fit[group]`, wrapper params dying with `M-070..M-077`, and params inherited from `DifferenceInDifferences.__init__`). Today the rules are normative prose and the ledger holds only what an audit remembered — two successive manual sweeps each found surfaces the prior one missed (`M-094`/`M-095`, then the 19-row `M-097..M-115` function sweep). Should land BEFORE Phase 2c so the rename PR works from a mechanically-verified list. | `tests/`, `docs/v4-design.md` | gating-completeness amendment | Mid | Medium | +| Section-8 naming-completeness guard test: sweep the public surface (module-level functions, class `__init__`/`fit` params, results dataclass fields) for contract-rename violations and fail unless each hit is either a `docs/v4-deprecations.yaml` row or an allowlist entry carrying a stated reason (seed the allowlist with the documented domain-vocabulary exemptions: the staggered family's ATT(g,t) `group`/`groups`, `TripleDifference.fit[group]`, wrapper params dying with `M-070..M-077`, and params inherited from `DifferenceInDifferences.__init__`). Today the rules are normative prose and the ledger holds only what an audit remembered — two successive manual sweeps each found surfaces the prior one missed (`M-094`/`M-095`, then the 19-row `M-097..M-115` function sweep). Should land BEFORE Phase 2c so the rename PR works from a mechanically-verified list. Two further checks belong in the same guard, each having been missed by hand across successive review rounds: (a) **phase-table agreement** — every row's `phase` must appear in the matching `docs/v4-design.md` section 9 table entry and vice versa (the section 9 checklist asserts this agreement but nothing verifies it, and three separate rounds caught a stale table); (b) **consumer coverage** (section 8 rule 11) — for each rename row, grep `diff_diff/` and `docs/methodology/` for the old name and require every hit to be either in the row's `code_refs` or allowlisted, since `getattr(obj, "old_name", default)` degrades silently rather than raising after removal. | `tests/`, `docs/v4-design.md` | gating-completeness amendment | Mid | Medium | | 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 | | Real-data CI canary for dataset-backed replication tests: `test_methodology_lwdid.py`'s Prop 99 / Walmart goldens skip (visibly) when loaders fall back to synthetic; add a lane or canary asserting `df.attrs["source"] == "lwdid_ssc_ancillary"` in CI so network regressions cannot silently de-gate the replication tests. Pairs with the loader-fallback repair row above. | `tests/test_methodology_lwdid.py`, `.github/workflows/` | LWDiD validation suite | Quick | Low | | `worktree-rm` safety via a tested argv helper: the prose rewrite (ask-before-remove confirmation gate, detached-HEAD reachability/rescue, tip-identity force-delete guard) was reverted to the main version because editing the prose repeatedly reintroduced shell-injection (last: sourcing a state file built from a git-derived branch name). Restore those guards in a `worktree_rm.py` that takes the name via file ingress, invokes git through argv arrays, and has a metacharacter-branch/path injection regression test — the pattern that worked for `pr_prepare.py`/`premerge_scan.py`. | `.claude/commands/worktree-rm.md`, `.claude/scripts/` | skill-audit | Heavy | Medium | diff --git a/docs/v4-deprecations.yaml b/docs/v4-deprecations.yaml index 071846a1..8acdeed7 100644 --- a/docs/v4-deprecations.yaml +++ b/docs/v4-deprecations.yaml @@ -511,8 +511,8 @@ rows: status: planned phase: 2 warning: FutureWarning - code_refs: [diff_diff/stacked_did.py] - notes: "Same concept as control_group elsewhere; underscored value spellings win library-wide (spec section 8)." + code_refs: [diff_diff/stacked_did.py, diff_diff/power.py, diff_diff/practitioner.py] + notes: "Same concept as control_group elsewhere; underscored value spellings win library-wide (spec section 8). CONSUMER MIGRATION: two sites read the ESTIMATOR attribute via getattr(estimator, 'clean_control', ...) and gate on it - power.py (a 'strict' survey_config rejection and a cross-check against control_group) and practitioner.py (guidance text naming the param). Same getattr-returns-None-after-removal failure mode as [M-095]'s results-side consumers, so both rows migrate their consumers in the shared Phase 2c diff; [M-095] covers results.clean_control readers, this row covers estimator.clean_control readers." - id: M-095 kind: field group: renames-control-group @@ -524,8 +524,8 @@ rows: status: planned phase: 2 warning: FutureWarning - code_refs: [diff_diff/stacked_did_results.py] - notes: "Results-side mirror of [M-043]: a public dataclass field carrying the constructor param's name, emitted by to_dict() and rendered by summary() ('Clean control:'). Renaming the param alone would leave the results metadata contradicting the param it reports. The VALUE spellings are already canonical ('not_yet_treated'); only the field name moves. Flips WITH [M-043] in the same Phase 2c diff; the summary() label follows." + code_refs: [diff_diff/stacked_did_results.py, diff_diff/_reporting_helpers.py, diff_diff/business_report.py, docs/methodology/REPORTING.md] + notes: "Results-side mirror of [M-043]: a public dataclass field carrying the constructor param's name, emitted by to_dict() and rendered by summary() ('Clean control:'). Renaming the param alone would leave the results metadata contradicting the param it reports. The VALUE spellings are already canonical ('not_yet_treated'); only the field name moves. Flips WITH [M-043] in the same Phase 2c diff; the summary() label follows. CONSUMER MIGRATION IS PART OF THIS ROW - two reporting sites read results.clean_control via getattr(..., None) and branch on 'never_treated'/'strict' (_reporting_helpers.py, business_report.py). A getattr default silently returns None after removal, so BOTH would fall through to the default 'not_yet_treated' description and misreport the identifying comparison group - a wrong-answer regression, not a crash. business_report.py also EMITS block['clean_control'] as a serialized reporting key, which section 5's canonical-names-only policy retires at 4.0 (the block keeps 'control_group'). REPORTING.md documents the field and updates with it. Tests must cover a results object exposing ONLY control_group, across all three values." - id: M-044 kind: param group: renames-level @@ -1005,7 +1005,7 @@ rows: status: planned phase: 2 code_refs: [diff_diff/estimators.py, diff_diff/twfe.py] - notes: "Spec section 7 selector contract + roster guard. inference='wild_bootstrap' selects wild CLUSTER bootstrap offered as an ALTERNATIVE to analytical SEs; the SUPPORTING roster is DifferenceInDifferences + TwoWayFixedEffects. NOT a uniformity mandate over every bootstrap - each estimator whose bootstrap IS its inference method runs a materially different procedure and keeps n_bootstrap as documented domain vocabulary (section 6 precedent): CallawaySantAnna = influence-function multiplier bootstrap; SunAbraham = unit-pairs bootstrap (Rao-Wu rescaled under stratified/PSU survey designs); ChaisemartinDHaultfoeuille = group-level influence-function multiplier bootstrap, upgrading to PSU-level Hall-Mammen wild clustering under survey designs with strictly-coarser PSUs. Spelling any of those 'wild_bootstrap' would make a uniform name carry altered meaning (section 8 rule 8). MultiPeriodDiD EXPOSES the param but has no wild-bootstrap path (warns, falls back to analytical); it is removed at 4.0 [M-010] and section 4.1 replaces the fallback with a raise - so the roster pins SUPPORT, not param presence. SCOPE: the selector does not fail closed today - inference= is stored unvalidated (no valid-value check anywhere), and DiD silently routes wild_bootstrap WITHOUT cluster= to analytical (estimators.py, pinned by tests/test_wild_bootstrap.py::test_did_wild_bootstrap_requires_cluster), so a typo or a missing prerequisite silently changes the SE/p/CI procedure - a no-silent-failures violation. Phase 2 must make it fail closed: validate the accepted value set on __init__ AND transactional set_params, and handle wild_bootstrap-without-cluster explicitly (raise or warn - the implementing PR decides, but silence is not an option; flipping the pinned test is expected and its CHANGELOG entry must say so). done requires a test_ref covering: valid/invalid selector values, DiD with and without cluster, TWFE, MultiPeriodDiD's fallback, and the roster itself (a future estimator gaining WCR must adopt the selector or land its own row). Companion to [M-081] (n_bootstrap semantics). introduced_in gates the 3.9 cut for the same reason as [M-081]; behavior-at-done requires test_ref." + notes: "Spec section 7 selector contract + roster guard. inference='wild_bootstrap' selects wild CLUSTER bootstrap offered as an ALTERNATIVE to analytical SEs; the SUPPORTING roster is DifferenceInDifferences + TwoWayFixedEffects. NOT a uniformity mandate over every bootstrap - each estimator whose bootstrap IS its inference method runs a materially different procedure and keeps n_bootstrap as documented domain vocabulary (section 6 precedent): CallawaySantAnna = influence-function multiplier bootstrap; SunAbraham = unit-pairs bootstrap (Rao-Wu rescaled under stratified/PSU survey designs); ChaisemartinDHaultfoeuille = group-level influence-function multiplier bootstrap, upgrading to PSU-level Hall-Mammen wild clustering under survey designs with strictly-coarser PSUs. Spelling any of those 'wild_bootstrap' would make a uniform name carry altered meaning (section 8 rule 8). MultiPeriodDiD EXPOSES the param but has no wild-bootstrap path (warns, falls back to analytical); it is removed at 4.0 [M-010] and section 4.1 replaces the fallback with a raise - so the roster pins SUPPORT, not param presence. SCOPE: the selector does not fail closed today - inference= is stored unvalidated (no valid-value check anywhere), and DiD silently routes wild_bootstrap WITHOUT cluster= to analytical (estimators.py, pinned by tests/test_wild_bootstrap.py::test_did_wild_bootstrap_requires_cluster), so a typo or a missing prerequisite silently changes the SE/p/CI procedure - a no-silent-failures violation. Phase 2 makes it fail closed, and 'fail closed' means REFUSE, not warn-and-degrade: (a) the accepted value set is exactly {'analytical', 'wild_bootstrap'}, validated on __init__ AND transactional set_params (an unrecognized spelling raises, never falls through to analytical); (b) wild_bootstrap WITHOUT cluster= raises ValueError - REGISTRY defines WCR as 'inference=\"wild_bootstrap\" (with cluster=)', so the combination is incoherent rather than merely unsupported, and a warned analytical fallback would still run a procedure the caller did not ask for. Flipping the pinned test is expected and its CHANGELOG entry must say so. done requires a test_ref covering: the exact accepted value set and an invalid spelling, transactional set_params, DiD with and without cluster, TWFE, MultiPeriodDiD's fallback, and the roster itself (a future estimator gaining WCR must adopt the selector or land its own row). Companion to [M-081] (n_bootstrap semantics). introduced_in gates the 3.9 cut for the same reason as [M-081]; behavior-at-done requires test_ref." - id: M-082 kind: param group: renames-post diff --git a/docs/v4-design.md b/docs/v4-design.md index 7ec4dc9a..cf38b98e 100644 --- a/docs/v4-design.md +++ b/docs/v4-design.md @@ -137,16 +137,21 @@ signatures. Lifecycle facts live in the cited rows. `unit` [M-033], `controls` -> `covariates` [M-034]. - `HeterogeneousAdoptionDiD.fit(outcome=, dose=, time=, unit=, first_treat=, ...)` - `_col` suffixes dropped [M-035]..[M-039]. -- `RegressionDiscontinuity.fit(outcome=, running=, treatment=, ...)` - - [M-040]..[M-042]. +- `RegressionDiscontinuity.fit(outcome=, running=, takeup=, ...)` - + [M-040]..[M-042]; `takeup` rather than `treatment` because the column + accepts non-binary dose take-up (rule 7's corollary, section 8), with the + results-field mirror at [M-094]. - `StackedDiD(control_group=...)` - `clean_control` renamed [M-043]. - `WooldridgeDiDResults.to_dataframe(level=...)` - `aggregation` -> `level` [M-044]; the `"event"` value spelling unifies across its existing `aggregate(type=)` surface [M-086] and `summary(aggregation=)` is retired for the uniform `summary(alpha=None)` [M-087]. - `robust` constructor param dropped everywhere it exists - [M-045]..[M-047] - fully redundant with `vcov_type`, and its default even - differed across estimators (True/True/False). + [M-045]..[M-047] [M-115] - fully redundant with `vcov_type`, and its default + even differed across estimators (True/True/False). Four sites, not three: + [M-115] adds `LinearRegression`, which owns its `__init__`; the + TwoWayFixedEffects / MultiPeriodDiD occurrences inherit + `DifferenceInDifferences.__init__` and ride [M-045]. ### 3.5 The diagnostic family @@ -419,7 +424,7 @@ domain vocabulary, not drift. ## 7. Inference surface - `vcov_type` becomes the single SE-type selector wherever analytical SEs - exist; the redundant `robust` flag dies [M-045]..[M-047]. Estimators without + exist; the redundant `robust` flag dies [M-045]..[M-047] [M-115]. Estimators without analytical SEs (SyntheticControl, TROP, CiC) document their native variance methods instead of growing a dead param. - Wild bootstrap is exposed via `inference="wild_bootstrap"` uniformly where @@ -437,9 +442,13 @@ domain vocabulary, not drift. `tests/test_wild_bootstrap.py::test_did_wild_bootstrap_requires_cluster`), so a typo or a missing prerequisite quietly changes which SE/p-value/CI procedure runs - the no-silent-failures principle applied to inference - selection. Phase 2 validates the accepted value set on `__init__` and - transactional `set_params`, and handles wild-bootstrap-without-cluster - explicitly; silence is not an option. + selection. Fail closed means REFUSE, not warn-and-degrade: Phase 2 pins the + accepted set to exactly `{"analytical", "wild_bootstrap"}` on `__init__` and + transactional `set_params`, and makes `wild_bootstrap` without `cluster=` + raise `ValueError`. REGISTRY specifies WCR as `inference="wild_bootstrap"` + *(with `cluster=`)*, so that combination is incoherent rather than merely + unsupported - and a warning still leaves a procedure running that the caller + did not ask for. The apparent `inference=` vs `n_bootstrap>0` split is NOT drift: an estimator whose bootstrap IS its inference method runs a different procedure - @@ -530,6 +539,18 @@ missed.** as a class surface; only surfaces scheduled for REMOVAL ([M-070]..[M-077]) are exempt, because the removal moots the name. +11. **A rename's scope includes every READER of the old name.** A row is not + done when the definition site is renamed - it is done when nothing still + reads the old name. Internal consumers migrate in the same diff, and the + row's `code_refs` name them so the removal PR cannot miss them (see + [M-043] / [M-095], whose readers span `power.py`, `practitioner.py`, + `_reporting_helpers.py`, `business_report.py` and REPORTING.md). The + specific hazard is `getattr(obj, "old_name", )`: after removal it + returns the DEFAULT instead of raising, so the consumer silently reports + the wrong thing rather than crashing - a wrong-answer regression that no + removal pin catches. Grep for the old name across `diff_diff/` and + `docs/methodology/` before marking any rename row terminal. + **Domain vocabulary that is NOT a violation** (recorded so the sweep is not re-litigated): the staggered family's `group`/`groups` on results containers and `GroupTimeEffect.group` name the ATT(g,t) COHORT in Callaway-Sant'Anna's @@ -561,7 +582,7 @@ above; anything only one PR cares about stays in that PR's plan.** | 2: contract foundations | 3.9 | (a) results base + unified event-study representation [M-092] + to_dict completion + the Diagnostic marker base on the diagnostic result roster [M-091] (section 3.5); (b) `aggregate()` + fit(aggregate=) shims [M-020..M-027]; (c) param renames [M-030..M-047] [M-084] [M-086..M-089] + their results-field mirrors [M-094] [M-095] (section 8 rule 9) + the public-function completeness sweep [M-097..M-113] (section 8 rule 10) + the dCDH results mirror [M-114] + the fourth `robust` site [M-115] + BaseEstimator mixin + ContinuousDiD covariates move; (d) alias introductions [M-062] [M-063] + wrapper deprecations [M-070..M-077] + the two inference-surface policies: `n_bootstrap` semantic unification [M-081] and the wild-cluster-bootstrap roster guard [M-096] | | 3: merges | 3.9 | (a) TWFE event-study mode [M-010] + EventStudy warn [M-060] (gates: section 4.1's equivalence/divergence/pooled-parity test triple); (b) TripleDifference facade [M-013]; (c) CiC method= [M-015] | | 4: release + soak | 3.9 cut | Migration guide written (skeleton: section 10); maintainer cuts 3.9; maint/3.8 rule active | -| 5: enforcement | 4.0 | Removals [M-010..M-016, M-030..M-047 old names, M-060, M-061, M-070..M-077, M-001..M-003]; storage flips [M-050..M-058]; default policies [M-004..M-006, M-080]; warning retirement [M-007]; fastpath go/no-go [M-008]; diagnostic-family docs/roster reorganization [M-090]; docs/llms.txt/README refresh | +| 5: enforcement | 4.0 | Removals [M-010..M-016, M-030..M-047 old names, M-060, M-061, M-070..M-077, M-001..M-003] + the amendment's old names [M-094] [M-095] [M-097..M-115] (incl. their consumer migrations and the `clean_control` serialized reporting key); storage flips [M-050..M-058]; default policies [M-004..M-006, M-080]; warning retirement [M-007]; fastpath go/no-go [M-008]; diagnostic-family docs/roster reorganization [M-090]; sentinel retirement [M-093]; docs/llms.txt/README refresh | | 6: front door | 4.1 | `event_study(data, outcome, unit, time, first_treat, estimator=...)` comparison entry point over the staggered family (sketch only; specified in its own plan) | **3.9-cut checklist (un-rowed obligations).** `test_due_rows_are_terminal`