-
Notifications
You must be signed in to change notification settings - Fork 4
Use gufe charge utils. gufe/openfe 1.10+ support #139
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
Changes from 14 commits
c59456a
28a6662
5d75fa1
e07522d
054ab37
8da0403
fc94b80
0401ad3
75d8dda
f29c7f8
cda67bd
3696332
2d43609
1ebb346
49c3283
f48ba0c
403ace1
2d838fb
2d2d7f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| for the specific integrator settings. | ||
| """ | ||
|
|
||
| from typing import Annotated, TypeAlias | ||
| from typing import Annotated, TypeAlias, Literal | ||
|
|
||
| from pydantic import ConfigDict, field_validator | ||
|
|
||
|
|
@@ -35,6 +35,15 @@ class PeriodicNonequilibriumIntegratorSettings(SettingsBaseModel): | |
| """Number of steps for the equilibrium parts of the cycle. Default 12500""" | ||
| nonequilibrium_steps: int = 12500 | ||
| """Number of steps for the non-equilibrium parts of the cycle. Default 12500""" | ||
| barostat: Literal["MonteCarloBarostat", "MonteCarloMembraneBarostat"] = ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will membrane support work out of the box?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm you are right, surely not since we are not handling membrane components at all. Maybe we just want to make this one just
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah maybe that's a good short term solution. |
||
| "MonteCarloBarostat" | ||
| ) | ||
| """ | ||
| The barostat to be used in the simulations. Default MonteCarloBarostat. | ||
| Notes | ||
| ----- | ||
| If the system contains a membrane, use the `MonteCarloMembraneBarostat`. | ||
| """ | ||
| barostat_frequency: TimestepQuantity = 25 * unit.timestep | ||
| """ | ||
| Frequency at which volume scaling changes should be attempted. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -636,6 +636,8 @@ def test_double_charge_fails( | |
| from feflow.utils.exceptions import ProtocolSupportError | ||
|
|
||
| settings = NonEquilibriumCyclingProtocol.default_settings() | ||
| # Change engine platform for tests | ||
| settings.engine_settings.compute_platform = "CPU" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly for @dotsdl - please be aware that the default for all this is now CUDA, so if you're running any kind of minimization, etc.. on F@H servers, they'll need to have a CUDA GPU.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I'll make a PR into |
||
| # We need to make sure we enable the alchemical charge correction | ||
| settings.alchemical_settings.explicit_charge_correction = True | ||
|
|
||
|
|
||
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.
[nit] you're losing validation re: not doing net charge transformations without PME - probably will want to add that back at some point.
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 good catch! This is probably why the CI was taking so long, since the error wasn't being raised and we have a test running a whole DAG with it!
Will re-add the validation and refactor the test such that it doesn't run a whole DAG, even in case the error isn't raised