Skip to content

feat(v4): complete event-study df provenance - SunAbraham/dCDH event_study_df, LPDiD pooled_df (M-092 follow-up) - #726

Merged
igerber merged 2 commits into
mainfrom
feat/v4-df-provenance-completion
Jul 25, 2026
Merged

feat(v4): complete event-study df provenance - SunAbraham/dCDH event_study_df, LPDiD pooled_df (M-092 follow-up)#726
igerber merged 2 commits into
mainfrom
feat/v4-df-provenance-completion

Conversation

@igerber

@igerber igerber commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Summary

Completes the TODO.md follow-up from #715: every event-study producer whose inference records a degrees-of-freedom value now exposes it, so EventStudyResults.df is no longer NaN-by-omission for any producer.

  • SunAbraham gains event_study_df (per-relative-time dict). SA computes its event-study inference at four sites, so the record is assembled at fit level: a baseline from _compute_iw_effects' scalar, overwritten per-event by the hc2_bm Bell-McCaffrey contrast DOFs, overwritten again by the replicate refit's post-drop survey df, and cleared under bootstrap. That clear is deliberately narrower than the existing event_study_vcov clear (which also fires under replicate refits): a replicate refit's rows genuinely used the replicate df, so keeping it is faithful provenance, whereas bootstrap percentile p/CIs never used a df at all.
  • dCDH gains a scalar event_study_df + adapter threading. A scalar is faithful because its effect and placebo rows are computed from the same design df in every mode (None analytically, the design df under Taylor linearization, the final effective df under replicate weights, where a late refresh re-runs every stored row to one value). The bootstrap clear is required rather than defensive: TSL survey + bootstrap is a live mode (only replicate weights + bootstrap is rejected), where the df expression evaluates finite while the stored p-values are percentiles.
  • LPDiD gains pooled_df ({"pre", "post"}) — the pre/post window dfs that fit assembly previously popped and discarded. The pooled window is the headline ATT, outside the unified event-study surface, so this follows the DiDResults.inference_df headline-provenance precedent; surfaced in to_dict(), native pooled frame schema unchanged.
  • dCDH Phase 1 refresh sync (hardening, no reproducible output change). Under L_max=None the single event-study row is a value copy of the overall inference taken before the final-df refresh, and the refresh's per-horizon loops cover only the Phase 2 surface — so the copy could in principle retain an intermediate df's t/p/CI while survey_metadata.df_survey reported the final one. It is now mirrored from the refreshed values. I could not construct a configuration where this changes output: in every case reachable (standard replicate fixture, leavers panel, rank-deficient designs) the contributing sites agree on their valid-replicate counts so the two dfs coincide, and heterogeneity — which the existing suite uses to force df drift — requires L_max>=1 and cannot reach the Phase 1 path. The tests are therefore written as invariant pins, and the CHANGELOG entry is filed under Changed with the non-reproducibility stated, not as a Fixed entry that would overclaim.

Methodology references (required if estimator / math changes)

  • Method name(s): SunAbraham (Sun & Abraham 2021), de Chaisemartin-D'Haultfoeuille (2020/2022), LP-DiD (Dube et al. 2025) — provenance exposure only. No estimator equation, weighting, control-group definition, point estimate, or SE changes; every surfaced value is a copy of a df the fit already passed to safe_inference.
  • Paper / source link(s): existing REGISTRY.md sections; new **Note:** entries under SunAbraham and ChaisemartinDHaultfoeuille document the provenance semantics per mode.
  • Any intentional deviations from the source (and why): none. One documented behavioral nuance recorded in the SA note: a helper-level BM-DOF failure warns and falls back to the shared analytical df, while a per-row non-finite DOF fails closed inside safe_inference's own guard — both are faithfully recorded (the latter as NaN provenance).

Validation

  • Tests added/updated: tests/test_event_study_surface.py (SA hc2_bm per-row df, survey broadcast, replicate-df-kept-while-vcov-cleared, bootstrap clear; dCDH analytic/TSL-with-placebos/TSL+bootstrap; LPDiD pooled_df round-trip), tests/test_sun_abraham.py (NaN-BM-DOF regression pin + the helper-failure fallback that distinguishes it), tests/test_survey_dcdh_replicate_psu.py (Phase 1 invariant pins).
  • Backtest / simulation / notebook evidence (if applicable): 798 tests green across the nine targeted suites; dCDH and SA methodology/parity suites green, confirming no computation drift from the Phase 1 mirror; lint and mypy clean.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

@github-actions

Copy link
Copy Markdown

Overall Assessment

Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Sun–Abraham, dCDH, and LPDiD df provenance matches the existing safe_inference() inputs.
  • dCDH’s Phase-1 refresh correctly synchronizes inference with the final replicate-weight df.
  • Bootstrap clearing and BM fallback behavior are documented Registry notes and are not defects.
  • One non-blocking P2 API-compatibility concern remains around dataclass field ordering.

Methodology

  • P3 — Documented bootstrap provenance under-claim.
    Impact: Sun–Abraham and dCDH clear the entire df channel under bootstrap, including any partially analytical rows. This may omit provenance but cannot report a false df. It is explicitly documented in docs/methodology/REGISTRY.md:L871-L882 and docs/methodology/REGISTRY.md:L1501-L1513.
    Concrete fix: None required.

Cross-checks found no estimator, weighting, SE, identification, or undocumented-methodology mismatch:

  • Sun–Abraham preserves per-contrast Bell–McCaffrey df semantics.
  • dCDH uses the final survey/replicate df consistently across effect and placebo rows.
  • LPDiD only exposes df already passed to existing inference calls; Dube et al. do not prescribe a competing SE formula.

Code Quality

No findings. New inference assignments consistently use safe_inference() and preserve all-or-nothing NaN behavior.

Performance

No findings. Added work is linear in the small number of event-study horizons.

Maintainability

  • P2 — New result fields change positional constructor semantics.
    Impact: event_study_df was inserted before existing optional fields in SunAbrahamResults and ChaisemartinDHaultfoeuilleResults. Existing positional construction can silently bind later arguments to the wrong fields. The generated __init__ is publicly documented.
    Locations: diff_diff/sun_abraham.py:L136-L167; diff_diff/chaisemartin_dhaultfoeuille_results.py:L574-L603.
    Concrete fix: Move each new field to the end of its dataclass field list, preserving all existing positional indices.

Tech Debt

No findings. The completed M-092 follow-up is appropriately removed from TODO.md; no new deferred correctness work is introduced.

Security

No findings. No secrets, unsafe input handling, or security-sensitive changes were identified.

Documentation / Tests

No findings. Coverage includes BM df, survey and replicate modes, bootstrap clearing, non-finite df propagation, dCDH Phase-1 synchronization, placebos, and LPDiD serialization.

Runtime tests could not be independently executed because the review environment lacks pytest and project dependencies; all affected Python files passed static AST parsing.

@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: efbe05ec029a8de872674857fc177d8fce953222


Overall Assessment

Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Sun–Abraham, dCDH, and LPDiD df provenance matches the values passed to safe_inference().
  • dCDH’s Phase-1 row is correctly synchronized with final replicate-weight df inference.
  • The prior positional-constructor concern is resolved: all new dataclass fields are last.
  • Bootstrap provenance clearing is documented and conservatively avoids reporting false df.

Methodology

  • Severity: P3 — Documented bootstrap under-claim
    Impact: Sun–Abraham and dCDH clear the entire df channel under bootstrap, potentially omitting provenance for analytically retained rows but never reporting an incorrect df. This is documented in docs/methodology/REGISTRY.md:L871-L882 and docs/methodology/REGISTRY.md:L1501-L1513.
    Concrete fix: None required.

Cross-checks found no undocumented changes to estimator equations, weighting, SEs, or identification assumptions:

  • Sun–Abraham preserves per-contrast Bell–McCaffrey df semantics.
  • dCDH refreshes event and placebo inference to one final survey df at diff_diff/chaisemartin_dhaultfoeuille.py:L4041-L4219.
  • LPDiD exposes df already used by its pooled regressions; Dube et al. do not prescribe a competing SE formula.

Code Quality

No findings. New inference paths use safe_inference() consistently, with no new inline inference or partial NaN guards.

Performance

No findings. The added work is constant-time metadata capture or linear in event-study horizons.

Maintainability

No findings. The previous positional-constructor issue is resolved by declaring the new fields last:

  • diff_diff/sun_abraham.py:L154-L170
  • diff_diff/chaisemartin_dhaultfoeuille_results.py:L668-L681
  • diff_diff/lpdid_results.py:L72-L80

Tech Debt

No findings. The completed M-092 follow-up is appropriately removed from TODO.md.

Security

No findings. No secrets or security-sensitive behavior were introduced.

Documentation / Tests

No findings. Tests cover BM, survey, replicate, bootstrap, non-finite df, dCDH Phase-1 synchronization, placebos, and LPDiD serialization.

Runtime tests could not be executed because NumPy and pytest are unavailable; all ten affected Python files passed AST parsing.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 25, 2026
@igerber
igerber merged commit 6bdd5ac into main Jul 25, 2026
39 of 40 checks passed
@igerber
igerber deleted the feat/v4-df-provenance-completion branch July 25, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant