Skip to content

NVE energy conservation in neighbor rebuilds - #153

Open
laserkelvin wants to merge 4 commits into
NVIDIA:0.2.0-rcfrom
laserkelvin:151-nve-conservation
Open

NVE energy conservation in neighbor rebuilds#153
laserkelvin wants to merge 4 commits into
NVIDIA:0.2.0-rcfrom
laserkelvin:151-nve-conservation

Conversation

@laserkelvin

Copy link
Copy Markdown
Collaborator

ALCHEMI Toolkit Pull Request

Description

This PR closes #151, where NVE simulations with wrapped periodic boundaries eventually invalidate neighbor list caches once atoms are wrapped, but continue to compute the energy with the wrong set of periodic shifts.

The fix changes the rebuild check to use raw cartesian displacements instead

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Performance improvement
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or infrastructure change

Related Issues

Changes Made

Testing

  • Unit tests pass locally (make pytest)
  • Linting passes (make lint)
  • New tests added for new functionality meets coverage expectations?

Checklist

  • I have read and understand the Contributing Guidelines
  • I have updated the CHANGELOG.md
  • I have performed a self-review of my code
  • I have added docstrings to new functions/classes
  • I have updated the documentation (if applicable)

Additional Notes

Tip

This repository uses Greptile, an AI code review service, to help conduct
pull request reviews. We encourage contributors to read and consider suggestions
made by Greptile, but note that human maintainers will provide the necessary
reviews for merging: Greptile's comments are not a qualitative judgement
of your code, nor is it an indication that the PR will be accepted/rejected.
We encourage the use of emoji reactions to Greptile comments, depending on
their usefulness and accuracy.

Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@laserkelvin

Copy link
Copy Markdown
Collaborator Author

/ok to test 83b991e

@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR switches neighbor-list rebuild detection to raw Cartesian displacement so periodic wrapping invalidates cached shifts.

  • Removes the local in-place rebuild custom operation and its distributed passthrough registration.
  • Uses the upstream nvalchemiops rebuild detector directly.
  • Adds periodic-fold and long-running NVE energy-conservation regression coverage.

Important Files Changed

Filename Overview
nvalchemi/hooks/neighbor_list.py Replaces minimum-image rebuild detection with upstream raw-Cartesian displacement checks so wrapped coordinates refresh cached periodic shifts.
nvalchemi/dynamics/_ops/neighbor_list_rebuild.py Removes the superseded local in-place neighbor-list rebuild custom operation.
nvalchemi/distributed/shard_wrappers.py Removes passthrough registration for the deleted custom rebuild operation.
test/hooks/test_neighbor_list_hook.py Adds focused coverage proving that periodic folding triggers a rebuild and produces a valid cached shift.
test/hooks/test_neighbor_list_nve_conservation.py Adds a deterministic slow regression test comparing NVE energy drift with skin caching against full rebuilds.

Reviews (1): Last reviewed commit: "refactor(hooks): simplify rebuild import" | Re-trigger Greptile

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.

1 participant