Skip to content

Added simple rounding error solution #1167

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
24 changes: 23 additions & 1 deletion openff/interchange/components/interchange.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ def to_gromacs(
hydrogen_mass: PositiveFloat = 1.007947,
monolithic: bool = True,
_merge_atom_types: bool = False,
_normalize_charges: bool = False,
):
"""
Export this Interchange object to GROMACS files.
Expand All @@ -353,6 +354,14 @@ def to_gromacs(
_merge_atom_types: bool, default = False
The flag to define behaviour of GROMACSWriter. If True, then similar atom types will be merged.
If False, each atom will have its own atom type.
_normalize_charges: bool, default = False
If True charges written to the topology are normalized per molecule to ensure that each molecule has a
charge which is exactly integer. Charges that are exactly 0 are not touched.
If False, charges are untouched.
_normalize_charges: bool, default = False
If True charges written to the topology are normalized per molecule to ensure that each molecule has a
charge which is exactly integer. Charges that are exactly 0 are not touched.
If False, charges are untouched.
Comment on lines +357 to +364
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_normalize_charges: bool, default = False
If True charges written to the topology are normalized per molecule to ensure that each molecule has a
charge which is exactly integer. Charges that are exactly 0 are not touched.
If False, charges are untouched.
_normalize_charges: bool, default = False
If True charges written to the topology are normalized per molecule to ensure that each molecule has a
charge which is exactly integer. Charges that are exactly 0 are not touched.
If False, charges are untouched.
_normalize_charges: bool, default = False
If True charges written to the topology are normalized per molecule to ensure that each molecule has a
charge which is exactly integer. Charges that are exactly 0 are not touched.
If False, charges are untouched.


Notes
-----
Expand All @@ -371,7 +380,11 @@ def to_gromacs(
gro_file=prefix + ".gro",
)

writer.to_top(monolithic=monolithic, _merge_atom_types=_merge_atom_types)
writer.to_top(
monolithic=monolithic,
_merge_atom_types=_merge_atom_types,
_normalize_charges=_normalize_charges,
)
writer.to_gro(decimal=decimal)

self.to_mdp(prefix + "_pointenergy.mdp")
Expand Down Expand Up @@ -407,6 +420,7 @@ def to_top(
hydrogen_mass: PositiveFloat = 1.007947,
monolithic: bool = True,
_merge_atom_types: bool = False,
_normalize_charges: bool = False,
):
"""
Export this Interchange to a GROMACS topology file.
Expand All @@ -425,12 +439,19 @@ def to_top(
_merge_atom_types: book, default=False
The flag to define behaviour of GROMACSWriter. If True, then similar atom types will be merged.
If False, each atom will have its own atom type.
_normalize_charges: bool, default = False
If True charges written to the topology are normalized per molecule to ensure that each molecule has a
charge which is exactly integer. Charges that are exactly 0 are not touched.
If False, charges are untouched.

Notes
-----
Molecule names in written files are not guaranteed to match the `Moleclue.name` attribute of the
molecules in the topology, especially if they are empty strings or not unique.

Charges written to the topology are normalized per molecule to ensure that each molecule has a charge which is
exactly integer. Charges that are exactly 0 are not touched.

Comment on lines +452 to +454
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not the default behavior and the opt-in would be private here, I think this information should only live in the argument's description

"""
from openff.interchange.interop.gromacs.export._export import GROMACSWriter
from openff.interchange.smirnoff._gromacs import _convert
Expand All @@ -442,6 +463,7 @@ def to_top(
).to_top(
monolithic=monolithic,
_merge_atom_types=_merge_atom_types,
_normalize_charges=_normalize_charges,
)

def to_gro(self, file_path: Path | str, decimal: int = 3):
Expand Down
56 changes: 52 additions & 4 deletions openff/interchange/interop/gromacs/export/_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@
top_file: pathlib.Path | str
gro_file: pathlib.Path | str

def to_top(self, monolithic: bool = True, _merge_atom_types: bool = False):
def to_top(
self,
monolithic: bool = True,
_merge_atom_types: bool = False,
_normalize_charges: bool = False,
):
"""Write a GROMACS topology file."""
with open(self.top_file, "w") as top:
self._write_defaults(top)
Expand All @@ -52,6 +57,7 @@
monolithic=monolithic,
mapping_to_reduced_atom_types=mapping_to_reduced_atom_types,
merge_atom_types=_merge_atom_types,
normalize_charges=_normalize_charges,
)

self._write_system(top)
Expand Down Expand Up @@ -173,6 +179,7 @@
monolithic: bool,
mapping_to_reduced_atom_types,
merge_atom_types: bool,
normalize_charges: bool,
):
for molecule_name, molecule_type in self.system.molecule_types.items():
# this string needs to be something that plays nicely in file paths
Expand All @@ -198,6 +205,7 @@
molecule_type,
mapping_to_reduced_atom_types,
merge_atom_types,
normalize_charges,
)
self._write_pairs(molecule_file, molecule_type)
self._write_bonds(molecule_file, molecule_type)
Expand All @@ -218,11 +226,51 @@
molecule_type,
mapping_to_reduced_atom_types,
merge_atom_types: bool,
normalize_charges: bool,
):
top.write("[ atoms ]\n")
top.write(";index, atom type, resnum, resname, name, cgnr, charge, mass\n")

for atom in molecule_type.atoms:
def _adjust_charges(charges: numpy.array, tolerance=8) -> numpy.array:
"""
Adjust charges so that written charge for a molecule type is integer.

We first get the initial charges and round them.
Note, that charges that are exactly equal to zero are never touched.
"""
# placeholder for output charges
rounded_charges = numpy.round(charges, tolerance)

Check warning on line 242 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L242

Added line #L242 was not covered by tests
# integer total charge
total_charge = numpy.round(numpy.sum(charges), 0)

Check warning on line 244 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L244

Added line #L244 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have formal charges of each atom at this point, right?

# non-zero charge indices
indices = numpy.where(rounded_charges != 0)[0]

Check warning on line 246 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L246

Added line #L246 was not covered by tests

def _rounding_error(_arr, _sum, _tolerance):
return numpy.round(numpy.sum(_arr) - _sum, _tolerance)

Check warning on line 249 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L248-L249

Added lines #L248 - L249 were not covered by tests
Comment on lines +248 to +249
Copy link
Member

Choose a reason for hiding this comment

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

(not blocking) My external perspective here is that needing to come back to this definition repeatedly as this is called in multiple places decreases readability substantially, and that the line length is about the same if numpy.round were called directly on each line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the point, but when initially I coded it without a function the code looked to heavy as for my liking. With the function it looked tidier, so I opted for having this function. I don't have any strong opinion here, so I am happy to get rid of the function if this is more in line with openff codestyle.


# Initial error due to rounding
rounding_error = _rounding_error(rounded_charges, total_charge, tolerance)

Check warning on line 252 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L252

Added line #L252 was not covered by tests

# We correct rounded_charges to achieve tolerance
if rounding_error != 0:
rounded_charges[indices] += numpy.round(

Check warning on line 256 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L255-L256

Added lines #L255 - L256 were not covered by tests
-rounding_error / len(indices),
tolerance,
)
diff = _rounding_error(rounded_charges, total_charge, tolerance)

Check warning on line 260 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L260

Added line #L260 was not covered by tests

# If there's a remainder of a charge after spreading everything out evenly,
# dump the remainder on the first nonzero atom.
rounded_charges[indices[0]] = numpy.round(

Check warning on line 264 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L264

Added line #L264 was not covered by tests
rounded_charges[indices[0]] - diff,
tolerance,
)
return rounded_charges

Check warning on line 268 in openff/interchange/interop/gromacs/export/_export.py

View check run for this annotation

Codecov / codecov/patch

openff/interchange/interop/gromacs/export/_export.py#L268

Added line #L268 was not covered by tests

charges_to_write = numpy.array([atom.charge.m for atom in molecule_type.atoms])
rounded_charges = _adjust_charges(charges_to_write) if normalize_charges else charges_to_write

for atom, charge in zip(molecule_type.atoms, rounded_charges):
if merge_atom_types:
top.write(
f"{atom.index:6d} "
Expand All @@ -231,7 +279,7 @@
f"{atom.residue_name:8s} "
f"{atom.name:6s}"
f"{atom.charge_group_number:6d}"
f"{atom.charge.m:20.12f}"
f"{charge:20.12f}"
f"{atom.mass.m:20.12f}\n",
)
else:
Expand All @@ -242,7 +290,7 @@
f"{atom.residue_name:8s} "
f"{atom.name:6s}"
f"{atom.charge_group_number:6d}"
f"{atom.charge.m:20.12f}"
f"{charge:20.12f}"
f"{atom.mass.m:20.12f}\n",
)

Expand Down