-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enable extending NonEquilibriumCyclingProtocols #44
Open
ianmkenney
wants to merge
13
commits into
OpenFreeEnergy:main
Choose a base branch
from
ianmkenney:feature-noneq_cycling_extends
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
fb60ab5
Enabled extension of NonEquilibriumCyclingProtocol
ianmkenney 3991187
Allow extending with replicates
ianmkenney 96f0693
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 1aa6669
Updated serialization/compression of OpenMM objects
ianmkenney 67392bc
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 8c69b31
Updated ValueError messages when extending
ianmkenney c244ae9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 714c4d0
Reduced loop count
ianmkenney 1d46275
Use `gufe` methods to determine if extends is ok
ianmkenney 9c42a25
Use dict literal syntax over constructor
ianmkenney 1e9cea2
Merge branch 'main' into feature-noneq_cycling_extends
ianmkenney 093b76a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 8c11858
Merge branch 'main' into feature-noneq_cycling_extends
dotsdl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this is a good check to have, and is not too strict at all. The philosophy behind a
Protocol
with a givenSettings
object is that thoseSettings
are immutable once given to theProtocol
object, and so callingProtocol.create(..., extends=<protocol_dag_result>)
should feature identical settings between the callingProtocol
and theProtocolDAGResult
we wish to extend from.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.
Just chiming in here - there are a few cases where this is going to be "not ideal" (mostly QoL but possibly could be frustrating).
So one example here could be a case where you extend but you switch compute platform - the results should be the same, but the settings will be different. Similarly the trajectory write frequency (you can imagine a case where someone decides that it's just not that necessary to have the trajectory after extending), etc...
It's of course not a blocker right now, but a "smarter" equality might be necessary (e.g. OpenFreeEnergy/gufe#329).
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.
I see your point, and think it makes sense to have a
thermodynamically_equals
method or function forProtocol
settings objects for this purpose.Systems like
alchemiscale
may limit the way users can doextends
in the sense thatTask
s may only extend otherTask
s from the sameTransformation
, but theProtocol
system on its own isn't as strict, since the concept of aTransformation
sits outside of it.