-
Notifications
You must be signed in to change notification settings - Fork 74
Neighborlist api to handle batch #348
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
base: main
Are you sure you want to change the base?
Conversation
orionarcher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! The API makes sense to me. It would be helpful to have a specific example though. How about creating a native neighborlist that handles this API and include it in the PR? As an intermediate solution, that could be as simple as pushing the loop inside the vesin_nl_ts to fake batching.
|
Yeah, probably we would wrap the non batched neighborlist with this api. I think making a We can have a Looking at the models which use this we could have the following:
This will also make model integration simple. |
orionarcher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have a DEFAULT_NL which uses vesin if it is installed or the fallback option.
Looking at the models which use this we could have the following:
edge_idx, unit_shifts, cartesian_shifts = neighborlist_fn(state or state info)
This will also make model integration simple.
Do you want to do these things before merging in? Maybe this PR could clean up neighbors.py a bit before merging in. A couple things come to mind:
- I'd prefer to not have two types of neighborlists with different APIs where one will fail and the other will work.
- I like the DEFAULT_NL idea but let's make sure that follows the batched API and get rid of/make private the vesin_nl_ts and/or wrap it in a for loop so it conforms with the API
- What is the linked cell algorithm? Is that just a batched API or is it something else entirely?
|
Yes, I will add this to this PR. I think I would just wrap the neighborlist implementations even when they do not have explicit batch handling (just need to for loop here) as a generic function which returns the above. Both the torch_nl versions api handle Performance wise, I am removing the |
|
Note that #347 makes some changes to the neighborlist that cover some of what we discuss here. |
|
Yeah, will keep those in mind when I have the API changes |
… into ag/batch_nl_api
Summary
Checklist
Before a pull request can be merged, the following items must be checked: