Skip to content

perf(linalg): compute hc2_bm CR2 sandwich once, not twice (#475)#565

Merged
igerber merged 1 commit into
mainfrom
perf/hc2bm-cr2-compute-once
Jun 28, 2026
Merged

perf(linalg): compute hc2_bm CR2 sandwich once, not twice (#475)#565
igerber merged 1 commit into
mainfrom
perf/hc2bm-cr2-compute-once

Conversation

@igerber

@igerber igerber commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • LinearRegression(vcov_type="hc2_bm"): compute the CR2 sandwich once, not twice. The fit path computed the CR2/HC2-BM vcov inside solve_ols, then recomputed the entire Bell-McCaffrey sandwich (per-cluster A_g, the O(n²k) Satterthwaite loop) a second time via compute_robust_vcov(return_dof=True) just to extract the per-coefficient DOF. _compute_cr2_bm / _compute_bm_dof_oneway already return (vcov, dof) together, so the second sandwich was pure waste.
  • Fix (low blast radius): on the hc2_bm-and-not-survey path, skip solve_ols's vcov (return_vcov=False via a new _is_bm_path flag) and source BOTH self.vcov_ and self._bm_dof from a single compute_robust_vcov(..., return_dof=True) call, reusing the existing _expand_vcov_with_nan rank-deficient expansion. solve_ols (and its ~50 callers) is left untouched. Affects every hc2_bm fit — weighted one-way WLS-CR2, weighted/unweighted clustered CR2-BM (DiD/TWFE/MPD with vcov_type="hc2_bm").
  • Closes the #475 Performance backlog row (CI-codex P3 from PR Lift hc2_bm + weights gates via clubSandwich WLS-CR2 port #475).

Methodology references

  • Method: LinearRegression HC2-BM / CR2 variance — Imbens & Kolesár (2016) one-way Bell-McCaffrey; Pustejovsky & Tipton (2018) / clubSandwich CR2 (incl. the pweight WLS-CR2 port). See docs/methodology/REGISTRY.md "Variance Estimation, Cluster-Robust SE".
  • Intentional deviations: None. The CR2/BM formula is unchanged — this only removes a duplicate evaluation. Bit-identical SEs and per-coefficient DOF (both the prior solve_ols vcov and the combined call route through the same _compute_robust_vcov_numpy). No methodology / numerical / public-API change → no REGISTRY edit.

Validation

  • Tests added (tests/test_methodology_wls_cr2.py): (a) test_lr_hc2_bm_vcov_value_matches_helper — directly pins self.vcov_ against an independently-computed CR2 vcov + asserts each SE is its sqrt-diagonal (weighted cluster / weighted one-way / unweighted); (b) test_lr_hc2_bm_cr2_sandwich_computed_once — mechanism guard: patches _compute_robust_vcov_numpy and asserts the sandwich runs once, not twice across weighted/unweighted × one-way/clustered (covers both the _compute_cr2_bm and unweighted-one-way _compute_bm_dof_oneway sub-paths).
  • Proven bit-identical at atol=0 (dev capture) across weighted/unweighted × one-way/clustered + rank-deficient: vcov, _bm_dof, and per-coefficient SE/DOF/p all exact-match pre/post.
  • Existing golden guards stay green: test_methodology_wls_cr2.py (vcov + dof atol=1e-10), test_linalg_hc2_bm.py (clubSandwich atol=1e-6). Targeted suite: test_methodology_wls_cr2.py + test_linalg_hc2_bm.py (64) and test_linalg.py + test_estimators.py (269 passed / 1 skipped) all green. black / ruff / mypy clean (0 new).
  • Minor behavior note: the HC2→HC1 high-leverage-fallback UserWarning now fires once per fit instead of twice (no test asserts a count).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

LinearRegression(vcov_type="hc2_bm").fit computed the CR2/HC2-BM sandwich
twice: once inside solve_ols for the vcov, then again via
compute_robust_vcov(return_dof=True) just to extract the per-coefficient
Bell-McCaffrey DOF. The CR2 helper returns (vcov, dof) together, so the
second sandwich (per-cluster A_g + the O(n^2 k) Satterthwaite loop) was
pure waste (#475).

Skip solve_ols's vcov on the hc2_bm-not-survey path (return_vcov=False via
a new _is_bm_path flag) and source BOTH vcov and dof from one
compute_robust_vcov(return_dof=True) call, reusing the existing
_expand_vcov_with_nan rank-deficient expansion. solve_ols (and its ~50
callers) is left untouched. Affects every hc2_bm fit: weighted one-way
WLS-CR2, weighted/unweighted clustered CR2-BM (DiD/TWFE/MPD with
vcov_type="hc2_bm").

Bit-identical SEs + per-coef DOF (proven at atol=0 across weighted/
unweighted x one-way/clustered + rank-deficient; both vcov computations
route through the same _compute_robust_vcov_numpy). No methodology,
numerical, or public-API change. Minor: the HC2->HC1 high-leverage
fallback UserWarning now fires once per fit instead of twice.

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

Copy link
Copy Markdown

Overall Assessment

✅ Looks good. No unmitigated P0/P1 findings.

Executive Summary

  • Affected method: LinearRegression(vcov_type="hc2_bm") HC2-BM / CR2 Bell-McCaffrey variance path.
  • The PR preserves the documented methodology: vcov and per-coefficient DOF are still sourced from compute_robust_vcov(..., return_dof=True) / _compute_cr2_bm.
  • Rank-deficient expansion and survey-vcov precedence appear preserved.
  • The remaining MPD contrast-DOF recomputation is documented in TODO.md, so it is non-blocking.
  • I could not run tests because pytest is not installed in this environment.

Methodology

No findings.

The changed path in diff_diff/linalg.py:L3544-L3614 only changes where LinearRegression.fit() obtains the HC2-BM vcov: it skips the earlier solve_ols vcov computation and then obtains (vcov, _bm_dof) together from compute_robust_vcov(..., return_dof=True). This matches the registry’s HC2-BM / CR2 Bell-McCaffrey contract in docs/methodology/REGISTRY.md:L224-L249 and the WLS-CR2 clubSandwich notes in docs/methodology/REGISTRY.md:L3047-L3055.

No undocumented formula, weighting, variance, SE, or assumption deviation found.

Code Quality

No findings.

The implementation keeps the change localized to LinearRegression.fit() and preserves rank-deficient behavior by recomputing on kept columns and expanding via _expand_vcov_with_nan at diff_diff/linalg.py:L3597-L3614.

Performance

  • Severity: P3 informational
  • Impact: The PR removes the duplicated LinearRegression HC2-BM sandwich computation. A separate MPD cluster+hc2_bm contrast-DOF recomputation remains.
  • Concrete fix: No action required for this PR. The remaining item is tracked in TODO.md:L58-L60.

Maintainability

No findings.

The new tests in tests/test_methodology_wls_cr2.py:L464-L557 cover value plumbing and mechanism-level single-call behavior across weighted one-way, weighted clustered, unweighted one-way, and unweighted clustered paths.

Tech Debt

  • Severity: P3 informational
  • Impact: The PR removes the obsolete TODO row for the fixed duplicated LinearRegression.fit() cost and leaves the still-valid MPD follow-up tracked.
  • Concrete fix: No action required. Future work can share/plumb CR2 precomputes for MPD contrast DOF as described in TODO.md:L58-L60.

Security

No findings. No secret-handling or external-input security surface is changed.

Documentation/Tests

No PR defect found.

The changelog documents the performance change and the warning-count side effect. Test coverage was added for the changed HC2-BM path. Verification note: PYTHONDONTWRITEBYTECODE=1 pytest -p no:cacheprovider tests/test_methodology_wls_cr2.py -q could not run here because pytest is unavailable.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jun 28, 2026
@igerber igerber merged commit 0ded9d0 into main Jun 28, 2026
33 of 34 checks passed
@igerber igerber deleted the perf/hc2bm-cr2-compute-once branch June 28, 2026 09:59
igerber added a commit that referenced this pull request Jun 28, 2026
MultiPeriodDiD(cluster=..., vcov_type="hc2_bm") built the expensive CR2
Bell-McCaffrey per-cluster precomputes (A_g eigendecompositions, S_W, the
residual-maker M = I - H) twice per fit: once in solve_ols's vcov path
(whose per-coefficient DOF was then discarded) and again in the separate
_compute_cr2_bm_contrast_dof call for the per-coef + post-period-average
ATT Satterthwaite DOF.

Extract a shared core `_compute_cr2_bm_vcov_and_dof` (vcov + DOF for
arbitrary contrasts from one precompute build); `_compute_cr2_bm` and
_compute_cr2_bm_contrast_dof` become thin bit-identical wrappers over it
(removing the formerly-duplicated ~50-line precompute block, so every CR2
caller routes through one implementation). The shared filter guards the
residuals subscript with `residuals is not None` so DOF-only callers that
pass weights with zero rows do not crash. At the MPD fit level, bypass
solve_ols's vcov on the cluster+hc2_bm (non-survey, unweighted) path and
compute vcov + the combined per-coef/avg-ATT DOF from one call, then
expand with NaN for dropped columns; the `survey_weights is None` clause
keeps the bypass byte-identical to solve_ols (else it falls back to the
prior two-call behavior).

Bit-identical vcov_, per-period DOF, avg-ATT DOF, p-values, and CIs
(proven at atol=0 across balanced, unbalanced, and rank-deficient
designs); a mechanism test asserts the per-cluster adjustment matrix is
built exactly once per cluster (down from twice). No methodology,
numerical, or public-API change. Updates the stale REGISTRY note that
described the prior twice-built precompute.

Resolves the MPD cluster+hc2_bm Performance row in TODO.md and completes
the MultiPeriodDiD follow-up noted in the #565 LinearRegression entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
igerber added a commit that referenced this pull request Jun 28, 2026
MultiPeriodDiD(cluster=..., vcov_type="hc2_bm") built the expensive CR2
Bell-McCaffrey per-cluster precomputes (A_g eigendecompositions, S_W, the
residual-maker M = I - H) twice per fit: once in solve_ols's vcov path
(whose per-coefficient DOF was then discarded) and again in the separate
_compute_cr2_bm_contrast_dof call for the per-coef + post-period-average
ATT Satterthwaite DOF.

Extract a shared core `_compute_cr2_bm_vcov_and_dof` (vcov + DOF for
arbitrary contrasts from one precompute build); `_compute_cr2_bm` and
_compute_cr2_bm_contrast_dof` become thin bit-identical wrappers over it
(removing the formerly-duplicated ~50-line precompute block, so every CR2
caller routes through one implementation). The shared filter guards the
residuals subscript with `residuals is not None` so DOF-only callers that
pass weights with zero rows do not crash. At the MPD fit level, bypass
solve_ols's vcov on the cluster+hc2_bm (non-survey, unweighted) path and
compute vcov + the combined per-coef/avg-ATT DOF from one call, then
expand with NaN for dropped columns; the `survey_weights is None` clause
keeps the bypass byte-identical to solve_ols (else it falls back to the
prior two-call behavior).

Bit-identical vcov_, per-period DOF, avg-ATT DOF, p-values, and CIs
(proven at atol=0 across balanced, unbalanced, and rank-deficient
designs); a mechanism test asserts the per-cluster adjustment matrix is
built exactly once per cluster (down from twice). No methodology,
numerical, or public-API change. Updates the stale REGISTRY note that
described the prior twice-built precompute.

Resolves the MPD cluster+hc2_bm Performance row in TODO.md and completes
the MultiPeriodDiD follow-up noted in the #565 LinearRegression entry.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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