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

Added triclinic boxes for lammps #1095

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wutobias
Copy link

@wutobias wutobias commented Nov 6, 2024

Description

This PR adds functionality to use triclinc boxes in simulations with LAMMPS. The simulation box vectors are transformed to LAMMPS's restricted triclinix box format. A warning is raised when the box vectors before and after the transformation come out to be different. Users want to be aware of that in case they start with a pre-existing set of velocities. See also the LAMMPS documentation.

Checklist

  • Add tests
  • Lint
  • Update docstrings

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.58%. Comparing base (df397f4) to head (93384ed).

Additional details and impacted files

Comment on lines 75 to 78
tolerances={
"Nonbonded": 1 * unit.kilojoule_per_mole,
"Torsion": 0.02 * unit.kilojoule_per_mole,
},
Copy link
Member

Choose a reason for hiding this comment

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

I figure this is copy-pasted code from my own technical debt, but what happens when this is set to a lower threshold? I only care about the non-bonded energies here, since those would be what are wrong if the box vectors aren't written correctly

Suggested change
tolerances={
"Nonbonded": 1 * unit.kilojoule_per_mole,
"Torsion": 0.02 * unit.kilojoule_per_mole,
},
tolerances={
"Nonbonded": 0.01 * unit.kilojoule_per_mole,
"Torsion": 0.02 * unit.kilojoule_per_mole,
},

Copy link
Author

Choose a reason for hiding this comment

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

Right, this is copy-paste. I agree it would make sense to have tighter tolerance. I have tested this localy with 0.01 kJ/mol energy tolerance. Some of the triclinic box tests are failing, but so are they in the regular test test_to_lammps_single_mols with default rectangular boxes when using this low energy tolerance... Something else is going on I think.

Copy link
Member

Choose a reason for hiding this comment

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

Right, there are some funky things going on with how OpenFF's non-bonded settings get translated differently to LAMMPS than OpenMM, and that's out of scope here. Still, it'd be nice to have this value dropped a bit (i.e. even 0.1 would be great, if that passes tests) and a comment encoding what you just said.

Copy link
Author

Choose a reason for hiding this comment

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

0.4 kJ/mol is the lowest I can drop this tolerance before the test_to_lammps_single_mols (and hence the triclinic one) fails.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me, thanks

Copy link
Author

Choose a reason for hiding this comment

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

I modified the test so that it tests whether we get a higher energy deviation with the triclinic test than the orthogonal one. Is that maybe more to the point of what we actually want to test?

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

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

Are there any tools that will parse LAMMPS data files and read these box vectors? I know MDTraj can read some LAMMPS trajectories and InterMol can convert LAMMPS to GROMACS and maybe some other formats. Maybe the LAMMPS API itself is the easiest way to get this done.

What I'm looking for, because of the (upstream) non-bonded quirks, is a test that does the first half of what you've already done, but also writes out an openmm.System and then reads them back in and compares the box vectors.

If there isn't a quick way to do that, I'll just write a few files locally and see if I can visualize as a spot-check. But it'd be great to have it in CI right off the bat.

@wutobias
Copy link
Author

wutobias commented Nov 6, 2024

Are there any tools that will parse LAMMPS data files and read these box vectors? I know MDTraj can read some LAMMPS trajectories and InterMol can convert LAMMPS to GROMACS and maybe some other formats. Maybe the LAMMPS API itself is the easiest way to get this done.

I do not know. I am relatively new to LAMMPS, but I guess the box vectors should be stored in the trajectory (at least in NPT runs), which can be read by MDTraj.

What I'm looking for, because of the (upstream) non-bonded quirks, is a test that does the first half of what you've already done, but also writes out an openmm.System and then reads them back in and compares the box vectors.

Not 100% answering the question, but the box vectors should be identical to OpenMM box vectors. I used this code from openmm as a starting point.

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.

2 participants