-
Notifications
You must be signed in to change notification settings - Fork 19
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
Use Interchange for some OpenMM object creation #703
base: main
Are you sure you want to change the base?
Conversation
Hello @mattwthompson! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-02-21 15:08:10 UTC |
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.
I was under the impressions that this was going into https://github.com/OpenFreeEnergy/openfe_skunkworks so maybe this isn't meant to aim towards a merge.
Just leaving the one comment in case stuff progresses whilst I'm out - mostly a style thing about moving selection logic to a separate class method to keep run
pretty much just something where you can subclass and swap out methods.
openfe/protocols/openmm_afe/base.py
Outdated
prot_comp, solv_comp, smc_comps, system_generator, | ||
settings['solvation_settings'] | ||
) | ||
if ( |
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.
(just in case this moves towards merge in the next couple of weeks): it may be better if this logic went into the getter than in the run
of base - the idea here is that run
should be kept pretty lean so it's easy to swap out class methods.
See https://github.com/OpenFreeEnergy/openfe/blob/ahfe-packmol/openfe/protocols/openmm_afe/base.py#L433 for example.
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.
I see the benefit of that design; it didn't come about naturally since the Interchange code path does not always need to call _get_system_generator
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #703 +/- ##
==========================================
- Coverage 93.59% 90.78% -2.81%
==========================================
Files 133 133
Lines 9694 9713 +19
==========================================
- Hits 9073 8818 -255
- Misses 621 895 +274
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Do not merge
Developers certificate of origin