SafeSVD alternative

[Jutho]

No description provided.

[github-actions]

Your PR no longer requires formatting changes. Thank you for your contribution!

[codecov]

Codecov Report

❌ Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/yalapack.jl	97.91%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/MatrixAlgebraKit.jl	100.00% <ø> (ø)
src/implementations/svd.jl	90.99% <100.00%> (+0.25%)	⬆️
src/interface/decompositions.jl	64.00% <ø> (ø)
src/interface/svd.jl	68.42% <100.00%> (ø)
src/yalapack.jl	60.94% <97.91%> (+1.01%)	⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jutho]

Ok, errors here seem to result from a formatting issue (which I will fix) and the type stability issues, so I think this is ready for a review. Also pinging @VictorVanthilt , as this is an alternative to #145 .

[lkdvos]

Overall looks good to me, I also definitely like having this as the default.

One question here is that I think we can avoid a double-allocation in the case of svd_compact(A), which now calls:

Ac = copy_input(A)
sdd!(copy(Ac))
svd!(Ac)

By replacing this with:

Ac = copy_input(A)
sdd!(Ac)
copy!(Ac, A)
svd!(Ac)

I think that does require some slight reorganizing of the code logic, specialized to this algorithm, but given that this is the default it probably is not unreasonable to invest a bit of time in this

[Jutho]

That is indeed a very good remark; I'll try to come up with something. Of course this problem permeates through to polar and svd_trunc etc, which also do copy_input first and then call svd_compact!. So this will be annoying.

[Jutho]

I thought some more about reusing or otherwise avoiding the double copying going on in calling the non-mutating methods svd_compact and friends, but I don't see a great solution that doesn't break most of our lowering logic. The problem is also that svd_compact(A) must be able to work with an A that might need to be type promoted (e.g. Matrix{Int}). So the best I can come up with is something where Ac = copy_input(svd_compact, A) is still called and passed to svd_compact!(Ac, ...), but where the original A is also kept and passed on to the lower functions, so that we can still do copy!(Ac, A) in the gesvd branch of the gesdvd! routine.

[lkdvos]

I'm also happy to just leave this as is for now and leave that for later

[Jutho]

Well, I do feel uncomfortable about it myself. It does seem like a major annoyance that there would be an unnecessary copy in what will probably be the most called method for computing the SVD decomposition.