Codecov Report

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

... and 3 files with indirect coverage changes

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

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

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.

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

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

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.

I think this is probably ready?

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.

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.

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.