-
Notifications
You must be signed in to change notification settings - Fork 35
Fast/ adaptive settings #1523
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
Fast/ adaptive settings #1523
Conversation
IAlibay
left a comment
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.
Couple of quick thoughts - I'm not really into the flag (i.e. it doesn't feel like it adds anything useful), but it's not a strongly held view.
| f"{mapping.componentA.name} and {mapping.componentB.name}. " | ||
| "A more expensive protocol with 22 lambda windows, sampled " | ||
| "for 20 ns each, will be used here.") | ||
| warnings.warn(wmsg) |
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.
Thoughts on warning here but not warning on the default behaviour (below)? I.e. should this be just at INFO level since we expect it to be adaptive?
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.
This was to maintain the current CLI experience, where this warning is displayed for charge change edges. However, more generally, perhaps an INFO-level log would be better for each edge, indicating if the settings are adapted from the defaults.
|
|
||
| else: | ||
| # apply the fast settings we have benchmarked | ||
| protocol_settings.forcefield_settings.nonbonded_cutoff = 0.9 * unit.nanometer |
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.
Probably aim to only merge this PR after fast settings have been validated for net charge transformations - it would be good if we could avoid having to set any of these (or maybe we can set the default to the solvent case).
hannahbaumann
left a comment
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.
Thanks @jthorton this looks great! I agree with Irfan that this could also just be the new default without a flag, unless it would be too confusing for our users.
Other than that we'll probably just have to wait to see if we can switch charge changes to fast settings as well and tests.
IAlibay
left a comment
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.
Couple of things - we should just update the main settings objects so the changes are reflected everywhere.
| equilibration_length=1.0 * unit.nanosecond, | ||
| production_length=5.0 * unit.nanosecond, | ||
| # fast settings | ||
| time_per_iteration=2.5 * unit.picosecond |
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.
Can you change this in the underlying settings file? That way it propagates to all the other Protocols: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/protocols/openmm_utils/omm_settings.py#L553
| solvation_settings=OpenMMSolvationSettings(), | ||
| solvation_settings=OpenMMSolvationSettings( | ||
| # fast settings | ||
| box_shape="dodecahedron", |
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.
Same thing here, can you update this in the original settings object? that way we can update septop/abfes at the same time automatically
| forcefield_settings=settings.OpenMMSystemGeneratorFFSettings(), | ||
| forcefield_settings=settings.OpenMMSystemGeneratorFFSettings( | ||
| # fast settings | ||
| nonbonded_cutoff=0.9 * unit.nanometer |
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.
Should we update this in gufe? @atravitz thoughts?
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 would be great as it is the recommended cutoff for openff so we probably should use that everywhere.
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.
updated in gufe
| # fast settings | ||
| box_shape="dodecahedron", | ||
| # make the default padding work for all cases | ||
| solvent_padding=1.5 * unit.nanometer |
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've raised #1589 for us to update settings in SepTop & AFEs.
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.
This has broken some of the SetTop tests, is it best to update the solvent padding for SepTop here as well?
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.
Sure!
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 (putting aside needed changes in gufe)
|
just a reminder to check that any changes are included on the CLI side, where applicable. #1591 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1523 +/- ##
==========================================
- Coverage 95.45% 93.38% -2.08%
==========================================
Files 174 174
Lines 14553 14564 +11
==========================================
- Hits 13892 13600 -292
- Misses 661 964 +303
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
# Conflicts: # openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py # openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py
atravitz
left a comment
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 an opinion on the news entry
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
|
No API break detected ✅ |
This PR adds partial support for adaptive settings (#974) based on the transformation for the hybrid topology RBFE protocol. This is used to expose the fast settings for net neutral transformations. For these transformations the following settings will be updated:
Questions:
I plan on adding tests once we are happy with the proposal.
Checklist
newsentryDevelopers certificate of origin