-
Notifications
You must be signed in to change notification settings - Fork 438
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
Orbitals should be multiplied by sqrt(occ) not occ #3138
base: master
Are you sure you want to change the base?
Conversation
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.
Would it be possible to add a test that fails before but passes after this change? Clearly there is a testing gap, given how long this bug has been present, and how something like this can be changed without having to change any of tests.
Yes, by testing the guess energy for H atom. |
Looks like there are some unrelated test failures, but this one is ready to go. As shown by the changes in the pytest guess energies, the new code reaches a considerably lower SAD guess energy for the HF molecule: -100.02909951427 vs -99.63941801281894. Large improvements are expected especially for systems containing hydrogen atoms, since the existing code only includes 0.5 electrons on those atoms. |
tests/pytests/test_scf_options.py
Outdated
"sap": -99.50144270485751, | ||
"core": -88.99202896495, | ||
"gwh": -94.35585144645, | ||
"sad": -99.34836107817, |
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.
Is it a concern that with the change SAD UHF & ROHF (and often sadno, *huckel) guess energies are less deep?
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.
No, the issue is that the test here is FH+, which has orbitals that are more compact than the neutral FH. It is logical that the proper guess for the neutral molecule yields a slightly higher energy for the cation.
Like I mention above, right now the SAD code is placing 0.5
electrons for the hydrogen atom, whereas fluorine probably has 7 electrons distributed evenly on 4 spatial orbitals [no symmetry, this is the key feature in the OpenOrbitalOptimizer guess in #3136] which means that its number of electrons is also wrong by -0.875
. Therefore, the current guess is spuriously good for FH+ as it corresponds to a cation with charge 1.375.
14c9a6f
to
dc01cc9
Compare
* Orbitals should be multiplied by sqrt(occ) not occ, since otherwise the density matrix becomes C occ^2 C^T. * Update pytest reference values * Update number of screened shells based on SAD guess * Specify occupation in large atoms test * Increase gradient test tolerance due to very small mismatch * clang-format * Relax test criteria due to small failures * Update test variable to remove deprecation warning * SAD guess energy check is for SAD energy not first variational energy * There is a 1e-7 change to the orbital energy in the HF/DZ test on SH2. * Wave function needs to be converged more tightly for test to pass * Increase test limit * Bump tolerance also for other gradient tests
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.
LGTM! Anything holding this PR up from merging?
…he density matrix becomes C occ^2 C^T.
Description
Fixes a bug in
sad.cc
: pseudo-orbitals should be obtained by multiplying the orbital with the square root of their occupation to reproduce the correct density matrix C occ C^T.Closes #3137.
User API & Changelog headlines
Dev notes & details
Questions
Checklist
Status