-
Notifications
You must be signed in to change notification settings - Fork 440
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
PyDIIS #2369
PyDIIS #2369
Conversation
0df7d40
to
bebfd36
Compare
Code updated to use a C++ class as a wrapper to the Python interface. The result is _very _ similar to the old interface, but I was able to eliminate redundant arguments. There's more ripping out of old |
b2904b7
to
48f9c01
Compare
Should the new 'psi4/driver/procrouting/scf_proc/diis.py' file be moved to a more common directory? New developers may think it shouldn't be used in new code if it lives in the SCF directory. |
9e7eaab
to
e36cc53
Compare
I've run some timing tests. First, with DLPNO-MP2 on 1644 basis functions, the same job in triplicate:
And now DF-DCT with 384 basis functions, this time in duplicate:
I can run additional tests if requested, but the PR has negligible impact on performance time, as expected. The PR is ready for more serious review. |
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.
I looked through this before and thought it looked good then!
After memory profiling, I've discovered that this PR improves memory efficiency. For my 384 basis function DF-DCT system, memory requirements plummet from 14.0 GB to 7.8 GB. I've run some tests on a single benzene system instead of a two benzene system, and I understand the behavior: This isn't due to shifting to Python, but shifting how we clear some large arrays. The C-DIIS code uses The large change in memory is most likely because C-DIIS was keeping the allocation space for multiple T2 tensors. |
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! This PR will serve as a nice model for moving code dealing with logic to the python layer without sacrificing performance.
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.
Nice! 👍
I'll give another 48 hours for any devs to say if they want more time to review this, but otherwise, I'll merge this in on Wednesday. |
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.
this became quite a big code shift! much easier to read now.
@@ -35,6 +35,8 @@ | |||
#include "psi4/libmints/vector3.h" | |||
#include "psi4/psi4-dec.h" | |||
|
|||
#include "psi4/pybind11.h" |
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.
I don't see a way around it, but this adds to the very limited (detci & mrcc) use of pb11 deeper than psi4/src/*
, so keep this in mind if compile-time increases.
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 parts of the SCF code that need this should be Py-side anyways, but that's outside of scope. My laptop is super slow for compile time, but the hit wasn't as bad as I feared.
include_directories( | ||
${Python_INCLUDE_DIRS} | ||
) | ||
target_link_libraries(core | ||
PRIVATE | ||
Python::Module | ||
) |
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.
did this come about b/c the build couldn't find Python.h at some point? using raw variables and general scope (e.g., ${Python_INCLUDE_DIRS}
) instead of targets (e.g., Python::Module
and core
) isn't the best practice. I've looked around https://pybind11.readthedocs.io/en/stable/compiling.html#advanced-interface-library-targets some and not fastened upon an ideal solution. Python::Embed
is what we want to avoid iirc because that explicitly links core to libpython.so . a target_include_directories(core SCOPE ${Python_INCLUDE_DIRS})
would be preferable if it could be here only or wherever the pybind11.h appeared only. This is rambling, so the main question is what was the error that this solved?
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.
Exactly.
In file included from /Users/jonathonmisiewicz/psi4/psi4/src/psi4/scfgrad/response.cc:50:
In file included from /Users/jonathonmisiewicz/psi4/psi4/src/psi4/libscf_solver/rhf.h:33:
In file included from /Users/jonathonmisiewicz/psi4/psi4/src/psi4/libscf_solver/hf.h:38:
In file included from /Users/jonathonmisiewicz/psi4/psi4/include/psi4/pybind11.h:38:
In file included from /Users/jonathonmisiewicz/miniconda3/envs/p4dev/include/pybind11/pybind11.h:44:
In file included from /Users/jonathonmisiewicz/miniconda3/envs/p4dev/include/pybind11/attr.h:13:
In file included from /Users/jonathonmisiewicz/miniconda3/envs/p4dev/include/pybind11/cast.h:13:
In file included from /Users/jonathonmisiewicz/miniconda3/envs/p4dev/include/pybind11/pytypes.h:12:
/Users/jonathonmisiewicz/miniconda3/envs/p4dev/include/pybind11/detail/common.h:128:10: fatal error: 'Python.h' file not found
#include <Python.h>
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.
You could try the below in libscf_solver/CMakeLists.txt. Otherwise, merge as it, and I can play with it later.
target_include_directories(scf_solver_or_whatever
PRIVATE
${Python_INCLUDE_DIRS}
)
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.
I played with it myself and found that I needed to include it not only in libdiis
and libscf_solver
(both reasonable) but anything that imports libscf_solver/hf.h
. That would be a nightmare for downstream. I'll leave this one for you to play with later.
y.axpy(alpha, x) | ||
elif isinstance(y, (core.dpdbuf4, core.dpdfile2)): | ||
y.axpy_matrix(x, alpha) |
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.
whoa, both (alpha, x)
and (x, alpha)
. typical development at different times :-)
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.
libmints
wants (alpha, x)
and libdpd
wants (x, alpha)
. But this particular function is one I created to pass libmints
objects to libdpd
. As the functions lived in libdpd
, I opted to have them use libdpd
convention, but I can go the other way.
Description
This is a preliminary PR for moving DIIS Python-side, submitted for general review over PsiCon and to see if Windows hates this.
This is going to expedite EDIIS/ADIIS, and is a prototype for moving more code to the Python layer.
Todos
Questions
Checklist
Status