Move SCF-DIIS to Python #2298

JonathonMisiewicz · 2021-09-23T01:48:19Z

Description

In preparation for some PRs adding EDIIS and ADIIS to Psi, this PR moves the compute_orbital_gradient functions, which call DIIS, to the Python layer.

There are multiple things going on:

For pybinding, it was convenient to convert the enums in libdiis to strongly typed enums. This means the enums have their own scope, which changes how other functions had to call them. Every change in occ, dfocc, dct, fisapt, and libdiis is solely due to this. This occupies commit one.
Many other functions had to be pybound. In particular, it was necessary to convert diis_manager into a property. Old code would call HF.diis_manager(), but now they call HF.diis_manager_. We're not bothering to deprecate the old way of calling it. I'm not aware if there's a way to still support the old syntax. It might work if we change the property name to diis_manager? I haven't tested this.
It is not possible to pybind a variadic, so I had to pybind every set_error_vector_size and set_vector_size and add_entry type combination that Python might use. Sad, but necessary.

And with that done, all four of the compute_orbital_gradient functions could finally be moved to the Python layer. compute_orbital_gradient is kept as a virtual function, C++ side, to signal that Psi expects such a function to exist. My tests indicate that if the function is defined C++ side, it still works, so this won't break any SCF subclasses defined in plugins.

Todos

Lots of pybind-ing involving DIIS
Alert! HF.diis_manager() has changed to HF.diis_manager_
HF.compute_orbital_gradient moved to the Python layer. C-side compute_orbital_gradient is still supported, but not used by the core Psi.

Questions

Who is responsible for updating the Great DFOCC Branch with the libdiis API change?
How do we feel about the new functions I Pybind-ed over? Anything amiss?

Checklist

ctest -L scf passes

Status

Ready for review
Let's discuss the two questions before merging this in, please.

psi4/driver/procrouting/scf_proc/subclass_methods.py

loriab

lgtm!

psi4/driver/procrouting/scf_proc/subclass_methods.py

loriab · 2021-09-23T15:22:31Z

psi4/driver/procrouting/scf_proc/subclass_methods.py

+
+    if save_fock:
+        if not self.initialized_diis_manager_:
+            storage_policy = getattr(core.DIISManager.StoragePolicy, "InCore" if self.scf_type() == "DIRECT" else "OnDisk")


Suggested change

storage_policy = getattr(core.DIISManager.StoragePolicy, "InCore" if self.scf_type() == "DIRECT" else "OnDisk")

if self.scf_type() == "DIRECT":

storage_policy = core.DIISManager.StoragePolicy.InCore

else:

storage_policy = core.DIISManager.StoragePolicy.OnDisk

equivalent? I usually save getattr for when attribute only known in a variable. But that's my readability opinion. Your choice.

Yes, equivalent. The right thing to do for readability is probably to import from core.DIISManager import StoragePolicy, RemovalPolicy. Then I can have a terse ternary without using getattr.

that works. only other consideration is if you'd ever want to track which paths were tested with codecov.

loriab · 2021-09-23T15:24:46Z

psi4/driver/procrouting/scf_proc/subclass_methods.py

+
+from psi4 import core
+
+def _RHF_orbital_gradient(self, save_fock: bool, max_diis_vectors: int) -> float:


general info to readers: the Sphinx docs can now process type annotations, so they're not added in vain. the Sphinx docs now also hate the old Parameters\n verbose : int style in-docstring type annotation, so those're worth moving up into the signature.

JonathonMisiewicz · 2021-09-24T18:52:12Z

On hold until #2296 gets approved, to see if we can reach a consensus on what pybind docstring format is now.

JonathonMisiewicz · 2021-09-27T17:31:42Z

Force pushed to bring this branch up-to-date. Should be good to review again.

jturney

Looks good!

PeterKraus

I only have a couple very minor style suggestions. I suggest @JonathonMisiewicz can decide whether to action them or merge straight away.

psi4/driver/procrouting/scf_proc/subclass_methods.py

PeterKraus · 2021-10-05T04:52:06Z

psi4/driver/procrouting/scf_proc/subclass_methods.py

+        return gradient.absmax()
+
+core.RHF.compute_orbital_gradient = _RHF_orbital_gradient
+core.UHF.compute_orbital_gradient = core.CUHF.compute_orbital_gradient = _UHF_orbital_gradient


I'm not a terribly big fan of this triple-assignment.

It looked strange to me at first as well, but I've made my peace with it. Putting these assignments on the same line makes it obvious that these two are intended to be the same function. I was a little surprised that CUHF didn't need custom logic here.

PhillCli reviewed Sep 23, 2021

View reviewed changes

psi4/driver/procrouting/scf_proc/subclass_methods.py Outdated Show resolved Hide resolved

loriab approved these changes Sep 23, 2021

View reviewed changes

PeterKraus mentioned this pull request Sep 24, 2021

Document epsilon_a_subset and epsilon_b_subset #2296

Merged

2 tasks

JonathonMisiewicz force-pushed the ediis branch from e5fd818 to e8ffabf Compare September 27, 2021 17:30

jturney approved these changes Sep 28, 2021

View reviewed changes

JonathonMisiewicz added this to the Psi4 1.5 milestone Sep 29, 2021

PeterKraus approved these changes Oct 5, 2021

View reviewed changes

JonathonMisiewicz added 2 commits October 5, 2021 09:52

Move libdiis enums to strictly typed enum classes

91ccc62

Move SCF functions setting up DIIS to Py-side

9a553de

JonathonMisiewicz force-pushed the ediis branch from 374a34f to 9a553de Compare October 5, 2021 13:53

JonathonMisiewicz merged commit d94b7bb into psi4:master Oct 5, 2021

JonathonMisiewicz deleted the ediis branch October 5, 2021 15:52

JonathonMisiewicz mentioned this pull request Oct 19, 2021

Update scf_iterator.py #2327

Merged

2 tasks

-            storage_policy = getattr(core.DIISManager.StoragePolicy, "InCore" if self.scf_type() == "DIRECT" else "OnDisk")
+            if self.scf_type() == "DIRECT":
+                storage_policy = core.DIISManager.StoragePolicy.InCore
+            else:
+                storage_policy = core.DIISManager.StoragePolicy.OnDisk


		from psi4 import core

		def _RHF_orbital_gradient(self, save_fock: bool, max_diis_vectors: int) -> float:

Move SCF-DIIS to Python #2298

Move SCF-DIIS to Python #2298

Uh oh!

Conversation

JonathonMisiewicz commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Todos

Questions

Checklist

Status

Uh oh!

Uh oh!

loriab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loriab Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

loriab Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

loriab Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonathonMisiewicz commented Sep 27, 2021

Uh oh!

jturney left a comment

Choose a reason for hiding this comment

Uh oh!

PeterKraus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PeterKraus Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JonathonMisiewicz commented Sep 23, 2021 •

edited

Loading

JonathonMisiewicz commented Sep 24, 2021 •

edited

Loading