-
Notifications
You must be signed in to change notification settings - Fork 35
Fix sampler setup for dry runs #1590
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
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.
Ah shoot! I forgot that 0 had a special meaning, thanks for catching this @hannahbaumann !
I'd prefer changing this in the underlying code and make it so that None just skips minimization, could you do this instead?
|
Putting the None check / skip here would do fine: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/protocols/openmm_rfe/_rfe_utils/multistate.py#L291 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1590 +/- ##
==========================================
- Coverage 95.45% 93.35% -2.11%
==========================================
Files 174 174
Lines 14552 14553 +1
==========================================
- Hits 13891 13586 -305
- Misses 661 967 +306
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:
|
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.
Thanks, too small things, otherwise it looks good to me!
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
…rgy/openfe into fix_minimization_dry
|
No API break detected ✅ |
|
Failing tests are down to AWS issues it looks. |
The sampler setup is very slow for dry runs of membrane systems.
We are using the
LocalEnergyMinimizerto do a short minimization, but don't want to do that in the dry run.minimization_stepsare set to zero in the dry run, but for theLocalEnergyMinimizerthat leads to running minimizations until converged:maxIterations (int) – the maximum number of iterations to perform. If this is 0, minimation is continued until the results converge without regard to how many iterations it takes. The default value is 0.quick fix would be just to change it to 1, but we could also turn off minimization completely during dry runs?