Skip to content

fix: reject matrices with fewer rows than columns in the LU and QR decompositions - #218

Closed
tayal-sarthak wants to merge 6 commits into
mljs:mainfrom
tayal-sarthak:decompositions
Closed

fix: reject matrices with fewer rows than columns in the LU and QR decompositions#218
tayal-sarthak wants to merge 6 commits into
mljs:mainfrom
tayal-sarthak:decompositions

Conversation

@tayal-sarthak

Copy link
Copy Markdown

Closes #203.

Summary

The issue asks whether the SVD is broken for matrices carrying more columns than rows. It is not, the decomposition is an economy one and it reconstructs its input exactly. What the report gets right is that this is undocumented, and looking into it turned up two neighbouring decompositions that genuinely fall over on the same input shape.

The SVD is correct

For the 2x3 matrix [[1, 2, 3], [4, 5, 6]], both with autoTranspose on and off, U * diag(s) * Vᵀ returns the input to machine precision. The economy form reports min(m, n) singular values, so the vectors it hands back do not span the null space of a wide matrix. That is a property of the economy form, not a defect. This PR documents the shape contract along with the effect of autoTranspose in the typings and the README.

LuDecomposition and QrDecomposition genuinely break

Both are ports of routines defined for a matrix with at least as many rows as columns. Neither checked. Feeding them a 2x3 matrix on main:

call result
new LuDecomposition(wide).isSingular() TypeError: Cannot read properties of undefined (reading '2')
new LuDecomposition(wide).solve(b) the same TypeError
new QrDecomposition(wide).orthogonalMatrix TypeError: Cannot set properties of undefined (setting '2')
new QrDecomposition(wide).householderVectors TypeError: Cannot read properties of undefined

The accessors that avoid the crash are worse, since they hand back a triangular factor of the wrong shape with no indication anything went wrong.

Both constructors now reject that input:

RangeError: Matrix must have at least as many rows as columns

solve routes a non square left hand side to QrDecomposition, so solve(wideMatrix, b) reports the shape now rather than the Matrix is rank deficient it produced before. Callers with a wide system want pseudoInverse, along with solve(a, b, true) to go through the SVD.

Nothing that worked before stops working. Every one of these calls already failed, either loudly with an internal error or quietly with a meaningless factor.

Notes

The Matrix must be square error on determinant is untouched. A 3x2 matrix still reaches it, a 2x3 matrix is now stopped one step earlier by the constructor.

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.88%. Comparing base (1a4a05a) to head (d25e590).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
+ Coverage   68.88%   69.88%   +0.99%     
==========================================
  Files          49       49              
  Lines        5904     5910       +6     
  Branches     1060     1084      +24     
==========================================
+ Hits         4067     4130      +63     
+ Misses       1824     1767      -57     
  Partials       13       13              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@targos targos closed this Aug 6, 2026
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.

is SVD broken for wide matrices? (undocumented economy implementation)

2 participants