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

Update neighboring #11

Merged
merged 6 commits into from
Dec 28, 2023

Conversation

chrisiacovella
Copy link
Member

@chrisiacovella chrisiacovella commented Dec 26, 2023

Description

This relates to issue #10 . The pair and neighborlist classes now also return the index of the pairs. This information was available, just not returned during the calculate function (as it wasn't necessary for initial testing/dev of a monatomic system).

Note: I tried a version of the pairlist that basically just removed non-interacting pairs to reduce the number of atoms used in the force calculation (current code will have all possible pairs, and the energy is just multiplied by either 0 or 1, i.e., based on the mask). Similar to the neighborlist, there was a n_max_neighbors variable that would pad things out. For the range of system sizes tested these extra steps took more time than extra LJ computations. I suspect for an NNP, this step to reduce extra interactions will likely speed things up given the cost of the NNP. This will be added in a follow-up commit to this PR; if it is ultimately not useful we can remove later. I'm not sure if this should be a new class or just part of the same class

Todos

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

  • Make pair information accessible to allow for mixed systems
  • Improve doc strings

Will move the other action items to a separate PR as they are not actually necessary here and not merging will potential block progress in other items.

  • Add in code that reduces and pads the Pairlist
  • Add in example using the minimize function
  • Create mixed LJ system to implement logic related to converting topology information into the most usable form.

Status

  • Ready to go

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2023

Codecov Report

Merging #11 (20f8c9a) into main (2133efc) will decrease coverage by 0.24%.
The diff coverage is 76.92%.

Additional details and impacted files

@wiederm wiederm added the enhancement New feature or request label Dec 27, 2023
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 improvments! This is ready to be merged.

chiron/neighbors.py Show resolved Hide resolved
chiron/neighbors.py Show resolved Hide resolved
@chrisiacovella chrisiacovella merged commit 58db5df into choderalab:main Dec 28, 2023
3 checks passed
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