Resolve inconsistency between bar_with_bootstrapped_uncertainty and df_and_err_from_u_kln #1324
Conversation
timemachine/fe/bar.py
Outdated
| # regardless, summarize as if normal | ||
| ddf = np.std(bootstrap_dfs) | ||
| # Take the max of the full error estimate and the bootstrapped error. Summarize as if normal regardless | ||
| ddf = max(ddf, np.std(bootstrap_dfs)) |
There was a problem hiding this comment.
@maxentile suggested taking the max of the full bar error vs the bootstrapped error
There was a problem hiding this comment.
Just double-checking, is the intent to return NaN here if the all-sample BAR calculation returns NaN for the uncertainty?
(Also might want to use np.maximum instead of max here since the latter has more consistent behavior with NaN inputs: max(1.0, nan) == 1.0 vs. np.maximum(1.0, nan) == nan, though both return NaN if the NaN argument is in the first position)
* Adds a warning so we at least know this is happening
Co-authored-by: Matt Wittmann <[email protected]>
| """ | ||
| test_ukln = Path(__file__).parent / "data" / "zero_overlap_ukln.npz" | ||
| u_kln = np.load(open(test_ukln, "rb"))["u_kln"] | ||
| boot_df, boot_df_err = bar_with_pessimistic_uncertainty(u_kln) |
There was a problem hiding this comment.
nit: might make test clearer to assert here that the BAR-computed overlap is ~0?
There was a problem hiding this comment.
Asserts the bar overlap is near zero in 632bbcf
There was a problem hiding this comment.
Looks good -- makes sense to pessimize over multiple error estimates if they might be under-estimates in different situations.
Sorry for delayed review -- started reviewing last week but failed to post. Small possible opportunity for increased descriptive precision noted, nothing blocking. (Although the BAR and MBAR estimates coincide in case of 2 states, I think their uncertainty estimates might not, based on https://github.com/choderalab/pymbar/blob/a314f001c9c3bef61e824382a70230f73509f842/pymbar/bar.py#L349-L448 )
Co-authored-by: Josh Fass <[email protected]>
unconverged * A bug introduced in #1324
bar_with_bootstrapped_uncertaintycan return an error of zero whendf_and_err_from_u_klnreturns a large error. I don't think this is actually an issue in practice, but seems like it would be good to address in case we ever use the error for something.