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

[WIP] Fix RTHP results serialization and test results load #550

Merged
merged 6 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions openfe/protocols/openmm_utils/multistate_analysis.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,9 @@
"""
# Do things that get badly cached later
self._replica_states = self.analyzer.reporter.read_replica_thermodynamic_states()
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)
Copy link
Member

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)

Copy link
Member Author

@IAlibay IAlibay Sep 14, 2023

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.

self._prod_iters = float(self.analyzer._equilibration_data[2])

# Gather estimate of free energy
self._free_energy, self._free_energy_err = self.get_equil_free_energy()
Expand Down Expand Up @@ -185,10 +186,10 @@
try:
# pymbar 3
DF_ij, dDF_ij = mbar.getFreeEnergyDifferences()
except AttributeError:
r = mbar.compute_free_energy_differences()
DF_ij = r['Delta_f']
dDF_ij = r['dDelta_f']

Check warning on line 192 in openfe/protocols/openmm_utils/multistate_analysis.py

View check run for this annotation

Codecov / codecov/patch

openfe/protocols/openmm_utils/multistate_analysis.py#L189-L192

Added lines #L189 - L192 were not covered by tests

DG = DF_ij[0, -1] * analyzer.kT
dDG = dDF_ij[0, -1] * analyzer.kT
Expand Down Expand Up @@ -246,9 +247,9 @@

# Check that the N_l is the same across all states
if not np.all(N_l == N_l[0]):
errmsg = ("The number of samples is not equivalent across all "

Check warning on line 250 in openfe/protocols/openmm_utils/multistate_analysis.py

View check run for this annotation

Codecov / codecov/patch

openfe/protocols/openmm_utils/multistate_analysis.py#L250

Added line #L250 was not covered by tests
f"states {N_l}")
raise ValueError(errmsg)

Check warning on line 252 in openfe/protocols/openmm_utils/multistate_analysis.py

View check run for this annotation

Codecov / codecov/patch

openfe/protocols/openmm_utils/multistate_analysis.py#L252

Added line #L252 was not covered by tests

# Get the chunks of N_l going from 10% to ~ 100%
# Note: you always lose out a few data points but it's fine
Expand Down Expand Up @@ -311,8 +312,8 @@
try:
# pymbar 3
overlap_matrix = self.analyzer.mbar.computeOverlap()
except AttributeError:
overlap_matrix = self.analyzer.mbar.compute_overlap()

Check warning on line 316 in openfe/protocols/openmm_utils/multistate_analysis.py

View check run for this annotation

Codecov / codecov/patch

openfe/protocols/openmm_utils/multistate_analysis.py#L315-L316

Added lines #L315 - L316 were not covered by tests

return overlap_matrix

Expand Down Expand Up @@ -417,4 +418,4 @@
return results_dict

def close(self):
self.analyzer.clear()

Check warning on line 421 in openfe/protocols/openmm_utils/multistate_analysis.py

View check run for this annotation

Codecov / codecov/patch

openfe/protocols/openmm_utils/multistate_analysis.py#L421

Added line #L421 was not covered by tests
Binary file added openfe/tests/data/openmm_rfe/vac_results.json.gz
Copy link
Member Author

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

Binary file not shown.
Loading