Support version 1.0 for openfe and gufe #38
Conversation
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 92.06% 85.84% -6.23%
==========================================
Files 7 10 +3
Lines 416 1399 +983
==========================================
+ Hits 383 1201 +818
- Misses 33 198 +165 ☔ View full report in Codecov by Sentry. |
| class Config: | ||
| arbitrary_types_allowed = True | ||
|
|
||
| timestep: FloatQuantity["femtosecond"] = 4 * unit.femtoseconds |
There was a problem hiding this comment.
Sorry very much a flyby 2 second review for now - the IntegratorSettings need to include information about the barostat frequency & possibly com removal, see: https://github.com/OpenFreeEnergy/openfe/blob/c4678745900e6425354407de5885c7c3eefd429b/openfe/protocols/openmm_utils/omm_settings.py#L195-L201
I'm not particularly sure it's the right design pattern, see: OpenFreeEnergy/openfe#712, however because get_system_generator (https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/protocols/openmm_utils/system_creation.py#L26) currently asks for an integrator settings object (and you're using it in the neq protocol), you end up needing that additional field.
There was a problem hiding this comment.
Ah maybe there's going to be a typing issue - I can see that maybe we have to have a discussion about this.
There was a problem hiding this comment.
The barostat_frequency doesn't seem to be needed or used, as far as I can tell, at least not in the NeqCycling protocol, which makes me wonder if this is something we would like to change. What are the scenarios where changing this frequency is desired?
There was a problem hiding this comment.
In the end it was indeed needed for "real-life" simulations, this probably means we need to add more testing that resembles this situation.
feflow/settings/integrators.py
Outdated
| equilibrium_steps: int = 250000 | ||
| """Number of steps for the equilibrium parts of the cycle. Default 250000""" | ||
| nonequilibrium_steps: int = 250000 |
There was a problem hiding this comment.
Usually we put this under "SimulationSettings"
There was a problem hiding this comment.
In this case these are specific to the integrator as a single cycle/run of the integrator does a "equilibration on endstate A" -> "nonequilibrium sim on A to B (changing lambda)" -> "equilibration on endstate B" -> "nonequilibrium sim on B to A". So these parameters are to change how many steps on each of the eq/neq parts. I hope that makes sense. More info in the docs
There was a problem hiding this comment.
That is, this is not about prior equilibration of the system to perform the NEQ parts. Which is something that this protocol doesn't do, so far.
There was a problem hiding this comment.
I see what you mean. This is a bit of an odd once since, as a user, I would expect these to be more "how I define my simulation protocol" than "something related to my integrator". It probably doesn't matter, but it's something to consider longer term.
for more information, see https://pre-commit.ci
|
This one should be now pretty much ready to go. The failing tests are the ones with "bad" mappings. Now the On the other hand, we should probably wait for a 1.0 release of both |
|
Awesome work here @ijpulidos ! I'll try to have a look tomorrow morning ahead of our call. |
This PR supersedes #33.
Solves #31
It aims to add the following changes in order to support the 1.0 version/rc of
gufeandopenfe.TODO: