Repository navigation
feat(v4): PR 3c of the 4.0 cut - remove the ImputationDiD/TwoStageDiD/ContinuousDiD fit-time keywords and relay the bootstrapped surfaces - #865
Conversation
…/ContinuousDiD fit-time keywords and relay the bootstrapped surfaces Spec section 9 item 3, sub-item 3c (3a #863 migrated the suites, 3b #864 removed the CS/EfficientDiD/StackedDiD/dCDH/HAD keywords). Ledger rows M-021, M-022, M-025, M-118 and M-119 flip to removed; M-093 and M-127 are amended. - diff_diff: ImputationDiD / TwoStageDiD / ContinuousDiD lose the fit-time aggregate= (and balance_e=) parameters (ContinuousDiD's "eventstudy" spelling and value validation, its dead analytic fit-time inference arm and ImputationDiD's unreachable pretrends+replicate fit-time gate go with them). A fit with n_bootstrap > 0 ALWAYS materializes the percentile event-study (+ group on Imputation/TwoStage) surfaces at fit - fail-soft, with a one-shot overall-only retry - and aggregate('event_study'|'group') RELAYS them; balance_e on such a fit fails closed; a bootstrapped fit with no stored surface raises the shared absent-surface ValueError (aggregation.absent_bootstrap_surface_message). Analytic fits are unchanged. practitioner / diagnostic_report / results_base hints / _reporting_helpers / plot_event_study messages and the llms*.txt guides describe the relay. - docs: ledger rows + M-093 producer table + M-127 exact predicate, spec section 6 amendment + HYBRIDS paragraph + "(SHIPPED, PR 3c)", REGISTRY Imputation/TwoStage/Continuous notes + three bootstrapped-fit relay Notes + CS/dCDH relay clauses, REPORTING, variance-conventions, migration guide (slot shifts, bootstrapped-fits box, Fix cells), ContinuousDiD API page and tutorial 14 markdown, performance docs, TODO rows. - tests: test_aggregate_contract retires the three remaining Test*Shim classes, fit-time fixtures and fail-closed pins for Test*Removal pins, relay-equals-stored-surface + oracle pins and failure-injection pins for the fail-soft materialization; the estimator suites, DiagnosticReport, event_study_surface (bootstrapped fits as native producers), practitioner, guides and naming-guard tests take their relay pins; the module-level FutureWarning filters for the fit-time route are gone.
Overall assessment: ✅ Looks goodNo unmitigated P0 or P1 findings identified in the supplied diff. Executive summary
MethodologyP3 — Informational: documented relay limitations. Cross-checked the changed paths against the registry, source-linked docstrings, and repository paper analyses covering BJS Theorems 2–3/Test 1, Gardner §3.3, and CGBS binarized aggregation. Bootstrap surfaces carry no joint covariance and clear per-row df; Imputation’s pretrend leads retain their analytical inference. Bootstrapped balanced-window requests fail closed. These conventions are explicitly documented at
No undocumented weighting, comparison-group, identification, or variance change was identified. Code QualityNo findings. The new branches handle absent surfaces explicitly and clear incomplete surfaces before analytical fallback. No new unsafe inline inference calculation was identified. PerformanceP3 — Informational: additional bootstrap fit work. Event-study/group materialization adds computation, intentionally documented and quantified in
MaintainabilityNo findings. Shared absent-surface guidance is centralized, and the three removed signatures contain no remaining references to the deleted parameters. Tech DebtP3 — Informational: replay work is tracked. Balanced-window bootstrap replay and ContinuousDiD post-fit replay remain recorded in
SecurityNo findings in the changed code. Documentation/TestsNo actionable findings. The release-note fragment follows Syntax checks passed for all 31 changed Python files. Runtime tests could not run because pytest, NumPy, pandas, and SciPy are absent. Primary-paper verification was limited to the repository’s source-mapped reviews because external retrieval failed. |
…bounded columns The Linux and Windows Rust-backend legs failed one contract pin, TestTwoStageAggregate::test_bootstrap_prop5_relay_matches_oracle_and_aggregator_warnings_fire: ts/boot/p5/es[1].t_stat drifted 1.56-1.59e-13 relative from the macOS capture while att and se each passed at rtol 1e-13. A t-statistic is att/se, so its relative drift compounds to 2 * rtol to first order; the bootstrapped Prop-5 arm had never been compared on a non-Accelerate platform before this PR (main pins only its analytic arm). Apply the derived bound to every oracle t_stat comparison (event-study, group, overall, total, ContinuousDiD overall) via _t_rtol(); att, se, p_value and CI tolerances are unchanged.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment: ✅ Looks goodNo unmitigated P0 or P1 findings identified. The prior review’s assessment remains unchanged. Executive summary
MethodologyP3 — Informational: documented bootstrap relay limitations. Cross-checked the changed paths against the registry, source-linked docstrings, and repository paper analyses covering BJS Theorems 2–3/Test 1, Gardner §3.3, and CGBS binarized event-study aggregation. The relays preserve stored inference, clear per-row df, and expose no joint covariance. Imputation’s pretrend leads retain analytical inference. Bootstrap balancing restrictions and ContinuousDiD’s absent-surface behavior are explicitly documented at
Direct primary-paper retrieval was blocked by DNS; paper verification relied on the repository’s source-mapped analyses. Code QualityNo actionable findings. The new failure branches clear incomplete surfaces, preserve the documented overall-only retry behavior, and reject unavailable surfaces explicitly. No new unsafe inline inference calculation was identified. PerformanceP3 — Informational: additional bootstrap fit work.
MaintainabilityNo findings. Absent-surface guidance is centralized, and static inspection found no stale references to removed parameters inside the three changed Tech DebtP3 — Informational: replay extensions are tracked.
SecurityNo findings in the supplied changes. Documentation/TestsNo actionable findings. The changelog fragment follows the project convention. Added coverage addresses oracle parity, retry failures, storage rules, positional binding, and downstream consumers. The t-statistic tolerance change at Validation completed: syntax checks for 31 changed Python files and |
Summary
aggregate=/balance_e=keywords -ImputationDiD(M-021/M-118),TwoStageDiD(M-022/M-119),ContinuousDiD(M-025) - and flips those five ledger rows toremoved; passing the keywords is now Python's nativeTypeError, andsurvey_designmoves to positional slot 7 on all three. With them go ContinuousDiD's"eventstudy"spelling and fit-time value validation, its dead analytic fit-time inference arm, and ImputationDiD's unreachable pretrends+replicate fit-time gate (M-127records the exact predicate).TripleDifference's staggered-mode keyword is the one surviving fit-timeaggregate=(M-140/M-141).n_bootstrap > 0now ALWAYS materializes the percentile event-study (+ group on Imputation/TwoStage) surfaces at fit - FAIL-SOFT with a one-shot overall-only retry - stores them in the legacy fields, andaggregate('event_study'|'group')relays them with kit-snapshot provenance (NaN df column,vcovNone).balance_eon such a fit fails closed; a bootstrapped fit with no stored surface (pre-4.0 pickle, fail-soft drop, ContinuousDiD with no post-treatment cells) raises the shared absent-surfaceValueError. Analytic fits are unchanged (legacy fields None, kit recompute). Storage rule, fail-soft retry and the ContinuousDiD no-analytical-fallback clause are stated in the spec, the changelog and the REGISTRY relay Notes.practitioner_next_steps,DiagnosticReport,results_basehints,_reporting_helpers,plot_event_study), thellms*.txtguides, REGISTRY (Imputation / TwoStage / Continuous notes + three bootstrapped-fit relay Notes, CS/dCDH relay clauses), REPORTING, variance-conventions, the migration guide (slot shifts, bootstrapped-fits box, Fix cells), the ContinuousDiD API page and tutorial 14 markdown, the performance docs and the two TODO rows describe the relay.docs/v4-design.mdsection 6 carries the amendment and the HYBRIDS paragraph; section 9 marks sub-item 3c shipped;M-093gains the producer table of which classes populate the legacy fields.tests/test_aggregate_contract.py: the three remainingTest*Shimclasses, fit-time fixtures and fail-closed bootstrap pins are retired forTest*Removalpins (TypeError, signature, positional-slot binding), relay pins (the relayed surfaces equal the stored dicts row-for-row and match the per-backend bootstrap oracles) and failure-injection pins for the fail-soft materialization and the storage rule. The estimator suites,DiagnosticReport,event_study_surface(bootstrapped Imputation / TwoStage / Continuous enrolled as native producers), practitioner, guides and naming-guard tests take their relay pins; the module-level FutureWarning filters for the fit-time route are gone. The committed oracle fixture is unchanged.Methodology references (required if estimator / math changes)
docs/methodology/REGISTRY.mdImputationDiD / TwoStageDiD / ContinuousDiD sections (bootstrapped-fit relay Notes added)Validation
tests/test_aggregate_contract.py(459 tests, numpy and rust backends),tests/test_imputation.py,tests/test_two_stage.py,tests/test_continuous_did.py,tests/test_methodology_imputation.py,tests/test_pretrends_event_study.py,tests/test_survey_phase8.py,tests/test_diagnostic_report.py,tests/test_event_study_surface.py,tests/test_practitioner.py,tests/test_guides.py,tests/test_naming_guard.py. The touched and consumer suites (23 files, 2,856 tests) pass;test_v4_matrixsees the five rows flip (due-at-4.0 non-terminal rows 16 -> 11); the contract file and the three estimator suites also pass under the Python 3.9 CI leg's scipy 1.13.1 / numpy 2.0.2 pins.mypy diff_diffclean; Sphinx-Wbuild clean; tutorial 14 executes under nbmake.Security / privacy
Changelog
changelog.d/20261021-v4-pr3c-aggregate-relay.md(### Removed,### Breaking Changes,### Changed,### Internal)Follow-ups
docs/v4-design.mdsection 9.