Skip to content

Nlists for inference#257

Merged
chrisiacovella merged 24 commits intochoderalab:mainfrom
chrisiacovella:nlists_for_inference
Oct 2, 2024
Merged

Nlists for inference#257
chrisiacovella merged 24 commits intochoderalab:mainfrom
chrisiacovella:nlists_for_inference

Conversation

@chrisiacovella
Copy link
Copy Markdown
Member

@chrisiacovella chrisiacovella commented Sep 26, 2024

Description

This changes how we handle neighbor lists for inference. This includes a more efficient brute force routine and a verlet scheme (although based still on an N^2 building step). These changes will make it easier to support other libraries.

I included a version of the verlet scheme that builds the neighbor list using NNPOps; however I commented it out right now to avoid adding that dependency as it does not seem to provide any improvements in speed/memory utilization after changing around the approach in the pure pytorch versions. I think this can be made faster if we can push more of the required code pieces to the cuda kernel.

box vectors are now stored in the NNPInput class to properly support periodic system.

Todo

  • update inference neighbor list tests to point to new routines in neighbors.py.

Status

  • Ready to go

@chrisiacovella
Copy link
Copy Markdown
Member Author

I think this is ready to go. I'm going to hold off on adding back in the multiple cutoffs and do that in a separate PR to keep this smaller and more focused.

Comment thread modelforge/dataset/dataset.py Outdated
Comment thread modelforge/dataset/dataset.py
Comment thread modelforge/potential/models.py Outdated
Comment thread modelforge/potential/neighbors.py
Comment thread modelforge/potential/neighbors.py
Comment thread modelforge/potential/neighbors.py Outdated
Comment thread modelforge/potential/neighbors.py Outdated
@wiederm wiederm self-requested a review October 1, 2024 05:58
Copy link
Copy Markdown
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.

That looks great!

Comment thread modelforge/potential/neighbors.py
@chrisiacovella
Copy link
Copy Markdown
Member Author

I added in a basic doc page about the neighbor lists. We will still need to think about the API for controlling things. We might ultimately need a wrapper inside the setup model that allows us to change the neighbor list implementation, instead of it being something that we build the model with (if I built with it, we need to build different versions of the model with different neighbor lists). But we can address that as we work on the openmm interface.

@chrisiacovella chrisiacovella merged commit 9727cc6 into choderalab:main Oct 2, 2024
@chrisiacovella chrisiacovella deleted the nlists_for_inference branch August 27, 2025 18:27
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.

2 participants