Use gufe charge utils. gufe/openfe 1.10+ support - #139
Conversation
This reverts commit f29c7f8.
for more information, see https://pre-commit.ci
ijpulidos
changed the title
|
|
||
| settings = NonEquilibriumCyclingProtocol.default_settings() | ||
| # Change engine platform for tests | ||
| settings.engine_settings.compute_platform = "CPU" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see. I'll make a PR into alchemiscale-fah that sets settings.engine_settings.compute_platform to None so that this doesn't trip users up. It's not a setting that has any bearing on the openmm-core used on F@H volunteer hosts downstream.
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Just a couple of comments since I was looking at the github notification - let us know if you need a proper review!
| """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"] = ( |
There was a problem hiding this comment.
Will membrane support work out of the box?
There was a problem hiding this comment.
Hmm you are right, surely not since we are not handling membrane components at all. Maybe we just want to make this one just Literal["MonteCarloBarostat"]? Or what do you suggest?
There was a problem hiding this comment.
Yeah maybe that's a good short term solution.
| except ValueError as e: | ||
| raise ProtocolSupportError(str(e)) | ||
| # Get the formal change difference between the end states | ||
| charge_difference = mapping.get_alchemical_charge_difference() |
There was a problem hiding this comment.
[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.
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
dotsdl
left a comment
dotsdl
left a comment
There was a problem hiding this comment.
Looks great to me @ijpulidos! Can you elaborate on what you saw in the CI failures? Are they hanging?
This reverts commit 1ebb346.
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 86.86% 87.90% +1.03%
==========================================
Files 11 14 +3
Lines 1439 1579 +140
==========================================
+ Hits 1250 1388 +138
- Misses 189 191 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
These set of changes use theEnabling support for gufe and openfe 1.9+ releases.gufe.mapping.LigandAtomMapping.get_alchemical_charge_differencecapabilities instead of the deprecated utility function in openfe (check https://github.com/OpenFreeEnergy/openfe/blob/c60db0685a937ce8ef0616ea2e563121bf1f9d2d/src/openfe/utils/ligand_utils.py#L6).We vendor the utility function which now lies inside a method in the upstream protocols.