Skip to content

Fix triclinic minimum image and Ewald/PME real-space cutoff - #622

Open
abhijeetgangan wants to merge 5 commits into
mainfrom
fix/592-615-open-issues
Open

abhijeetgangan wants to merge 5 commits into
mainfrom
fix/592-615-open-issues

Conversation

@abhijeetgangan

@abhijeetgangan abhijeetgangan commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Closes #592, closes #615.

  • Triclinic minimum_image_displacement (minimum_image_displacement only valid for orthogonal cells #592). Rounding fractional coordinates only finds the minimum image in orthogonal cells.
    • Now also checks the neighboring periodic images and keeps the shortest. Ties keep the rounded image, so orthogonal results are unchanged.
    • Matches a brute-force search on ~3,000 random Niggli-reduced cells. main returns the wrong image for 13–27% of displacements in primitive FCC/BCC/HCP cells.
    • The issue's example is a new case in test_minimum_image_displacement.
  • Ewald/PME real-space truncation (EwaldModel and PMEModel truncate the real-space sum when cutoff is below the estimated Ewald cutoff #615). Option 2 from the issue: alpha is derived from cutoff and accuracy with nvalchemiops' public estimators.
    • Rattled 512-atom NaCl, default cutoff: energy error 3.2 eV → 1.4e-4 eV.
    • test_slab_matches_2d_lattice_sum had the same truncation, hidden by a loose tolerance. It now uses atol=1e-4, rtol=0.

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.

@lil-lon lil-lon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing #615! Overall, LGTM. I left minor suggestions and comments.

Comment thread tests/models/test_electrostatics.py
Comment thread tests/models/test_electrostatics.py
Comment thread torch_sim/models/electrostatics.py Outdated

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants