Skip to content

Conversation

@lkdvos
Copy link
Member

@lkdvos lkdvos commented Nov 10, 2025

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.

@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

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

Files with missing lines Patch % Lines
src/factorizations/adjoint.jl 69.56% 7 Missing ⚠️
src/auxiliary/deprecate.jl 0.00% 1 Missing ⚠️
src/tensors/adjoint.jl 50.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/TensorKit.jl 22.72% <ø> (ø)
src/factorizations/diagonal.jl 63.15% <ø> (-6.74%) ⬇️
src/factorizations/factorizations.jl 94.11% <100.00%> (+6.31%) ⬆️
src/factorizations/matrixalgebrakit.jl 93.26% <100.00%> (+5.06%) ⬆️
src/factorizations/pullbacks.jl 57.14% <100.00%> (ø)
src/factorizations/truncation.jl 86.41% <100.00%> (+10.68%) ⬆️
src/factorizations/utility.jl 58.33% <ø> (-10.42%) ⬇️
src/auxiliary/deprecate.jl 2.19% <0.00%> (ø)
src/tensors/adjoint.jl 72.41% <50.00%> (-1.67%) ⬇️
src/factorizations/adjoint.jl 71.79% <69.56%> (+71.79%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 14, 2025

Your PR no longer requires formatting changes. Thank you for your contribution!

@lkdvos
Copy link
Member Author

lkdvos commented Nov 14, 2025

I'm linking #313 and #314 here, as this should hopefully be easier to resolve with the updated orthnull interfaces

@lkdvos lkdvos marked this pull request as ready for review November 15, 2025 15:00
@lkdvos lkdvos requested a review from Jutho November 16, 2025 14:09
@lkdvos
Copy link
Member Author

lkdvos commented Nov 16, 2025

Assuming that the tests pass here, I think this should be ready to be merged.
There are some parts that definitely could be improved (for example the AdjointTensorMap factorizations), but this depends on improvements in MatrixAlgebraKit itself so I want to hold off on that for now.

@Jutho
Copy link
Member

Jutho commented Nov 17, 2025

What change made it such that so much code in "src/factorizations/factorizations.jl" can be removed?

@lkdvos
Copy link
Member Author

lkdvos commented Nov 17, 2025

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 AbstractMatrix, and while there is a bit more boilerplate over there, we can remove quite a lot of it here.

@kshyatt
Copy link
Member

kshyatt commented Nov 21, 2025

What's the status here? This is blocking the GPU updates PR :)

@Jutho
Copy link
Member

Jutho commented Nov 21, 2025

I will try to go through tonight.

@Jutho
Copy link
Member

Jutho commented Nov 23, 2025

Ok, I went over this PR (and the whole src/factorizations/ code more generally) during the past few days. I made a few changes already, but also left some more comments.

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 tsvd (I guess that is the most important one). It would be good to exchange ideas about this.

@lkdvos
Copy link
Member Author

lkdvos commented Nov 28, 2025

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.
I am hoping I resolved the autodiff error, this was a slight hiccup with blocks that might be fully truncated.

As mentioned in one of the comments above, in the long run I would like to have a better way to deal with the AdjointTensorMap implementations, preferably already in MatrixAlgebraKit, but that probably has to wait for a bit anyways, so I might advocate trying to merge this PR first.

@Jutho
Copy link
Member

Jutho commented Nov 29, 2025

I think this is probably ready?

Jutho
Jutho previously approved these changes Nov 29, 2025
@Jutho
Copy link
Member

Jutho commented Nov 29, 2025

Let's merge this asap and then tag a new TensorKit version, because the fact that tsvd (besides being deprecated) no longer returns the truncation error seems to be breaking many people's codes.

@lkdvos
Copy link
Member Author

lkdvos commented Nov 29, 2025

I agree to merge this ASAP.

I had a more in-depth look at the adjoint implementations, and it turns out that copy_input requires f instead of f!, so our specializations were just never called since the fallback of copy_input instantiates a regular tensor instead of an adjoint one.
I now fixed this, and had to then make some additional changes to ensure that the tests pass, which I hope now they do again (and this should increase coverage a bit).

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 check_input functions at the tensor level.
Given the current MatrixAlgebraKit implementations, most of the checks will actually be repeated again at the block level, and this is quite a bit of code, just to have the error appear one layer sooner.
It would be straightforward to simply delete all of this code, and not have a call to check_input in the tensor implementations.
Of course it doesn't make a big difference now since we have all of these functions written anyways, but keeping less code does seem like a viable argument.
For the sake of merging this ASAP, I've gone ahead and simply implemented this, but it is easy to revert this last commit.

For the release, I think this has to count as breaking again, both because of #305 as well as the left_orth and right_orth changes, and the isisometry to isisometric. Are there other changes we want to include in this release as well?
I can think of #291 and #261, although the latter might not be considered breaking per se.

@lkdvos lkdvos enabled auto-merge (squash) November 30, 2025 02:33
@Jutho
Copy link
Member

Jutho commented Nov 30, 2025

I was wondering about the double action of check_input, but mostly from a performance concern. However, I assume these checks are mostly very lightweight, so I am wondering if removing them from the tensormap level is such a good idea. I guess it's a matter of weighing the benefits of less code to maintain vs clearer error messages.

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 initialize_output works as intended. So maybe we can indeed go ahead, and add specific more informative checks at the tensor level when the need presents itself.

So I will approve and then I think automerge will kick in.

@lkdvos lkdvos merged commit 3d65453 into main Nov 30, 2025
39 of 42 checks passed
@lkdvos lkdvos deleted the mak-v0.6 branch November 30, 2025 22:23
Jutho referenced this pull request Dec 9, 2025
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

truncation error with rtol right_null errors when trunc is provided

4 participants