restrict inv(::Factorization) to square matrices

[stevengj]

This adds a missing checksquare to inv(::SVD), which I would argue is a bug from JuliaLang/julia#32126. If you want the pseudo-inverse, you should call pinv(::SVD) explicitly (via the method introduced in #1387) , not rely on magic undocumented behavior of inv.

Similarly for inv(::Factorization) (see below), which previously computed a left inverse for non-square ("tall") QR factorizations (see below).

(Hoisted from #1387.)

[stevengj]

We could add a deprecation warning instead for the non-square case, but as far as I can tell it was undocumented.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.84%. Comparing base (db95a2a) to head (2640451).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1397   +/-   ##
=======================================
  Coverage   93.83%   93.84%           
=======================================
  Files          34       34           
  Lines       15823    15826    +3     
=======================================
+ Hits        14848    14852    +4     
+ Misses        975      974    -1     

☔ View full report in Codecov by Sentry.
📢 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.

[dkarrasch]

I checked all existing inv(<:Union{AbstractMatrix,Factorization}) methods, and all of them check (sooner or later; in the LU case, it is implicit in the *Triangular constructors) for squareness. So, I agree, this is to be considered a bug, which we should backport to the LTS?

[stevengj]

inv(qr(rand(5,3))) currently succeeds (it produces a left inverse), whereas inv(qr(rand(3,5))) gives an error.

The docstring for inv(M) says

Matrix inverse. Computes matrix N such that M * N = I

which (if anything) implies a right inverse for the non-square case. So it all seems rather inconsistent to me.

It really seems like non-square factorization inverses should require pinv. (Not that we should be encouraging people to compute explicit inverses in the first place.)

[stevengj]

I don't think it's a good idea to backport. On the latest version, if you actually want the old behavior, you could just switch to calling pinv, but that isn't an option on previous versions.

[stevengj]

cc @carstenbauer and @andreasnoack from JuliaLang/julia#32126.

[dkarrasch]

inv(qr(rand(5,3))) currently succeeds (it produces a left inverse), whereas inv(qr(rand(3,5))) gives an error.

Ha, so I missed this...

[stevengj]

There is a general fallback method inv(F::Factorization) = F \ I that is responsible for producing a left inverse for non-square factorizations that support it (currently QR).

This was added by @andreasnoack way back in JuliaLang/julia#5381, so maybe he can comment on this.

The behavior of inv doesn't seem clearly documented, at the very least (qr documents that QR objects support inv, but doesn't say what inv does in the non-square case, and the inv docs only talk about matrices, and only mention right inverses).

[andreasnoack]

It has been a long time but after reading the PR text again, my best guess is that it wasn't intentional to allow non-square matrices in the fallback inv. The PR is about LU and while you can LU a non-square matrix, the use LU in this module has mostly been for square matrices.

[stevengj]

Should we add a checksquare to the fallback inv as well?

On the one hand, non-square inv for QR has been around a long time. On the other hand, it's apparently undocumented, and it doesn't make a huge amount of sense.

[dkarrasch]

I think this was just oversight, or unintended generality.

[stevengj]

Okay, I've updated the PR to add a checksquare to inv(::Factorization) as well.

We'll see if any test failures result. Update: no failures, so apparently the non-square inv(::QR) case was untested.

[stevengj]

Okay to squash and merge?