-
Notifications
You must be signed in to change notification settings - Fork 19
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
[WIP] Fix RTHP results serialization and test results load #550
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
==========================================
- Coverage 90.99% 90.93% -0.06%
==========================================
Files 114 117 +3
Lines 6882 7163 +281
==========================================
+ Hits 6262 6514 +252
- Misses 620 649 +29
☔ View full report in Codecov by Sentry. |
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 I'm missing something really obvious. I'm trying to load in the results json back into a RelativeHybridTopologyProtocolResult
but my unit results end up without the .outputs
attribute.
Current testing looks something like this:
r = json.load(gzip.open('vac_results.json.gz', 'r'), cls=JSON_HANDLER.decoder)
rhtpr = RelativeHybridTopologyProtocolResult._from_dict(r['protocol_result']['data'])
Looks like it hasn't sufficiently deserialized the protocol unit results, is this even something we're meant to be able to do?
Loads fine, has the
looks like OpenFreeEnergy/gufe#229 actually needs fixing in gufe, since the specialised ProtocolResult doesn't override to/from dict |
self._equil_iters = self.analyzer.n_equilibration_iterations | ||
self._prod_iters = self.analyzer._equilibration_data[2] | ||
# float conversions to avoid having to deal with numpy dtype serialization | ||
self._equil_iters = float(self.analyzer.n_equilibration_iterations) |
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.
should these be int
instead of float
? (Variable name looks to me like an int
)
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've already raised that issue in openmmtools, currently they return float and it's been agreed that they will eventually change.
Developers certificate of origin