Skip to content
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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Scipy optimizer module #2

wants to merge 20 commits into from

Conversation

keceli
Copy link

@keceli keceli commented Dec 5, 2023

PR Type

  • Breaking change
  • Feature
  • Patch

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 satisfies simde.AOEnergy and the other simde.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

  • Task 1
  • Task 2

Copy link
Member

@ryanmrichard ryanmrichard left a 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.

.github/workflows/merge.yaml Outdated Show resolved Hide resolved
.github/workflows/pull_request.yaml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
Comment on lines 41 to 49
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
)
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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?

Copy link
Author

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.

Copy link
Member

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.

CMakeLists.txt Outdated Show resolved Hide resolved

class Scipy_optimizer_aoenergy(ModuleBase):
"""Optimization module based on scipy.optimize.minimize.
Satisfies simde.AOEnergy property type."""
Copy link
Member

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)

src/python/structurefinder/optimizer_modules.py Outdated Show resolved Hide resolved
tests/python/unit_tests/test_chemist_tools.py Outdated Show resolved Hide resolved
tests/python/unit_tests/test_optimizer_modules.py Outdated Show resolved Hide resolved
tests/python/unit_tests/test_structurefinder.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants