-
Notifications
You must be signed in to change notification settings - Fork 0
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
Scipy optimizer module #2
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.
Overall looks pretty good.
CMakeLists.txt
Outdated
cpp_find_or_build_dependency( | ||
nwchemex | ||
URL github.com/NWChemEx-Project/NWChemEx | ||
PRIVATE TRUE | ||
VERSION ${NWX_SIMDE_VERSION} | ||
BUILD_TARGET nwchemex | ||
FIND_TARGET nwx::nwchemex | ||
CMAKE_ARGS BUILD_TESTING=OFF | ||
) |
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.
Should be SimDE not NWX.
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 want to be able use NWChemEx driver modules to get CCSD, MP2 energies once available.
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.
It should still be SimDE. Requiring the user to build NWChemEx adds a lot of build overhead and makes your plugin dependent on literally the entire stack.
That said, building of other plugins is useful for testing, but should be limited to integration testing only. I'll warn you that, to my knowledge, no one has yet tried to use NWChemEx as a dependency for integration testing, so there may be hiccups.
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.
It should still be SimDE. Requiring the user to build NWChemEx adds a lot of build overhead and makes your plugin dependent on literally the entire stack.
Could you clarify? I assume the main role of this repo is to provide geometry optimization, which would require energies and gradients. NWChemEx is the repo that provides access to all plugins that can give those properties. How would I access them if I just use SimDE?
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 understand that you mean those dependencies would only be required during testing, but I think a user of this repo would expect to access them directly.
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 NWChemEx repo is the one that pulls together all of the functionalities. It's not meant to be used as a dependency of our plugins. This repo will more likely be a dependency of NWChemEx, actually. That way, when someone builds NWChemEx, they will get the Integrals, SCF, ChemCache, the geometry optimization modules here, and any other plugins we associate with it down the line.
|
||
class Scipy_optimizer_aoenergy(ModuleBase): | ||
"""Optimization module based on scipy.optimize.minimize. | ||
Satisfies simde.AOEnergy property type.""" |
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 feel like we should be creating a new property type for optimizers. Something like Optimize<X,Y...>
which optimizes X
by varying Y...
. So Optimize<AOEnergy, Nuclei>
would be the property type. I'd set it up so the inputs are the union of the inputs to X
as well as the Y...
objects and the returns are the return of X
and the optimized values of the Y
. So the API of the module would essentially be energy, optimized_nuclei (AOs, ChemicalSystem, Nuclei of chemical system)
…er into optimizer
PR Type
Brief Description
This is the initial PR to add a module for optimization. It is based on single point energies and uses
scipy.optimize.minimize
function. There are two similar modules, one satisfiessimde.AOEnergy
and the othersimde.Energy
.Not In Scope
Optimizer modules that make use of the gradient and geomeTRIC, PyBerny, and ASE will follow up this one.
PR Checklist
TODOs