svd_vals(::DiagonalTensorMap) should return a SectorVector

[lkdvos]

Noticed some test failures in #325 due to the diagonal algorithms not returning a SectorVector, so I fixed that, added some tests and then found some other inconsistencies: svd_vals was sorting the entire SectorVector, effectively mixing up the blocks.

Both of these are fixed here.

[codecov]

Codecov Report

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

Files with missing lines	Patch %	Lines
src/factorizations/diagonal.jl	83.33%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/factorizations/factorizations.jl	94.11% <ø> (+5.22%)	⬆️
src/factorizations/diagonal.jl	76.92% <83.33%> (+12.82%)	⬆️

... and 2 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]

I also noticed a small mistake in the PR that added LinearAlgebra.svd support. For a matrix, svd returns U, S as a vector, V as the adjoint of a matrix (Vᴴ). So I guess we want to replicate this, and thus have something like

U, S, Vᴴ = svd_compact(t) # or svd_full
return U, diagview(S), adjoint(Vᴴ)

Maybe you can also include this in the current PR?

[kshyatt]

Added a small fix to address @Jutho's comment (hopefully)

[lkdvos]

I'm slightly worried that the diagview will fail for the full case, since we don't return a diagonal in that case. LinearAlgebra actually outputs things that don't multiply correctly there, and I'm not sure how we want to handle this?

[lkdvos]

I guess this is why we didn't overload the linearalgebra methods 😂

[kshyatt]

I'm slightly worried that the diagview will fail for the full case, since we don't return a diagonal in that case. LinearAlgebra actually outputs things that don't multiply correctly there, and I'm not sure how we want to handle this?

Should I just add a test for full and see what happens?

[kshyatt]

Alternatively: is anyone actually using these overloads? Can we just remove them, for now?

[lkdvos]

I'm happy to remove them again, definitely is independent of this PR

[kshyatt]

Since they don't seem to be tested anyway I say kill em all!!!

[lkdvos]

I think there's still an entry in the changelog that can be deleted again, afterwards I would just merge this

[kshyatt]

Nightly failures are the same Zygote stuff as usual. Is this good to go?