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
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,69 @@

@needs_lmp
class TestLammps:
@pytest.mark.parametrize("n_mols", [1, 2])
@pytest.mark.parametrize(
"mol",
[
"C",
"CC", # Adds a proper torsion term(s)
"C=O", # Simplest molecule with any improper torsion
"OC=O", # Simplest molecule with a multi-term torsion
"CCOC", # This hits t86, which has a non-1.0 idivf
"C1COC(=O)O1", # This adds an improper, i2
],
)
def test_to_lammps_single_mols_triclinic(
self,
mol: str,
sage_unconstrained: ForceField,
n_mols: int,
) -> None:
"""
Test that Interchange.to_openmm Interchange.to_lammps report sufficiently similar energies
in triclinic simulation boxes.
"""
mol = MoleculeWithConformer.from_smiles(mol)
mol.conformers[0] -= numpy.min(mol.conformers[0], axis=0)
top = Topology.from_molecules(n_mols * [mol])

box = numpy.zeros((3, 3), dtype=float) * unit.angstrom

box[0] = [51.34903463831951, 0, 0] * unit.angstrom
box[1] = [-0.03849979989403723, 50.9134404144338, 0] * unit.angstrom
box[2] = [-2.5907782992729538, 0.3720740833800747, 49.80705567557188] * unit.angstrom

top.box_vectors = box

if n_mols == 1:
positions = mol.conformers[0]
elif n_mols == 2:
positions = numpy.concatenate(
[mol.conformers[0], mol.conformers[0] + 1.5 * unit.nanometer],
)

interchange = Interchange.from_smirnoff(sage_unconstrained, top)
interchange.positions = positions
interchange.box = top.box_vectors

reference = get_openmm_energies(
interchange=interchange,
round_positions=3,
)

lmp_energies = get_lammps_energies(
interchange=interchange,
round_positions=3,
)

lmp_energies.compare(
reference,
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?

)

@pytest.mark.parametrize("n_mols", [1, 2])
@pytest.mark.parametrize(
"mol",
Expand Down
51 changes: 42 additions & 9 deletions openff/interchange/interop/lammps/export/export.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,54 @@ def to_lammps(interchange: Interchange, file_path: Path | str):
axis=0,
).magnitude
if interchange.box is None:
L_x, L_y, L_z = 100, 100, 100
ax, by, cz = 100, 100, 100
xy, xz, yz = 0, 0, 0
elif (interchange.box.m == numpy.diag(numpy.diagonal(interchange.box.m))).all():
L_x, L_y, L_z = numpy.diag(interchange.box.to(unit.angstrom).magnitude)
ax, by, cz = numpy.diag(interchange.box.to(unit.angstrom).magnitude)
xy, xz, yz = 0, 0, 0
else:
raise NotImplementedError(
"Interchange does not yet support exporting non-rectangular boxes to LAMMPS",
)
a = interchange.box.to(unit.angstrom).magnitude[0]
b = interchange.box.to(unit.angstrom).magnitude[1]
c = interchange.box.to(unit.angstrom).magnitude[2]
a_len = numpy.linalg.norm(a)
b_len = numpy.linalg.norm(b)
c_len = numpy.linalg.norm(c)

alpha = numpy.arccos(numpy.dot(b, c) / (b_len * c_len))
beta = numpy.arccos(numpy.dot(c, a) / (c_len * a_len))
gamma = numpy.arccos(numpy.dot(a, b) / (a_len * b_len))

ax = a_len
bx = b_len * numpy.cos(gamma)
by = b_len * numpy.sin(gamma)
cx = c_len * numpy.cos(beta)
cy = c_len * (numpy.cos(alpha) - numpy.cos(beta) * numpy.cos(gamma)) / numpy.sin(gamma)
cz = numpy.sqrt(c_len**2 - cx**2 - cy**2)

xy, xz, yz = bx, cx, cy

a_new = numpy.array([ax, 0, 0])
b_new = numpy.array([bx, by, 0])
c_new = numpy.array([cx, cy, cz])

check = numpy.allclose(a, a_new)
check *= numpy.allclose(b, b_new)
check *= numpy.allclose(c, c_new)

if not check:
import warnings

warnings.warn(
f"We are rotating the unit cell matrix. Before: a={a}, b={b}, c={c}, After: a={a_new}, b={b_new}, c={c_new}",
)

lmp_file.write(
f"{x_min:.10g} {x_min + L_x:.10g} xlo xhi\n"
f"{y_min:.10g} {y_min + L_y:.10g} ylo yhi\n"
f"{z_min:.10g} {z_min + L_z:.10g} zlo zhi\n",
f"{x_min:.10g} {x_min + ax:.10g} xlo xhi\n"
f"{y_min:.10g} {y_min + by:.10g} ylo yhi\n"
f"{z_min:.10g} {z_min + cz:.10g} zlo zhi\n",
)

lmp_file.write("0.0 0.0 0.0 xy xz yz\n")
lmp_file.write(f"{xy:.10g} {xz:.10g} {yz:.10g} xy xz yz\n")

lmp_file.write("\nMasses\n\n")

Expand Down
Loading