Skip to content

Conversation

yu9824
Copy link

@yu9824 yu9824 commented Oct 6, 2025

Summary

This PR includes three main updates to improve stability and correctness in LAMMPS-based simulations.

  1. Fix overestimated pressure in NPT ensemble
    The virial term was incorrectly signed, leading to systematically higher pressure evaluations in NPT simulations.
    This PR corrects the sign convention and adds a dedicated NPT test to verify the fix.

  2. Wrap atomic positions to the unit cell
    Although this normally has no effect on computed energies or forces, large atomic displacements occasionally caused PyTorch warnings due to numerical instability.
    Wrapping positions mitigates such issues and improves numerical robustness.

  3. Add charge and spin configuration support
    Extended input options to allow specifying total charge and spin states, which is useful for systems that require explicit electronic parameters.


Implementation Details

  • Fixed virial sign in pressure computation for NPT.
  • Added unit-cell wrapping to prevent out-of-bound coordinates.
  • Added a new NPT test case to confirm stability and correct pressure behavior.

Testing

  • Verified that the corrected virial computation reproduces expected pressures for test systems.
  • Added an NPT test to validate the fix under variable-cell conditions.

Motivation

This patch improves both physical correctness (pressure evaluation) and numerical stability (coordinate wrapping).

Copy link

meta-cla bot commented Oct 6, 2025

Hi @yu9824!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

Copy link

meta-cla bot commented Oct 6, 2025

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla bot added the cla signed label Oct 6, 2025
@lbluque lbluque added bug Something isn't working patch Patch version release labels Oct 6, 2025
@yu9824
Copy link
Author

yu9824 commented Oct 7, 2025

It seems that the CI test stopped due to a temporary environment error (EADDRINUSE in PyTorch distributed initialization), rather than an issue with this PR’s code.
Could you please re-run the test pipeline?

@rayg1234
Copy link
Contributor

rayg1234 commented Oct 8, 2025

It seems that the CI test stopped due to a temporary environment error (EADDRINUSE in PyTorch distributed initialization), rather than an issue with this PR’s code. Could you please re-run the test pipeline?

Thanks for making this PR and contributing!, the CI tests all look good, we'll review it soon and looks mostly good!

@rayg1234 rayg1234 self-requested a review October 10, 2025 23:44
# virials need to be in this order: xx, yy, zz, xy, xz, yz. https://docs.lammps.org/Library_utility.html#_CPPv437lammps_fix_external_set_virial_globalPvPKcPd
virial_arr = [v[0], v[4], v[8], v[1], v[2], v[5]]
lmp.fix_external_set_virial_global(FIX_EXT_ID, virial_arr)
def wrap_to_unit_cell(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ASE's wrap positions instead since we already use this function in our code?

pos = wrap_positions(pos, cell, pbc=pbc, eps=0)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we can use ASE wrap_positions inside this function. Relatedly, it would be useful to add some simple lammps box to ASE unit cell and back tests to ensure consistency between lammps and ASE representation.

# stress is defined as virial/volume in lammps
assert "stress" in results, "stress must be in results to compute virial"
volume = torch.det(cell).abs().item()
v = (-results["stress"].detach().cpu() * volume)[0].tolist()
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this! The ASE v lammps test looks good. Can you update the comment as well: virial = - stress * volume? (in the lammps doc they didn't add the negative)

ase_kinetic, ase_pot = run_ase_npt()
lammps_kinetic, lammps_pot = run_lammps("tests/lammps/lammps_npt.file")
assert np.isclose(ase_kinetic, lammps_kinetic, atol=1.0)
assert np.isclose(ase_pot, lammps_pot, atol=1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a check for pressure as well since this is a NPT sim?

fix 1 all npt temp 300.0 300.0 0.1 iso 0.0 0.0 1.0
thermo_style custom step temp pe ke etotal press vol
thermo 1
run 300
Copy link
Contributor

Choose a reason for hiding this comment

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

can we run this for 100 steps instead to reduce the test time?

@rayg1234
Copy link
Contributor

It seems that the CI test stopped due to a temporary environment error (EADDRINUSE in PyTorch distributed initialization), rather than an issue with this PR’s code. Could you please re-run the test pipeline?

Thanks for making this PR and contributing!, the CI tests all look good, we'll review it soon and looks mostly good!

Looks great, mostly just cosmetic comments, thanks for the adding the NPT example test case, very useful!

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

Labels

bug Something isn't working cla signed patch Patch version release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants