-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add support for eigvals
, svdvals
and diag
, diagm
.
#130
Conversation
The easiest implementation would just be a wrapper around `eigen`, and this should await some more experiments with the eigenvector gauge fixing.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
src/tensors/factorizations.jl
Outdated
return SectorDict(c => LinearAlgebra.svdvals!(b) for (c, b) in blocks(t)) | ||
end | ||
|
||
# TODO: decide if we want to keep these specializations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line of the plans to stop the special casing of TrivialTensorMap
, I would support to remove these specialisations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely can, but in order to keep this consistent with diag
and diagm
, this would then also return a dictionary object for TrivialTensorMap
. I don't have a big opinion on this, as I usually write code for generic symmetries and have to deal with these cases separately anyways, but I just want to mention that this is a bit connected here.
I think this is good, although I think that the |
This would not be in line with the way it is implemented in |
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
andsvdvals
, which would otherwise necessarily involve accessing tensormap data, an operation that typically breaks Zygote.The implementation currently returns
Vector
objects for TrivialTensorMap's, andSectorDict{I,Vector}
objects in all other cases. I agree that this would be more elegantly solved with something likeDiagonalTensorMap
, 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 theeig
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 toeigvecs(t) = eig(t)[2]
, which does not really yield large benefits at the moment.