feat(reports): DiagnosticReport consumes the post-fit aggregate('event_study') surface - #767
Merged
Conversation
…t_study') surface
On a modern plain fit (no fit-time aggregate=), DiagnosticReport now derives
the EventStudyResults container internally - once, cached, fail-soft - so the
event-study-gated checks run without the deprecated kwarg the 4.0 release
removes: parallel_trends / pretrends_power / sensitivity on CallawaySantAnna
(the container is M-093-admitted into the consumers with pinned raw-route
parity), parallel_trends (pretrends=True fits) and heterogeneity on
ImputationDiD / TwoStageDiD, and heterogeneity on ContinuousDiD.
Raw-field precedence is absolute (a present event_study_effects - the
requested-but-empty {} sentinel included - is never re-derived, preserving
fit-time balance_e semantics); the M-093 consumer-admission set, dCDH's
placebo branch, and the EventStudyResults input rejection are unchanged.
Derivation failures fail closed to explicit per-check skip reasons (never
masked by other availability legs), remediation strings are estimator-
accurate (Wooldridge's in-place aggregate; the staggered-DDD canonical
fit-time route; MPD's omitted-reference case), and derived-route sections
carry pre_period_source provenance - attached exactly to the checks whose
gate consulted the derived surface, on success, error, and skip paths alike.
Warnings emitted by a kit recompute are captured exception-safely and
republished on the consuming sections and (deduplicated) the top-level
channel. BusinessReport lifts the provenance key through its whitelist.
Docs: report API pages migrate to plain-fit examples; the applicable_checks
laziness guarantee is amended (one cached derivation may run there);
REPORTING.md documents the new data source and schema key; REGISTRY.md gains
the M-024-style plain-fit notes for CS/Imputation/TwoStage/Continuous; the
M-020/M-021/M-022/M-025 ledger rows gain notes+code_refs; migration-4.0.md
notes the checks survive the kwarg migration. Retires the TODO.md row; a
pre-existing runner-skip bookkeeping gap is recorded as a new TODO row.
PR ReviewOverall assessment✅ Looks good — no unmitigated P0/P1 findings. Executive summary
Methodology
No other methodology mismatch or variance/SE issue found. Code QualityNo findings. PerformanceNo findings. Derivation is cached once per report. MaintainabilityNo findings. Tech Debt
SecurityNo findings or apparent secrets. Documentation/Tests
|
…source provenance The _lift_pre_trends early return (PT status != 'ran') dropped the pre_period_source key DiagnosticReport attaches to gate-skipped sections whose derivation succeeded but produced no pre-horizons; lift it on the skip path too (None on raw routes, matching the computed path). Pinned with a default-TwoStageDiD BusinessReport test.
|
🔁 AI review rerun (requested by @igerber) Head SHA: PR ReviewOverall assessment✅ Looks good — no unmitigated P0/P1 findings. Executive summary
MethodologyAffected methods: Callaway–Sant’Anna, ImputationDiD, TwoStageDiD, ContinuousDiD, Roth pre-trends power, and Rambachan–Roth HonestDiD. No estimator equations or variance estimators change; the report consumes existing aggregation surfaces.
No undocumented methodology, assumption-check, variance, or SE mismatch found. Code QualityNo findings. Raw-field precedence and fail-soft derivation are clearly isolated in PerformanceNo findings. The derived surface is cached and reused for all consumers. MaintainabilityNo findings. M-093 consumer admission remains source-restricted in Tech Debt
SecurityNo findings or apparent secrets. Documentation/TestsThe previous P2 is resolved: The added tests cover raw/derived parity, empty surfaces, NaN inference, bootstrap and missing-kit failures, warnings, caching, anticipation, and survey finite-df paths. Static parsing passed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DiagnosticReportnow derives the unifiedEventStudyResultssurface internally via the result's post-fitaggregate('event_study')when the rawevent_study_effectsfield is absent — once, cached, fail-soft — so the event-study-gated checks run on modern plain fits without the deprecated fit-timeaggregate=kwarg that 4.0 removes. Unlocked routes:parallel_trends/pretrends_power/sensitivityon CallawaySantAnna (the derived container is M-093-admitted intocompute_pretrends_power/HonestDiDwith raw-route parity pinned at 1e-12),parallel_trends(pretrends=Truefits) +heterogeneityon ImputationDiD and TwoStageDiD, andheterogeneityon ContinuousDiD.event_study_effects— including the requested-but-empty{}sentinel, which encodes fit-timebalance_econfiguration — is authoritative and never re-derived. The M-093 consumer-admission set, dCDH'splacebo_event_studybranch, and theEventStudyResultsINPUT rejection at DR/BR construction are all unchanged. Wooldridge's mutatingaggregate()is never auto-called.NotImplementedError, kit-less unpickles'ValueError, embedded verbatim and never masked by other availability legs), and the remediation taxonomy is estimator-accurate: Wooldridge points at its own in-placeresults.aggregate(type='event_study'), the staggered-DDD reason names the fit-time kwarg as that surface's canonical route (M-140), and a one-pre-period MultiPeriodDiD fit gets an accurate omitted-reference message instead of a wrong "no post-fit route" claim.pre_period_source="aggregate_event_study"schema key — attached exactly to the checks whose gate consulted the derived surface, on success, error, and skip paths alike (user-opt-out skips carry nothing). Warnings re-emitted by a kit recompute are captured exception-safely and republished on the consuming sections and, deduplicated, on the top-level channel.BusinessReportlifts the provenance key through its_lift_pre_trendswhitelist (its only source change).applicable_checksmay now trigger the one cached derivation; the documented laziness guarantee is amended across the docstring/module-docstring/API-page/llms-full surfaces, and the two report API pages migrate their examples to plain fits.main) as a new TODO row rather than bolting a cross-cutting schema change onto this PR.Methodology references (required if estimator / math changes)
aggregate('event_study')output (Callaway & Sant'Anna 2021; Borusyak, Jaravel & Spiess 2024; Gardner 2022; Callaway, Goodman-Bacon & Sant'Anna 2024; Roth 2022 pre-trends power; Rambachan & Roth 2023 HonestDiD).docs/methodology/REGISTRY.md; the new report-layer routing is documented in the M-020/M-021/M-022/M-025 post-fitaggregate()Notes (REGISTRY) anddocs/methodology/REPORTING.md.Validation
tests/test_diagnostic_report.py(newTestDerivedEventStudySurface, 30+ tests: raw-vs-derived parity for PT joint-Wald/Bonferroni, pretrends-power (1e-3, SciPy Genz jitter), HonestDiD grid CIs (1e-12), survey finite-df F-path parity, heterogeneity numeric parity for Imputation/TwoStage/Continuous, anticipation boundary, zero-count/reference-row adapter pins, requested-but-empty{}precedence with a zero-aggregate()-call assertion, bootstrap/missing-kit/replicate fail-closed reasons across all three consumers, warning capture/republish/dedup on success, error, skip, and wrong-type paths, provenance on opt-out vs consulted checks, caching (singleaggregate()call per report), and non-mutation);tests/test_business_report.py(derived-route lift + raw-routeNoneprovenance).tests/test_event_study_consumers.pyparity pins unchanged.Security / privacy