Skip to content

perf(linalg): scores-based CR2 Bell-McCaffrey DOF (PT2018 Appendix B)#656

Open
igerber wants to merge 2 commits into
mainfrom
perf/cr2-bm-scores-dof
Open

perf(linalg): scores-based CR2 Bell-McCaffrey DOF (PT2018 Appendix B)#656
igerber wants to merge 2 commits into
mainfrom
perf/cr2-bm-scores-dof

Conversation

@igerber

@igerber igerber commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • Closes the CR2-BM Heavy TODO row: the unweighted per-contrast Satterthwaite DOF materialized the dense n×n residual-maker M = I − X(X'X)⁻¹X' and contracted it over all cluster pairs — O(n²) time per contrast and O(n²) memory (3.2 GB at n=20k). Per Pustejovsky-Tipton (2018) Appendix B, with Ω the (n, G) matrix stacking ω_g on disjoint cluster supports, the pairwise matrix collapses to B = Ω'MΩ = diag(‖ω_g‖²) − P'M_U P with P = X'ΩO(nk + G²k) per contrast, no n×n allocation.
  • Measured: ~32x at n=5k/G=50 (0.57s → 0.018s); n=20k/G=100 completes in 0.12s where the old path would allocate 3.2 GB. Algebraically identical: a frozen pair-loop oracle (the previous implementation reimplemented verbatim in-test) agrees at rtol 1e-10 across balanced, unbalanced (cluster sizes 3–60), and compound-contrast designs. Both NaN-reliability guards (noise floor via max|B|, cluster-count bound DOF ≤ G) operate unchanged on the same B values.
  • The weighted clubSandwich P-array path is untouched (it never built the n×n M). All 432 consumer tests (linalg, vcov-type incl. the CI-inversion dof_hc2_bm pins, stacked-DiD methodology, estimators) pass unmodified. REGISTRY: per-fit setup wording updated + an Appendix-B evaluation sentence on the existing unweighted-DOF-guard Note.

Methodology references (required if estimator / math changes)

  • Method name(s): CR2 Bell-McCaffrey Satterthwaite DOF (Pustejovsky & Tipton 2018, Section 4 / Appendix A formula; Appendix B scores-based evaluation) — evaluation change only, same estimand
  • Paper / source link(s): REGISTRY.md DiD/MPD hc2_bm Notes (updated); PT2018 J. Bus. Econ. Statist.
  • Any intentional deviations from the source (and why): None — algebraically identical evaluation, oracle-locked.

Validation

  • Tests added/updated: tests/test_linalg.py::TestCR2BMScoresBasedDOF (3 frozen-oracle parity tests).
  • Backtest / simulation / notebook evidence (if applicable): timing table above (in-session measurements).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Overall Assessment

✅ Looks good. No unmitigated P0/P1 findings.

Executive Summary

  • Method affected: unweighted CR2 Bell-McCaffrey Satterthwaite DOF in diff_diff/linalg.py.
  • The scores-based B = Omega' M Omega expansion is algebraically consistent with the previous residual-maker contraction and with the project’s REGISTRY note.
  • Existing NaN/noise-floor and cluster-count guards remain in place.
  • P2 performance issue: the implementation materializes P_all for all contrasts, so batched per-coefficient DOF can allocate O(G*k*m) memory, contradicting the stated O(G*k) memory bound.
  • I could not run tests: numpy, scipy, and pytest are not installed in this environment.

Methodology

No blocking findings.

Severity: P3 informational
Impact: This is an evaluation rewrite, not a methodology change. The affected method is CR2 Bell-McCaffrey Satterthwaite DOF. PT2018 covers BRL/CRVE with Satterthwaite degrees of freedom, and clubSandwich’s Satterthwaite_df reduces to (trace P)^2 / sum(P^2), matching the registry/code contract. (arxiv.org)
Concrete fix: No required change.

Code Quality

No findings.

Performance

Severity: P2
Impact: diff_diff/linalg.py:L2151-L2168 allocates P_all = np.zeros((G, k, m)). For _compute_cr2_bm(..., contrasts=np.eye(k)), m == k, so this becomes O(G*k^2) memory. That can be worse than intended on full-dummy/absorbed-FE-style designs with many clusters and coefficients, and it contradicts the O(G*k) memory claim in diff_diff/linalg.py:L2124-L2126, CHANGELOG.md:L453-L456, and docs/methodology/REGISTRY.md:L246.
Concrete fix: Stream or chunk contrasts. Inside the for j in range(m) loop, build P_j from each cluster’s omega_all[g][:, j] instead of storing all P_all; alternatively chunk m so peak memory is bounded. Update the documented memory bound to include the G x G B_j matrix if it remains materialized.

Maintainability

No blocking findings.

Severity: P3
Impact: diff_diff/linalg.py:L1856-L1858 still says the shared core precomputes the unweighted residual-maker M, but this PR removes that allocation.
Concrete fix: Remove the stale M reference from the docstring.

Tech Debt

No new untracked correctness debt found. The old TODO row for CR2-BM dense residual-maker allocation is reasonably removed, subject to the P2 memory follow-up above.

Security

No findings.

Documentation/Tests

Severity: P3
Impact: The added oracle tests cover balanced, unbalanced, and compound-contrast parity, but I could not execute them locally because test dependencies are unavailable.
Concrete fix: Run tests/test_linalg.py::TestCR2BMScoresBasedDOF and the existing CR2-BM NaN-guard tests in CI.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jul 8, 2026
@igerber igerber force-pushed the perf/cr2-bm-scores-dof branch from cb5c83e to 7bc8d10 Compare July 8, 2026 10:02
igerber and others added 2 commits July 8, 2026 07:18
The unweighted per-contrast Satterthwaite DOF materialized the dense
n x n residual-maker M = I - X(X'X)^-1 X' and contracted it over all
cluster pairs (O(n^2) time per contrast, O(n^2) memory — 3.2 GB at
n=20k). With Omega the (n, G) matrix stacking omega_g on disjoint
cluster supports, B = Omega' M Omega collapses to
diag(||omega_g||^2) - P' M_U P with P = X' Omega: O(nk + G^2 k) per
contrast, no n x n allocation. ~32x at n=5k/G=50 (0.57s -> 0.018s);
n=20k/G=100 in 0.12s. Algebraically identical — frozen pair-loop
oracle parity at rtol 1e-10 (balanced, unbalanced, compound
contrasts); both NaN-reliability guards unchanged; 432 consumer tests
pass unmodified. REGISTRY notes updated (per-fit setup wording +
Appendix-B evaluation sentence).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…P2) + doc syncs (P3s)

P2: P_all was (G, k, m) — O(G*k^2) on the batched per-coefficient sweep
(contrasts=eye(k)), contradicting the stated O(Gk) bound. The
per-cluster product buffers are now contrast-chunked under a 64 MB
module-constant cap (monkeypatchable); each contrast's B is computed
independently so chunking is bit-identical (locked by a forced
one-contrast-chunk test with assert_array_equal). P3s: stale
residual-maker-M mention removed from the shared-core docstring;
CHANGELOG + REGISTRY memory claims updated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@igerber igerber force-pushed the perf/cr2-bm-scores-dof branch from 7bc8d10 to 47d22dd Compare July 8, 2026 11:20
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