Skip to content

docs(v4): gating-completeness amendment - mirrored renames, public-function sweep, un-rowed obligations - #727

Merged
igerber merged 2 commits into
mainfrom
docs/v4-amendment-gating-completeness
Jul 25, 2026
Merged

docs(v4): gating-completeness amendment - mirrored renames, public-function sweep, un-rowed obligations#727
igerber merged 2 commits into
mainfrom
docs/v4-amendment-gating-completeness

Conversation

@igerber

@igerber igerber commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Summary

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. Every new row is planned — the shims themselves land in Phase 2c.

Rename coverage. The Phase 1 inventory was derived from the estimator roster, which left two classes of surface unaudited:

  • Results fields mirroring a renamed paramM-094 (RegressionDiscontinuityResults.treatment_col), M-095 (StackedDiDResults.clean_control), M-114 (ChaisemartinDHaultfoeuilleResults.groups.units, whose "groups" are unit ids). All three 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 functionsM-097 (twowayfeweights(group=)unit=, which section 3.3 retains past 4.0, so no wrapper removal covers it), M-098..M-112 (the 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. The TwoWayFixedEffects/MultiPeriodDiD hits inherit DifferenceInDifferences.__init__ and already ride M-045.

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), while rule 7 reserves treatment for a 0/1 indicator and never a treatment level. takeup is the spelling the RD docstring already uses. 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 expressible as a row. 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 while __version__ is still 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 DifferenceInDifferences 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. Repairing that is now gated Phase 2 scope.

M-008's notes record that its telemetry soak window is the 3.9 → 4.0 gap.

Counts. Ledger 77 → 99 rows (floor and immutable-id snapshot extended in the same diff); rows gating a 3.9 bump 45 → 68.

Methodology references (required if estimator / math changes)

  • Method name(s): N/A — no estimator, variance, weighting, or default changes. No files under diff_diff/, rust/src/, or docs/methodology/ are touched.
  • Paper / source link(s): N/A for changes. The amendment does assert bootstrap taxonomy in section 7 and M-096; those statements are sourced from docs/methodology/REGISTRY.md and the implementations — CallawaySantAnna an influence-function multiplier bootstrap, SunAbraham a unit-pairs bootstrap (Rao–Wu rescaled on stratified/PSU survey paths), dCDH a group-level influence-function multiplier bootstrap upgrading to Hall–Mammen PSU wild clustering under strictly-coarser survey PSUs, against DiD/TWFE's residual wild cluster bootstrap with test-inverted CIs.
  • Any intentional deviations from the source (and why): None.

Validation

  • Tests added/updated: tests/test_v4_matrix.pyROW_COUNT_FLOOR 77 → 99, immutable-id snapshot extended with the (94, 96) and (97, 115) ranges. The ledger's existing per-row parametrization covers the 19 new rows: 215 passed, 10 skipped (was 177 passed). tests/test_docs_ia.py 18 passed. Every new locator resolves and every planned precondition holds (old surface present, new surface absent).
  • Backtest / simulation / notebook evidence (if applicable): N/A — no notebooks or numerical surfaces affected.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

…nction 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.
@github-actions

Copy link
Copy Markdown

Overall assessment

⚠️ Needs changes — two unmitigated P1 findings.

Executive summary

  • The fuzzy-RD takeup retarget and bootstrap taxonomy agree with the Methodology Registry.
  • M-096 contradicts its own fail-closed requirement by permitting analytical fallback.
  • M-095 omits reporting consumers that will silently describe the wrong control group after removal.
  • Several canonical-signature and phase-table references remain stale.

Methodology

P1 — Wild-bootstrap prerequisite is not actually fail-closed

  • Location: docs/v4-design.md:L425-L442, docs/v4-deprecations.yaml:L997-L1008
  • Impact: The registry defines WCR as requiring cluster= (docs/methodology/REGISTRY.md:L100-L107), yet M-096 allows either “raise or warn.” A warned analytical fallback still executes a different inference procedure than explicitly requested and contradicts the normative “must fail closed” statement.
  • Concrete fix: Lock the accepted values to {"analytical", "wild_bootstrap"} and require ValueError when wild_bootstrap lacks cluster=. Require tests for constructor validation, transactional set_params, DiD, and TWFE.

The fuzzy-RD takeup semantics and the CS/SA/dCDH bootstrap distinctions otherwise match the registry and docstrings.

Code Quality

P1 — M-095 misses downstream field consumers

  • Location: docs/v4-deprecations.yaml:L516-L528; consumers at diff_diff/business_report.py:L1415-L1444 and diff_diff/_reporting_helpers.py:L199-L214
  • Impact: Both consumers read results.clean_control directly. Once that compatibility property is removed, "strict" and "never_treated" fits silently fall into the default "not_yet_treated" description, misreporting the identifying comparison group.
  • Concrete fix: Add these consumers and docs/methodology/REPORTING.md to M-095’s migration scope. Add tests using a result exposing only control_group, covering all three values.

Performance

No findings; runtime code is unchanged.

Maintainability

P2 — Phase 5 omits the new 4.0 removals

  • Location: docs/v4-design.md:L558-L579
  • Impact: The phase table omits M-094/M-095 and M-097–M-115 from Phase 5 despite asserting agreement with the ledger.
  • Concrete fix: Add those removal rows to the Phase 5 entry.

Tech Debt

P3 — Naming-completeness automation is tracked

  • Location: TODO.md:L48
  • Impact: The public-surface sweep remains manual.
  • Concrete fix: None required for this PR; properly tracked in TODO.md.

Security

No findings or apparent secrets.

Documentation/Tests

P2 — Canonical API documentation is internally inconsistent

  • Location: CHANGELOG.md:L15-L17, docs/v4-design.md:L140-L149, docs/v4-design.md:L421-L423
  • Impact: The changelog and canonical signature still say treatment, while M-042/M-094 now specify takeup; the robust references omit M-115.
  • Concrete fix: Replace the stale target with takeup and extend both robust-row references through M-115.

Focused tests could not run because pytest is unavailable in the review environment.

Path to Approval

  1. Make M-096 require an error for wild_bootstrap without cluster= and pin the exact accepted selector values.
  2. Expand M-095 to migrate and test the reporting consumers against the new control_group field.

…ale 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.
@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 4b15d6767f24b91d7105a1aa763d49614d742624


Overall assessment

Looks good — both prior P1 findings are resolved; no unmitigated P0/P1 findings remain.

Executive summary

  • M-096 now requires fail-closed validation and ValueError without cluster=.
  • M-095 now covers all reporting consumers and the serialized legacy key.
  • Fuzzy-RD takeup and bootstrap taxonomy match the Methodology Registry.
  • One consumer-coverage item remains, but is explicitly tracked by the new TODO guard.

Methodology

No findings. The fuzzy-RD local-Wald semantics and distinct DiD/TWFE, CS, SA, and dCDH bootstrap procedures agree with REGISTRY.md.

Code Quality

No findings. The previous M-095 reporting-consumer omission is addressed at docs/v4-deprecations.yaml:L516-L528.

Performance

No findings; runtime code is unchanged.

Maintainability

No findings. Phase 2 and Phase 5 now include the amendment rows.

Tech Debt

P3 — M-115 consumer coverage is tracked

  • Location: docs/v4-deprecations.yaml:L1298-L1309; consumer at diff_diff/sun_abraham.py:L1677-L1685; tracking at TODO.md:L48
  • Impact: Removing LinearRegression.robust without migrating Sun–Abraham’s robust=True call would raise TypeError.
  • Concrete fix: No immediate action required. The tracked pre-Phase-2c guard should add sun_abraham.py to M-115’s scope and require removing that keyword with a regression test.

Security

No findings or apparent secrets.

Documentation/Tests

No findings. The ledger parses as 99 unique rows and the modified test file compiles. The pytest suite could not run because pytest and runtime dependencies are unavailable in the review environment.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 25, 2026
@igerber
igerber merged commit cba7258 into main Jul 25, 2026
35 of 36 checks passed
@igerber
igerber deleted the docs/v4-amendment-gating-completeness branch July 25, 2026 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant