Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
draft: Add atom type merging for Interchange.to_gromacs() #962
draft: Add atom type merging for Interchange.to_gromacs() #962
Changes from 15 commits
20aadef
36fcd52
c98e14e
6954a9a
4ddc1e0
e38de61
f8907ae
b85edc7
817141c
2070603
185d88d
85bff31
3d071f4
2b8fec9
09c44bd
ff52df3
66acca8
9092e3f
0f72fe5
28171c8
50b98d8
712ab1f
daf225c
363802e
cd650ee
5948f36
fe3cd93
51c9465
4531a75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 requireINTERCHANGE_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fair, I added underscore to all public methods