Added simple rounding error solution #1167
Conversation
There was a problem hiding this comment.
Some questions about the implementation here. I think also it could be commented a bit better - it took me a while to grok why some intermediates were getting rounded and others not just by reading the code. This new behavior should also be described in the docs.
| def _rounding_error(_arr, _sum, _tolerance): | ||
| return numpy.round(numpy.sum(_arr) - _sum, _tolerance) |
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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.
| rounded_charges += numpy.round( | ||
| -rounding_error / len(charges_to_write), | ||
| tolerance, | ||
| ) |
There was a problem hiding this comment.
(blocking) Wouldn't 0 charges be affected here, contradicting the "never adjust 0 charges" comment below?
There was a problem hiding this comment.
You are right, should be resolved now.
| tolerance, | ||
| ) | ||
| diff = _rounding_error(rounded_charges, total_charge, tolerance) | ||
| while diff != 0: |
There was a problem hiding this comment.
(not blocking) Under what circumstances would this while loop iterate more than once? I can't think of any.
There was a problem hiding this comment.
I agree, since tolerance is exactly the same, then one round should be enough.
…es to interchange `to_top` and `to_gromacs` methods' docstrings
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1167 +/- ##
==========================================
- Coverage 93.38% 93.22% -0.17%
==========================================
Files 70 70
Lines 6036 6050 +14
==========================================
+ Hits 5637 5640 +3
- Misses 399 410 +11 ☔ View full report in Codecov by Sentry. |
jameseastwood
requested review from
mattwthompson and
j-wags
and removed request for
j-wags
There was a problem hiding this comment.
I see what you're after here and think it's probably a net improvement. I still think most of the cases that would be fixed by this ought to be handled earlier in a workflow, but I see some value in safeguarding against small errors propagating through to GROMACS files on an opt-in basis.
| 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. | ||
|
|
There was a problem hiding this comment.
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
| _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. |
There was a problem hiding this comment.
| _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. |
| # placeholder for output charges | ||
| rounded_charges = numpy.round(charges, tolerance) | ||
| # integer total charge | ||
| total_charge = numpy.round(numpy.sum(charges), 0) |
There was a problem hiding this comment.
I don't think we have formal charges of each atom at this point, right?
|
@pbuslaev what do you think about trying to fix this problem earlier by "fixing" charges during |
Description
This is a proposed solution for #1166 on the GROMACS writer side.
If this approach is approved I can add tests/parameters
Checklist