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

Integration of OpenMM #87

Merged
merged 23 commits into from
Apr 8, 2024
Merged

Integration of OpenMM #87

merged 23 commits into from
Apr 8, 2024

Conversation

JMorado
Copy link
Contributor

@JMorado JMorado commented Mar 8, 2024

This PR builds upon previous features introduced in PR #55, implementing the following changes:

  • Refactored the code to align its structure with that of md.py, aiming for uniformity, consistency, and improved readability
  • Added the possibility to use either a Langevin (NVT) or a Verlet (NVE) integrator
  • Implemented restart functionality
  • Added functionality to run the OpenMM MD in a temporary directory
  • Added option to select the OpenMM Platform
  • Added tests

@juraskov juraskov self-requested a review March 8, 2024 18:43
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 92.62295% with 18 lines in your changes missing coverage. Please review.

Project coverage is 68.26%. Comparing base (e809fe8) to head (fb54bd1).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
mlptrain/sampling/md_openmm.py 91.48% 12 Missing ⚠️
mlptrain/training/active.py 0.00% 5 Missing ⚠️
mlptrain/sampling/md.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   65.10%   68.26%   +3.16%     
==========================================
  Files          50       51       +1     
  Lines        4662     4847     +185     
==========================================
+ Hits         3035     3309     +274     
+ Misses       1627     1538      -89     
Flag Coverage Δ
python-3.9 63.31% <19.67%> (-1.57%) ⬇️
python-3.9-mace 68.10% <90.98%> (+3.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JMorado JMorado marked this pull request as ready for review April 3, 2024 13:02
Copy link
Member

@juraskov juraskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, thanks! I have only a minor comment regarding triggering a NotImplemented error. If it is too much work to implement, we can merge the code without it.

mlptrain/sampling/md_openmm.py Show resolved Hide resolved
@Hanwen1018
Copy link
Member

Dear all, I just have one comment, for now OpenMM only support MACE, correct me if I am wrong. Do we need to ensure that we have MACE before running OpenMM dynamics?

@JMorado
Copy link
Contributor Author

JMorado commented Apr 8, 2024

Hi @Hanwen1018,

Indeed, OpenMM-ML currently only supports MACE (and ANI, but that MLP is not part of mlp-train). MACE is installed when running ./install_mace.sh (as well as OpenMM and OpenMM-ML). If, for some unusual reason, the MACE package cannot be found, OpenMM-ML by default throws an ImportError:

https://github.com/openmm/openmm-ml/blob/main/openmmml/models/macepotential.py#L136-L143

Additionally, if you install the other environments (./install_gap.sh or ./install_ace.sh), you cannot run an OpenMM simulation:

https://github.com/JMorado/mlp-train/blob/openmm-features/mlptrain/sampling/md_openmm.py#L210-L220

Could you please clarify what you are suggesting?

@Hanwen1018
Copy link
Member

Hi @Hanwen1018,

Indeed, OpenMM-ML currently only supports MACE (and ANI, but that MLP is not part of mlp-train). MACE is installed when running ./install_mace.sh (as well as OpenMM and OpenMM-ML). If, for some unusual reason, the MACE package cannot be found, OpenMM-ML by default throws an ImportError:

https://github.com/openmm/openmm-ml/blob/main/openmmml/models/macepotential.py#L136-L143

Additionally, if you install the other environments (./install_gap.sh or ./install_ace.sh), you cannot run an OpenMM simulation:

https://github.com/JMorado/mlp-train/blob/openmm-features/mlptrain/sampling/md_openmm.py#L210-L220

Could you please clarify what you are suggesting?

Thank you for clarification. I want to add a checking step before run OpenMM. It is not about whether MACE is installed. I want to check it is the MACE potential used in the _create_openmm_simulation function. As in _create_openmm_simulation, the argument is mlp (belong to mlt.potentials._base.MLPotential), which can also be ACE or GAP, and will make issue.

@JMorado
Copy link
Contributor Author

JMorado commented Apr 8, 2024

I understand your point. In such a scenario, OpenMM-ML might throw an error that isn't very informative. I agree that an additional check would be beneficial. I'll implement that. Thanks!

@juraskov
Copy link
Member

juraskov commented Apr 8, 2024

This is probably part of the same issue. For example, when I activate the mace environment and specify ACE in AL, it will still train the ACE and the code will fail only after the training is finished - it is a similar issue as with the plumed check when the error was triggered after the training. The environments are quite similar and on my system, the ACE training finished even though I activated a different environment (which is not great tbh). So the MLP type check should be able to resolve also this.

@juraskov juraskov merged commit 3d1c9d8 into duartegroup:main Apr 8, 2024
3 checks passed
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.

4 participants