Skip to content
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
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ianmkenney
Copy link
Collaborator

This PR introduces the ability to extend a NonEquilibriumCycling protocol by including serialized and compressed OpenMM State, System, and Integrator in the main simulation units. When instantiating new NonEquilbriumProtocols which extend a previous result, these objects are used to bypass the normal function of the SetupUnit. Closes #2.

* Added new (de)compression + (de)serialize functions in
  feflow.utils.data
* Include compressed System, State, and Integrator as a result for the
  main simulation unit.
* SetupUnits now take `extends_data`, which when populated, spoof the
  running of the `SetupUnit._execute` method. It instead immediately
  returns results with values consistent to the end state of the
  extended SimulationUnit.
* Added new test `test_pdr_extend` to test the extension functionality.
@ianmkenney ianmkenney changed the title Enable extending NonEquilibriumCyclingProtocols [WIP] Enable extending NonEquilibriumCyclingProtocols Apr 10, 2024
@ianmkenney ianmkenney marked this pull request as draft April 10, 2024 15:42
@ianmkenney ianmkenney force-pushed the feature-noneq_cycling_extends branch from 2894417 to 3991187 Compare April 10, 2024 22:34
@dotsdl
Copy link
Member

dotsdl commented Apr 11, 2024

One limitation of GufeTokenizables, including ProtocolDAGResults: they must be JSON-serializable, and therefore can't have binary data. So any outputs of a ProtocolUnit._execute method must also be data types that are JSON-serializable.

Is this the case for the outputs added in this PR?

@ianmkenney
Copy link
Collaborator Author

These are not json serializable. I can add another step in (de)serialization for converting to and from base64.

ianmkenney and others added 3 commits April 12, 2024 11:53
GufeTokenizables must be JSON serializable, so including bytes is not an
option. Instead, we take the compressed bytes and encode them into a
Base64 string.
* When `mapping` is `None`, use the mapping provided by the
  ProtocolDAGResult
@ianmkenney ianmkenney force-pushed the feature-noneq_cycling_extends branch from b2aa129 to 8c69b31 Compare April 12, 2024 20:42
@ianmkenney
Copy link
Collaborator Author

@ijpulidos any idea why my tests fail on license keys?

@ijpulidos
Copy link
Contributor

@ianmkenney My guess is that the secrets fail since it's coming from a fork? Let me dig around and see if there's a way to make this work.

One way would be making a separate PR in the repo, but I'll see if there's another way.

@mikemhenry
Copy link
Contributor

@ianmkenney My guess is that the secrets fail since it's coming from a fork? Let me dig around and see if there's a way to make this work.

One way would be making a separate PR in the repo, but I'll see if there's another way.

That is exactly what is going on, the quick fix is for someone who has write access, to push this branch to the choderalab/feflow remote, that will trigger the tests to run with the secrets and since the commit hash is the same, github will report the test results here

@mikemhenry
Copy link
Contributor

to kick off ci (you need write access to the repo for this to work)

$ gh pr checkout 44
$ git push origin feature-noneq_cycling_extends

@dotsdl
Copy link
Member

dotsdl commented Apr 30, 2024

This is currently blocked by #38.

@dotsdl dotsdl added this to the 0.1.0 release milestone Apr 30, 2024
@ijpulidos
Copy link
Contributor

@ianmkenney @dotsdl Is this ready to be reviewed? If so, can you mark it as such? Thanks!

@ianmkenney ianmkenney changed the title [WIP] Enable extending NonEquilibriumCyclingProtocols Enable extending NonEquilibriumCyclingProtocols May 2, 2024
@ianmkenney ianmkenney marked this pull request as ready for review May 2, 2024 17:24
@ianmkenney ianmkenney requested a review from ijpulidos May 2, 2024 17:24
@ianmkenney
Copy link
Collaborator Author

@ijpulidos I think an initial review works, not sure if you want to wait for gufe/openfe v1.0.0 before merging though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add extends support to the NonEquilibriumCyclingProtocol
4 participants