Compute, Don't Store, DOCC/SOCC #2619
Conversation
JonathonMisiewicz
added
wavefunction
|
Eco failure is, quite predictably, because it breaks |
|
After discussion with Lori, work on getting this PR through will resume after MQM. We also have a plan to deal with plugin incompatibilities. Feel free to review earlier if you wish. |
JonathonMisiewicz
force-pushed
the
cleanup
branch
2 times, most recently
from
e9760c7 to
673acfc
Compare
JonathonMisiewicz
force-pushed
the
cleanup
branch
from
3a09e94 to
d85056a
Compare
|
@susilehtola There was a request among core devs for you to sign off on this PR before we get a third review. |
| nalpha_ = doccpi_.sum() + soccpi_.sum(); | ||
| nbeta_ = doccpi_.sum(); | ||
| nalpha_ = nalphapi_.sum(); | ||
| nbeta_ = nbetapi_.sum(); |
There was a problem hiding this comment.
Shouldn't there be a spin state check here?
There was a problem hiding this comment.
I don't know. The force functions are marked as intended for expert use only because careless use can produce inconsistencies. Nothing in Psi uses them. I can't tell whether the potential to cause a spin state violation is a bug or a feature. Maybe a warning is in order if the number of unpaired electrons changes?
There was a problem hiding this comment.
OK, in that case we can forget about it
There was a problem hiding this comment.
Actually... I think the force functions may break with these changes, since socc and docc are recomputed. One would have to override socc and docc simultaneously lest the occupations be changed in the other code.
There was a problem hiding this comment.
I would remove the force functions altogether, since it sounds like they aren't even used..
There was a problem hiding this comment.
a combo docc_and_socc can work: shannonhouck/psi4fockci@9a39165#diff-e33326fe2aaf9f7d7d385252ae30fa0c5a6d47751f890592c8ddc82bd8b861e4R83
There was a problem hiding this comment.
I'll give that a look. Do we have FockCI test coverage?
possibly not, 😳 . She was working on two plugins, old and new. It was near a psi release, and I got very confused and packaged the wrong one. She added https://github.com/psi4/psi4/blob/master/doc/sphinxman/source/plugin_psi4fockci.rst though, so the internal build and that input might work.
What does "internal build" mean?
There was a problem hiding this comment.
toggle ENABLE_psi4fockci=ON in <objdir>/CMakeCache.txt and rebuild. hopefully cmake will pull a copy and install it into stage/ so it's ready to run.
There was a problem hiding this comment.
@loriab It doesn't look like FockCI has any tests registered with Psi. Am I mistaken?
There was a problem hiding this comment.
you could try https://github.com/psi4/psi4/blob/master/tests/pytests/test_addons.py#L1236 . because of my repo confusion, though, it may not be what you're after.
|
FYI, I'm in the process of revising the PR and am marking comment as resolved as I fix them locally. |
JonathonMisiewicz
force-pushed
the
cleanup
branch
from
d85056a to
9609f0c
Compare
|
All comments except FockCI testing addressed. |
|
@susilehtola Comments added. Per discussion with @loriab , I'll leave a comment on the FockCI repo letting them know about this API-breaking change. It sounds like that repo has its own issues that need to be dealt with, beyond the scope of this PR. |
|
@JonathonMisiewicz as far as I can see, this should be OK now except the one wrong printout above. |
JonathonMisiewicz
force-pushed
the
cleanup
branch
from
052f961 to
7db756f
Compare
zachglick
left a comment
zachglick
left a comment
There was a problem hiding this comment.
LGTM, thanks for working on this. Any reason for not adding a test or two?
No tests came to mind as worth adding. If you have suggestions, I'll probably take them. |
|
No suggestions, merge when ready. |
|
PRd to FockCI, so I can wash my hands of the |
Any one of the various failing SCF calculations where occupations break old invalid assumptions? |
On my to-do list, I just wasn't confident those would already be fixed by this PR. Thanks for the reminder. |
5 participants
Description
#2476 and #2594 indicate a major problem in Psi's SCF code: the simplistic formula
nalphapi = doccpi + soccpiandnbetapi = doccpiis incapable of describing references where there are more beta than alpha orbitals of a given irrep. This occurs in MOM (where we instead use ugly hacks) and in UHF (where we crash). Furthermore, storing all of these irrep quantities is redundant.This PR remedies the situation by only storing
nalphapiandnbetapi.doccpiandsoccpican be computed from these when needed. The Py-side API is unchangeddoccpi(), soccpi(), while the C-side API changes fromdoccpi_, soccpi_todoccpi(), soccpi(). While we are doing some extra work to compute docc and socc every time they're needed, the computational cost is negligible in comparison to Fock diagonalizations, integral transforms, and tensor contractions.To prevent scope creep, this PR aims solely to change the wavefunction. This is a major undertaking and should not be done lightly. A subsequent PR will fix the linked issues (if not fixed by this PR) and add them as test cases once the fix is confirmed.
Obligatory @susilehtola ping.
Todos
Checklist
Status