Repository navigation
test(v4): PR 3a of the 4.0 cut - migrate the suites to the post-fit aggregation containers - #863
Conversation
…ggregation containers
Spec section 9 item 3, split three ways (3a/3b/3c) so each piece fits the AI
reviewer's input cap. This is the tests-first piece: no library source change
and no ledger row flips; the deprecated fit-time aggregate=/balance_e= keywords
still work and still warn.
- Test and benchmark sites read event-study / group surfaces from
results.aggregate("event_study") / ("group") instead of fit(aggregate=...)
and the legacy event_study_effects / group_effects fields. Bootstrapped
ImputationDiD / TwoStageDiD / ContinuousDiD sites stay on the fit-time route
until PR 3c adds the post-fit relay (the post-fit route raises
NotImplementedError there today).
- tests/test_aggregate_contract.py: fit-time-reference parity converted 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); the Test*Shim classes, fit-time
fixtures and fail-closed pins stay until the removals land.
- New tests/helpers/legacy_surface.py (es_dict, group_dict, legacy_standin)
for consumer tests whose subject is the native (legacy-pickle) route.
- Benchmarks call aggregate() post-fit inside the timed region and emit the
same JSON row shape the R refresh joins on.
- docs/v4-design.md section 9 records the split; changelog fragment under
Internal.
Overall assessment✅ Looks good — no unmitigated P0/P1 findings. One nonblocking P2 test issue. Executive summary
MethodologyNo defects identified. The migrated tests preserve R/Stata numerical comparisons and respect documented reference-row normalization, backend-specific bootstrap streams, and level-matched replicate inference. These conventions are covered by the existing Notes in Code QualityNo additional findings. PerformanceNo findings. Aggregations remain inside benchmark timing regions, including the separate bootstrap replays required by the post-fit API. MaintainabilityNo findings. Shared helpers centralize the legacy-surface conversions. Tech DebtP3 — Existing bootstrap limitations remain tracked.
SecurityNo findings in the changed code or fixtures. Documentation/TestsP2 — Clustering comparison succeeds because reference SEs are NaN.
Validation: all 48 changed Python files parsed successfully. Fixture checks found no label/covariance-shape issues or inference-consistency issues across 870 non-reference rows. Pytest execution was unavailable because pytest, NumPy, pandas, and SciPy are absent. |
…ed oracle Review follow-up (PR 3a, P2). The clustered TwoStageDiD contract test compared the clustered and unclustered event-study SE arrays with a negated np.array_equal; both carry a NaN reference-row SE, so array_equal was always False and the assertion passed even if the post-fit aggregation ignored cluster=. The numerical coverage the fit-time comparison used to give was also missing on this arm. - tests/test_aggregate_contract.py: _twostage_clustered_panel() helper (mirrors _imputation_clustered_panel); test_cluster_arm_matches_oracle pins the event-study and group surfaces to the new TwoStageDiD clustered_all oracle arm at ANALYTIC_TOL, and the clustering-changes-variance check now compares the estimated (non-reference) rows only, asserting they are finite and not allclose. - tests/_capture_v4_aggregate_oracles.py: capture the clustered_all arm. - tests/fixtures/v4_aggregate_oracles.json: the one new arm; re-captured under both backends on a tree whose diff_diff/ is byte-identical to the recorded captured_at_head, every pre-existing analytic and bootstrap arm reproduced exactly (0 differences), numpy and rust analytic within rtol 1e-12 / atol 1e-13.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — the previous P2 finding is resolved. No unmitigated P0/P1 findings identified. Executive summary
MethodologyNo defects identified. The revised clustering coverage agrees with the Gardner GMM variance contract and documented reference-row conventions in Code QualityNo findings. PerformanceNo new findings. The clustering regression adds a small deterministic fit without changing production execution. MaintainabilityNo findings. The capture script and test use the same clustered-panel builder. Tech DebtP3 — Existing bootstrap limitations remain tracked.
SecurityNo findings in the changed code or fixtures. Documentation/TestsPrevious P2 resolved — clustering comparison is now meaningful.
Validation found no label, covariance-shape, covariance-diagonal, or inference-consistency problems across 110 fixture bundles, including 876 non-reference rows and 32 covariance matrices. The changelog fragment check passed. |
…py t.ppf drift) CI follow-up (PR 3a). The Python 3.9 leg failed seven analytic oracle checks, every one on a survey arm's ci_lo at relative differences of 1e-12 to 3e-11 against ANALYTIC_TOL's rtol 1e-12; the other 16 legs passed. That leg pins scipy 1.13.1, whose t.ppf differs from 1.14+ by up to 3.4e-11 relative for df in the 100-130 range (the survey arms' df), while t.sf (p-values) agrees to 1e-15. Reproduced: scipy 1.13.1 + numpy 2.0.2 on Python 3.12 fails exactly those seven ids and passes 463/463 with this change. - tests/test_aggregate_contract.py: CI_RTOL = 1e-9, documented beside ANALYTIC_TOL; the three oracle helpers compare ci_lo / ci_hi (and the sup-t band bounds) at max(rtol, CI_RTOL). att / se / t_stat / p_value / vcov keep the arithmetic tolerance. - changelog fragment: states the CI-bound tolerance.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — the previous clustering finding remains resolved. No unmitigated P0/P1 findings identified. Executive summary
MethodologyNo findings. The clustering assertions agree with the Gardner GMM variance contract and covariance conventions in The interval tolerance adjustment in Code QualityNo findings. No new inline-inference or zero-SE fallback anti-patterns were found. PerformanceNo new findings. Migrated benchmark aggregations remain inside their timed regions where both aggregation surfaces are measured. MaintainabilityNo findings. The clustered-panel builder remains shared between oracle capture and regression tests. Tech DebtP3 — Existing bootstrap limitations remain tracked.
SecurityNo findings in the changed code or fixtures. Documentation/TestsPrevious P2 resolved — clustering comparison remains meaningful.
Static validation found no label, inference-consistency, covariance-shape, symmetry, or diagonal problems across 110 fixture bundles, including 876 non-reference rows and 32 covariance matrices. The changelog fragment check and diff whitespace check passed. Runtime behavior remains unverified in this environment. |
1 participant
Summary
diff_diff/change and no ledger row flips. The fit-timeaggregate=/balance_e=keywords still work and still warn.results.aggregate("event_study")/results.aggregate("group")(the container API that has existed since 3.9) instead offit(aggregate=...)and the legacyevent_study_effects/group_effectsfields. Bootstrapped ImputationDiD / TwoStageDiD / ContinuousDiD sites stay on the fit-time route until PR 3c adds the post-fit relay (the post-fit route raisesNotImplementedErroron those fits today).tests/test_aggregate_contract.pyno longer uses the fit-time route as its numerical reference: every analytic surface is pinned atrtol=1e-12and every bootstrapped CS / EfficientDiD replay at the replay-parity tolerances againsttests/fixtures/v4_aggregate_oracles.json, captured bytests/_capture_v4_aggregate_oracles.pyunder both weight backends on a tree whosediff_diff/is byte-identical to8724fecd(origin/main). Every design arm the file exercises is captured; each event-study bundle carriesref_labelsso reference-row membership is pinned by the oracle rather than read off the container under test (a tamper regression test covers this). TheTest*Shimclasses, fit-time fixtures and fail-closed pins stay until the removals land.tests/helpers/legacy_surface.py(es_dict,group_dict,legacy_standin) for consumer tests whose subject is the native (legacy-pickle) route.benchmarks/python/benchmark_callaway.py,benchmark_imputation.py, thespeed_reviewscripts) callaggregate()post-fit inside the timed region and emit the same JSON row shapebenchmarks/refresh_2026_07/run_refresh.pyjoins on.docs/v4-design.mdsection 9 records the three-way split of cut-sequence item 3 (3a flips no rows, by design).Methodology references (required if estimator / math changes)
Validation
tests/test_aggregate_contract.py(oracle conversion, 463 tests, run underDIFF_DIFF_BACKEND=pythonandrust),tests/helpers/legacy_surface.py,tests/fixtures/v4_aggregate_oracles.json+README.md,tests/_capture_v4_aggregate_oracles.py, and ~40 migrated suites (test_staggered,test_csdid_ported,test_methodology_callaway,test_efficient_did,test_imputation,test_two_stage,test_continuous_did,test_had,test_had_pretests,test_diagnostic_report,test_event_study_surface,test_honest_did,test_bootstrap_chunking,test_visualization,test_business_report,test_event_study_consumers,test_lpdid, the survey phase suites, ...). All touched suites pass against unchanged main source: 3,237 + 463 passed.benchmark_callaway.pyandbenchmark_imputation.pysmoke-run on a 36-unit three-cohort panel; emittedevent_study/group_effectsrows keep the{event_time|group, att, se}shape.Security / privacy
Changelog
changelog.d/20261020-v4-pr3a-aggregate-test-migration.md(### Internal)Follow-ups