Exponential#94
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
|
||
| function exponential!(A::AbstractMatrix, expA::AbstractMatrix, alg::ExponentialViaEigh) | ||
| D, V = eigh_full(A, alg.eigh_alg) | ||
| copyto!(expA, V * Diagonal(exp.(diagview(D))) * inv(V)) |
There was a problem hiding this comment.
Reduced allocation strategy:
| copyto!(expA, V * Diagonal(exp.(diagview(D))) * inv(V)) | |
| iV = inv(V) | |
| map!(exp, diagview(D)) | |
| mul!(expA, rmul!(V, D), iV) |
There was a problem hiding this comment.
It has to be map!(exp, diagview(D), diagview(D)) instead of map!(exp, diagview(D)), but good suggestion otherwise. I have also added it for the ExponentialViaEig.
EDIT: the suggested change works only for Julia 1.12 onwards. That's why I will keep the version with
3 arguments.
There was a problem hiding this comment.
Why not just diagview(D) .= exp.(diagview(D))?
There was a problem hiding this comment.
Is that more efficient than the current code? If not, I'd prefer to keep it that way, since it feels a bit more natural to me.
lkdvos
left a comment
There was a problem hiding this comment.
Overall I'm not fully convinced by the interface of exponential(!), especially in its current form and implementation this looks slightly strange.
LinearAlgebra uses an in-place version, i.e. it reuses the input array to return exp!, and looking at the different implementations you have here, it is not obvious that trying to fit this into a exponentiate!(A, expA, alg) signature is really helping us - on the contrary, all this is really doing is creating an additional copy at the end just to make sure that it is allocated in the provided output.
As we discussed for your previous PR, this really is not the purpose of being able to provide the output argument.
For the algorithms, thinking a bit ahead, it might be appropriate to just call these something along the lines of matrix functions via eig, since presumably these approaches are actually generic for all of these implementations.
|
|
||
| function exponential!(A::AbstractMatrix, expA::AbstractMatrix, alg::ExponentialViaEigh) | ||
| D, V = eigh_full(A, alg.eigh_alg) | ||
| copyto!(expA, V * Diagonal(exp.(diagview(D))) * inv(V)) |
There was a problem hiding this comment.
Why not just diagview(D) .= exp.(diagview(D))?
The idea of putting it in this framework is to allow for different sorts of algorithms, i.e. ones that would also work for BigFloats. I get that in the BLASFloat case, we should avoid extra allocations, but could you elaborate on your suggestion for
I may be missing your point here, but that was the idea behind the |
I am indeed referring to the preallocated output, not the algorithms part. It really only makes sense to have the option of giving a preallocated output if we are actually able to use this, and for the current implementations you have this is not saving us any work, rather it is increasing it because you add an extra allocation at the beginning and an extra copy at the end. While it is definitely possible to have
Sorry I should have explained that better, I meant that I would like to avoid having to also define |
Is your suggestion then to just skip the whole
Okay, I see. I agree and will change this. |
|
Regarding the algorithm names, I agree with Lukas and also think we want to have a general Regarding the role of the output arguments, I only partially agree. The whole point of why we started MatrixAlgebraKit.jl, is because in TensorKit we first want to define the output tensor, and then compute block per block the result, where we want to store the result in the corresponding block of the output tensor. Ideally, yes, the computation is such that we also use that output data as storage during the computation, in such a way that the end result "naturally" ends up there, but if that is difficult, a final Note that the LinearAlgebra |
|
Regardless of the comment about general matrix functions, it is a fact that the exponential is by far the most useful and common one that we need, so I am also not opposed to first thinking carefully about this one, and having some part of the implementation be specific for matrix exponentials. In particular, one important consideration that we might want to include in this design, that is specific to our use case, is that we also might be interested in computing |
|
To comment on the TensorKit interaction, I definitely agree with the purpose, but this is not actually currently the design we ended up with. So basically there are two comments I have: On the one hand, there is the question about whether or not there are implementations that benefit from providing an additional output array. On the other hand, given that interface, if there is no way of naturally making the output end up in the provided destination, I would really like to avoid ending up with a final |
|
I guess I am a bit confused, because most of the implementations now do actually perform the final step in the calculation in such a way that the result is directly stored in the output array, no? It is only the algorithm that goes via Base/LinearAlgebra that requires the extra But it is also true that, by the time the final step of the calculation is reached; the memory of |
change name to `MatrixFunctionViaEig` etc change `decompositions` to `matrixfunctions` add default algorithm for Diagonal matrices add input checks add @testthrows to catch non-hermitian matrices being given to MatrixFunctionViaEigh change default exponential algorithm to e.g. `MatrixFunctionViaEig` of the default `eig_alg`
lkdvos
left a comment
There was a problem hiding this comment.
I think I left a final set of comments, otherwise this is good to go for me. You'll see it's quite nitpicky, so feel free to share your opinions as well!
| return nothing | ||
| end | ||
|
|
||
| function check_input(::typeof(exponential!), τ::Number, A::AbstractMatrix, expA::AbstractMatrix, alg::AbstractAlgorithm) |
There was a problem hiding this comment.
Another question that is kind of irrelevant, but just want to check if you have a specific reasoning for choosing check_input(exponential!, (τ, A), expA, alg) vs check_input(exponential!, τ, A, expA, alg). I guess there is some annoyance with the tuple and dispatch, but this is somewhat less in line with the other single-argument functions where f!(in, out, alg) calls check_input(f!, in, out, alg), instead of here check_input(f!, in..., out, alg). I assume this is a remnant of using the 2-arg @functiondef from before?
There was a problem hiding this comment.
I agree that check_input(exponential!, (τ, A), expA, alg) is the more logical choice. I've changed that now.
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Thanks for the final comments. I agreed with all of them! |
lkdvos
left a comment
There was a problem hiding this comment.
Thanks for all the work @sanderdemeyer, this looks like a great addition!
@Jutho, @kshyatt I'll leave this open in case you still want to have a look, and at the latest I'll merge this Monday evening to keep things moving, while not imposing on weekends 😉
| function check_input(::typeof(exponential!), A::AbstractMatrix, expA::AbstractMatrix, alg::MatrixFunctionViaEigh) | ||
| m = LinearAlgebra.checksquare(A) | ||
| @check_size(expA, (m, m)) | ||
| @check_scalar(expA, A) | ||
| return nothing | ||
| end |
There was a problem hiding this comment.
Isn't this one covered by the previous definition with alg::AbstractAlgorithm?
| expD = map_diagonal!(x -> exp(x / 2), D, D) | ||
| VexpD = rmul!(V, expD) | ||
| return mul!(expA, VexpD, V') |
There was a problem hiding this comment.
This doesn't seem right. I think you do x -> exp(x / 2) with the intention to then do mul(expA, VexpD, VexpD') ?
There was a problem hiding this comment.
We should probably rename the variables slightly, but V and expD share memory so the two are in fact the same
| expD = map_diagonal(x -> exp(x * τ), D) | ||
| VexpD = V * expD | ||
| if eltype(A) <: Real && eltype(τ) <: Real | ||
| return expA .= real.(VexpD * V') | ||
| else | ||
| return mul!(expA, VexpD, V') | ||
| end |
There was a problem hiding this comment.
if eltype(A) <: Real && eltype(τ) <: Real, everything should remain real, so no real call is necessary.
| expD = map_diagonal(x -> exp(x * τ), D) | |
| VexpD = V * expD | |
| if eltype(A) <: Real && eltype(τ) <: Real | |
| return expA .= real.(VexpD * V') | |
| else | |
| return mul!(expA, VexpD, V') | |
| end | |
| if eltype(A) <: Real && eltype(τ) <: Real | |
| V .*= exp.(transpose(diagview(D)) .* τ ./ 2) | |
| return mul!(expA, V, V') | |
| else | |
| VexpD = V .* exp.(transpose(diagview(D)) .* τ) | |
| return mul!(expA, VexpD, V') | |
| end |
| function exponential!((τ, A)::Tuple{Number, AbstractMatrix}, expA::AbstractMatrix, alg::MatrixFunctionViaLA) | ||
| check_input(exponential!, (τ, A), expA, alg) | ||
| expA .= A .* τ | ||
| return LinearAlgebra.exp!(expA) | ||
| end |
There was a problem hiding this comment.
To avoid code duplication:
| function exponential!((τ, A)::Tuple{Number, AbstractMatrix}, expA::AbstractMatrix, alg::MatrixFunctionViaLA) | |
| check_input(exponential!, (τ, A), expA, alg) | |
| expA .= A .* τ | |
| return LinearAlgebra.exp!(expA) | |
| end | |
| exponential!((τ, A)::Tuple{Number, AbstractMatrix}, expA::AbstractMatrix, alg::MatrixFunctionViaLA) = exponential!(A * τ, expA, alg) |
There was a problem hiding this comment.
| function exponential!((τ, A)::Tuple{Number, AbstractMatrix}, expA::AbstractMatrix, alg::MatrixFunctionViaLA) | |
| check_input(exponential!, (τ, A), expA, alg) | |
| expA .= A .* τ | |
| return LinearAlgebra.exp!(expA) | |
| end | |
| function exponential!((τ, A)::Tuple{Number, AbstractMatrix}, expA::AbstractMatrix, alg::AbstractAlgorithm) | |
| expA .= A .* τ | |
| return exponential!(expA, expA, alg) | |
| end |
I think this way we avoid one more allocation, and I immediately relaxed the alg::AbstractAlgortihm signature since I guess this is a good default implementation? (Note that while we could modify A itself, this might fail because of eltype issues, so it has to be expA.
| function exponential!(A::AbstractMatrix, expA::AbstractMatrix, alg::MatrixFunctionViaEig) | ||
| check_input(exponential!, A, expA, alg) | ||
| D, V = eig_full!(A, alg.eig_alg) | ||
| expD = map_diagonal!(exp, D, D) | ||
| iV = inv(V) | ||
| VexpD = rmul!(V, expD) | ||
| if eltype(A) <: Real | ||
| expA .= real.(VexpD * iV) | ||
| else | ||
| mul!(expA, VexpD, iV) | ||
| end | ||
| return expA | ||
| end |
There was a problem hiding this comment.
| function exponential!(A::AbstractMatrix, expA::AbstractMatrix, alg::MatrixFunctionViaEig) | |
| check_input(exponential!, A, expA, alg) | |
| D, V = eig_full!(A, alg.eig_alg) | |
| expD = map_diagonal!(exp, D, D) | |
| iV = inv(V) | |
| VexpD = rmul!(V, expD) | |
| if eltype(A) <: Real | |
| expA .= real.(VexpD * iV) | |
| else | |
| mul!(expA, VexpD, iV) | |
| end | |
| return expA | |
| end | |
| exponential!(A::AbstractMatrix, expA::AbstractMatrix, alg::MatrixFunctionViaEig) = exponential!((1, A), expA, alg) |
There was a problem hiding this comment.
Should we do 1, one(eltype(A)), or true?
| function exponential!(A, expA, alg::MatrixFunctionViaEigh) | ||
| check_input(exponential!, A, expA, alg) | ||
| D, V = eigh_full!(A, alg.eigh_alg) | ||
| expD = map_diagonal!(x -> exp(x / 2), D, D) | ||
| VexpD = rmul!(V, expD) | ||
| return mul!(expA, VexpD, V') | ||
| end |
There was a problem hiding this comment.
| function exponential!(A, expA, alg::MatrixFunctionViaEigh) | |
| check_input(exponential!, A, expA, alg) | |
| D, V = eigh_full!(A, alg.eigh_alg) | |
| expD = map_diagonal!(x -> exp(x / 2), D, D) | |
| VexpD = rmul!(V, expD) | |
| return mul!(expA, VexpD, V') | |
| end | |
| exponential!(A, expA, alg::MatrixFunctionViaEigh) = exponential!((1, A), expA, alg) |
| expD = map_diagonal!(x -> exp(x * τ), D, D) | ||
| iV = inv(V) | ||
| VexpD = rmul!(V, expD) | ||
| if eltype(A) <: Real && eltype(τ) <: Real | ||
| expA .= real.(VexpD * iV) | ||
| return expA | ||
| else | ||
| return mul!(expA, VexpD, iV) | ||
| end |
There was a problem hiding this comment.
| expD = map_diagonal!(x -> exp(x * τ), D, D) | |
| iV = inv(V) | |
| VexpD = rmul!(V, expD) | |
| if eltype(A) <: Real && eltype(τ) <: Real | |
| expA .= real.(VexpD * iV) | |
| return expA | |
| else | |
| return mul!(expA, VexpD, iV) | |
| end | |
| if eltype(A) <: Real && eltype(τ) <: Real | |
| VexpD = V .* exp.(transpose(diagview(D)) .* τ) | |
| expAc = rdiv!(VexpD, LinearAlgebra.lu!(V)) | |
| return expA .= real.(expAc) | |
| else | |
| expA .= V .* exp.(transpose(D) .* τ) | |
| return rdiv!(expA, LinearAlgebra.lu!(V)) | |
| end |
| # -------------- | ||
| function exponential!(A, expA, alg::DiagonalAlgorithm) | ||
| check_input(exponential!, A, expA, alg) | ||
| return map_diagonal!(exp, expA, A) |
There was a problem hiding this comment.
I actually like the broadcasting version more. Together with the suggestions above, I think this removes the need for map_diagonal:
| return map_diagonal!(exp, expA, A) | |
| diagview(expA) .= exp.(diagview(A)) | |
| return expA |
There was a problem hiding this comment.
I think I suggested/introduced the map_diagonal to have a specialization point in case we end up using this for SectorVector/DiagonalTensorMap as well. (Currently broadcasting for those doesn't actually work on GPU, since it falls back to scalar indexing). Happy to remove this if you prefer though, but then we probably should investigate properly implementing the broadcasting :)
|
|
||
| function exponential!((τ, A)::Tuple{Number, AbstractMatrix}, expA, alg::DiagonalAlgorithm) | ||
| check_input(exponential!, (τ, A), expA, alg) | ||
| return map_diagonal!(x -> exp(x * τ), expA, A) |
There was a problem hiding this comment.
| return map_diagonal!(x -> exp(x * τ), expA, A) | |
| diagview(expA) .= exp.(diagview(A) .* τ) | |
| return expA |
| for f in (:exponential!,) | ||
| @eval function default_algorithm(::typeof($f), ::Type{A}; kwargs...) where {A} | ||
| return default_exponential_algorithm(A; kwargs...) | ||
| end | ||
| end | ||
|
|
||
| for f in (:exponential!,) | ||
| @eval function default_algorithm(::typeof($f), ::Tuple{A, B}; kwargs...) where {A, B} | ||
| return default_exponential_algorithm(B; kwargs...) | ||
| end | ||
| end |
There was a problem hiding this comment.
No need to do these via an @eval block since it is only one function:
| for f in (:exponential!,) | |
| @eval function default_algorithm(::typeof($f), ::Type{A}; kwargs...) where {A} | |
| return default_exponential_algorithm(A; kwargs...) | |
| end | |
| end | |
| for f in (:exponential!,) | |
| @eval function default_algorithm(::typeof($f), ::Tuple{A, B}; kwargs...) where {A, B} | |
| return default_exponential_algorithm(B; kwargs...) | |
| end | |
| end | |
| function default_algorithm(::typeof(exponential!), ::Type{A}; kwargs...) where {A} | |
| return default_exponential_algorithm(A; kwargs...) | |
| end | |
| function default_algorithm(::typeof(exponential!), ::Tuple{A, B}; kwargs...) where {A, B} | |
| return default_exponential_algorithm(B; kwargs...) | |
| end |
| """ | ||
| MatrixFunctionViaEigh() | ||
|
|
||
| Algorithm type to denote finding the exponential `A` by computing the hermitian eigendecomposition of `A`. |
There was a problem hiding this comment.
| Algorithm type to denote finding the exponential `A` by computing the hermitian eigendecomposition of `A`. | |
| Algorithm type for computing a function of a matrix by computing its hermitian eigenvalue decomposition and applying the function to the eigenvalues. |
| @algdef MatrixFunctionViaLA | ||
|
|
||
| """ | ||
| MatrixFunctionViaEigh() |
There was a problem hiding this comment.
| MatrixFunctionViaEigh() | |
| MatrixFunctionViaEigh(eigh_alg) |
| MatrixFunctionViaEig() | ||
|
|
||
| Algorithm type to denote finding the exponential `A` by computing the eigendecomposition of `A`. | ||
| The `eig_alg` specifies which eigendecomposition implementation to use. |
There was a problem hiding this comment.
| MatrixFunctionViaEig() | |
| Algorithm type to denote finding the exponential `A` by computing the eigendecomposition of `A`. | |
| The `eig_alg` specifies which eigendecomposition implementation to use. | |
| MatrixFunctionViaEig(eig_alg) | |
| Algorithm type for computing a function of a matrix by computing its eigenvalue decomposition and applying the function to the eigenvalues. | |
| The `eig_alg` specifies which eigendecomposition implementation to use. |
Jutho
left a comment
There was a problem hiding this comment.
I think there some more required changes that could improve allocations, code duplication and in one case correctness.
| D, V = eigh_full!(A, alg.eigh_alg) | ||
| expD = map_diagonal!(x -> exp(x / 2), D, D) | ||
| VexpD = rmul!(V, expD) | ||
| return mul!(expA, VexpD, V') |
There was a problem hiding this comment.
| return mul!(expA, VexpD, V') | |
| return mul!(expA, VexpD, VexpD') |
This implements the exponential of a matrix for both
BLASFloatsandBigFloats.I have named these functions
exponentialandexponential!, instead of the usualexpandexp!fromLinearAlgebra. Extending these methods while keeping the current structure using @algdef and @ functiondef results in some naming conflicts. The default for BLASFloats is to useLinearAlgebra.exp!. InTensorKit, we can still stick to theexpnaming convention.