NEq cycling rebase #4
Conversation
|
Awesome @ijpulidos! Excited for this! |
Welcome to Codecov 🎉Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment. Thanks for integrating Codecov - We've got you covered ☂️ |
|
@dotsdl I think this one should be ready to go to start working on optimizing the DAG units and thinking about what components migrate to openmmtools that should be shared between different protocols. Those changes I think are more suited for future iterations/PRs. |
| ) | ||
| ####### END OF SETUP ######### |
There was a problem hiding this comment.
Basically up to here can be moved to a separate unit for #3 and also to help thinking about the openmmtools API points that should be doing these things.
There was a problem hiding this comment.
@ijpulidos looking great! Only one blocking question on hardcodes in HybridTopologyFactorModded init. Otherwise mostly clarification questions.
| concurrency: | ||
| group: "${{ github.workflow }}-${{ github.ref }}" | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
Oh cool, didn't know this was a thing now.
| defaults: | ||
| run: | ||
| shell: bash -l {0} |
.github/workflows/ci.yaml
Outdated
| fail-fast: false | ||
| matrix: | ||
| os: ["ubuntu"] | ||
| pydantic-version: [">1"] |
There was a problem hiding this comment.
Where does this actually get used below?
There was a problem hiding this comment.
We shouldn't need this, you are correct.
| # Perses depends (TODO: Remove once we don't depend on perses) | ||
| - openmoltools | ||
| - cloudpathlib | ||
| - dask | ||
| - openeye-toolkits | ||
|
|
| @@ -2,26 +2,17 @@ name: feflow-test | |||
| channels: | |||
| - conda-forge | |||
| - defaults | |||
| - openeye # TODO: Remove once we don't depend on openeye | |||
| ) | ||
| ####### END OF SETUP ######### |
| @@ -79,7 +79,7 @@ distance-dirty = "{base_version}+{distance}.{vcs}{rev}.dirty" | |||
| method = "git" # <- The method name | |||
| # Parameters to pass to the method: | |||
| match = ["*"] | |||
| default-tag = "1.0.0" | |||
| default-tag = "0.1.0" | |||
There was a problem hiding this comment.
I'm actually not sure what this does? For alchemiscale we have it set to "0.0.0"...
| @@ -67,7 +67,7 @@ feflow = [ | |||
| ] | |||
|
|
|||
| [tool.versioningit] | |||
| default-version = "1+unknown" | |||
| default-version = "0.1.0+unknown" | |||
There was a problem hiding this comment.
Not sure what this does; we leave it as "1+unknown" in alchemiscale. @mikemhenry?
feflow/utils/hybrid_topology.py
Outdated
| use_dispersion_correction=False, | ||
| softcore_alpha=0.5, | ||
| softcore_LJ_v2=True, | ||
| softcore_LJ_v2_alpha=0.85, | ||
| interpolate_old_and_new_14s=False, | ||
| flatten_torsions=False, |
There was a problem hiding this comment.
Are these intended to be hardcodes? The args in above will have no effect.
There was a problem hiding this comment.
Also, if there is no change to the base __init__, is it necessary to include it here?
There was a problem hiding this comment.
Good catch! These should indeed be passed from the __init__. And I just thought that even if the signature of the method is the same, I figured it would be convenient to have them explicitly as helping documentation.
| # Lambda settings | ||
| lambda_functions = DEFAULT_ALCHEMICAL_FUNCTIONS | ||
|
|
||
| # alchemical settings | ||
| softcore_LJ_v2 = True | ||
| interpolate_old_and_new_14s = False | ||
| alchemical_settings: AlchemicalSettings |
There was a problem hiding this comment.
There may be some settings, such as lambda_windows, which don't actually apply here, I think?
There was a problem hiding this comment.
Yes, definitely, there are settings in AlchemicalSettings that do not apply to this protocol, but I think we would need to stick with these for now until we can come up with a base alchemical settings object that should cover the common settings for both protocols. Raised the issue in #6
This set of changes aim to rebase the
NonEquilibriumCyclingProtocolto use the openfe objects in order to have a common code base for both protocols (openfe and neq cycling).A significant portion of the settings and code for creating the individual OpenMM topologies and systems should be able to be shared between both protocols, as well as the creation of the creation of the Hybrid Topology.
Resolves #1