fix(linalg): reject non-square input in svd_invert instead of returning garbage (refs #102) - #106
Merged
Merged
Conversation
…ng garbage (refs #102) `svd_invert` inherited a defect from jsfeat: for a non-square matrix it silently returns a WRONG pseudo-inverse — only the first column is correct — with no error of any kind. Its TSDoc even claimed "Valid for any matrix shape/rank", promising exactly what it fails to deliver. Option (c) of #102: the method now throws on non-square input. Rationale, in short — upstream jsfeat is frozen, so the bug cannot be fixed there; handing a caller wrong numbers quietly is worse than failing; and "we raise an error where jsfeat returned garbage" is a far easier divergence to justify than "we return different numbers than jsfeat". Chose throwing over a numeric failure code deliberately: a wrong input SHAPE is a programmer error, whereas `lu_solve` returning 0 signals a singular matrix, which is a legitimate numerical outcome. Different situations, so a different mechanism. Throwing also avoids changing the public signature from `void` to `number`, which would itself be a typed-API break. Scope of the divergence is narrow: square inversion is untouched and remains bit-compatible with jsfeat, so tests/parity/linalg.test.ts (which uses a 4x4) is unaffected. Nothing inside the library calls svd_invert at all — it is purely public API — so there is no internal regression risk. Adds tests/divergences.test.ts, a deliberate counterpart to tests/parity/: a registry of places where jsfeatNext intentionally does NOT match jsfeat, each with its reason and tracking issue. Keeping them out of the parity suite matters — a divergence buried there reads as a broken test, whereas here it reads as a decision. The three cases added assert that we throw, that jsfeat does not throw and really is wrong for the same input, and that square input still matches jsfeat exactly and satisfies A * A^-1 = I. Also corrects the misleading TSDoc, and updates examples/linalg_example.html, whose comment said the non-square result was "not trustworthy" — it now throws, so the wording had to change. This does NOT close #102: the correct rectangular pseudo-inverse (option b) is still open, and is a good candidate for the correctness work in #87. Verified: tsc --noEmit clean; npm test 66/66 (63 existing + 3 new), parity suite unchanged; square inversion still returns the exact inverse (0.6, -0.7, -0.2, 0.4 for [[4,7],[2,6]]) while non-square now throws; all five API-demo examples still run clean; prettier clean.
kalwalt
added
bug
This was referenced Jul 27, 2026
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
1 participant
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.
Implements option (c) of #102. Does not close it — the correct rectangular pseudo-inverse (option b) stays open.
The problem
svd_invertinherited a defect from jsfeat: for a non-square matrix it silently returns a wrong pseudo-inverse (only the first column is correct), with no error at all. Its TSDoc even claimed "Valid for any matrix shape/rank" — promising precisely what it fails to deliver.The decision
It now throws on non-square input. The reasoning:
Why throw rather than a
0failure code: a wrong input shape is a programmer error, whereaslu_solvereturning0signals a singular matrix — a legitimate numerical outcome. Different situations, different mechanisms. Throwing also avoids changing the signaturevoid → number, which would itself be a typed-API break.Scope is narrow
tests/parity/linalg.test.ts(a 4×4) is unaffected.svd_invert— it's purely public API — so there's no internal regression risk.New:
tests/divergences.test.tsA deliberate counterpart to
tests/parity/— a registry of places where jsfeatNext intentionally does not match jsfeat, each with its reason and tracking issue.Keeping these separate matters: a divergence buried in the parity suite looks like a broken test; here it reads as a decision. The three cases assert that (1) we throw, (2) jsfeat does not throw and really is wrong for the same input, and (3) square input still matches jsfeat exactly and satisfies
A · A⁻¹ = I.Also
examples/linalg_example.html, whose comment said the non-square result was "not trustworthy" — it now throws, so the wording had to change.Verification
tsc --noEmitclean ·npm test66/66 (63 existing + 3 new), parity suite unchanged · square inversion still exact (0.6, -0.7, -0.2, 0.4for[[4,7],[2,6]]) while non-square throws · all five API-demo examples still run clean · prettier clean.🤖 Generated with Claude Code