feat(v4): EventStudyResults consumers + dCDH aggregate shim (2(b) PR-1, M-026) - #744
Merged
Conversation
…1, M-026; M-092/M-093 amended)
v4 program section 9 item 5, PR-1 of the 2(b) wave: the unified
event-study container becomes CONSUMABLE (the TODO consumability row's
acceptance gates), and dCDH - the one row whose fit-time aggregate=
never computed anything - ships its post-fit aggregate() view.
Container consumers (M-092/M-093 pre-cut half):
- compute_honest_did, compute_pretrends_power and plot_event_study
accept EventStudyResults from
CallawaySantAnnaResults.aggregate('event_study');
plot_honest_event_study renders HonestDiDResults computed from a
container. Route parity is the pinned gate: HonestDiD bounds equal
across routes at 1e-14; pretrends extraction bit-exact (its MVN-CDF
power carries scipy's internal jitter either way).
- Admission in HonestDiD/PreTrendsPower is SOURCE-SCOPED to CS-produced
containers; dCDH l1 containers rejected BY DESIGN (placebo semantics
need the native branch); plotters take no source guard.
- EventStudyResults gains four provenance fields declared last:
base_period, anticipation, df_survey (scalar inference df with the
replicate-undefined 0.0 sentinel the per-row df column cannot
encode), and reference_event_times (distinct per-cohort
positional-base event times, computed at fit under universal base and
recomputed per-aggregation over retained cohorts under balance_e).
Threaded by all builders incl. the requested-but-empty path;
to_dict() JSON-safe.
Correctness hardening that landed with the wave (both routes each):
- Common-reference guard: universal-base fits whose cohorts carry more
than one positional-base event time (gapped grids - including the
overlap layout where a cohort's base coincides with another cohort's
estimated horizon and NO reference-only row marks it) fail closed in
HonestDiD and PreTrendsPower; provenance-less universal inputs derive
the bases from materialized reference cells (pre-3.9 pickles) or warn
fail-safe (hand-built containers). Multi-reference containers also
fail closed in HonestDiD (deliberate deviation from the fit-time
first-marker split).
- Roth-anchored linear violations: pretrends relative times are
measured from the omitted reference (|t - t_ref|) on the
CS-universal, SunAbraham and container routes, matching the MPD
branch's existing anchor; universal-base CS/SA linear power/MDV
numbers change to the Roth-correct values (REGISTRY linear-convention
note records the correction). Varying-base input keeps raw labels
under a new interpretation warning scoped to the linear benchmark.
- Anticipation boundary unified: HonestDiD's no-reference split, the
plotters' pre-shading, and pretrends' cutoff all treat [e=-k, -1] as
anticipated treatment effects (e < -k is the clean pre set).
- Zero-SE rows drop on every HonestDiD adapter (MPD/dCDH joined CS/
container); MPD extraction fails closed when a dropped horizon breaks
the positional grid and returns the estimated period labels beta_hat
was built from; explicit empty pre_periods subsets are rejected.
- Plotting: multi-reference containers render every anchor hollow via
row-aligned reference marks (explicit renormalization around a
non-anchor fails closed; REGISTRY Event Study Plotting edge case);
interval bars draw endpoint-based so stored percentile/bootstrap
intervals that exclude the point estimate render exactly; container
stored-interval levels are honesty-checked against the requested
alpha; the honest plotter restricts to HonestDiD-retained rows with
the reference kept as a hollow normalization anchor.
dCDH shim (M-026, phase 5):
- fit(aggregate=) is deprecated: supplying it warns; any non-None value
raises pointing at results.aggregate() (it never computed anything -
"reserved for Phase 3").
- ChaisemartinDHaultfoeuilleResults.aggregate() ships as a pure VIEW:
'event_study' returns the unified container (Phase-1 L_max=None fits
the 2-row l=1 view); 'simple' a one-row AggregationResult relaying
the overall estimand bit-exactly with estimand-aware target/n/n_kind
(DID_M/switcher_cells, DID_1/groups, delta/no-scalar-count, the
trends-linear all-NaN first-difference row) and df resolved from the
ACTUAL inference path (the L_max>=2 delta keeps analytical survey df
under bootstrap per the REGISTRY delta-SE note). Because nothing is
recomputed, bootstrap fits are permitted - unlike CallawaySantAnna's
kit-based aggregate(), which stays fail-closed.
- AggregationMixin routing hooks are ClassVar (the annotated override
had leaked into the dataclass __init__/repr/eq surface); dynamic
regression enrolls every dataclass mixin adopter.
Ledger/docs: M-026 flips to shimmed; M-092 gains its provenance-field
amendments; M-093 records the pre-cut consumer delivery with
source-scoped admission widening per-estimator; v4-design section 5 and
REGISTRY notes updated (dCDH view Note, common-reference guard Note,
plotting edge cases, pretrends linear-convention correction);
autosummary rosters extended; TODO consumability row delivered and
removed, two follow-up methodology rows added (varying-base violation
transformation; the pre-existing native-route items resolved in-tree).
Overall assessmentExecutive summary
MethodologyM1 — Container rows are not reordered into pre/post blocks
M2 — Covariance integrity is not validated at the new consumer boundary
M3 — Documented deviations
Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo additional findings beyond centralizing the validation described in M1–M2. Tech DebtNo untracked findings. The varying-base linear-violation limitation is tracked in SecurityNo findings or accidental secrets observed. Documentation/TestsThe added coverage is extensive, but it lacks the permuted-row and malformed-covariance cases described above. Runtime validation was unavailable because the test dependencies are not installed. Path to Approval
|
…riance validation (PR #744 review M1/M2) - HonestDiD subsets beta_hat/sigma in EXPLICIT [sorted pre; sorted post] label order instead of row order: an interleaved hand-built container passed the within-block grid checks while beta_hat stayed row-ordered, silently violating the Rambachan-Roth [beta_pre; beta_post] contract (reproduced: permuted rows moved bounds from (1.9625, 2.0375) to (-0.35, 2.5)). Duplicate event_time labels are rejected on both consumers; permuted-row parity is regression-pinned for HonestDiD and PreTrendsPower (whose elementwise label alignment was already order-consistent). - Consumer-boundary covariance validation (shared results_base._validate_vcov_subblock, called by the HonestDiD container branch and the pretrends container subblock helper): finite entries, symmetry, diagonal == stored se**2 (the container contract clears vcov rather than ship an inconsistent matrix), no material indefiniteness (singular passes - consumers document their own singular handling), and unique vcov_index labels. Parameterized malformed-container rejections + a valid-matrix acceptance pin. CS-produced containers are unaffected (sorted unique rows and consistent vcov by construction - all parity suites unchanged).
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyM1 — Covariance validation remains incomplete
M2 — Prior ordering issue resolved
M3 — Documented deviations
Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo additional findings; the shared covariance validator is the appropriate centralization point. Tech Debt
SecurityNo findings or accidental secrets observed. Documentation/TestsNo additional findings. Runtime tests were unavailable because the environment lacks Path to Approval
|
… sub-blocks (PR #744 review round 2) - Every validator tolerance is now RELATIVE to the covariance scale: the previous max(eigmax, 1.0) floor made the indefiniteness check effectively absolute, waving through a uniformly tiny indefinite matrix (diag 1e-10, eigenvalues [-1e-10, 3e-10]); the symmetry atol scales with max|sigma| and the diagonal-vs-se**2 check is pure relative (retained rows have se > 0, so the target is strictly positive). Low-scale indefinite regression added on both consumers. - HonestDiD's container boundary passes allow_singular=False: Rambachan-Roth inference assumes covariance eigenvalues bounded away from zero, so singular/near-singular sub-blocks (eigmin < 1e-10 * eigmax) are rejected with an actionable message. PreTrendsPower keeps its documented singular-covariance handling (default True) - regression pins the split (perfectly-correlated pre-rows: honest rejects, pretrends computes). Real-fit parity suites unchanged.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyM1 — Container pre-periods are not chronologically ordered
M2 — Previous covariance finding resolved
M3 — Documented deviations
Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo additional findings. Tech Debt
SecurityNo findings or accidental secrets observed. Documentation/TestsD1 — Bootstrap guidance contradicts runtime behavior
Runtime tests could not run because pytest and NumPy are unavailable. Path to Approval
|
…; bootstrap exception in the fit docstring (PR #744 review round 3) - The pretrends container branch sorts retained pre-periods CHRONOLOGICALLY before extraction: row order is harmless for the elementwise linear weights but silently mis-maps POSITIONAL violation patterns on a hand-built unsorted container - last_period assigned weights[-1] to whichever row came last (reproduced: power 0.999992 sorted vs 0.998876 reversed), and user-supplied custom weights are positional too. Reversed-row last_period regression pins power parity and extraction-level label equality. Native CS/SA branches already sorted. - The CS fit(aggregate=) docstring now carries the bootstrap exception the guides already document: post-fit aggregate() raises on bootstrapped fits (percentile inference is not reproducible from retained analytical state), so fit-time aggregation remains the supported route there - the blanket 'no consumer requires the fit-time surface' claim contradicted runtime behavior.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyPrevious P1 resolved
Documented deviations
Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech Debt
SecurityNo findings or accidental secrets observed. Documentation/TestsPrevious documentation issue resolved
Runtime tests were unavailable because pytest, NumPy, pandas, and SciPy are not installed; repository-wide AST parsing succeeded. |
…ear violation The cs_fit fixture pin predated the Roth-anchoring correction: the linear violation is measured from the omitted reference, so the universal-base pre-periods [-4, -3, -2] around the e=-1 reference carry offsets [-3, -2, -1] and the deterministic fixture value moves from mdv*max|t| = 0.0937*4 = 0.375 to mdv*max|t - t_ref| = 0.1337*3 = 0.401. The ratio (0.225) stays under the 0.25 well_powered threshold, so the no-downgrade contract the test pins is unchanged - only the level-scale constant moved.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyPrevious P1 resolved
Documented methodology decisions
Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech Debt
SecurityNo findings or accidental secrets observed. Documentation/Tests
|
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
compute_honest_did,compute_pretrends_powerandplot_event_studyaccept the unifiedEventStudyResultscontainer fromCallawaySantAnnaResults.aggregate('event_study');plot_honest_event_studyrenders HonestDiD results computed from a container. Route parity is the pinned gate (HonestDiD bounds equal across routes at 1e-14; pretrends extraction bit-exact). Admission in HonestDiD/PreTrendsPower is source-scoped to CS-produced containers - dCDH l1 containers rejected by design; widening is each estimator's ownaggregate()migration. Plotters take no source guard.EventStudyResults(declared last, positional compat preserved):base_period,anticipation,df_survey(scalar inference df incl. the replicate-undefined0.0sentinel the per-rowdfcolumn cannot encode), andreference_event_times(distinct per-cohort positional-base event times; fit-level under universal base, recomputed over retained cohorts per aggregation underbalance_e). Threaded by all builders;to_dict()JSON-safe.|t - t_ref|) on the CS-universal, SunAbraham and container routes, matching the MPD branch's existing anchor. Universal-base CS/SA linear power/MDV numbers change to the Roth-correct values (weights e.g.[3, 2]→[2, 1]for pre[-3, -2]aroundt_ref = -1); the REGISTRY linear-convention note records the correction. Varying-base input keeps raw labels under a new linear-scoped interpretation warning (transformation tracked in TODO.md).[e=-k, -1]as anticipated treatment effects.beta_hatwas built from; empty explicitpre_periodssubsets rejected on all pretrends paths.alpha(warning on mismatch); the honest plotter restricts to HonestDiD-retained rows with the reference kept as a hollow normalization anchor.fit(aggregate=)deprecated - supplying it warns, any non-None value raises pointing atresults.aggregate()(it never computed anything).ChaisemartinDHaultfoeuilleResults.aggregate()ships as a pure view:'event_study'returns the unified container (Phase-1 fits the 2-row l=1 view);'simple'a one-rowAggregationResultrelaying the overall estimand bit-exactly with estimand-awaretarget/n/n_kindanddfresolved from the actual inference path (theL_max>=2delta keeps analytical survey df under bootstrap per the REGISTRY delta-SE note). Bootstrap fits permitted (nothing recomputed - documented contrast with CS's fail-closed kit recompute).AggregationMixinhooks areClassVar(dynamic regression enrolls every dataclass adopter).Methodology references (required if estimator / math changes)
docs/methodology/REGISTRY.mdsections for CallawaySantAnna, HonestDiD, PreTrendsPower, and de Chaisemartin-D'Haultfoeuille (citations therein).Validation
tests/test_event_study_consumers.py(new, 80+ tests: end-to-end gates, route parity at 1e-14, source-scoped admission, common-reference guard incl. the overlap layout, provenance threading, anchoring regressions with hand-calculated weights, plotting fidelity on both backends),tests/test_aggregate_contract.py(dCDH shim + view classes incl. the estimand ladder and df provenance),tests/test_event_study_surface.py,tests/test_honest_did.py,tests/test_pretrends.py,tests/test_visualization.py,tests/test_chaisemartin_dhaultfoeuille.py(shim pins).Security / privacy