Skip to content

Conversation

@jla-gardner
Copy link
Contributor

@jla-gardner jla-gardner commented Apr 16, 2025

Summary

  • I found that the npt_langevin integrator appears to be broken in NPT bug #152
  • I think this is arising because the kinetic energy term currently being used is per-atom, whereas it needs to be per-structure
  • this fix now allows npt_langevin to reproduce the expected volume of a perfect-gas:
## Checklist

Before a pull request can be merged, the following items must be checked:

  • Doc strings have been added in the Google docstring format.
    Run ruff on your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

@cla-bot cla-bot bot added the cla-signed Contributor license agreement signed label Apr 16, 2025
@jla-gardner
Copy link
Contributor Author

tests/test_neighbors.py::test_neighbor_lists_time_and_memory is failing, but I am unsure if this is caused by my changes or is a known flaky test (#133 ?)

@janosh
Copy link
Collaborator

janosh commented Apr 16, 2025

thanks a lot for catching this and submitting a fix! 🙏

but I am unsure if this is caused by my changes or is a known flaky test (#133 ?)

most likely not caused by your changes. i just added that test yesterday. the thresholds still need to be tweaked based on CI variance. feel free to increase the threshold to match observed time and/or mem usage

@janosh janosh added fix Bug fix md Molecular dynamics labels Apr 16, 2025
Copy link
Collaborator

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks @jla-gardner 👍

@janosh janosh enabled auto-merge (squash) April 16, 2025 19:07
@janosh janosh merged commit 0b7a44a into TorchSim:main Apr 16, 2025
90 checks passed
@janosh janosh mentioned this pull request Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed Contributor license agreement signed fix Bug fix md Molecular dynamics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants