-
Notifications
You must be signed in to change notification settings - Fork 56
Updates for MatrixAlgebraKit v0.6 #312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
|
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. |
|
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 |
|
I'm definitely happy to discuss having higher-level interfaces in TensorKit, but probably that does not have to hold back this PR, and should be handled separately. As mentioned in one of the comments above, in the long run I would like to have a better way to deal with the |
|
I think this is probably ready? |
|
Let's merge this asap and then tag a new TensorKit version, because the fact that |
|
I agree to merge this ASAP. I had a more in-depth look at the adjoint implementations, and it turns out that One last thing I have been considering and wanted to bring up, although this can be handled in a separate PR as well since it wouldn't be breaking, is how much we value the For the release, I think this has to count as breaking again, both because of #305 as well as the |
|
I was wondering about the double action of Then again, mostly these checks are there for the output tensors, and if most users do not actually allocate these themselves, the checks are also quite useless, since they just check that our So I will approve and then I think automerge will kick in. |
* initial basic design SectorVector * some additional functionality * relax `foreachblock` signature * replace `SectorDict` with `SectorVector` for eig/svdvals * export `svd_vals` * clean up SectorVector design * small fix * add finitedifferences support * update changelog * some simplifications and extensions * some further fixes * some more fixes * update dates --------- Co-authored-by: Jutho Haegeman <[email protected]>
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.