fix(strategy): nystrom uses 3 landmark spaces, not 4 (drop redundant col(B))#270
Open
galuis116 wants to merge 1 commit into
Open
fix(strategy): nystrom uses 3 landmark spaces, not 4 (drop redundant col(B))#270galuis116 wants to merge 1 commit into
galuis116 wants to merge 1 commit into
Conversation
…col(B)) NystromTransform split its M-column budget across col(A), row(A), col(B), row(B) -- but col(B) is redundant. With P = Q Qᵀ, Ĉ = P A P B P = A B once range(Q) contains col(A), row(A), row(B) (P A = A, A P = A, B P = B); col(B) contributes nothing. This is the same argument rsvd already uses (3 sketches, zeokin#91). So a full M/4 of the landmark budget was wasted and exact recovery of a rank-r product needed M >= 4r instead of 3r -- ~25% worse accuracy per M on the low-rank/decaying-spectrum regimes nystrom targets. Split M across the three necessary spaces (drop the col(B) block). Same basis cost (gather M columns + thin QR), strictly higher accuracy per M. Adds a CPU test that recovers a rank-4 product at M=15 (5 per space) -- which the old 4-space split (M/4=3 < 4 per space) could not. Distinct from zeokin#226 (a new rrqr-nystrom transform; this fixes the existing one). Fixes zeokin#269
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #269.
NystromTransformsplit its M-column landmark budget across four spaces —col(A), row(A), col(B), row(B) — but col(B) is redundant. With the
projector
P = Q Qᵀ,Ĉ = P A P B P = A Boncerange(Q)contains onlycol(A), row(A), row(B):
col(B) contributes nothing — the exact argument
rsvdalready uses (3 sketches,#91 "rsvd wastes ~25% of the rank budget").
nystromnever got it, so a fullM/4 of the budget was wasted, and exact recovery of a rank-
rproduct neededM ≳ 4rinstead of3r— ~25% worse accuracy per M on the low-rank /decaying-spectrum regimes it targets.
The fix splits M across the three necessary spaces (drops the col(B) block).
Same basis cost (gather M columns + thin QR — latency/VRAM unchanged),
strictly higher accuracy per M.
Test plan
python strategy/tests/test_nystrom_three_spaces.py— 3/3 (CPU): recovers arank-4
A@Bat M=15 (≈5 landmarks/space); basis orthonormal & correctly shaped.This test fails on the current 4-space nystrom (M/4 = 3 < r = 4 per space →
cannot recover) and passes on the fix — a direct demonstration of the wasted budget.
ruff check strategy/transforms.py strategy/tests/test_nystrom_three_spaces.py— clean.Notes
rrqr-nystromtransform and does nottouch the existing
NystromTransform; this fixes the existing one.NystromTransform.basis's signature in line with the base class /rsvd(frac=None), completing the frac-signature consistency of [bug] transform_template's basis() still omits frac, so every copied transform silently ignores --vram-fraction (the #211 bug, re-introduced per contribution) #250–fix(strategy): finish the frac basis() signature across contributor-facing snippets (Fixes #252) #253(nystrom gathers on the host and doesn't stream, so
fracis simply accepted).No scorecard: this is a correctness/efficiency
fix:to a built-in transform (thebudget split), not a new strategy —
fix:PRs are scorecard-exempt in the gate chain.The change only reallocates existing landmark budget, so latency/VRAM are unchanged
while accuracy-per-M strictly improves.