-
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
base: main
Are you sure you want to change the base?
Enable extending NonEquilibriumCyclingProtocols #44
Conversation
* 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.
2894417
to
3991187
Compare
for more information, see https://pre-commit.ci
One limitation of Is this the case for the outputs added in this PR? |
These are not json serializable. I can add another step in (de)serialization for converting to and from base64. |
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.
for more information, see https://pre-commit.ci
* When `mapping` is `None`, use the mapping provided by the ProtocolDAGResult
b2aa129
to
8c69b31
Compare
for more information, see https://pre-commit.ci
@ijpulidos any idea why my tests fail on license keys? |
@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 |
to kick off ci (you need write access to the repo for this to work)
|
This is currently blocked by #38. |
@ianmkenney @dotsdl Is this ready to be reviewed? If so, can you mark it as such? Thanks! |
@ijpulidos I think an initial review works, not sure if you want to wait for gufe/openfe v1.0.0 before merging though. |
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.