Repository navigation
feat(v4): item 8a of the 4.0 cut - retire the event-study reference-row sentinel (M-093, first half) - #882
Conversation
…ow sentinel (M-093, first half) Through 3.x every native event-study dict marked its normalization (reference) row with a zero-count sentinel: n_groups == 0 on the CallawaySantAnna aggregation, n_obs == 0 on StackedDiD, ImputationDiD, TwoStageDiD and SpilloverDiD. Every producer that materializes that row now marks it explicitly - "is_reference": True and NO count key - and every remaining zero count is a genuine count (a non-estimable horizon: TwoStageDiD's all-filtered horizons, SpilloverDiD's rectangular cells). SpilloverDiD's event-study att_dynamic / spillover_effects frames gain a boolean is_reference column with n_obs = NaN on the reference rows (aggregate-mode frames, which have no reference row, are unchanged). SunAbraham (omitted row, reference_period) and EfficientDiD (PT-Post anchor, an estimated cell named by reference_period) are untouched. The unified builder results_base._from_relative_dict reads only the is_reference key unioned with the named reference_period: the zero-count heuristic is gone, and a keyed row with a finite nonzero effect raises instead of being silently normalized to 0 by __post_init__. A reference-only TwoStageDiD surface now carries n_kind=None (no row records a count). 3.x pickles migrate on load: _deprecation.migrate_event_study_reference_sentinel rewrites a zero-count row whose effect is exactly 0.0 into the keyed shape (NaN-effect zero-count rows stay genuine) and is the single place the retired sentinel is still recognized - run by the __setstate__ of CallawaySantAnnaResults (DMLDiD inherits), StackedDiDResults, ImputationDiDResults, TwoStageDiDResults and a new SpilloverDiDResults.__setstate__ (which also adds the frame column for event-study-mode fits). A fresh pickle round-trips as a no-op. Consumers take the minimal swap: plot_event_study's raw-dict route reads the reference_period attribute, else the is_reference key, else today's -1 fallback; plot_honest_event_study's constraint-row signature and verified-anchor check, HonestDiD's native CS filter / reference detection / _estimate_max_pre_violation, PreTrendsPower's CS and SunAbraham filters and DiagnosticReport's pre-period / post-effect collectors replace the count conjunct with the key (genuine zero counts stay excluded from n_dropped_undefined). Dispatch, admission, rendering and exception types are unchanged. The Stacked / Imputation / TwoStage summary() renderers key their [ref: h] line on the marker - TwoStageDiD's genuine all-filtered horizons therefore print as NaN rows instead of a mislabeled reference line; the bootstrap loops skip the marked row. The native to_dataframe(level='event_study') frames render the reference row's n_obs as NaN (float column) until the 8b schema rewire. Tests: new tests/test_v4_sentinel_retirement.py (M-093 test_ref) pins the native dict and frame shape on every materializing producer, the CS carrier dict (captured at the builder boundary, not reconstructed), the count-agnostic builder (unkeyed zero count marks nothing; keyed row with NaN count; keyed nonzero effect raises; reference-only dict -> n_kind None), the pickle migration on every carrier plus the aggregate-mode Spillover no-op and fresh round trips, the consumers' key reads (plot extractor, honest plot anchor + all-undefined guard, raw/container parity for HonestDiD and PreTrendsPower, DiagnosticReport collector, summary(), bootstrap skip) and the ledger row. tests/helpers/legacy_surface emits the keyed shape; the enumerated count-key pins across the stacked, imputation, two-stage, spillover, staggered, honest, pretrends, consumer, methodology and diagnostic-report suites move to the key; the visualization / DR hand-built fixtures mark the reference by key (one DR fixture keeps the genuine zero-count shape). Ledger: M-093 gains test_ref and a ninth amendment note; it stays planned until 8b (the to_dataframe(level='event_study') schema rewire on every results class, incl. the dCDH native frame's estimand 'ref' row). Spec item 8 records 8a shipped; REGISTRY (SpilloverDiD reference-row shape, StackedDiD / ImputationDiD / TwoStageDiD reference-row Notes, the plotting and relay Notes, the CS post-fit Note), the migration guide, llms-full.txt and docs/api/spillover.rst describe the marking; notebook 13 re-validated with nbmake.
Overall assessment✅ Looks good — no unmitigated P0 or P1 findings identified in the reviewed diff. Executive summary
MethodologyNo actionable findings. Reviewed Callaway–Sant’Anna/DMLDiD, StackedDiD, ImputationDiD, TwoStageDiD, SpilloverDiD, and the affected HonestDiD/PreTrendsPower consumers against the Registry, docstrings, and available repository paper reviews. The changes preserve the documented normalization and inference semantics. The builder rejects explicitly marked reference rows with finite nonzero effects ( Code QualityNo findings. The changed paths consistently read the explicit marker, and no new inline inference computation or partial NaN guard was introduced. PerformanceNo findings. Added bookkeeping is linear in the event-study surface size; estimation routines are unchanged. MaintainabilityNo findings. Legacy sentinel recognition is centralized in Tech DebtP3 — Informational: unified DataFrame schema remains scheduled for item 8b.
SecurityNo findings. No added secrets or new external execution paths were identified. Documentation/TestsNo actionable findings. The migration guide, Registry, API documentation, and changelog describe the behavior change. Added tests cover reference marking, empty horizons, migration, consumer parity, and bootstrap exclusion. Validation: Migration smoke checks passed, including input preservation and idempotence. All 20 changed production modules passed Python 3.9 syntax parsing. The pytest suite remains unverified in this environment because pytest and NumPy are unavailable. |
The CI test legs install only the core stack (numpy, pandas, scipy, pytest), so test_honest_plot_anchor_from_key_and_all_undefined_guard failed on every leg with the plotter's "matplotlib is required" ImportError. The test now follows the test_event_study_consumers.py::TestPlotting convention - importorskip matplotlib, force the Agg backend, close figures on exit - so it runs wherever matplotlib is present and skips where it is not. The sentinel retirement itself is unchanged.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings identified. The prior review’s assessment remains supported. Executive summary
MethodologyNo actionable findings. Cross-checked the affected Callaway–Sant’Anna/DMLDiD, StackedDiD, ImputationDiD, TwoStageDiD, and SpilloverDiD paths and HonestDiD/PreTrendsPower consumers against the Registry, docstrings, and available repository source-paper audits. The changes preserve the documented normalization and inference semantics. The builder rejects explicitly marked reference rows with finite nonzero effects while preserving genuine empty horizons ( Code QualityNo findings. No new inline inference computation or partial NaN guard was identified. Count-key removal is handled by the affected readers. PerformanceNo findings. Added marker bookkeeping and migration scans are linear in surface size; numerical estimation routines are unchanged. MaintainabilityNo findings. Legacy sentinel conversion is centralized in Tech DebtP3 — Informational: native DataFrame schema enforcement remains scheduled for item 8b.
SecurityNo findings. No added secrets or new external execution paths were identified. Documentation/TestsNo actionable findings. The required changelog fragment is present and passes the repository validator. Added tests cover reference marking, genuine empty horizons, migration, consumer parity, and bootstrap exclusion. The plotting test now guards its optional dependency ( Validation performed: migration smoke checks passed, including input preservation and idempotence; all 20 changed production modules and the new regression module passed Python 3.9 syntax parsing. Numerical regression tests could not run with the dependencies available here. |
Summary
n_groups == 0/n_obs == 0reference-row SENTINEL retires from every native event-study dict. Every producer that materializes its normalization row (StackedDiD,ImputationDiD,TwoStageDiD,SpilloverDiD, theCallawaySantAnna/DMLDiDuniversal-base aggregation carrier) marks it explicitly -"is_reference": Trueand NO count key - and every remaining zero count is a genuine count (a non-estimable horizon).SpilloverDiD's event-studyatt_dynamic/spillover_effectsframes gain a booleanis_referencecolumn withn_obs = NaNon the reference rows; aggregate-mode frames are unchanged.SunAbraham(omitted row viareference_period) andEfficientDiD(PT-Post anchor, an estimated cell named byreference_period) are untouched.results_base._from_relative_dictreads only theis_referencekey unioned with the namedreference_period: the zero-count heuristic is gone, and a keyed row with a finite nonzero effect raises instead of being silently normalized to 0. A reference-only TwoStageDiD surface carriesn_kind=None._deprecation.migrate_event_study_reference_sentinel(the single place the retired sentinel is still recognized), run by the__setstate__of the CS / Stacked / Imputation / TwoStage carriers and a newSpilloverDiDResults.__setstate__(frame column included). Fresh pickles round-trip as a no-op.plot_event_study(attribute, else key, else the-1fallback),plot_honest_event_study(signature + verified anchor), HonestDiD, PreTrendsPower andDiagnosticReportreplace the count conjunct with the key; dispatch, admission, rendering and exception types are unchanged. The Stacked / Imputation / TwoStagesummary()renderers key[ref: h]on the marker (TwoStageDiD's genuine all-filtered horizons now print as NaN rows); the bootstrap loops skip the marked row; the nativeto_dataframe(level="event_study")frames render the reference row'sn_obsas NaN until the 8b schema rewire.test_refand a ninth amendment note and staysplanneduntil 8b (the per-classto_dataframe(level="event_study")schema enforcement). Spec item 8, REGISTRY (SpilloverDiD shape, Stacked/Imputation/TwoStage reference-row Notes, plotting and relay Notes, CS post-fit Note), migration guide,llms-full.txtanddocs/api/spillover.rstupdated; notebook 13 re-validated.Methodology references (required if estimator / math changes)
docs/v4-design.mdsection 5 ("explicit reference-period marking via anis_referencecolumn - no sentinel-value conventions"); REGISTRY SpilloverDiD / StackedDiD / ImputationDiD / TwoStageDiD reference-row Notes and the Event Study Plotting Notes.Validation
tests/test_v4_sentinel_retirement.py(M-093test_ref: native dict and frame shape on every materializing producer, the CS carrier dict captured at the builder boundary, the count-agnostic builder incl. the keyed-nonzero guard and the reference-onlyn_kind=Nonecase, pickle migration on every carrier plus aggregate-mode Spillover and fresh round trips, the consumers' key reads with raw/container parity for HonestDiD and PreTrendsPower,summary(), bootstrap skip, ledger).tests/helpers/legacy_surface.pyemits the keyed shape; count-key pins moved to the key intest_event_study_surface.py,test_aggregate_contract.py,test_stacked_did.py,test_imputation.py,test_two_stage.py,test_spillover.py,test_staggered.py,test_honest_did.py,test_pretrends_event_study.py,test_event_study_consumers.py,test_methodology_callaway.py,test_methodology_stacked_did.py,test_diagnostic_report.py,test_visualization_new.py,test_visualization_plotly.py;tests/_capture_v4_aggregate_oracles.pyreads the key (the committed oracle is unchanged).docs/tutorials/13_stacked_did.ipynbre-validated with nbmake under the Rust backend (stores no outputs). Full default suite green; mypy clean; py3.9 compile clean.Security / privacy
Changelog
changelog.d/20261030-v4-pr8a-m093-sentinel-retirement.md)