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.

Sign up for GitHub

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
pbuslaev wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Added simple rounding error solution #1167

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dda5d58
Added simple rounding error solution
pbuslaev Feb 12, 2025
2256fb8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 12, 2025
e9bfd28
Updated and commented out the logics for charge adjustment. Added Not…
pbuslaev Mar 5, 2025
d6cbe74
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 5, 2025
84922e3
Made charge normalization optional
pbuslaev Mar 5, 2025
9fb2794
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 5, 2025
a8223e6
Fixed typo
pbuslaev Mar 5, 2025
dd48e2e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 5, 2025
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
37 changes: 34 additions & 3 deletions openff/interchange/interop/gromacs/export/_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,38 @@ def _write_atoms(
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 0.
"""
rounded_charges = numpy.round(charges, tolerance)
total_charge = numpy.round(numpy.sum(charges_to_write), 0)

def _rounding_error(_arr, _sum, _tolerance):
return numpy.round(numpy.sum(_arr) - _sum, _tolerance)
Comment on lines +248 to +249
Copy link
Copy Markdown
Member

@j-wags j-wags Mar 3, 2025

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
Copy Markdown
Contributor Author

@pbuslaev pbuslaev Mar 5, 2025

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.


rounding_error = _rounding_error(rounded_charges, total_charge, tolerance)

if rounding_error != 0:
rounded_charges += numpy.round(
-rounding_error / len(charges_to_write),
tolerance,
)
Copy link
Copy Markdown
Member

@j-wags j-wags Mar 3, 2025

Choose a reason for hiding this comment

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

(blocking) Wouldn't 0 charges be affected here, contradicting the "never adjust 0 charges" comment below?

Copy link
Copy Markdown
Contributor Author

@pbuslaev pbuslaev Mar 5, 2025

Choose a reason for hiding this comment

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

You are right, should be resolved now.

diff = _rounding_error(rounded_charges, total_charge, tolerance)
while diff != 0:
Copy link
Copy Markdown
Member

@j-wags j-wags Mar 3, 2025

Choose a reason for hiding this comment

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

(not blocking) Under what circumstances would this while loop iterate more than once? I can't think of any.

Copy link
Copy Markdown
Contributor Author

@pbuslaev pbuslaev Mar 5, 2025

Choose a reason for hiding this comment

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

I agree, since tolerance is exactly the same, then one round should be enough.

# Never adjust 0 charges
indices = numpy.where(rounded_charges != 0)[0]
rounded_charges[indices[0]] = numpy.round(
Comment thread
pbuslaev marked this conversation as resolved.
Outdated
rounded_charges[indices[0]] - diff,
tolerance,
)
diff = _rounding_error(rounded_charges, total_charge, tolerance)
return rounded_charges

charges_to_write = numpy.array([atom.charge.m for atom in molecule_type.atoms])
rounded_charges = _adjust_charges(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 +262,7 @@ def _write_atoms(
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 +273,7 @@ def _write_atoms(
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