-
Notifications
You must be signed in to change notification settings - Fork 24
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
Improve testing of GROMACS import #1118
Conversation
92e6e3a
to
29acdc0
Compare
This could be ramped up a bit by running each of these tests as monolithic vs ITP structure |
def gmx_roundtrip(state: Interchange, apply_smirnoff_defaults: bool = False): | ||
with temporary_cd(): | ||
state.to_gromacs(prefix="state", decimal=8) |
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.
def gmx_roundtrip(state: Interchange, apply_smirnoff_defaults: bool = False): | |
with temporary_cd(): | |
state.to_gromacs(prefix="state", decimal=8) | |
@pytest.mark.parametrize("monolithic", [False, True]) | |
def gmx_roundtrip(state: Interchange, apply_smirnoff_defaults: bool = False, monolithic): | |
with temporary_cd(): | |
state.to_gromacs(prefix="state", decimal=8, monolithic=monolithic) |
Intended change in #1120 after this is merged in
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.
LGTM, one nonblocking suggestion on a slightly confusing test method but otherwise good to go!
from openff.interchange.drivers import get_gromacs_energies | ||
|
||
|
||
def gmx_monolithic_vs_itp(state: Interchange): | ||
def gmx_roundtrip(state: Interchange, apply_smirnoff_defaults: bool = False): |
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.
(not blocking) My understanding of this method is that it just ensures that an error isn't raised when writing and then reading the system in GROMACS formats. It doesn't evaluate anything numerical or return the roundtripped system for further inspection. This was a little confusing to me, it might be good to update the method name or docstring to make that clear.
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.
Good find, I think that was incidentally removed in a merge
Description
Checklist