Skip to content
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

WIP: implementation of DiagonalTensorMap #174

Merged
merged 14 commits into from
Nov 18, 2024
Merged

WIP: implementation of DiagonalTensorMap #174

merged 14 commits into from
Nov 18, 2024

Conversation

Jutho
Copy link
Owner

@Jutho Jutho commented Nov 10, 2024

I think we do want to have a DiagonalTensorMap type for the singular values and eigenvalues in the corresponding factorisations. Changing those tensor types arguably constitutes a breaking change, so we do not want to do this after releasing 1.0.

In this first PR, I will just implement the type (finished) and a minimal implementation with some tests, but not yet start to use it.

Switching svd and eig, eigh to use DiagonalTensorMap will constitute a second PR.

Copy link
Collaborator

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very reasonable to me!
I would think that definitely here, we can probably get an efficient block iterator quite easily, which might alleviate the overhead of computing the block location, but for now, it might not even show up in profilers anyways.

src/tensors/diagtensor.jl Outdated Show resolved Hide resolved
@Jutho
Copy link
Owner Author

Jutho commented Nov 11, 2024

Looks very reasonable to me!
I would think that definitely here, we can probably get an efficient block iterator quite easily, which might alleviate the overhead of computing the block location, but for now, it might not even show up in profilers anyways.

Yes I agree the current blocks is not optimal, as it recomputes the offset for every block separately. One could write an iterator, or actually make that one return a SectorDict, as in many cases people might actually be interested in having all of them available at the same time, e.g. to make a plot of the entanglement spectrum.

src/tensors/diagtensor.jl Outdated Show resolved Hide resolved
src/tensors/diagtensor.jl Outdated Show resolved Hide resolved
src/spaces/vectorspaces.jl Outdated Show resolved Hide resolved
src/TensorKit.jl Outdated Show resolved Hide resolved
@Jutho
Copy link
Owner Author

Jutho commented Nov 17, 2024

Ok, if all tests work, I think this is getting more or less to a final stage for now.

@lkdvos
Copy link
Collaborator

lkdvos commented Nov 17, 2024

Before merging, as a feature request that will be useful for PEPSKit, powers of DiagonalTensorMap with real exponents would be nice to define here (along with the rrules). (https://github.com/QuantumKitHub/PEPSKit.jl/blob/90375ca9b34b8c3fc2c7e3550cbdfb033163ccff/src/utility/util.jl#L26)
For negative exponents, it would also be convenient to have a function that has some kind of cutoff, but I am not sure if such a function already exists for matrices even? pseudopower?

@Jutho
Copy link
Owner Author

Jutho commented Nov 17, 2024

Before merging, as a feature request that will be useful for PEPSKit, powers of DiagonalTensorMap with real exponents would be nice to define here (along with the rrules). (https://github.com/QuantumKitHub/PEPSKit.jl/blob/90375ca9b34b8c3fc2c7e3550cbdfb033163ccff/src/utility/util.jl#L26)
For negative exponents, it would also be convenient to have a function that has some kind of cutoff, but I am not sure if such a function already exists for matrices even? pseudopower?

Checking the matrix implementation, it is another one of those that is type unstable. For a real matrix, if p is integer, or A is positive definite, then the result is real, otherwise it becomes complex. What is furthermore funny, is this:

julia> A^0.57
2×2 Symmetric{ComplexF64, Matrix{ComplexF64}}:
  0.121409+0.155758im  -0.251072+0.107136im
 -0.251072+0.107136im   0.313729+0.0736924im

julia> Diagonal(eigvals(A))^0.57
ERROR: DomainError with -0.07888175758153752:
Exponentiation yielding a complex result requires a complex argument.
Replace x^y with (x+0im)^y, Complex(x)^y, or similar.
Stacktrace:
...

Because of all of this, I would very much like to postpone this to a separate PR.

@lkdvos
Copy link
Collaborator

lkdvos commented Nov 17, 2024

I'm definitely okay with having this as a separate PR.

The results with the Diagonal you show are however exactly the behavior I would expect, and I would be completely okay with that throwing a DomainError. The cases where PEPSKit uses this, the diagonal tensor is the result of an SVD, so it shouldn't be a problem there.

src/tensors/diagonal.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 83.25123% with 34 lines in your changes missing coverage. Please review.

Project coverage is 81.39%. Comparing base (cf796ae) to head (0111bc9).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/tensors/diagonal.jl 83.05% 30 Missing ⚠️
src/tensors/indexmanipulations.jl 66.66% 3 Missing ⚠️
src/tensors/braidingtensor.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   80.93%   81.39%   +0.45%     
==========================================
  Files          41       42       +1     
  Lines        5015     5218     +203     
==========================================
+ Hits         4059     4247     +188     
- Misses        956      971      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@Jutho
Copy link
Owner Author

Jutho commented Nov 18, 2024

Ok, thanks for the extra additions @lkdvos . I think this is now ready to be merged.

@Jutho Jutho merged commit 002e1d8 into master Nov 18, 2024
14 checks passed
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.

2 participants