Skip to content

perf(linalg): scores-based one-way Bell-McCaffrey DOF denominator#668

Merged
igerber merged 3 commits into
mainfrom
perf/oneway-bm-scores
Jul 10, 2026
Merged

perf(linalg): scores-based one-way Bell-McCaffrey DOF denominator#668
igerber merged 3 commits into
mainfrom
perf/oneway-bm-scores

Conversation

@igerber

@igerber igerber commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Summary

  • The one-way (non-clustered) unweighted Bell-McCaffrey DOF denominator a'(M∘M)a was evaluated against an explicit dense M = I − H (O(n²k) hat build; ~20 GB at n=50k, documented "practical for n < 10k"). The Schur-product expansion Σ a_i²(1−2h_ii) + tr((B·S_a)²) with S_a = X'diag(a)X (k×k) is exact algebra — (M∘M)_{il} = δ_{il}(1−2h_ii) + (x_i'Bx_l)² — costing O(nk² + k³) per contrast with no n×n allocation: n=50k/k=20 in ~16 ms.
  • New noise-floor cancellation guard: the dense denominator is ≥ 0 exactly (M∘M is PSD by the Schur product theorem), but the expanded difference of ~1e20-scale terms can collapse below float precision on extreme-leverage contrasts (e.g. a single-observation dummy at leverage 1); such denominators now NaN instead of reporting an arbitrarily inflated DOF — the same failure mode the clustered scores path guards, which the prior dense den > 0 kept.
  • Completes the scores-evaluation family: clustered CR2-BM (perf(linalg): scores-based CR2 Bell-McCaffrey DOF (algebraic identity; PT2018 §3.1 Satterthwaite) #656), low-rank A_g (perf(linalg): low-rank factored CR2-BM adjustment operators (8-108x) #664), one-way (this PR). Deletes the one-way TODO follow-up row.

Methodology references (required if estimator / math changes)

  • Method name(s): one-way HC2 Bell-McCaffrey / Imbens-Kolesár Satterthwaite DOF — evaluation change only, no formula change.
  • Paper / source link(s): Bell & McCaffrey (2002); Imbens & Kolesár (2016); REGISTRY hc2_bm one-way scores-evaluation note (added).
  • Any intentional deviations from the source (and why): the noise-floor guard NaNs extreme-leverage contrasts whose expanded denominator is float noise (documented REGISTRY Note; suppression-not-garbage per the no-silent-failures contract).

Validation

  • Tests added/updated: tests/test_linalg.py::TestOneWayBMScoresDOF — frozen dense-oracle parity at rtol 1e-10 (basic / high-leverage / k=40 + compound contrast) + a guard-firing regression (leverage-1 dummy NaNs, ordinary contrasts finite).
  • Backtest / simulation / notebook evidence (if applicable): benchmark table in the CHANGELOG entry.
  • 305 tests across test_linalg.py, test_linalg_hc2_bm.py (R-anchored goldens unchanged), and test_estimators_vcov_type.py pass.

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

Copy link
Copy Markdown

PR Review Report

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: one-way, non-clustered, unweighted hc2_bm Bell-McCaffrey / Imbens-Kolesar Satterthwaite DOF in _compute_bm_dof_from_contrasts.
  • The Schur-product denominator expansion matches the documented dense a'(M∘M)a formula and preserves the existing contrast semantics.
  • The new noise-floor NaN guard is documented in REGISTRY.md with a Note, so it is a P3 informational documented deviation, not a methodology defect.
  • Downstream inference paths already propagate non-finite BM DOF to all-NaN inference, avoiding misleading finite t/p/CI output.
  • Minor docstring issue: the Returns block still says NaN only when den <= 0, but implementation now also NaNs at/below the noise floor.

Methodology

Finding M1 — P3 informational, documented deviation / no action required

  • Location: diff_diff/linalg.py:L2767-L2802, docs/methodology/REGISTRY.md:L247
  • Impact: The new one-way scores evaluation and cancellation guard are documented in the Methodology Registry as exact algebra plus a noise-floor NaN guard. Under the review rules, this documented guard is not a blocker.
  • Concrete fix: None required.

Code Quality

No findings.

  • Severity: None
  • Impact: The implementation is scoped to the unweighted one-way path; weighted dispatch remains on the existing singleton-cluster CR2 route.
  • Concrete fix: None.

Performance

No findings.

  • Severity: None
  • Impact: The change removes the dense n x n residual-maker allocation and replaces it with O(n k^2 + k^3) per-contrast work.
  • Concrete fix: None.

Maintainability

No findings.

  • Severity: None
  • Impact: The inline derivation is clear and tied to the registry note and tests.
  • Concrete fix: None.

Tech Debt

No findings.

  • Severity: None
  • Impact: The removed TODO.md row corresponds to the implemented one-way BM scores denominator.
  • Concrete fix: None.

Security

No findings.

  • Severity: None
  • Impact: No secrets, unsafe IO, or security-sensitive changes observed.
  • Concrete fix: None.

Documentation/Tests

Finding D1 — P3 minor documentation mismatch

  • Location: diff_diff/linalg.py:L2733-L2736, diff_diff/linalg.py:L2801-L2802
  • Impact: The Returns block says DOF is NaN when den <= 0, but the new implementation also returns NaN when den <= noise_floor. The earlier docstring prose mentions the guard, so this is minor.
  • Concrete fix: Update the Returns block to say NaN is returned when the denominator is non-positive or at/below the cancellation noise floor.

Verification note: I could not run the targeted tests in this environment because pytest and numpy are not installed. The added tests at tests/test_linalg.py:L2719-L2792 cover dense-oracle parity and the leverage-one guard.

@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: d2715251cee3671e657d24816ff4d08ddbc11a4e


Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: one-way, non-clustered, unweighted hc2_bm Bell-McCaffrey / Imbens-Kolesar Satterthwaite DOF.
  • The scores-based denominator implements the documented Schur-product expansion of a'(M∘M)a; I did not find a methodology mismatch.
  • The new cancellation guard is documented in REGISTRY.md with a Note, so it is P3 informational, not a defect.
  • Prior P3 docstring mismatch is resolved: the Returns block now mentions the cancellation noise floor.
  • I could not run tests because this environment lacks pytest and numpy.

Methodology

Finding M1 — P3 informational, documented deviation / no action required

  • Location: diff_diff/linalg.py:L2777-L2803, docs/methodology/REGISTRY.md:L247
  • Impact: The denominator is still the same a'(M∘M)a BM/Satterthwaite quantity, evaluated by exact algebra without dense M. The guard changes extreme-leverage behavior to NaN, but it is explicitly documented as a registry Note.
  • Concrete fix: None required.

Code Quality

No findings.

  • Severity: None
  • Impact: The changed path is scoped to one-way unweighted DOF; weighted one-way hc2_bm still delegates to the singleton-cluster CR2 route.
  • Concrete fix: None.

Performance

No findings.

  • Severity: None
  • Impact: The dense n x n residual-maker is removed from the changed path and replaced with O(n k^2 + k^3) per-contrast work.
  • Concrete fix: None.

Maintainability

No findings.

  • Severity: None
  • Impact: The derivation is documented inline and matched by focused dense-oracle tests.
  • Concrete fix: None.

Tech Debt

No findings.

  • Severity: None
  • Impact: The removed TODO.md row corresponds to the implemented one-way BM scores denominator.
  • Concrete fix: None.

Security

No findings.

  • Severity: None
  • Impact: No secrets or security-sensitive changes observed.
  • Concrete fix: None.

Documentation/Tests

No unmitigated findings.

  • Severity: None
  • Impact: Prior review item is resolved at diff_diff/linalg.py:L2733-L2737. Added tests cover dense-oracle parity and the leverage-one NaN guard at tests/test_linalg.py:L2719-L2792.
  • Concrete fix: None.

Verification note: python -m pytest tests/test_linalg.py::TestOneWayBMScoresDOF -q could not run because pytest is not installed; a direct import check also failed because numpy is not installed.

igerber and others added 3 commits July 10, 2026 06:01
The non-clustered unweighted BM DOF computed a'(MoM)a against an
explicit dense M = I - H (O(n^2 k) hat build; ~20 GB at n=50k,
documented "practical for n < 10k"). The Schur-product expansion

  den = sum_i a_i^2 (1 - 2 h_ii) + tr((B S_a)^2),  S_a = X' diag(a) X

is exact algebra ((MoM)_{il} = delta_{il}(1-2h_ii) + (x_i'B x_l)^2):
O(n k^2 + k^3) per contrast, n=50k/k=20 in ~16 ms. Frozen dense-oracle
parity at rtol 1e-10 (basic/high-leverage/k40 + compound contrast);
R-anchored hc2_bm goldens pass unchanged.

New noise-floor cancellation guard: the dense den >= 0 exactly (MoM is
PSD by the Schur product theorem) but the expanded difference can
collapse below float precision on extreme-leverage contrasts; such
denominators now NaN instead of inflating the DOF arbitrarily (the
same failure mode the clustered scores path guards; the prior dense
den > 0 kept them).

Deletes the one-way TODO row (#656's follow-up). Completes the
scores-evaluation family: clustered CR2-BM (#656), low-rank A_g (#664),
one-way (this PR).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw
…ry contrasts finite

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lbd6nqWmg4snvvBmegwqiw
@igerber igerber force-pushed the perf/oneway-bm-scores branch from d271525 to 6395260 Compare July 10, 2026 10:02
@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 6395260c1a6526c34adb7ae897263641cc0288d7


Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: one-way, non-clustered, unweighted hc2_bm Bell-McCaffrey / Imbens-Kolesar Satterthwaite DOF.
  • The PR changes evaluation of the same denominator, a'(M∘M)a, from dense residual-maker allocation to the documented Schur-product expansion.
  • The new extreme-leverage NaN guard is documented in REGISTRY.md under a **Note...** label, so it is P3 informational, not a blocker.
  • Prior docstring/registry concern remains resolved: the docstring now describes the cancellation noise floor.
  • I could not run tests here because numpy is not installed.

Methodology

Finding M1 — P3 informational, documented deviation / no action required

  • Severity: P3
  • Location: diff_diff/linalg.py:L2738-L2850, docs/methodology/REGISTRY.md:L247
  • Impact: The changed unweighted one-way path still targets the documented BM/Satterthwaite denominator. The expansion follows from M = I-H: M_il^2 = δ_il(1-2h_ii) + h_il^2, and the cross term becomes tr(B S_a B S_a) with S_a = X'diag(a)X. The registry explicitly documents both the scores evaluation and the noise-floor NaN guard. This is consistent with the cited small-sample Satterthwaite/BRL lineage; Pustejovsky-Tipton describe BRL with Satterthwaite degrees of freedom and connect it to Bell-McCaffrey small-sample corrections. (arxiv.org)
  • Concrete fix: None required.

Code Quality

No findings.

  • Severity: None
  • Impact: The weighted path still dispatches before the modified unweighted algebra at diff_diff/linalg.py:L2789-L2800; no new public parameter or inline inference anti-pattern was introduced.
  • Concrete fix: None.

Performance

No findings.

  • Severity: None
  • Impact: The change removes the dense n x n residual-maker from the modified path and replaces it with O(n k^2 + k^3) per contrast work at diff_diff/linalg.py:L2824-L2838.
  • Concrete fix: None.

Maintainability

No findings.

  • Severity: None
  • Impact: The derivation is documented inline, and the helper remains scoped to BM DOF computation.
  • Concrete fix: None.

Tech Debt

No findings.

  • Severity: None
  • Impact: The removed TODO.md row corresponds to the implemented one-way BM scores-denominator work.
  • Concrete fix: None.

Security

No findings.

  • Severity: None
  • Impact: Secret-pattern scan of changed files found no obvious credentials.
  • Concrete fix: None.

Documentation/Tests

No unmitigated findings.

  • Severity: None
  • Impact: Added tests cover dense-oracle parity and the leverage-one NaN guard at tests/test_linalg.py:L2719-L2792; changelog and registry document the behavior at CHANGELOG.md:L626-L637 and docs/methodology/REGISTRY.md:L247.
  • Concrete fix: None.

Verification note: a direct numerical smoke check could not run because importing numpy fails with ModuleNotFoundError.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 10, 2026
@igerber igerber merged commit 121b6b2 into main Jul 10, 2026
35 of 36 checks passed
@igerber igerber deleted the perf/oneway-bm-scores branch July 10, 2026 11:42
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