Skip to content

Conversation

@lkdvos
Copy link
Member

@lkdvos lkdvos commented Dec 5, 2025

This PR is an implementation of the data structure suggested for svd_vals, eig_vals, ... but also thinking of entanglement_spectrum functionality etc.

I introduced a SectorVector type, which acts as a (dense) vector with the option of taking views for the different sectors.
This is a better return type for svd_vals etc, since their MatrixAlgebraKit equivalents are supposed to output "vector"-like objects instead of "matrix"-like objects.
Therefore, SectorVector relates to DiagonalTensorMap in the same way Vector relates to Diagonal.

There are quite a few opinionated choice in here that we may carefully consider, so let me mention them here and motivate my initial choices:

  • I made SectorVector <: AbstractVector in the end. The main reason is that I remembered people do actually like to inspect the singular and eigenvalues, and this is probably not an unreasonable point to actually be able to call maximum, minimum, etc on this object.
  • I ended up not restricting I <: Sector, even though the data structure does somewhat assume this is the case. It didn't really feel necessary, and leaves the door open for weird alternative uses, although I'm not sure if this really makes a difference.
  • This results in needing to explicitly call pairs, values or keys if you want to deal with the AbstractDict-like interface, i.e. iteration by default iterates like a vector, you can iterate over the blocks using pairs or blocks instead.

This last thing is definitely a breaking change with respect to the previous behavior, also for LinearAlgebra.svdvals, but I think that the added convenience of having vector functionality work out of the box might be worth it.

Additionally, we could consider using this Vector as the data type of DiagonalTensorMap, further showing how they are interrelated. I didn't do this (yet), but I would be open for that change, since that would avoid us having to recompute the offsets in block etc.

@Jutho, I won't continue working on this, so feel free to make some more changes and merge/release if you like.

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 53.60000% with 58 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/tensors/sectorvector.jl 39.65% 35 Missing ⚠️
ext/TensorKitFiniteDifferencesExt.jl 0.00% 15 Missing ⚠️
src/factorizations/truncation.jl 85.71% 6 Missing ⚠️
src/factorizations/matrixalgebrakit.jl 75.00% 1 Missing ⚠️
src/tensors/blockiterator.jl 50.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/TensorKit.jl 13.63% <ø> (+8.37%) ⬆️
src/factorizations/diagonal.jl 64.10% <100.00%> (+64.10%) ⬆️
src/factorizations/factorizations.jl 94.11% <100.00%> (+91.17%) ⬆️
src/tensors/linalg.jl 82.69% <100.00%> (+82.69%) ⬆️
src/factorizations/matrixalgebrakit.jl 92.92% <75.00%> (+92.92%) ⬆️
src/tensors/blockiterator.jl 38.61% <50.00%> (+38.61%) ⬆️
src/factorizations/truncation.jl 87.93% <85.71%> (+87.93%) ⬆️
ext/TensorKitFiniteDifferencesExt.jl 53.12% <0.00%> (+53.12%) ⬆️
src/tensors/sectorvector.jl 39.65% <39.65%> (ø)

... and 38 files with indirect coverage changes

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

@lkdvos
Copy link
Member Author

lkdvos commented Dec 7, 2025

Looks good to me. I think the main questions I still have, especially since we are overloading more of the VectorInterface functions is whether or not we want to switch some of the DiagonalTensorMap implementations to make use of this, to avoid duplicating too much code.

Some other idea, that I'm not sure about whether it is good or not, is to restrict the SectorVector to <:DenseVector and make it subtype <:DenseVector, and change the DiagonalTensorMap storage to be this instead.
This feels more in line to the standard Diagonal holding a Vector kind of setup, which seems intuitive, and avoids having to recompute the offsets when accessing it, which we probably don't care about too much but could still be useful.
We could then also consider just moving this code in the diagonal.jl file and bundling these concepts together more.

As a sidenote, I would have guessed that some of these VectorInterface overloads work simply by the <:AbstractVector subtype + overloads, as broadcasting etc should just work (which is precisely why I made that an <:AbstractVector subtype, to have as much of the "expected" behavior for a vector of values "just work".

I do think that changing the DiagonalTensorMap storage type would not be breaking though, so that could also happen in a follow-up, but it seems a bit more natural to already include that decision here.

@Jutho
Copy link
Member

Jutho commented Dec 7, 2025

Looks good to me. I think the main questions I still have, especially since we are overloading more of the VectorInterface functions is whether or not we want to switch some of the DiagonalTensorMap implementations to make use of this, to avoid duplicating too much code.

Some other idea, that I'm not sure about whether it is good or not, is to restrict the SectorVector to <:DenseVector and make it subtype <:DenseVector, and change the DiagonalTensorMap storage to be this instead. This feels more in line to the standard Diagonal holding a Vector kind of setup, which seems intuitive, and avoids having to recompute the offsets when accessing it, which we probably don't care about too much but could still be useful. We could then also consider just moving this code in the diagonal.jl file and bundling these concepts together more.

I am definitely in favor of the <:DenseVector restriction. As for using this in Diagonal, the only "strange" aspect would be that we both have the space and the structure stored in DiagonalTensorMap instances. In the end, we will still need to write the VectorInterface implementations for DiagonalTensorMap, so whether they lower to data::Vector or to data::SectorVector, I don't know if we gain much (except a little bit for inner`). So I don't have strong opinion about this.

As a sidenote, I would have guessed that some of these VectorInterface overloads work simply by the <:AbstractVector subtype + overloads, as broadcasting etc should just work (which is precisely why I made that an <:AbstractVector subtype, to have as much of the "expected" behavior for a vector of values "just work".

I did this to make use of the specialized notations for Vector, though that probably doesn't matter very much for these BLAS level 1 operations. Another reason would be for GPU vectors, where I guess the default implementation would result in scalar getindex calls.

@lkdvos lkdvos merged commit 412a45c into main Dec 8, 2025
27 of 29 checks passed
@lkdvos lkdvos deleted the sectorvector branch December 8, 2025 23:53
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.

3 participants