Make orthnull more customizable and general

[mtfishman]

As suggested by @lkdvos. Requires #23.

[codecov]

Codecov Report

Attention: Patch coverage is 89.28571% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/implementations/orthnull.jl	88.46%	12 Missing ⚠️

Files with missing lines	Coverage Δ
src/interface/orthnull.jl	66.66% <100.00%> (ø)
src/implementations/orthnull.jl	91.28% <88.46%> (-5.50%)	⬇️

🚀 New features to boost your workflow:

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

[mtfishman]

@lkdvos @Jutho this is ready for review.

The motivation here is both improved code organization and generality (it removes some AbstractMatrix type constraints) and to provide lower level functions like left_orth_qr! that are an extra layer of customizability, for example they can be overloaded for GPU arrays, block sparse arrays, etc. where the current design may not be general enough.

The main considerations I can think of are:

1. Making those functions public. I'm leaning towards holding off on that to see how they get used in practice.
2. Should the select_algorithm logic be inside the lower level functions left_orth_qr!, or outside? If they are inside (the current design), it provides a bit more customizability, while if they are outside it means the implementations of functions like left_orth_qr! can be simpler. Again, I'm leaning towards keeping the implementation as-is for now to see how this is used in practice.

[Jutho]

I am open to omitting the AbstractMatrix restriction above, but then his part of the implementation (namely the use of Diagonal) is again very much (Abstract)Matrix specific, whereas left_orth_qr! and left_orth_polar! are quite generic and probably work for other types as well.

[Jutho]

Maybe a more general implementation can be added that is less economic with its memory usage:

function left_orth_svd!(A, VC, alg, trunc::Nothing=nothing)
    alg′ = select_algorithm(svd_compact!, A, alg)
    U, S, Vᴴ = svd_compact!(A, alg′)
    V, C = VC
    return copy!(V, U), mul!(C, S, Vᴴ)
end

[mtfishman]

Good point, I wasn't being careful about the type restriction in this case. I think that's a good suggestion to have a more generic version.

Another thing I can think of is replacing Diagonal with diagonal, and then diagonal could be an interface function that types can overload to construct a diagonal matrix-like object. But I like your suggestion better.

[Jutho]

Well, internally there is already the diagview to have a uniform interface to extracting a view of the diagonal of both Matrix and Diagonal, but that could also be specialized for other types. So a type-agnostic construction of a diagonal matrix could also be useful. With diagonal as name, it might get confusing though, as LinearAlgebra.diag is exactly the opposite, namely extracting the diagonal of a matrix and returning it as a vector. But I wouldn't have another naming suggestion.

I am also not sure if we generally want that the output type of svd_vals can be used to construct the S output of svd_compact. In TensorKit tensors, for example, a DiagonalTensorMap would still store all the singular values in a single list (Vector), but with internal structure such that it is known which parts of this vector are associated with which sectors/quantum numbers. svd_vals would than rather return that information as a Dict where for every sector (being the keys into the dict) there is a separate vector with only the singular values for that sector.

[mtfishman]

Yeah, it definitely would require some work to make the design based on diagview/diagonal work for exotic types like that.

I've updated the code to add a non-AbstractMatrix codepath based on your suggestion above. I also added a test that defines a simple non-AbstractMatrix to test that codepath.

[Jutho]

I definitely approve of splitting the different branches into separate functions that can be further specialized upon, and in omitting the AbstractMatrix restriction where possible. But as mentioned in the comments, it would still be good to add this restriction in the methods that use specific matrix constructions such as Diagonal.

[Jutho]

I think this is ready? I have no further comments.