feat(v4): results-contract foundation - BaseResults, Diagnostic marker, unified event-study surface (Phase 2a)#710
Conversation
Overall assessmentExecutive summary
Methodology
Code Quality
PerformanceNo findings. MaintainabilityNo findings. Tech DebtThe material VCV/df limitation is appropriately tracked in SecurityNo findings or exposed secrets identified. Documentation/Tests
The new suite covers all listed producers and several reference edge cases, but misses the in-range gapped Sun–Abraham case above. Path to Approval
|
… safety Address CI review on #710: - P1 (SunAbraham false reference on gapped grids): the min<=e<=max range guard could not distinguish the omitted-but-observed baseline from a genuine gap (native keys {-2, 0} with -1 never observed would fabricate an anchor). SunAbrahamResults now persists reference_period + reference_observed (computed at fit as `-1-anticipation in all_rel_times`), and the adapter synthesizes the anchor ONLY when it was genuinely observed. This unifies reference handling: a named reference is MARKED if present (SpilloverDiD's materialized zero row) or SYNTHESIZED if omitted-but-observed (SunAbraham); the separate _from_sun_abraham path and the range heuristic are removed. - P2 (caller-array mutation): EventStudyResults.__post_init__ normalizes reference rows in place, so it now copies inputs (np.array, not np.asarray) - caller-owned arrays are never overwritten and read-only inputs are accepted. - P3 (producer count): spec section 5 said "today there are five"; corrected to "fourteen event-study producers use four incompatible native shapes". Tests: gapped-grid {-2,0} observed/unobserved cases, caller-array non-mutation, read-only-input acceptance.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good Executive summary
Methodology
Code Quality
PerformanceNo findings. MaintainabilityNo findings. Tech DebtThe VCV/df follow-up is properly tracked in SecurityNo findings or exposed secrets identified. Documentation/Tests
|
igerber
added
ready-for-ci
…r, unified event-study surface (Phase 2a) Phase 2 PR (a) of the 4.0 API-unification program (docs/v4-design.md sec 9a). Strictly additive to the estimator surface; the only behavior change is diagnostic input-validation in the report consumers (ledger row M-091). Symbols described as future 4.0 surface intentionally do not exist at HEAD; tests/test_v4_matrix.py enforces their absence until their phase ships. New leaf module diff_diff/results_base.py (numpy/pandas only, cycle-free): - BaseResults: shared estimator-results base, a transitional marker in 3.9 that anchors the 4.0 storage flip and the estimand self-description hook. - Diagnostic: marker base on the 17-member diagnostic result roster (Bacon, RDPlot, HonestDiD/Sensitivity, pre-trends/power, placebo, HAD pretests, DiagnosticReport results). Marked results carry no inference row. - EventStudyResults: the unified per-event-time representation (columnar, pinned to_dataframe schema, explicit is_reference marking - no count sentinels). build_event_study_surface() adapts all 14 event-study producers; public exposure rides aggregate(type="event_study") in a later Phase 2 PR. Producer-aware reference resolution (a named reference is MARKED if materialized (Spillover) or SYNTHESIZED if omitted-but-observed (SunAbraham, gated on reference_observed); a zero-count sentinel row is a reference iff its effect is exactly 0, else a preserved non-estimable row). Multiple and zero reference rows both legal; requested-empty vs absent distinguished; JSON-safe calendar labels; vcov round-trips; caller arrays never mutated. Also: seven estimator results classes gain to_dict() (CS, Stacked, Continuous, SunAbraham, Wooldridge, dCDH, Bacon); DiagnosticReportResults gains summary()/to_dict()/to_dataframe() (moved from the builder, which now delegates); PreTrendsPowerCurve gains summary(); SunAbrahamResults persists reference_period + reference_observed. Consumer propagation (M-091): BusinessReport, practitioner_next_steps, and DiagnosticReport route diagnostics by the marker instead of by result-class name. Passing a marked non-Bacon diagnostic as a primary estimator input now raises TypeError (previously: name-only Bacon special-case in BR; silent zero-check report in DR). Estimator numerical output is unchanged. Ledger: M-091 -> done; M-092 (unified event-study surface, born done) and M-093 (4.0 sentinel retirement, phase 5) added. Matrix floor 75->77; snapshot extended by (92,93). Spec sec 5/11 citations; doc-deps entry for the new module; api/results.rst + api/index.rst paired autoclass/autosummary; M-090 code_refs += tests/test_docs_ia.py. Tests: tests/test_diagnostic_marker.py, tests/test_event_study_surface.py, tests/test_results_serialization.py (all analytical small fits). Follow-up tracked in TODO.md: persist full event-study VCV + per-producer df round-trip for StackedDiD/TwoStageDiD (additive estimator-container work).
578e958 to
6d1daa3
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
Methodology[Newly identified] P1 — dCDH count units are mislabeledImpact: Concrete fix: Set producer-accurate metadata—e.g. The prior Sun–Abraham P1 is resolved: synthesis is gated by genuinely observed support ( Code QualityNo additional findings. Caller-owned arrays are copied before normalization ( PerformanceNo findings. MaintainabilityNo additional findings. Tech DebtP3 — Tracked VCV/df omissionsImpact: Some producers cannot yet round-trip full VCV or inference degrees of freedom. Concrete fix: No approval action required; tracked in SecurityNo findings or exposed secrets identified. Documentation/TestsThe dCDH adapter needs regression tests asserting both Focused tests could not run because pytest and NumPy are unavailable. All 92 Python sources parsed successfully. Path to Approval
|
dCDH stores N_l (eligible switcher GROUPS per horizon, and N_pl_l for placebo horizons) under its legacy "n_obs" inner key - a group-level count, NOT observations (chaisemartin_dhaultfoeuille.py:2378). The event-study adapter hardcoded n_kind="obs", so a consumer doing sample-size logic would misread switcher groups as observations. Label it "groups" and broaden the n_kind field doc (groups = cohorts for CS/SA, eligible switcher groups for dCDH). Added regression tests asserting n/n_kind for the multi-horizon and legacy single-horizon dCDH paths.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
MethodologyP1 — Legacy dCDH count units remain mislabeledImpact: Concrete fix: Set Code QualityNo additional findings. PerformanceNo findings. MaintainabilityNo findings. Tech DebtP3 — Tracked VCV/df omissionsImpact: Some producers cannot yet round-trip full event-study VCV or inference degrees of freedom. Concrete fix: No approval action required; tracked in SecurityNo findings or exposed secrets identified. Documentation/TestsThe legacy regression test asserts the incorrect Concrete fix: Replace it with an Path to Approval
|
The dCDH count under the legacy "n_obs" key is L_max-dependent (never observations): with L_max >= 1 it is N_l = eligible switcher GROUPS per horizon; with L_max is None it is N_S = switching (g,t) CELLS, where one group can contribute several (chaisemartin_dhaultfoeuille_results.py:333-339). Labeling both "groups" was wrong for the legacy path. n_kind is now "groups" when L_max >= 1 and "switcher_cells" (new documented vocabulary value) when L_max is None. Regression test asserts the legacy path yields switcher_cells.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good Executive summary
MethodologyP3 — Prior dCDH finding resolvedImpact: None. Multi-horizon Concrete fix: None required. Regression coverage exists at No estimator equations, weighting, identification assumptions, or variance calculations were changed. The adapters for all 14 event-study producers copy their native statistical outputs. Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech DebtP3 — Tracked VCV/df round-trip limitationsImpact: Some producers expose only marginal SEs or omit producer-specific inference degrees of freedom. Concrete fix: No approval action required; follow-up work is tracked in SecurityNo findings or exposed secrets identified. Documentation/TestsNo findings. The added tests cover the diagnostic roster, reference normalization, empty surfaces, NaN propagation, covariance serialization, and all 14 producers. Static parsing succeeded for 243 Python files. Runtime tests could not run because the review environment lacks NumPy and pytest. |
1 participant
Summary
Phase 2 PR (a) of the 4.0 API-unification program (
docs/v4-design.md§9a). Strictly additive to the estimator surface; the only behavior change is diagnostic input-validation in the report consumers (ledger row M-091). Symbols described as future 4.0 surface intentionally do not exist at HEAD —tests/test_v4_matrix.pyenforces their absence until their phase ships.diff_diff/results_base.py(numpy/pandas only, cycle-free):BaseResults— shared estimator-results base; a transitional marker in 3.9 that anchors the 4.0 storage flip and the estimand self-description hook.Diagnostic— marker base on the 17-member diagnostic result roster (Bacon, RDPlot, HonestDiD/Sensitivity, pre-trends/power, placebo, HAD pretests, DiagnosticReport results). Marked results carry no inference row.EventStudyResults— the unified per-event-time representation (columnar, pinnedto_dataframeschema, explicitis_referencemarking — no count sentinels).build_event_study_surface()adapts all 14 event-study producers; public exposure ridesaggregate(type="event_study")in a later Phase 2 PR. Producer-aware reference resolution (explicitreference_period/ synthesized anchor / finite-effect zero-count sentinel); multiple and zero reference rows both legal; JSON-safe calendar labels; vcov round-trips.to_dict()(CS, Stacked, Continuous, SunAbraham, Wooldridge, dCDH, Bacon);DiagnosticReportResultsgainssummary()/to_dict()/to_dataframe()(moved from the builder, which now delegates);PreTrendsPowerCurvegainssummary().BusinessReport,practitioner_next_steps, andDiagnosticReportroute diagnostics by the marker instead of by result-class name. Passing a marked non-Bacon diagnostic as a primary estimator input now raisesTypeError(previously: name-only Bacon special-case in BusinessReport; silent zero-check report in DiagnosticReport). Estimator numerical output is unchanged.done; M-092 (unified event-study surface, borndone) and M-093 (4.0 sentinel retirement, phase 5) added. Matrix row-count floor 75→77; immutable id snapshot extended by(92, 93). Spec §5/§11 citations;doc-deps.yamlentry for the new module;api/results.rst+api/index.rstpaired:no-index:autoclass / autosummary; M-090code_refs+=tests/test_docs_ia.py.Follow-up tracked in
TODO.md: persist the full event-study VCV and thread per-producer inferencedffor StackedDiD/TwoStageDiD — additive estimator-container work, faithful adapter behavior today (those containers retain only marginal SEs).Methodology references
e=-1-anticipationomitted baseline; CS universal-base cohort positional bases; dCDHl1_first_switch).Validation
tests/test_diagnostic_marker.py,tests/test_event_study_surface.py,tests/test_results_serialization.py,tests/test_v4_matrix.py(row-count + snapshot), plustests/helpers/results_foundation.py. All fits are small and analytical.black/ruff/mypyclean; Sphinx-Wclean.Security / privacy