Add a layer of indirection in orthnull functions to minimize potential ambiguities

[mtfishman]

Followup to #79.

In some downstream packages, I want to define catch-all definitions like:

left_orth!(A::MyAbstractMatrix, VC, alg::LeftOrthAlgorithm) = # code that works for any LeftOrthAlgorithm

which is helpful if A is some kind of wrapper where I just want to forward any LeftOrthAlgorithm backend to the wrapper and rewrap the result. However, the current code is written like this:

left_orth!(A, VC, alg::AbstractAlgorithm) = left_orth!(A, VC, left_orth_alg(alg))
left_orth!(A, VC, alg::LeftOrthViaQR) = qr_compact!(A, VC, alg.alg)
left_orth!(A, VC, alg::LeftOrthViaPolar) = left_polar!(A, VC, alg.alg)

which means that to avoid method ambiguities, in practice I have to define:

left_orth!(A::MyAbstractMatrix, VC, alg::LeftOrthViaQR) = left_orth_mymatrix!(A, VC, alg)
left_orth!(A::MyAbstractMatrix, VC, alg::LeftOrthViaPolar) = left_orth_mymatrix!(A, VC, alg)
left_orth_mymatrix!(A, VC, alg::LeftOrthAlgorithm) = # code that works for any LeftOrthAlgorithm

i.e. I have to overload left_orth! (and related functions) for every backend LeftOrthViaQR, LeftOrthViaPolar, etc.

This PR is designed to address that with a layer of indirection:

left_orth!(A, VC, alg::AbstractAlgorithm) = left_orth!(A, VC, left_orth_alg(alg))
left_orth!(A, VC, alg::LeftOrthAlgorithm) = left_orth_via!(A, VC, alg)
left_orth_via!(A, VC, alg::LeftOrthViaQR) = qr_compact!(A, VC, alg.alg)
left_orth_via!(A, VC, alg::LeftOrthViaPolar) = left_polar!(A, VC, alg.alg)

with analogous changes to check_input and initialize_output.

[codecov]

Codecov Report

❌ Patch coverage is 79.54545% with 9 lines in your changes missing coverage. Please review.

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

Files with missing lines	Coverage Δ
src/implementations/orthnull.jl	75.00% <79.54%> (+4.68%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

[lkdvos]

Is it even necessary to define these overloads for the wrappers? I would have guessed that the general logic correctly dispatches to the underlying factorization, at which point we can define qr_compact etc?

Ie, could it be that deleting the overloads actually solves the problem, or am I missing something?

[mtfishman]

Is it even necessary to define these overloads for the wrappers? I would have guessed that the general logic correctly dispatches to the underlying factorization, at which point we can define qr_compact etc?

Ie, could it be that deleting the overloads actually solves the problem, or am I missing something?

Good question. I think that would be ideal, the primary use case I can think of where that didn't work was for immutable types, since LeftOrthViaSVD assumes mutability (https://github.com/QuantumKitHub/MatrixAlgebraKit.jl/blob/v0.6.0/src/implementations/orthnull.jl#L74). So then to handle those sorts of cases I found it was easier in practice to just directly overload and forward all left_orth! calls. The new orthnull design from #79 should make it easier to just overload LeftOrthViaSVD, I haven't tried that strategy yet, but still I think it would be nice to make it easier to forward all left_orth! calls so downstream packages that need some custom orth implementations don't have to be too sensitive to the implementation details of the different LeftOrthAlgorithm backends. There may have also been some subtlety related to initialize_output for certain types where that wasn't easy to define (say in the case of block sparse where you need to work out the shapes blockwise), though maybe that is now easier to deal with given the new orthnull design from #79, I didn't investigate that very much.

[mtfishman]

Maybe an alternative for these kinds of cases would be to directly overload select_algorithm for orth and null functions for custom matrix types, so then I can make overloads like left_orth!(A, VC, alg::MyMatrixAlgorithm) instead and avoid the method ambiguities that I'm hitting that way. I didn't consider that at first because I was perturbing away from code designed before #79 and forgot that #79 would now allow that kind of approach.

[mtfishman]

Maybe an alternative for these kinds of cases would be to directly overload select_algorithm for orth and null functions for custom matrix types, so then I can make overloads like left_orth!(A, VC, alg::MyMatrixAlgorithm) instead and avoid the method ambiguities that I'm hitting that way. I didn't consider that at first because I was perturbing away from code designed before #79 and forgot that #79 would now allow that kind of approach.

I guess the subtle part with that approach is that for inputs like left_orth!(A::MyMatrix; alg = :svd), if we want that to ultimately call left_orth!(A, VC, alg::MyMatrixAlgorithm), we would need to hijack parts of this code that processes symbol inputs into algorithms: https://github.com/QuantumKitHub/MatrixAlgebraKit.jl/blob/v0.6.0/src/interface/orthnull.jl#L328-L353. Not impossible but maybe not the nicest approach.

[mtfishman]

I guess the subtle part with that approach is that for inputs like left_orth!(A::MyMatrix; alg = :svd), if we want that to ultimately call left_orth!(A, VC, alg::MyMatrixAlgorithm), we would need to hijack parts of this code that processes symbol inputs into algorithms: https://github.com/QuantumKitHub/MatrixAlgebraKit.jl/blob/v0.6.0/src/interface/orthnull.jl#L328-L353. Not impossible but maybe not the nicest approach.

Sorry for all the noise. I tried that strategy and it seems to be a good alternative. In practice what I did was analogous to the following:

function default_algorithm(::typeof(left_orth!), A::MyMatrix)
    return MyMatrixAlgorithm(default_algorithm(left_orth!, parent(A)))
end
function select_algorithm(::typeof(left_orth!), A::MyMatrix, alg::Symbol)
    return MyMatrixAlgorithm(select_algorithm(left_orth!, parent(A), alg))
end
function left_orth!(A, VC, alg::MyMatrixAlgorithm)
    return MyMatrix.(left_orth!(parent(A), parent.(VC), parent(alg)))
end

which seems to work and avoid the original method ambiguities this PR was designed to alleviate.

[mtfishman]

I'll close this since I think there are better alternatives as discussed above.