-
Notifications
You must be signed in to change notification settings - Fork 25
Adapt simple update to DiagonalTensorMap #124
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
Conversation
Codecov ReportAttention: Patch coverage is
|
|
The failed tasks involve the backprop of |
lkdvos
left a comment
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.
The rrule you have looks fine, the tests are just complaining because you removed the implementations of these functions for the TensorMap case, which I don't think is necessary.
(In any case we still have to support TensorMap for now since the projector onto the diagonal type is currently not working properly, and we might lose that information. See also QuantumKitHub/TensorKit.jl#208.
lkdvos
left a comment
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.
Overall, looks good to me! In principle, the examples could do with some clean up, but maybe we can leave that for when we uniformize the documentation.
(Also, if possible in the future to split of different topics into different PRs that would be great).
After updating with the master and my final comment this should be good to go for me.
Co-authored-by: Lukas Devos <[email protected]>
pbrehmer
left a comment
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.
Thanks for the work @Yue-Zhengyuan, this looks good! As long as we make sure to credit the YASTN code bits and adapt the naming to our internal conventions in the follow-up PR, I'm happy to merge this :)
Changes:
SUWeightandsdiag_pow) to work withDiagonalTensorMaps.