Conversation
ryanmrichard
left a comment
There was a problem hiding this comment.
Overall looks pretty good.
| 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.
I want to be able use NWChemEx driver modules to get CCSD, MP2 energies once available.
There was a problem hiding this comment.
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.
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.
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.
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.
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.minimizefunction. There are two similar modules, one satisfiessimde.AOEnergyand 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