Skip to content

feat(v4): PR 3 of the 4.0 cut - remove the fit-time aggregate=/balance_e= keywords - #861

Closed
igerber wants to merge 1 commit into
mainfrom
feat/v4-pr3-aggregate-removals
Closed

igerber wants to merge 1 commit into
mainfrom
feat/v4-pr3-aggregate-removals

Conversation

@igerber

@igerber igerber commented Oct 19, 2026

Copy link
Copy Markdown
Owner

Summary

PR 3 of the 4.0 cut (docs/v4-design.md section 9 "4.0 cut sequence", item 3). The deprecated fit-time aggregation keywords are removed and 13 ledger rows flip terminal under the cut window opened by #854: M-020..M-027 (fit(aggregate=) on CallawaySantAnna, ImputationDiD, TwoStageDiD, EfficientDiD, StackedDiD, ContinuousDiD, ChaisemartinDHaultfoeuille, HeterogeneousAdoptionDiD), M-117..M-120 (fit(balance_e=) on CS / Imputation / TwoStage / EfficientDiD) and M-139 (did_had_pretest_workflow(aggregate=)). __version__ stays 3.13.0.

  • Removed: passing aggregate= / balance_e= to fit() is a native TypeError; the successor is post-fit results.aggregate(type, balance_e=). HAD and the pretest workflow select their mode from the panel shape (_infer_aggregate_mode is the sole selector; the HADPretestReport.aggregate field survives as output metadata). ContinuousDiD's "eventstudy" spelling dies with the keyword; StackedDiD's group/all ValueErrors die with it (aggregate("group") raises the mixin's vocabulary error).
  • CS / EfficientDiD compute no event-study or group surface at fit any more (post-fit aggregate() replays bootstrapped fits); their legacy event_study_effects / group_effects / event_study_vcov / cband_crit_value / event_study_df fields are None on every fit (retired later by M-093).
  • Bootstrapped ImputationDiD / TwoStageDiD / ContinuousDiD fits now always materialize the event-study (and group) surfaces with the existing fit-time bootstrap code, and aggregate("event_study") / aggregate("group") relay them (kit-snapshot provenance, NaN df column, no vcov). Today those levels fail closed on bootstrapped fits, so the removed keyword was the only route to bootstrap event-study inference there. balance_e on such a fit raises NotImplementedError. Materialization is fail-soft with a one-shot overall-only retry, and surfaces are stored iff the fit carries bootstrap inference, so a plain bootstrapped fit's overall quintet and its success/abort outcome are unchanged; analytic fits recompute from the kit exactly as before.
  • Positional slots shift: survey_design 9 -> 7 on CS / Imputation / TwoStage / EfficientDiD (EDiD store_eif 10 -> 8); StackedDiD population / survey_design / covariates 7/8/9 -> 6/7/8; ContinuousDiD survey_design / covariates 8/9 -> 7/8; dCDH L_max through survey_design each one slot left; HAD none.
  • Downstream: steering messages, _ABSENT_SURFACE_HINTS, practitioner_next_steps, the plotter's unsupported-input message, DiagnosticReport comments; DiagnosticReport / BusinessReport / plot_event_study now run on the relayed surfaces of bootstrapped relay-adopter fits. Guides (llms*.txt), API rst, troubleshooting, performance docs, migration guide (warning box + appendix rows + slot shifts), REGISTRY Notes (incl. three new Note (bootstrapped-fit relay, 4.0) entries), REPORTING / variance-conventions, TODO rows 40-41, spec section 6 amendment + item 3 shipped, two notebook markdown cells, benchmarks (fit + post-fit aggregate workload; legacy-field readers rewired to the containers, same emitted JSON shape).

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no estimator equation, weighting, control-group or variance computation changes. The relay returns the fit's own percentile surface; the analytic recompute path is untouched.
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None. The documented behaviour changes (relay on bootstrapped fits, fail-soft materialization, the aggregators' warnings now reachable on bootstrapped plain fits, pre-4.0 restricted-pickle limitation) are recorded in docs/methodology/REGISTRY.md under the three estimators' Note (bootstrapped-fit relay, 4.0) entries and in docs/v4-design.md section 6.

Validation

  • Tests added/updated: tests/test_aggregate_contract.py converted from fit-time-reference parity to committed per-backend numeric oracles (tests/fixtures/v4_aggregate_oracles.json, captured on the unmodified tree at 8724fec by tests/_capture_v4_aggregate_oracles.py, numpy + rust weight streams), removal pins, relay / storage-rule / failure-injection pins; ~45 suites migrated to the containers through the new tests/helpers/legacy_surface.py; tests/test_naming_guard.py allowlist, tests/test_guides.py absence pins, tests/test_practitioner.py, tests/test_v4_rename_mechanical.py (warn_deprecated_kwarg unit pin).
  • Local runs: full default suite 15,304 passed / 0 failed; the 15 shim-free suites under -W error::FutureWarning -W error::DeprecationWarning (1,883 passed); black / ruff / mypy clean; changelog_compile.py check OK; Sphinx -W build clean; benchmarks py_compile + smoke; oracle parity on both backends (max deviation 1.8e-15 analytic, 0.0 bootstrapped). Due-count probe 0 / 76 / 43 at 3.13.0 / 3.14.0 / 4.0.0 (the 4.0 count drops by the 13 flipped rows).
  • Backtest / simulation / notebook evidence (if applicable): N/A (notebook edits are markdown cells only).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Changelog

  • changelog.d/ fragment added (or N/A - no user-visible change): Yes - changelog.d/20261019-v4-pr3-aggregate-removals.md

…e_e= keywords

Spec section 9 "4.0 cut sequence" item 3. Thirteen ledger rows flip terminal
under the open cut window (__version__ stays 3.13.0): M-020..M-027 (the eight
estimators' fit(aggregate=)), M-117..M-120 (fit(balance_e=) on CS / Imputation /
TwoStage / EfficientDiD) and M-139 (did_had_pretest_workflow(aggregate=)).

- Passing aggregate= / balance_e= to fit() is a native TypeError; the successor
  is the post-fit results.aggregate(type, balance_e=). HAD and the pretest
  workflow select their mode from the panel shape (_infer_aggregate_mode is
  the sole selector; the HADPretestReport.aggregate FIELD survives).
- CS / EfficientDiD compute no event-study/group surface at fit any more
  (post-fit replay covers bootstrapped fits); their legacy fields are None on
  every fit. Positional slots shift: survey_design 9->7 (+ EDiD store_eif
  10->8); StackedDiD population/survey_design/covariates 7/8/9->6/7/8;
  ContinuousDiD survey_design/covariates 8/9->7/8; dCDH L_max..survey_design
  each one left.
- Bootstrapped ImputationDiD / TwoStageDiD / ContinuousDiD fits ALWAYS
  materialize the event-study (+ group) surfaces with the existing fit-time
  bootstrap code and aggregate('event_study' | 'group') RELAYS them with
  kit-snapshot provenance (NaN df, no vcov); balance_e on such a fit fails
  closed (NotImplementedError). Materialization is fail-soft with a one-shot
  overall-only retry, and surfaces are stored iff the fit carries bootstrap
  inference, so a plain bootstrapped fit's overall quintet and its
  success/abort outcome are unchanged; analytic fits are untouched (kit
  recompute). ContinuousDiD's "eventstudy" spelling dies with the param.
- Steering messages, guides, API rst, migration guide, REGISTRY / REPORTING /
  variance-conventions Notes, TODO rows, benchmarks (fit + post-fit aggregate
  workload, readers rewired to the containers) and two notebook markdown
  cells updated; DiagnosticReport / BusinessReport / plot_event_study now
  run on the relayed surfaces of bootstrapped relay-adopter fits.
- Tests: tests/test_aggregate_contract.py converted from fit-time-reference
  parity to committed per-backend numeric oracles
  (tests/fixtures/v4_aggregate_oracles.json, captured on the unmodified tree
  by tests/_capture_v4_aggregate_oracles.py), removal pins, relay / storage
  rule / failure-injection pins; ~45 suites migrated to the containers via
  tests/helpers/legacy_surface.py; naming-guard allowlist and guide pins
  updated; warn_deprecated_kwarg keeps a unit pin.
@igerber

igerber commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Closing in favour of the three-way split; the first piece is #863 (3a, tests-first). 3b and 3c follow once it merges.

@igerber igerber closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant