restrict type exponential

[sanderdemeyer]

I came across this small inconsistency when testing exponential for TensorKit. If we don't restrict this for eigh (like we already did for eig), there is some ambiguity later on.

[lkdvos]

What kind of ambiguity are you running into? It seems like I would have expected the opposite way to be true, where we unrestrict for eigh so we don't even have to implement these functions. I do recall this being a bit finicky, so if this is an easy fix I'm okay with that too

[sanderdemeyer]

This function in TensorKit (in src/factorizations/matrixalgebrakit)

for f! in (
        :qr_null!, :lq_null!,
        :svd_vals!, :eig_vals!, :eigh_vals!,
        :project_hermitian!, :project_antihermitian!, :project_isometric!,
        :exponential!
    )
    @eval function MAK.$f!(t::AbstractTensorMap, N, alg::AbstractAlgorithm)
        $(f! in (:eig_vals!, :eigh_vals!, :project_hermitian!, :project_antihermitian!) && :(LinearAlgebra.checksquare(t)))
        foreachblock(t, N) do _, (tblock, Nblock)
            Nblock′ = $f!(tblock, Nblock, alg)
            # deal with the case where the output is not the same as the input
            Nblock === Nblock′ || copy!(Nblock, Nblock′)
            return nothing
        end
        return N
    end
end

is ambiguous with this function in MatrixAlgebraKit (in src/implementations/exponential)

exponential!(A, expA, alg::MatrixFunctionViaEigh) = exponential!((one(eltype(A)), A), expA, alg)
exponential!(A::AbstractMatrix, expA::AbstractMatrix, alg::MatrixFunctionViaEig) = exponential!((one(eltype(A)), A), expA, alg)

unless we restrict the types for eigh. I get this ambiguity when testing @constinferred exponential(A, alg) in TensorKit when alg is a MatrixFunctionViaEigh, but not when it is a MatrixFunctionViaEig.

[lkdvos]

Hmm, the question is of course if we should remove add the restriction here, or just disambiguate in TensorKit itself. Unfortunately we have not been super consistent about this throughout this library so I'm definitely happy with this solution if you prefer. I'm not sure if @Jutho has opinions?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/implementations/exponential.jl	100.00% <100.00%> (ø)

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

Would the TensorKit methods be more specific if we define them as

    @eval function MAK.$f!(t::AbstractTensorMap, N, alg::Alg) for Alg <: AbstractAlgorithm
        ...
    end

?

[sanderdemeyer]

Then the ambiguity remains. The only solution I can think of in TensorKit is to relax the type constraint of ``t::AbstractTensorMap(and the same for(τ,t)::Tuple{E,T}`), but I think this is not the way to go.

[Jutho]

Yes, these kind of ambiguities in functions that take an algorithm and some data input, and then you sometimes want to specialize on the algorithm, and sometimes on the data input type but not the algorithm, is quite annoying. For TensorOperations.jl we also developed some logic to circumvent those. But I would already need to check again how this was done and how much extra boilerplate code it requires, to know whether it is feasible and useful to adopt the same strategy in MatrixAlgebraKit. Maybe @lkdvos remembers by heart?

[lkdvos]

I don't think we really fixed this in TensorOperations though. The main strategy for things like this is to do dispatch in two waves, say first based on the algorithm, and then with a new function only based on the input type, but I don't think we very strictly adhered to that in this repo.
The original idea was to dispatch on Algorithm type only, but then over time I think I(we) kind of forgot about this and then TensorKit absolutely violated that for convenience purposes, so to really fix this I think requires some substantial organization that might not be worth it in the immediate future.
TLDR, I'm okay with just merging this and going from there for now.