-
Notifications
You must be signed in to change notification settings - Fork 603
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 norm for second quantized Hamiltonian #2653
Add norm for second quantized Hamiltonian #2653
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## resource_estimation_algorithms #2653 +/- ##
===============================================================
Coverage 99.61% 99.61%
===============================================================
Files 255 256 +1
Lines 20821 20830 +9
===============================================================
+ Hits 20741 20750 +9
Misses 80 80
Continue to review full report at Codecov.
|
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.
Looks good Soran! Just a few quick comments:
- Is this quantity differentiable? Could we compute the gradient wrt the one and two electron integral parameters? If this doesn't make sense to do then no worries, otherwise it might be worth adding a unit test to check for
- I imagine this function will not be user facing but mostly used internally in the resource estimation pipeline. If it is user facing, it might be worth adding tests to make sure that if the parameters
one
,two
,eigvals
are given in a different framework (autograd or tf arrays) then this function doesn't fail. - Codecov is complaining about a few lines which still need to be tested. (you can use
#pragma no cover
to suppress lines you think have been sufficiently tested but still cause errors in Codcov)
Thanks Jay. I am not actually sure about the benefit of making the norm differentiable wrt the input parameters.
I tested the function with inputs given as autograd numpy array and tensorflow varibales and it works. I can add such tests but regarding what mentioned above about differentiability, not sure if these tests are still necessary. If they are still useful, I can add them.
hmm, I see a full coverage for |
Cool, if differentiability isn't a concern then I doubt we need to test it. Thats weird, when I reviewed it initially there were some inline comments from codecov complaining about some trivial lines of code. I guess if its not complaining now then this should be good to go. 💯 |
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.
Looks good !
Context:
This PR adds functions for computing the 1-norm of a molecular Hamiltonian from the one- and two-electron integrals and eigenvalues of the factorized two-electron integral tensor.
Description of the Change:
Benefits:
Possible Drawbacks:
Related GitHub Issues: