You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is an implementation of the data structure suggested for
svd_vals,eig_vals, ... but also thinking ofentanglement_spectrumfunctionality etc.I introduced a
SectorVectortype, which acts as a (dense) vector with the option of taking views for the different sectors.This is a better return type for
svd_valsetc, since their MatrixAlgebraKit equivalents are supposed to output "vector"-like objects instead of "matrix"-like objects.Therefore,
SectorVectorrelates toDiagonalTensorMapin the same wayVectorrelates toDiagonal.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:
SectorVector <: AbstractVectorin 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 callmaximum,minimum, etc on this object.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.pairs,valuesorkeysif you want to deal with theAbstractDict-like interface, i.e. iteration by default iterates like a vector, you can iterate over the blocks usingpairsorblocksinstead.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
Vectoras the data type ofDiagonalTensorMap, 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 inblocketc.@Jutho, I won't continue working on this, so feel free to make some more changes and merge/release if you like.