Skip to content

feat(v4): PR 3b of the 4.0 cut - remove the fit-time aggregate=/balance_e= keywords on CS, EfficientDiD, StackedDiD, dCDH and HAD - #864

Merged
igerber merged 1 commit into
mainfrom
feat/v4-pr3b-aggregate-removals-mechanical
Oct 21, 2026
Merged

igerber merged 1 commit into
mainfrom
feat/v4-pr3b-aggregate-removals-mechanical

Conversation

@igerber

@igerber igerber commented Oct 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • 4.0 cut, PR 3b of three (3a test(v4): PR 3a of the 4.0 cut - migrate the suites to the post-fit aggregation containers #863 migrated the suites; 3c carries the ImputationDiD / TwoStageDiD / ContinuousDiD removals plus the bootstrapped-fit relay). This is the mechanical half: the fit-time aggregate= / balance_e= keywords are removed where the post-fit route already exists - CallawaySantAnna (M-020/M-117), EfficientDiD (M-023/M-120), StackedDiD (M-024), ChaisemartinDHaultfoeuille (M-026), HeterogeneousAdoptionDiD (M-027) and did_had_pretest_workflow (M-139). The eight ledger rows flip to removed; passing the keywords is now Python's native TypeError.
  • CallawaySantAnna and EfficientDiD compute no event-study/group surface at fit: the legacy fields (and CS's cband_crit_value / event_study_vcov / event_study_vcov_index / event_study_df) are None after every fit, and the post-fit replay is the only bootstrap route (unchanged). StackedDiD's fit-time group/all ValueErrors and the HAD _VALID_AGGREGATES sets go with the keywords; _infer_aggregate_mode is HAD's sole selector. Positional slots after the removed parameters shift left (documented in the migration guide).
  • Runtime messages (honest_did, pretrends, plot_event_study, _reporting_helpers, diagnostic_report), the four llms*.txt guides, REGISTRY / REPORTING notes, the migration guide, API and performance docs stop steering users to the removed route on these five estimators. Wording about ImputationDiD / TwoStageDiD / ContinuousDiD stays true at this PR (their keyword is still deprecated, not removed). docs/v4-design.md section 6 gains the 3b amendment and section 9 marks sub-item 3b shipped; ledger row M-093 records that CS/EDiD populate the legacy fields on no fit.
  • tests/test_aggregate_contract.py: the five Test*Shim classes, the fit-time fixtures (fit_time, efficient_fit_time, _boot_fit_time, _efficient_boot_fit_time) and the inertness comparisons are retired for Test*Removal pins (TypeError, signature, positional-slot binding); the committed oracle fixture stays the numerical reference. tests/_capture_v4_aggregate_oracles.py is marked historical (it drives the removed route; runnable only at the recorded captured_at_head). warn_deprecated_kwarg loses its last library caller and is retained per spec section 8 rule 11 with a direct unit pin.

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no estimator mathematics changes (parameter removals, message and documentation sweep; every retained numerical surface is pinned against the committed per-backend oracles captured on the pre-removal tree)
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_aggregate_contract.py (437 tests, numpy and rust backends), tests/test_staggered.py, tests/test_efficient_did.py, tests/test_stacked_did.py, tests/test_chaisemartin_dhaultfoeuille.py, tests/test_had.py, tests/test_had_pretests.py, tests/test_had_dual_knob_deprecation.py, tests/test_v4_rename_mechanical.py, tests/test_guides.py, tests/test_naming_guard.py, tests/test_event_study_surface.py, comment-only wording in nine CS/EDiD consumer suites. The touched and consumer suites (35 files, 4,132 tests) pass; test_v4_matrix sees the eight rows flip (due-at-4.0 non-terminal rows 24 -> 16). mypy diff_diff clean; Sphinx -W build clean; tutorial 02 executes.
  • Backtest / simulation / notebook evidence (if applicable): N/A

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/20261021-v4-pr3b-aggregate-removals-mechanical.md (### Removed, ### Breaking Changes, ### Changed, ### Internal)

Follow-ups

  • PR 3c: the ImputationDiD / TwoStageDiD / ContinuousDiD removals (M-021/M-022/M-025/M-118/M-119) plus the bootstrapped-fit relay (spec section 6 amendment), prepared on top of this branch and opened once it merges.

…ce_e= keywords on CS, EfficientDiD, StackedDiD, dCDH and HAD

Spec section 9 item 3, sub-item 3b (3a landed the tests-first migration in #863;
3c carries the ImputationDiD/TwoStageDiD/ContinuousDiD removals and the
bootstrapped-fit relay). Ledger rows M-020, M-023, M-024, M-026, M-027, M-117,
M-120 and M-139 flip to removed.

- diff_diff: CallawaySantAnna / EfficientDiD / StackedDiD /
  ChaisemartinDHaultfoeuille / HeterogeneousAdoptionDiD lose the fit-time
  aggregate= (and balance_e=) parameters; did_had_pretest_workflow loses
  aggregate= (the HADPretestReport.aggregate output field survives). CS and
  EfficientDiD compute no event-study/group surface at fit, so their legacy
  fields are None after every fit; the post-fit replay is the only bootstrap
  route. StackedDiD's fit-time group/all ValueErrors and the HAD
  _VALID_AGGREGATES sets go with the keywords. honest_did / pretrends /
  plot_event_study / _reporting_helpers / diagnostic_report messages and the
  four llms*.txt guides stop steering users to the removed route;
  _deprecation.py records that warn_deprecated_kwarg has no library caller.
- docs: ledger rows + M-093 note (CS/EDiD populate the legacy fields on no fit),
  spec section 6 amendment + "(SHIPPED, PR 3b)" marker, REGISTRY CS / EDiD /
  Stacked / HAD notes, REPORTING, migration guide (slot shifts), API/perf docs,
  tutorial 02 comment.
- tests: test_aggregate_contract retires the five Test*Shim classes, the
  fit-time fixtures and the inertness comparisons for Test*Removal pins
  (TypeError, signature, positional-slot binding); the estimator suites take
  their removal pins; test_guides pins the signature blocks without fit-time
  lines; naming-guard allowlist entries for llms.txt / llms-full.txt;
  _capture_v4_aggregate_oracles.py marked historical (runnable only at the
  recorded captured_at_head); warn_deprecated_kwarg gets a direct unit pin.
@github-actions

Copy link
Copy Markdown

Overall assessment: ✅ Looks good

No unmitigated P0 or P1 findings identified.

Executive summary

  • The five estimator removals and HAD workflow removal consistently enforce the intended API.
  • No changes to estimands, weighting, identification checks, or variance formulas were found.
  • Bootstrap replay remains available through post-fit aggregation.
  • Breaking changes have Registry notes, migration guidance, and a changelog fragment.
  • Numerical tests could not run: the available Python lacks pytest, NumPy, and pandas.

Methodology

Reviewed Callaway–Sant’Anna, EfficientDiD, StackedDiD, dCDH, and HAD/pretests against Registry equations, in-code references, and available on-file paper reviews.

  • P3 — Documented API transition. Removing fit-time aggregation and leaving CS/EfficientDiD legacy fields empty matches the explicit Registry notes (docs/methodology/REGISTRY.md:L1284, L1873, L2468, L5022).
    • Impact: Callers must use post-fit containers; positional parameters shift as documented.
    • Concrete fix: None required. These are documented behavior changes, not methodology defects.

No undocumented methodology deviation or newly incorrect inference was identified.

Code Quality

No findings. Static caller checks found no remaining library or benchmark fit(aggregate=...)/fit(balance_e=...) calls. No new inline-inference anti-patterns or incomplete parameter propagation were found.

Performance

No findings. Bootstrap replay retains its documented per-call cost; this PR does not introduce a new replay implementation.

Maintainability

No findings. Public signatures, removal tests, ledger statuses, and runtime guidance agree.

Tech Debt

No new untracked limitation identified. Historical oracle capture instructions explain how to reproduce the committed reference data after removal.

Security

No security issues or accidental secrets identified in the changes.

Documentation/Tests

No actionable findings. The changelog fragment covers the user-visible removals, and regression coverage retains numerical oracles plus keyword-rejection and positional-binding checks.

Validation completed: syntax checks for 18 modified modules and seven dependency-free guide assertions passed. Full pytest execution and numerical oracle verification remain unverified in this environment.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Oct 21, 2026
@igerber
igerber merged commit 3da2f0f into main Oct 21, 2026
43 of 44 checks passed
@igerber
igerber deleted the feat/v4-pr3b-aggregate-removals-mechanical branch October 21, 2026 12:17
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