Jutho / TensorKit.jl

A Julia package for large-scale tensor computations, with a hint of category theory
MIT License
218 stars 38 forks source link

Add support for `eigvals`, `svdvals` and `diag`, `diagm`. #130

Closed lkdvos closed 2 months ago

lkdvos commented 3 months ago

This PR extends some of the LinearAlgebra functions for obtaining information about diagonal entries. This is particularly useful as it allows the definition of rrules for eigvals and svdvals, which would otherwise necessarily involve accessing tensormap data, an operation that typically breaks Zygote.

The implementation currently returns Vector objects for TrivialTensorMap's, and SectorDict{I,Vector} objects in all other cases. I agree that this would be more elegantly solved with something like DiagonalTensorMap, but as this is not (yet) implemented and requires a bit more effort, this could work as an intermediate solution.

Note however that this does introduce a subtlety when dealing with non-abelian symmetries, as this yields an intermediate representation where people will have to keep in mind that the inner product is non-trivial. See the finitedifferences implementation in test/ad.jl#L50 for the consequence.

I did not add any docstrings to these methods, as this really does form a valid implementation of the original functions (which thus already have docstrings), but I could of course add them if needed.

Finally, I choose to not include eigvecs into this PR, as the implementation of the eig functions and the respective gauge choices should be re-evaluated, which I think is beyond the scope of this PR, and thus the implementation would really boil down to eigvecs(t) = eig(t)[2], which does not really yield large benefits at the moment.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.15%. Comparing base (b026cf2) to head (de03da5).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #130 +/- ## ========================================== + Coverage 81.94% 82.15% +0.20% ========================================== Files 42 42 Lines 5667 5698 +31 ========================================== + Hits 4644 4681 +37 + Misses 1023 1017 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Jutho commented 2 months ago

I think this is good, although I think that the diag and diagm functions only make sense voor the N1 = N2 = 1 case, probably with domain == codomain. It will be interesting to see if other use cases have emerged if we replace this functionality at some point (hopefully soon) with a full fledged DiagonalTensorMap implementation.

lkdvos commented 2 months ago

I think this is good, although I think that the diag and diagm functions only make sense voor the N1 = N2 = 1 case, probably with domain == codomain. It will be interesting to see if other use cases have emerged if we replace this functionality at some point (hopefully soon) with a full fledged DiagonalTensorMap implementation.

This would not be in line with the way it is implemented in LinearAlgebra however, where these functions can obtain the diagonal or generate "diagonal" rectangular matrices. I agree that it's not obvious that these cases would appear in our typical use-cases.