Closed dotsdl closed 2 months ago
An alternative approach suggested by @ijpulidos is to define a new unit (MinimizationUnit
) that depends on SetupUnit
and that CycleUnit
(s) consume as input that performs energy minimization, and subclassing Protocol
s such as FahNonEquilibriumCyclingProtocol
in alchemiscale-fah
could then leave this unit out of their ProtocolDAG
creation in _create
.
@dotsdl These changes look great and I am okay to have them here but just a comment to consider as follows.
To expand a little bit on why maybe having a MinimizationUnit
is more reasonable is because I believe this change (in this PR) to be very specific to the FAH protocol and the FAH execution model. While we could support it here in our protocol directly, I don't know how "correct" it is to support it here. It generates some redundant code (which is small, fair) but also a little bit of conceptual complexity as to why the protocol itself needs some setting to defer the minimization.
Again, I'm okay to let it happen as it is in this set of changes, but just wanted a quick iteration on this point before reaching a conclusion. Thanks for the contribution!
@ijpulidos: after a discussion last week on this with @IAlibay, we came to the conclusion that it makes more sense to move minimization entirely into the CycleUnit
alongside equilibration, since this then reduces SetupUnit
down to everything that doesn't practically require a GPU. This removes the need for a defer_minimization
setting entirely, and greatly simplifies the change we are making here.
Thoughts?
I can't get the tests to successfully execute locally; really unclear why, but pytest
seems to crash for me.
@ijpulidos I will reach out for a time to troubleshoot live with you if possible!
Was able to get the test_sanity.py
test to pass on my local GPU host with this change. Think we're good to merge if you're satisfied @ijpulidos!
On conventional compute resources, it makes sense to perform energy minimization in the SetupUnit, since there may be many CycleUnits that use those same minimized coordinates as their starting point.
However, uses of this same protocol on unconventional compute, such as Folding@Home, may require minimization to be performed outside of the SetupUnit, and instead in the CycleUnit (the F@H
openmm-core
running on a volunteer host). This is because the host running the SetupUnit is unlikely to have a GPU, but hosts running the dynamics for CycleUnit(s) will, and minimization on a CPU is orders of magnitude slower.This change introduces a new setting for this protocol,
defer_minimization
, that defaults toFalse
, yielding current behavior. When set toTrue
, minimization is instead performed in the CycleUnit(s), and not performed in the SetupUnit.