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

Fixes bar_with_pessimistic_uncertainty convergence error #1344

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

badisa
Copy link
Collaborator

@badisa badisa commented Jul 16, 2024

@badisa badisa requested review from mcwitt and maxentile July 16, 2024 20:46
@@ -180,6 +180,11 @@ def test_df_from_u_kln_does_not_raise_on_incomplete_convergence():
assert np.isfinite(df)
assert np.isnan(ddf) # returns NaN for uncertainty on incomplete convergence

bootstrap_df, bootstrap_ddf = bar_with_pessimistic_uncertainty(u_kln, maximum_iterations=1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises an exception without the fix. Didn't launch a CI job as it is an unambigious failure

df, ddf = mbar.getFreeEnergyDifferences()
full_bar_result = df[0, 1]
full_bar_err = ddf[0, 1]
full_bar_result, full_bar_err = df_and_err_from_u_kln(u_kln, maximum_iterations=maximum_iterations)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

df_and_err_from_u_kln handles the behavior correctly: https://github.com/proteneer/timemachine/blob/master/timemachine/fe/bar.py#L136-L139

@badisa badisa changed the title Avoid raising exception in bar_with_pessimistic_uncertainty when Fixes bar_with_pessimistic_uncertainty convergence error Jul 16, 2024
Comment on lines 192 to 193
u_kn, N_k = ukln_to_ukn(u_kln)
mbar = pymbar.MBAR(u_kn, N_k, maximum_iterations=maximum_iterations)
Copy link
Collaborator

@mcwitt mcwitt Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this seems unnecessary vs. setting initial_f_k=None below (or maybe better, initial_f_k=np.array([0.0, full_bar_result]))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, will replace it with initial_f_k=np.array([0.0, df]) which is the same value without constructing an extra MBAR object

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set using pre-existing value in a7de23f

Copy link
Collaborator

@mcwitt mcwitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Makes sense to reuse df_and_err_from_u_kln since the latter has the desired workaround.

@badisa badisa enabled auto-merge (squash) July 16, 2024 21:20
@badisa badisa merged commit ab3def7 into master Jul 16, 2024
1 check passed
@badisa badisa deleted the bugfix/avoid-convergence-failure-in-mbar branch August 15, 2024 22:48
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.

3 participants