feat(v4): StackedDiD post-fit aggregate() + honest/pretrends container admission (2(b) PR-2, M-024) - #747
Merged
Merged
Conversation
…r admission (2(b) PR-2, M-024)
The event-study surface is always computed at fit (both fit-time gates
removed, incl. per-event hc2_bm BM DOFs); fit(aggregate=) deprecates via
the NOT_SUPPLIED sentinel (warn-and-still-work; value validation kept).
StackedDiDResults adopts AggregationMixin: aggregate('event_study') is a
pure builder view with base_period="universal" + singleton
reference_event_times provenance; aggregate('simple') relays the stored
overall inference bit-exactly (target="att", n=n_treated_units/units,
df=inference_df). compute_honest_did/compute_pretrends_power admit
Stacked-sourced containers (kappa_pre >= 2; honest needs a non-singular
pre-covariance); the seven container guard messages derive the producer
from surface.source with producer-conditional remedies; withheld-inference
rows admit with a source-scoped warning in both consumers.
EventStudyResults.__post_init__ now copies vcov/vcov_index (cross-producer
de-aliasing; int index dtype preserved). DiagnosticReport's heterogeneity
+ parallel_trends (kappa_pre >= 2) checks now run on plain fits with a
per-type PT skip remediation; the practitioner balance step moves off the
colliding "heterogeneity" step key. Ledger: M-024 -> shimmed (phase 5),
M-093 second pre-cut amendment; v4-design §5/§6 amended (view-relay
exception + container copy contract); REGISTRY M-024/admission Notes;
variance-conventions tail-df pin (309,)*3 + regenerated table; guides/
README/API/choosing/notebook migrated to the post-fit route.
Overall assessmentExecutive summary
MethodologyP1 — Common-reference provenance is not verified
P3 — Documented methodology choices
Code QualityNo findings. PerformanceP3 — Deliberate fit-time overhead
MaintainabilityNo findings. Tech DebtP3 — Tracked practitioner-key collisions
SecurityNo findings or exposed secrets identified. Documentation/TestsP2 — Covariance requirement is understated
Path to Approval
|
…nce-wording fix (CI review R1) P1 (verified by execution): on a gapped panel where every retained cohort's calendar period a-1-anticipation is absent, the omitted reference cell is empty, the design rank-drops, and QR pivoting silently re-normalizes against an arbitrary horizon while the surface synthesizes the e=-1-k reference row and the container certifies reference_event_times/base_period="universal" - a fabricated delta_0=0 claim HonestDiD/PreTrendsPower would trust. fit() now raises a fail-closed ValueError when the reference cell lacks treated or control rows stack-wide (both cells are load-bearing: either absence collinears a design block). Partial support - some cohorts' own ref period absent while the pooled omitted category stays populated - still fits, verified by execution: the coefficient-vector normalization is real and the per-cohort raggedness is the documented ragged-window estimand composition, so no legitimate fit changes. Gapped-panel tests pin both sides; REGISTRY/CHANGELOG document the guard. P2: the singularity requirement wording understated the check - honest's container branch validates the FULL retained event-study covariance (pre+post sub-block, Rambachan-Roth Assumption 3), not just the pre-period block; reworded in the docstrings, REGISTRY, ledger notes, and CHANGELOG.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyP1 — Reference support ignores effective weights
P3 — Documented methodology choices
Code QualityNo additional findings. PerformanceP3 — Deliberate fit-time overhead
MaintainabilityNo findings. Tech DebtP3 — Tracked practitioner-key collisions
SecurityNo findings or exposed secrets identified. Documentation/TestsNo separate findings. The covariance wording now correctly describes the full retained covariance. The P1 fix needs positive-weight reference-support tests. Path to Approval
|
…not raw rows (CI review R2) Verified by execution: Equation 3 is Q-weighted WLS, so a reference cell can exist in raw rows yet carry zero effective mass - (a) under aggregate weighting, a sub-experiment's control rows get Q=0 at any event time where that sub-experiment has no treated rows, so a ragged panel whose only control-reference rows sit in such sub-experiments passed the raw-row guard while the effective control baseline was empty (reproduced: rank drop, distorted surface, container still certifying reference_event_times=(-1,)); (b) survey pweights can zero a cell the same way. The guard now runs after weight composition and requires positive total composed weight in BOTH reference cells, with the causes enumerated in the error. Two new tests pin the zero-Q ragged case and the survey-zeroed case; the stack-wide-missing and partial-support scenarios keep their prior behavior (re-verified). REGISTRY/CHANGELOG descriptions updated to the weighted formulation.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — the prior P1 is resolved; no unmitigated P0/P1 findings. Executive summary
MethodologyP3 — Prior reference-support finding resolved
P3 — Documented methodology choices
Code QualityNo findings. PerformanceP3 — Deliberate fit-time overhead
MaintainabilityNo findings. Tech DebtP3 — Tracked practitioner-key collisions
SecurityNo findings or exposed secrets identified. Documentation/TestsNo findings. Effective-weight tests cover aggregate Q-weight and survey-weight failures at Runtime tests could not be executed because the review environment lacks pytest/numpy; changed Python files passed AST parsing. |
…ue tolerance Two test-only fixes for the PR CI matrix: - test_plot_event_study_renders_on_plain_fit failed on every leg with ModuleNotFoundError: matplotlib is an optional dependency the CI test env does not install. Guard with pytest.importorskip per the existing convention (test_visualization.py, test_bacon.py, test_event_study_consumers.py). - test_bm_dof_batch_parity_and_overall_reconstruction failed on the Linux x86 legs only: batched-vs-m=1 BLAS kernels differ at ~1 ULP in the Satterthwaite dof (OpenBLAS reassociation), and the overall contrast's deep-tail p-value (~1e-54 at t~29) amplifies that relative noise by ~t^2 (d ln p ~ -t dt), landing at 1.5e-14 - just over the uniform rtol=1e-14. Split the assertion: t/CI stay at rtol=1e-14, the p-value alone loosens to rtol=1e-12 with the amplification documented inline. macOS Accelerate and Windows were bit-stable; no library code changes.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — the prior P1 is resolved; no unmitigated P0/P1 findings. Executive summary
MethodologyP3 — Prior reference-support finding resolved
P3 — Documented methodology choices
Code QualityNo findings. New inference continues to use PerformanceP3 — Deliberate fit-time overhead
MaintainabilityNo findings. Aggregation is isolated in Tech DebtP3 — Tracked practitioner-key collisions
SecurityNo findings or exposed secrets identified. Documentation/TestsP3 — Coverage is comprehensive
Runtime tests could not run because NumPy is unavailable in the review environment. All 19 affected Python files and all 25 notebook cells passed AST parsing. |
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
hc2_bm) the per-event Bell-McCaffrey contrast DOFs were fit-time-gated - both gates are removed. Plain fits gainsummary()'s event-study table, a totalto_dataframe('event_study'),plot_event_studyrendering, and DiagnosticReport'sheterogeneity(always) /parallel_trends(kappa_pre >= 2) checks.StackedDiD.fit(aggregate=)deprecates via the sharedNOT_SUPPLIEDsentinel (warn-and-still-work;group/all/unknown-valueValueErrors persist after the warning; thestacked_did()wrapper forwards the shim).StackedDiDResultsadoptsAggregationMixin:aggregate('event_study')is a pure builder view carryingbase_period='universal'+ singletonreference_event_timesprovenance;aggregate('simple')relays the stored overall inference bit-exactly (target='att',n = n_treated_units/'units'treated-only scope,df = inference_df).compute_honest_did/compute_pretrends_poweradmit Stacked-sourced containers (second M-093 pre-cut amendment):kappa_pre >= 2required; honest additionally needs a non-singular pre-period covariance; analytical fits carrydf_survey=None(normal-theory FLCI critical values - deliberate, REGISTRY Note); withheld-inference rows (finite se, non-finite p - hc2_bm BM-DOF fail-close, replicate-undefined) admit with a source-scoped warning in both consumers; the seven producer-derived guard messages derive the producer fromsurface.sourcewith producer-conditional remedies.EventStudyResults.__post_init__now copiesvcov/vcov_index(the post-fit view aliased the producer's stored matrix; int index labels keep their dtype). The practitioner "Check sub-experiment balance" step moves off the collidingheterogeneitystep key.(309.0,)*3+ regenerated table, guides/README/API/choosing/notebook migrated to the post-fit route, ~35 deprecated call sites swept.Methodology references (required if estimator / math changes)
## StackedDiDNote (post-fit aggregate() - row M-024) and Note (container admission - rows M-024/M-093) - the widened hc2_bm noise-floor carve-out (fail-closed + warned), normal-theory FLCI critical values on analytical Stacked containers (the overall post-average contrast df is not a design-level cluster df), and the admit-plus-warn treatment of withheld-inference rows (bounds/power consume only beta/sigma, which remain valid).Validation
aggregate=sweeps in tests/test_methodology_stacked_did.py, tests/test_event_study_surface.py, tests/test_survey_estimator_validation.py.Security / privacy