fix: use relative cutoffs for singular values and residuals#213
Open
gaoflow wants to merge 1 commit into
Open
Conversation
pseudoInverse compared singular values against an absolute constant (Number.EPSILON) and linearDependencies compared the solve residual against an absolute 10e-10. Both quantities scale with the matrix, so the answers depended on the magnitude of the input rather than on its rank: a rank-deficient matrix gets a pseudo-inverse made of rounding noise, and a small enough matrix gets truncated to zero or reported as fully dependent. pseudoInverse now uses threshold * max(rows, columns) * s[0], the tolerance SVD.rank already applies, which is also the rcond * max(s) rule of LAPACK pseudo-inverses. linearDependencies divides the residual by the magnitude of the row it explains; thresholdValue stays absolute because the coefficients it filters are scale-free.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 68.58% 68.60% +0.02%
==========================================
Files 49 49
Lines 5847 5852 +5
Branches 1042 1043 +1
==========================================
+ Hits 4010 4015 +5
Misses 1824 1824
Partials 13 13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stropitek
self-requested a review
July 27, 2026 07:47
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.
pseudoInversecompares singular values against an absolute constant (Number.EPSILON) andlinearDependenciescompares its solve residual against an absolute10e-10. Both quantities scale with the matrix, so both functions answer according to the magnitude of the input rather than its rank — in both directions:The library already contains the correct rule:
SVD.thresholdandSVD.rankare relative tos[0], andSVD.solve/SVD.inverseuse it — sosvd.inverse()returns the right pseudo-inverse from the very same decomposition thatpseudoInversegets wrong.numpy.linalg.pinvuses the same relative rule (rcond * max(s)).Fix. The
pseudoInversecutoff becomesthreshold * max(rows, columns) * s[0], which is exactlySVD.rank's tolerance, so the two can no longer disagree about which singular values are zero.linearDependenciesdivides the residual by the magnitude of the row it explains.thresholdValuestays absolute — it filters the coefficients, which are already scale-free. This changesthresholdandthresholdErrorfrom absolute to relative parameters;matrix.d.tsis updated.Tests. Five new tests, all red before the change: numpy values for two rank-deficient matrices at ordinary magnitude,
pinv(k*A) === pinv(A)/kforkfrom 1e-17 to 1e17 over four shapes, the four Moore-Penrose conditions at three magnitudes, and scale invariance oflinearDependenciesin both directions. The existing pinned tests are unchanged and still pass (261 -> 266) — the oldpseudoinversetest only used matrices at the one magnitude where an absolute cutoff happens to work. I also ran a property sweep over ~12.5k random cases (shapes 1..6, magnitudes 1e-18..1e18, 40% forced rank-deficient): 2042 violations before, 0 after.One note from auditing the other tolerance constants in
src/: theeps * tst1/eps * normtests insvd.jsandevd.jsare already relative and correct, butnipals.jshas the same shape of problem —terminationCriteria = 1e-10is compared againstsum((t - tOld)^2), which scales with the data. On the iris fixture the tests use, scalingxby 1e-6 moves the first loading vector from[0.5211, -0.2693, 0.5804, 0.5649]to[0.5490, -0.2041, 0.5802, 0.5660]. I left it out of this PR because making it relative also means re-picking the default constant; happy to send it separately if you want it.