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

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

Merged
merged 13 commits into from
Jun 26, 2024
Merged

Conversation

lkdvos
Copy link
Collaborator

@lkdvos lkdvos commented Jun 11, 2024

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.

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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.
📢 Have feedback on the report? Share it here.

return SectorDict(c => LinearAlgebra.svdvals!(b) for (c, b) in blocks(t))
end

# TODO: decide if we want to keep these specializations:
Copy link
Owner

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.

Copy link
Collaborator Author

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.

@Jutho
Copy link
Owner

Jutho commented Jun 26, 2024

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
Copy link
Collaborator Author

lkdvos commented Jun 26, 2024

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.

@lkdvos lkdvos merged commit 53929cd into master Jun 26, 2024
14 checks passed
@lkdvos lkdvos deleted the eig-svd-vals branch June 26, 2024 10:14
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.

None yet

2 participants