Skip to content

fix(inference): three standard-error parity fixes vs canonical R#620

Merged
igerber merged 2 commits into
mainfrom
se-audit
Jul 5, 2026
Merged

fix(inference): three standard-error parity fixes vs canonical R#620
igerber merged 2 commits into
mainfrom
se-audit

Conversation

@igerber

@igerber igerber commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • HonestDiD Δ^SD (method="smoothness"): return a finite FLCI when the estimated identified set is empty (pre-trend curvature exceeds M) instead of NaN-propagating all inference. Matches R createSensitivityResults; previously smoothness sensitivity yielded no inference on any event study with non-trivial pre-trend curvature.
  • ChaisemartinDHaultfoeuille phase-1 placebo (DID_M^pl): correct the point-estimate sign. The single-horizon path used the opposite (forward) difference order from the multi-horizon placebo path and R, so it was sign-flipped on pure-direction panels (magnitude bit-identical). Now uses the backward-difference × switch-direction convention.
  • Unweighted clustered CR2 / Bell-McCaffrey per-coefficient Satterthwaite DOF: guard against non-physical values on high-leverage FE-dummy / collinear nuisance columns (float64 noise → ~1e61, or finite-but-inflated above the cluster count). Adds the weighted path's noise-floor guard (scale-invariant batch-relative criterion) plus a DOF ≤ G bound.

Methodology references

  • Method(s): HonestDiD (Rambachan & Roth 2023) fixed-length CI; de Chaisemartin & D'Haultfœuille dynamic placebo DID_M^pl; Bell-McCaffrey / Pustejovsky-Tipton (2018) CR2 Satterthwaite DOF.
  • Source(s): R HonestDiD::createSensitivityResults; R DIDmultiplegtDYN::did_multiplegt_dyn; R clubSandwich::vcovCR/coef_test. Each fix verified against the live R implementation (HonestDiD FLCI ~1e-3 at M=0; dCDH pure-direction placebo <1e-8; CR2 treatment/event-study/compound-average contrasts match clubSandwich).
  • Intentional deviations (documented in REGISTRY.md, filed in TODO.md): HonestDiD Δ^SD FLCI optimizer/center divergence from R at intermediate M (CI width unaffected); dCDH mixed-direction placebo magnitude retains the documented period-vs-cohort control-set deviation; CR2 unweighted high-leverage nuisance DOFs are NaN'd (exact clubSandwich P-array reproduction of those non-user-facing columns deferred).

Validation

  • Tests added/updated: tests/test_methodology_honest_did.py (empty-id-set FLCI, finite-across-M grid), tests/test_chaisemartin_dhaultfoeuille_parity.py (pure-direction placebo parity), tests/test_estimators_vcov_type.py (no-non-physical DOF, scale-invariant guard).
  • 660 targeted tests pass across the touched areas (HonestDiD, dCDH, CR2/vcov, TWFE, stacked, SunAbraham).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

Standard-error parity follow-ups, each independently verified against the
canonical R implementation.

- HonestDiD Delta^SD (`method="smoothness"`): stop gating the FLCI on the
  identified-set LP. When the observed pre-trend curvature exceeds M the
  estimated identified set is empty (`lb`/`ub` = NaN), but the optimal FLCI
  is well-defined given (Sigma, M); R's createSensitivityResults returns it.
  Previously fit() NaN-propagated the whole result, yielding no inference on
  any event study with non-trivial pre-trend curvature. FLCI matches R to
  ~1e-3 at M=0; a known optimizer/center divergence at intermediate M (CI
  width unaffected) is documented in REGISTRY.md and filed in TODO.md.

- ChaisemartinDHaultfoeuille phase-1 placebo (DID_M^pl): correct the point
  estimate sign. The single-horizon path used the opposite (forward)
  difference order from the multi-horizon placebo path and R, so it was
  sign-flipped on pure-direction panels (magnitude bit-identical). Now uses
  the backward-difference x switch-direction convention; joiners_only /
  leavers_only match R to <1e-8. Mixed-direction magnitude retains the
  separately-documented period-vs-cohort control-set deviation.

- Unweighted clustered CR2/Bell-McCaffrey per-coefficient Satterthwaite DOF:
  guard against non-physical values on high-leverage FE-dummy / collinear
  nuisance columns (float64 noise -> ~1e61, or finite-but-inflated above the
  cluster count). Adds the weighted path's noise-floor guard (batch-relative
  criterion computed on the scale-normalized max|B|/||c||^2 so it is invariant
  to contrast rescaling) plus a DOF <= G physical bound. The treatment /
  event-study / compound-average contrasts estimators consume are unchanged
  and match R clubSandwich.

Each carries a regression test, a REGISTRY note, and a CHANGELOG entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0178r8ZK6VRiXbaBiknrWjQB
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Overall Assessment

Blocker — one P0 inference-consistency issue remains.

Executive Summary

  • Affected methods: HonestDiD Δ^SD FLCI, dCDH DID_M^pl, and unweighted clustered CR2-BM Satterthwaite DOF.
  • The HonestDiD and dCDH methodology changes are documented in REGISTRY.md; I do not see an unmitigated methodology mismatch there.
  • The new CR2-BM guard can now intentionally return NaN DOF, but some downstream inference paths do not fail closed.
  • This can produce finite or fallback inference where the BM DOF was explicitly declared unreliable.
  • I did not run tests; the local Python environment could not import numpy.

Methodology

Finding: P0 — CR2-BM NaN DOF Does Not Fail Closed In All Inference Callers

Location: diff_diff/linalg.py:L2138-L2153, diff_diff/estimators.py:L2135-L2140, diff_diff/estimators.py:L2186-L2189, diff_diff/sun_abraham.py:L1149-L1167, diff_diff/utils.py:L353-L360

Impact: _cr2_bm_dof_inner now correctly suppresses unreliable/non-physical Satterthwaite DOF by returning NaN. LinearRegression.get_inference() handles that correctly, but MultiPeriodDiD falls back to the shared residual df for period effects when BM DOF is non-finite, and passes NaN BM DOF through for the average. SunAbraham also passes stored contrast DOFs directly to safe_inference(). Since safe_inference() only rejects df <= 0, not non-finite df, a guarded BM DOF can still yield finite or fallback inference fields. That violates the all-or-nothing NaN inference contract.

Concrete fix: Treat non-finite BM DOF as undefined inference everywhere:

  • Add if df is not None and not np.isfinite(df): return np.nan, np.nan, (np.nan, np.nan) to safe_inference().
  • In MultiPeriodDiD, when _bm_dof_per_coef exists and the relevant entry is non-finite, set t_stat, p_value, and conf_int to NaN instead of falling back to df.
  • Do the same for _bm_dof_avg.
  • Add regression tests that force/monkeypatch NaN BM DOF for a user-facing period and aggregate contrast.

Finding: P3 — Documented HonestDiD Δ^SD FLCI Deviation

Location: docs/methodology/REGISTRY.md:L3525-L3526, TODO.md:L39, diff_diff/honest_did.py:L2469-L2505

Impact: The empty estimated identified-set behavior and intermediate-M optimizer divergence are documented with registry notes and TODO tracking. Per review rules, this is informational and not a defect.

Concrete fix: No action required for approval.

Code Quality

No additional code-quality findings beyond the P0 inference propagation issue above.

Performance

No performance findings. The new CR2-BM guard reuses values from the existing loop and does not change the main asymptotic cost.

Maintainability

No maintainability blocker. The main improvement needed is centralizing non-finite df handling in safe_inference() so each estimator wrapper does not need to rediscover the same guard.

Tech Debt

Finding: P3 — Tracked HonestDiD Optimizer Parity Work

Location: TODO.md:L39

Impact: The Δ^SD FLCI center divergence from R at intermediate M is tracked as deferred work. Per rules, this is not a blocker.

Concrete fix: No approval action required.

Security

No security findings.

Documentation/Tests

Finding: P2 — dCDH Placebo Formula Text Conflicts With New Sign Note

Location: docs/methodology/REGISTRY.md:L747-L756, diff_diff/chaisemartin_dhaultfoeuille.py:L4666-L4689

Impact: The new note documents the backward-difference × switch-direction convention, and the code follows it. The formula just above still reads like the forward-difference Theorem 3 analogue. This is not a runtime methodology defect because the note documents the convention, but it makes the registry internally confusing.

Concrete fix: Rewrite the displayed DID_M^pl formula to explicitly use Y_{t-2} - Y_{t-1} or stable0_avg - joiner_avg / leaver_avg - stable1_avg.

Path To Approval

  1. Update safe_inference() to return all-NaN inference when df is non-finite.
  2. Update MultiPeriodDiD hc2_bm period and average inference to fail closed when BM DOF is NaN, instead of falling back to residual/shared df.
  3. Add regression coverage for NaN BM DOF on a user-facing period effect and aggregate contrast.
  4. Update the dCDH placebo formula text in REGISTRY.md to match the new documented sign convention.

The CR2/Bell-McCaffrey guard added in the previous commit can surface a
non-finite (NaN) Satterthwaite DOF for a high-leverage / collinear
nuisance coefficient. Two inference paths did not treat that as
undefined:

- `safe_inference()` rejected `df <= 0` but let a NaN `df` through
  (`NaN <= 0` is False), computing a finite t-stat with a NaN p-value /
  CI - an inconsistent, partially-finite inference tuple. It now returns
  all-NaN for any non-finite `df`.
- `MultiPeriodDiD(vcov_type="hc2_bm")` fell back to the shared residual
  `df` for a period effect whose per-coefficient BM DOF was non-finite,
  reporting finite t/p/CI for a coefficient whose DOF was declared
  unreliable. It now passes the BM DOF through unconditionally on the
  hc2_bm path, so a non-finite DOF fails closed via `safe_inference`. The
  post-period-average and SunAbraham contrast paths already route their
  DOF through `safe_inference`, so the central guard covers them too.

Only coefficients whose BM DOF is non-finite are affected; well-
conditioned treatment / event-study / average contrasts are unchanged.

Also rewrites the REGISTRY `DID_M^pl` placebo formula to display the
backward-difference x switch-direction convention (`stable0_avg -
joiner_avg` / `leaver_avg - stable1_avg`) so it matches the code and the
adjacent sign note.

Regression tests: non-finite `df` in `safe_inference`; monkeypatched
NaN BM DOF failing closed on user-facing MPD period and average contrasts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0178r8ZK6VRiXbaBiknrWjQB
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 096a7208743fdeeca09f07fa4f1eea1ff15058d7


Overall Assessment

Looks good — no unmitigated P0 or P1 findings.

Executive Summary

  • The prior P0 is addressed: non-finite BM DOF now fails closed through safe_inference(), and MultiPeriodDiD no longer falls back to residual df for NaN BM DOF.
  • HonestDiD Δ^SD empty estimated-id-set FLCI behavior is documented in REGISTRY.md; the remaining R optimizer-center divergence is also documented and tracked in TODO.md.
  • dCDH DID_M^pl now matches the documented R / multi-horizon backward-difference sign convention; the literal AER 2020 sign difference is documented, so it is informational.
  • Unweighted clustered CR2-BM nuisance DOF suppression is documented and tested; user-facing treatment/event-study/average contrasts remain covered.
  • I could not run tests locally because this environment cannot import numpy; this review is static plus methodology cross-check.

Methodology

Finding: P3 — Prior CR2-BM Non-Finite DOF Blocker Is Resolved

Severity: P3 informational
Impact: safe_inference() now returns all-NaN inference for non-finite df, and MultiPeriodDiD passes per-coefficient BM DOF through even when NaN. The average contrast also routes through safe_inference() with _bm_dof_avg, so the prior partial/fallback inference issue is closed.
Location: diff_diff/utils.py:L327-L366, diff_diff/estimators.py:L2131-L2194, tests/test_estimators_vcov_type.py:L547-L590
Concrete fix: No action required.

Finding: P3 — Documented dCDH DID_M^pl Sign Convention

Severity: P3 informational
Impact: The changed phase-1 placebo sign matches the existing multi-horizon path and R parity tests. The local paper review’s AER 2020 formula uses the forward-difference sign, but REGISTRY.md now explicitly documents the backward-difference × switch-direction convention. Per review rules, this documented deviation is not a defect.
Location: diff_diff/chaisemartin_dhaultfoeuille.py:L4666-L4689, docs/methodology/REGISTRY.md:L747-L758, tests/test_chaisemartin_dhaultfoeuille_parity.py:L177-L196
Concrete fix: No action required.

Finding: P3 — Documented HonestDiD Δ^SD FLCI Behavior

Severity: P3 informational
Impact: Returning finite optimal FLCI with lb/ub = NaN for an empty estimated identified set is documented in the registry and covered by tests. The intermediate-M optimizer center divergence from R is explicitly documented and tracked in TODO.md, so it is not an approval blocker.
Location: diff_diff/honest_did.py:L2469-L2505, docs/methodology/REGISTRY.md:L3527-L3528, TODO.md:L39, tests/test_methodology_honest_did.py:L414-L503
Concrete fix: No action required.

Finding: P3 — Documented CR2-BM Nuisance DOF Suppression

Severity: P3 informational
Impact: The unweighted CR2-BM guard NaNs non-physical/noise-floor nuisance-column DOF with a warning. The registry documents this as a reliability guard, and tests assert R parity for well-conditioned user-facing contrasts.
Location: diff_diff/linalg.py:L2081-L2155, docs/methodology/REGISTRY.md:L246, tests/test_estimators_vcov_type.py:L1608-L1709
Concrete fix: No action required.

Code Quality

No findings. The inference fix is centralized in safe_inference() and the estimator code now delegates to that contract.

Performance

No findings. The CR2 guard adds bookkeeping inside the existing per-contrast loop but does not change the main asymptotic structure.

Maintainability

No findings. The registry notes and tests make the documented deviations explicit enough for future reviewers.

Tech Debt

Finding: P3 — HonestDiD Intermediate-M R-Parity Work Is Tracked

Severity: P3 informational
Impact: The remaining Δ^SD FLCI optimizer center divergence is tracked in TODO.md and documented in REGISTRY.md.
Location: TODO.md:L39, docs/methodology/REGISTRY.md:L3528
Concrete fix: No approval action required.

Security

No findings.

Documentation/Tests

Finding: P3 — Local Test Execution Not Available In This Review Environment

Severity: P3 informational
Impact: I could not independently run the targeted tests because numpy is not installed in the local Python environment. The diff adds focused regression coverage for the prior P0 and the methodology parity changes.
Concrete fix: Ensure CI runs the touched tests: tests/test_utils.py, tests/test_estimators_vcov_type.py, tests/test_methodology_honest_did.py, and tests/test_chaisemartin_dhaultfoeuille_parity.py.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 5, 2026
@igerber igerber merged commit c3d6878 into main Jul 5, 2026
35 of 36 checks passed
@igerber igerber deleted the se-audit branch July 5, 2026 20:30
@igerber igerber mentioned this pull request Jul 9, 2026
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