Handling unsampled states in offline/realtime analysis.

[ijpulidos]

Description

This aims to refactor MultiStateSampler._offline_analysis to handle unsampled states when doing the mbar estimate.

In this case the _offline_analysis method was decoupled from the _online_analisis one (which is still experimental). So that it initializes the first time with zeros as guess and subsequent results are updated using the previous calculation.

For this I had to make a new private attribute _last_mbar_f_k_offline which gets also written to the nc file independently of the attributes used in the _online_analysis method.

Resolves #592

Todos

• Implement feature / fix bug
• [n/a] Add tests
• Update documentation as needed
• Update changelogNotable points that this PR has either accomplished or will accomplish.

Status

• Ready to go

[ijpulidos]

This test, inside TestHarmonicOscillatorsMultiStateSampler, should be executing the _offline_analysis method using unsampled states. Therefore these changes should be already covered in the tests.

[mikemhenry]

LGTM

[zhang-ivy]

This test, inside TestHarmonicOscillatorsMultiStateSampler, should be executing the _offline_analysis method using unsampled states. Therefore these changes should be already covered in the tests.

@ijpulidos : Was this test passing before? If so, we may want to discuss editing it or adding another version of it that fails without this PR

[ijpulidos]

@zhang-ivy Yeah, good catch!

I guess it is worth mentioning that failing to make the MBAR estimates in the _offline_analysis method has never killed the simulation, I think we want to leave like that so far, just because I have the feeling there are many ways this estimate could fail (not just with the unsampled states thing).

My guess is that since we are relying more and more on this real time analysis file, it is a good idea to write a test for it. I'll raise an issue about it, and maybe also discuss in the same issue if it's worth to stop/kill the simulation if the MBAR estimates are not correctly computed. Does that sound okay?

[ijpulidos]

So this means running the _offline_analysis method could've been silently failing this whole time. Since the exception gets caught and it's never raised.

[zhang-ivy]

I guess it is worth mentioning that failing to make the MBAR estimates in the _offline_analysis method has never killed the simulation

Ah, I didn't realize this!

My guess is that since we are relying more and more in this real time analysis file, it is a good idea to write a test for it. I'll raise an issue about it, and maybe also discuss in the same issue if it's worth to stop/kill the simulation if the MBAR estimates are not correctly computed. Does that sound okay?

Yes sounds good, I think its ok for that to be a part of the next release, not openmmtools 0.21.4
I actually don't think we will want to kill the simulation if the offline analysis fails. I think the current behavior makes sense.
I think we just need a test to check that that the online/offline analysis is working properly.