draft: Add atom type merging for Interchange.to_gromacs() #962
Conversation
|
This is a draft of the solution, if anyone can suggest me with optimal tests for it, or other considerations to keep in mind, I will be happy to work on those |
There was a problem hiding this comment.
I think this is a good start, it's assuring that no existing tests seem to be passing. My primary worry is something subtly changing with the energies, perhaps in the relationship between nonbonded parameters and bonded parameter housekeeping, but maybe that's not founded.
To keep this moving, we'll definitely want some tests with the flag turned on and off (included in the test suite) for a variety of chemistries and some quick checks that this actually fixes the memory explosion (one-off analysis).
Co-authored-by: Matt Thompson <[email protected]>
Co-authored-by: Matt Thompson <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Matt Thompson <[email protected]>
for more information, see https://pre-commit.ci
Added `merge_atom_types` flag to `to_top` method. Changed variables names
for more information, see https://pre-commit.ci
… `Interchange.to_gromacs()`
for more information, see https://pre-commit.ci
|
So now I added a flag |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
openff/interchange/_tests/unit_tests/interop/gromacs/export/test_export.py
Outdated
Show resolved
Hide resolved
…st_export.py Co-authored-by: Matt Thompson <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
It seems that now all tests that do not require OpenEye are passing. I can add a couple of other tests (checking that atom type merging actually occurs) |
| @@ -60,31 +64,100 @@ def _write_defaults(self, top): | |||
| f"{self.system.coul_14:8.6f}\n\n", | |||
| ) | |||
|
|
|||
| def _write_atomtypes(self, top): | |||
| def _write_atomtypes(self, top, merge_atom_types: bool) -> dict[str, str]: | |||
There was a problem hiding this comment.
It's great to see work toward the "atom type explosion" problem that's plagued us for years. But given my recollection of the severity of the parmed bug that arose from seemingly innocuous atom type merging, it may make sense to have this be _merge_atom_types(with the leading underscore) or require INTERCHANGE_EXPERIMENTAL=1 for a few releases to communicate that it's new/experimental. Once early adopters have run with it for a while we can remove the underscore and make it a public method.
There was a problem hiding this comment.
That is fair, I added underscore to all public methods
|
Thanks @pbuslaev - in case I didn't earlier, I wanted to add a note to make it clear that this is probably in good shape but I need to spend some time tinkering with it in tests outside of the test suite. Some small errors in atom type accounting can lead to huge errors (for example, I've seen papers retracted because non-water parameters overwrote water parameters: ParmEd/ParmEd#1197). Keeping this in the "private" API is definitely the way to do it, but I also want to run it through a bunch of tests cases to ensure nothing funny happens since it might make its way to the public API in the future. |
|
Thanks @mattwthompson. Let me know if you have some specific tests in mind and I can run those locally as well. |
Description
This should be solving #961
Checklist