Updates for MatrixAlgebraKit v0.6 #312
Conversation
Codecov Report❌ Patch coverage is
... and 15 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
This doesn't actually hold for non-hermitian matrices
|
Assuming that the tests pass here, I think this should be ready to be merged. |
|
What change made it such that so much code in "src/factorizations/factorizations.jl" can be removed? |
|
Most of that code was introduced in the first place to work around the design of the orthnull parts. This was one of the main motivations to rework that, which now more cleanly reuses the different factorization functions. I think the biggest contribution is that we now first figure out the algorithm type, which dictates the factorization type, and only then start the rest of the logic, which makes it a lot easier to implement in a generic way. Additionally we've been more careful in MatrixAlgebraKit to not overly specialize on |
|
What's the status here? This is blocking the GPU updates PR :) |
|
I will try to go through tonight. |
| $right_f!(adjoint(t), reverse(adjoint.(F)), _adjoint(alg)) | ||
| return F | ||
| F′ = $right_f!(adjoint(t), reverse(adjoint.(F)), _adjoint(alg)) | ||
| return reverse(adjoint.(F′)) |
There was a problem hiding this comment.
These were incorrect before my latest commit, so reminder here to figure out why the tests didn't pick this up.
| return reverse(adjoint.(F′)) | ||
| end | ||
|
|
||
| # disambiguate by prohibition |
There was a problem hiding this comment.
Another out-of-place question due to the limitation to only add comments near edited code: line 113 below, namely
function MAK.svd_compact!(t::AdjointTensorMap, USVᴴ, alg::DiagonalAlgorithm)
return MAK.svd_compact!(t, USVᴴ, alg.alg)
endThis doesn't seem correct. I don't think DiagonalAlgorithm has an alg.alg field. Even if this is only to fix an ambiguity and is never called, we should try to make it some valid code. But I am a bit confused about this method definition.
|
Ok, I went over this PR (and the whole I think this is almost ready, but it would be good to have a discussion about the deprecation path of the old interface. For me, the MatrixAlgebraKit function naming was always a convenient way to implement all these methods as a library implementer. And I think it is very useful that they all exist at the level of TensorMaps as well. But a library as TensorKit might also want to have an additional more convenient interface with shorter function names (better discoverable) and more general behavior, where truncation is simply controlled by a keyword argument, such as the earlier |
This PR brings TensorKit up to speed with the MatrixAlgebraKit v0.6.
The main changes are the addition of the new projection functionality, and a rework of the orthnull interface.
The former is mostly just implemented here, while the latter means that a lot of code could be removed from TensorKit instead.
I additionally took the time to hopefully stabilize some of the truncated SVD AD tests, as these struggled with finite differences in combination with spaces that change.