Skip to content

perf: use covariance path in svd_whiten when n_samples >= n_features - #211

Open
jameschapman19 wants to merge 2 commits into
mainfrom
fix/svd-whiten-covariance-path
Open

perf: use covariance path in svd_whiten when n_samples >= n_features#211
jameschapman19 wants to merge 2 commits into
mainfrom
fix/svd-whiten-covariance-path

Conversation

@jameschapman19

@jameschapman19 jameschapman19 commented Apr 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • For tall matrices (n >= p), svd_whiten now forms the p×p sample covariance C = Xᵀ X / (n-1) and diagonalises it with eigh, rather than calling np.linalg.svd(X) which allocates an n×p intermediate U matrix.
  • For wide matrices (n < p) the original SVD path is retained.
  • No change to the public API.

Context

This PR was originally opened against rewrite/v3, which was merged into main back in March and never cleaned up as a branch — so the PR had been sitting stale against a dead-end base for months with an unchecked test plan. Retargeted to main (the diff is unchanged: rewrite/v3's tip is an ancestor of main, so nothing else has touched this function since).

Found and fixed a real correctness bug while verifying it before merging: the covariance path's lam > 0 filter isn't numerically safe. Forming X.T @ X squares the condition number, so on rank-deficient tall data (n >= p but effective rank < p) near-zero eigenvalues can carry noise of either sign and slip through a strict zero threshold — producing a whitening matrix with spuriously huge columns for directions that don't actually exist in the data. The existing test_rank_deficient_input regression test caught this immediately once run against the real suite (previously the PR's own test-plan checkboxes were never actually checked off). Fixed with a relative tolerance matching numpy.linalg.matrix_rank's convention (lam.max() * p * eps) instead of a bare > 0.

Verification performed

  • Full fast suite passes (483 tests) with the corrected version.
  • Whitening postcondition (X_white.T @ X_white / (n-1) ≈ I) verified to hold across tall/wide/square shapes, with and without regularisation, and on both full-rank and rank-deficient input.
  • Benchmarked the actual speedup rather than trusting the description: ~14x at this PR's own motivating scale (n=54,000, p=392).
  • Added direct unit coverage for the previously-untested wide (n < p) branch and the n == p boundary.

Test plan

  • Existing tests pass (pytest -m "not slow" — 483 passed)
  • Verified CCA, rCCA, PLS, CCA_EY give equivalent results before/after (rotation-invariant Gram-matrix check + full test suite for the two real consumers, rCCA/CCA_EY)
  • Confirmed speed improvement on large n >> p data (~14x at n=54,000, p=392)
  • Added regression coverage for the rank-deficiency bug found during review

Generated by Claude Code

Ports PR #211 forward onto main (it was opened against `rewrite/v3`,
which was merged into main back in March and never cleaned up as a
branch — the PR had been sitting stale against a dead-end base ever
since) and fixes a real correctness bug found while verifying it:
the covariance path's `lam > 0` filter isn't numerically safe. Forming
X.T @ X squares the condition number, so on rank-deficient tall data
(n >= p but effective rank < p) near-zero eigenvalues can carry noise
of either sign and slip through a strict zero threshold, producing a
whitening matrix with spuriously huge columns. Existing rank-deficient
test caught this immediately once verified against the real suite.
Fixed with a relative tolerance matching numpy.linalg.matrix_rank's
convention (`lam.max() * p * eps`) instead of a bare `> 0`.

Verified independently before merging:
- Full fast suite passes (483 tests) with the corrected version
- Whitening postcondition (X_white.T @ X_white / (n-1) ~= I) holds
  across tall/wide/square, regularised/unregularised, full and
  rank-deficient inputs
- Benchmarked the actual speedup: ~14x at the PR's motivating scale
  (n=54,000, p=392), confirming the optimization is real and worth
  having, not just plausible-sounding
- Added direct unit coverage for the previously-untested wide (n < p)
  branch and the n == p boundary

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CL3GT9jTbPvuCwbvmztghe
@jameschapman19
jameschapman19 force-pushed the fix/svd-whiten-covariance-path branch from 32d6aa5 to 4fc6869 Compare August 3, 2026 18:29
@jameschapman19
jameschapman19 changed the base branch from rewrite/v3 to main August 3, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants