Skip to content

fix: clamp negative vcov diagonal in LinearRegression SE (no NaN SE for degenerate coefs)#542

Merged
igerber merged 1 commit into
mainfrom
fix/cr2-bm-se-degenerate-variance-guard
Jun 22, 2026
Merged

fix: clamp negative vcov diagonal in LinearRegression SE (no NaN SE for degenerate coefs)#542
igerber merged 1 commit into
mainfrom
fix/cr2-bm-se-degenerate-variance-guard

Conversation

@igerber

@igerber igerber commented Jun 22, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes a pre-existing, nondeterministic NaN standard error in LinearRegression for degenerate/high-leverage coefficients, which surfaced as the lone failure in the Pure Python Fallback CI leg of PR #539 (a docs PR that can't have caused it).

Root cause: a high-leverage / degenerate coefficient (e.g. an absorbed-FE dummy near-collinear with the treatment, whose Bell-McCaffrey Satterthwaite DOF already hits the noise-floor guard) has a CR2/HC variance of ~0 (measured vcov[i,i] ≈ 3e-32, min vcov eigenvalue -3.2e-17). That tiny diagonal lands just-below-zero under BLAS-dependent float rounding, so np.sqrt(vcov[i,i]) returned NaN — passing single-threaded / in isolation, failing only under the parallel -n auto pure-Python full-suite run.

Fix: both SE sites (get_se, get_inference) clamp the vcov diagonal at 0 before sqrt. The SE is now finite (0 for a genuinely-zero variance), deterministic, and BLAS-independent. The clamp is a no-op for any positive variance, so no non-degenerate SE / t / p / CI changes and no R-parity impact.

  • diff_diff/linalg.py — clamp at both SE sites (max(vcov[i,i], 0.0)).
  • tests/test_methodology_wls_cr2.py — relax the guarded-coef assertion se > 0 → finite & >= 0 (rounding-robust), and add a deterministic regression test (test_negative_variance_artifact_yields_finite_se_not_nan) that injects a tiny-negative diagonal and asserts a finite 0 SE, not NaN.
  • docs/methodology/REGISTRY.md (Variance Estimation, Cluster-Robust SE) + CHANGELOG.md documented.

Why separate from #539: #539 is a docs PR; this is an unrelated pre-existing numerical bug. main doesn't run the Pure Python Fallback leg outside labeled PRs, so it had been latent. Once this merges, #539 rebases onto it and its full suite goes green.

Methodology references (required if estimator / math changes)

  • Method name(s): LinearRegression CR2/HC standard-error extraction (clubSandwich CR2-BM; Bell-McCaffrey / Pustejovsky-Tipton 2018).
  • Paper / source link(s): N/A — numerical-robustness clamp only; the CR2 vcov algorithm is unchanged and R-parity is unaffected (positive variances are untouched).
  • Any intentional deviations from the source: None. Defensive clamp of a numerical artifact (a variance estimate is never negative).

Validation

  • Tests added/updated: tests/test_methodology_wls_cr2.py (relaxed guarded assertion + new deterministic clamp regression test). Consumer sweep green: test_linalg.py + test_methodology_wls_cr2.py + test_estimators.py + test_methodology_did.py (344 passed, default backend; 136 passed pure-Python). black/ruff clean; pre-existing mypy notes only (none from this change).
  • Backtest / simulation / notebook evidence: N/A.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

A high-leverage / degenerate coefficient (e.g. an absorbed-FE dummy
near-collinear with the treatment, whose Bell-McCaffrey Satterthwaite DOF
already hits the noise-floor guard) can have a CR2/HC variance of ~0
(≈1e-32) whose vcov diagonal lands just-below-zero under BLAS-dependent
float rounding. `np.sqrt` of the negative then produced a NaN SE
NONDETERMINISTICALLY — passing single-threaded but failing only under the
parallel pure-Python full-suite run (the Pure Python Fallback CI leg), on
test_methodology_wls_cr2.py::TestLinearRegressionFENanGuardEndToEnd.

- linalg.py: clamp the vcov diagonal at 0 before sqrt in both SE sites
  (get_se, get_inference). No-op for positive variances; the SE is now
  finite (0 for a genuinely-zero variance), deterministic, BLAS-independent.
- test: relax the guarded-coef assertion to finite & >= 0 (rounding-robust)
  + a deterministic regression test (a tiny-negative diagonal yields a
  finite 0 SE, not NaN).
- REGISTRY (Variance Estimation, Cluster-Robust SE) + CHANGELOG documented.

Pre-existing failure surfaced by PR #539's labeled full-suite run; main
does not run the Pure Python Fallback leg outside labeled PRs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

PR Review Report

Overall Assessment

Looks good — No unmitigated P0/P1 findings.

Executive Summary

  • Affected method: shared LinearRegression.get_se() / get_inference() SE extraction from vcov_.
  • The vcov algorithm, CR2/HC weighting, Bell-McCaffrey DOF logic, and assumption checks are unchanged.
  • The clamp is explicitly documented in docs/methodology/REGISTRY.md:L189-L195, so it is not an undocumented methodology deviation.
  • get_inference() still routes the clamped zero SE through safe_inference(), which preserves the project’s all-NaN inference contract for zero SEs.
  • I could not run tests in this environment because pytest and numpy are unavailable.

Methodology

Finding: P3 informational — Documented numerical clamp

  • Location: diff_diff/linalg.py:L3868-L3874, diff_diff/linalg.py:L3917-L3918, docs/methodology/REGISTRY.md:L189-L195
  • Impact: The implementation changes SE extraction from sqrt(vcov[i,i]) to sqrt(max(vcov[i,i], 0.0)). This is a numerical robustness deviation from the literal extraction formula, but it is explicitly documented with a **Note:** in the Methodology Registry and does not alter positive variances.
  • Concrete fix: No action required. Optional: move or cross-reference the note from the MultiPeriodDiD area to the shared analytical-sandwich / LinearRegression registry section for discoverability.

No P0/P1 methodology issue found. The zero-SE inference path remains NaN-consistent via safe_inference() at diff_diff/utils.py:L327-L361.

Code Quality

No findings.

The changed code is localized and both direct SE extraction sites in LinearRegression were updated.

Performance

No findings.

The clamp is O(1) and has no material performance impact.

Maintainability

No findings.

The duplicated clamp in get_se() and get_inference() is acceptable for this narrow fix. A helper could reduce duplication later, but that is not needed for approval.

Tech Debt

No findings.

No new untracked deferral or silent correctness risk observed.

Security

No findings.

No secrets, I/O surface, subprocess use, or security-sensitive behavior introduced.

Documentation/Tests

Finding: P3 informational — Regression test could also pin inference fields

  • Location: tests/test_methodology_wls_cr2.py:L821-L826
  • Impact: The new deterministic test verifies get_se() and inf.se become finite zero, but it does not explicitly assert inf.t_stat, inf.p_value, and inf.conf_int are NaN after the clamp. The production code already satisfies this through safe_inference(), and nearby guarded-coefficient coverage asserts NaN inference fields at tests/test_methodology_wls_cr2.py:L766-L776, so this is not blocking.
  • Concrete fix: Optional: add assertions in test_negative_variance_artifact_yields_finite_se_not_nan that inf.t_stat, inf.p_value, and both CI endpoints are NaN.

Verification note: attempted focused pytest execution, but the environment lacks pytest; a direct Python smoke check also could not run because numpy is unavailable.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jun 22, 2026
@igerber igerber merged commit 804d7a1 into main Jun 22, 2026
33 of 34 checks passed
@igerber igerber deleted the fix/cr2-bm-se-degenerate-variance-guard branch June 22, 2026 22:01
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