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

Openmm integration #299

Merged
merged 24 commits into from
Nov 13, 2024
Merged

Conversation

chrisiacovella
Copy link
Member

@chrisiacovella chrisiacovella commented Oct 25, 2024

Pull Request Summary

This PR revamps the neighbor list (allowing us to change the strategy for a JITTED torchscript potential) and provide a wrapper allowing us to run this with openmm -torch.

Key changes

Notable points that this PR has either accomplished or will accomplish.

  • Neighborlists include "only_unique_pairs" as a buffer
  • Neighborlists for inference combined into a single class, with the ability to set the strategy either at run time or via function call (allowing a single model file)
  • wrapper that follows the structure of TorchForce for openmm
  • This creates a submodule for openmm integration (via modelforge.openmm)
  • Tests for modelforge-openmm wrapping/running openmm

Associated Issue(s)

Pull Request Checklist

  • Issue(s) raised/addressed and linked
  • Includes appropriate unit test(s)
  • Appropriate docstring(s) added/updated
  • Appropriate .rst doc file(s) added/updated
  • PR is ready for review

@chrisiacovella chrisiacovella marked this pull request as draft October 25, 2024 18:22
@chrisiacovella
Copy link
Member Author

Note this is currently failing due to reading in a checkpoint file; I added the unique_pairs variable to the buffers in the neighbor list, as that seemed to be a very import thing to set. However, it is also set up stream (and encoded likely in another location). So I wonder if this is necessary.

# Conflicts:
#	modelforge/potential/potential.py
…pful (to avoid reinitializing tensors) actually is problematic. I needed to add a separate forward call that takes the all the "slots" and passes them, creating a new NNPInput within the forward call.
@chrisiacovella
Copy link
Member Author

The switch from a named tuple to a class for NNPInput actually presents some problems when we go to use the torch script models (i.e., passing input after calling torch.jit.script). Torch script doesn't really handle getting a class as input well. it sees the the NNPInput class as a different type due to being different Python compilation unit (i.e., compiled using modelforge and then compiled by the wrapper class).

NNPInput is more of a convenience thing (has validation, sets default shapes, etc.), but for inference we just need to pass tensors directly.

…re on the same device (since neighborlists are initialized before we know what the target device is).
@wiederm
Copy link
Member

wiederm commented Oct 31, 2024

@chrisiacovella , NNPInput is torch scripted right now; we could also set a flag not to jit that class (there is no benefit to it at that point).
An alternative is to pass input as keyword arguments, which should also be fine and need minimal changes.

…y for neighborlists. added decorators to make strategy setting routines accessible for neighborlists during inference. xfailed checkpoint testing. will need to regenerate file.
@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 36.95652% with 58 lines in your changes missing coverage. Please review.

Project coverage is 34.22%. Comparing base (426171a) to head (95fae2e).

❗ There is a different number of reports uploaded between BASE (426171a) and HEAD (95fae2e). Click for more details.

HEAD has 7 uploads less than BASE
Flag BASE (426171a) HEAD (95fae2e)
unittests 8 1
Additional details and impacted files

@chrisiacovella chrisiacovella added the enhancement New feature or request label Nov 5, 2024
@chrisiacovella chrisiacovella marked this pull request as ready for review November 5, 2024 17:12
Copy link
Member

@wiederm wiederm left a comment

Choose a reason for hiding this comment

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

Great work!

I'd love to better understand the control flow from input to property with this module, I left a comment.

modelforge/potential/neighbors.py Show resolved Hide resolved
modelforge/potential/potential.py Show resolved Hide resolved
Copy link
Member

@wiederm wiederm left a comment

Choose a reason for hiding this comment

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

Great work!

@chrisiacovella chrisiacovella merged commit dbae94c into choderalab:main Nov 13, 2024
10 checks passed
chrisiacovella added a commit to chrisiacovella/modelforge that referenced this pull request Nov 13, 2024
… load_inference_model_from_checkpoint now accepts an optional argument "only_unique_pairs" to set this when reading a checkpoint file, for any models trained prior to PR choderalab#299 (openmm integration).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants