Enable MBAR to do bootstrap error estimation #322

xiki-tempula · 2023-06-03T20:32:35Z

Implement #320

codecov · 2023-06-03T20:41:30Z

Codecov Report

Merging #322 (b887659) into master (ef74784) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #322   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files          27       27           
  Lines        1762     1767    +5     
  Branches      388      389    +1     
=======================================
+ Hits         1740     1745    +5     
  Misses          2        2           
  Partials       20       20

Impacted Files	Coverage Δ
src/alchemlyb/estimators/mbar_.py	`100.00% <100.00%> (ø)`
src/alchemlyb/workflows/abfe.py	`99.67% <100.00%> (+<0.01%)`	⬆️

orbeckst

Overall looking good, by under semver we can't just change the way that ABFE computes the error. We need to deprecate the default (analytical) and then switch over to n_bootstraps=50 in, say, release 2.3.

The kwargs handling of n_bootstraps needs improving (and possibly a test — sorry).

src/alchemlyb/workflows/abfe.py

orbeckst · 2023-06-06T18:50:42Z

src/alchemlyb/workflows/abfe.py

+                    "By default, n_bootstraps=50 is used to estimate the "
+                    "MBAR error. Supply n_bootstraps=0 to use analytic error."
+                )
+                estimator_kwargs = {"n_bootstraps": 50, **kwargs}


This will fail when I actually supply n_bootstraps as a kwarg because then the dict contains the same key twice ... a test would have caught it.

Instead, I would do

# use 0 as default to remain backwards-compatible # change in release 2.3 kwargs.setdefault('n_bootstraps', 0) ... ... if estimator == "MBAR": estimator_kwargs = kwargs self.logger.info("Run MBAR estimator") self.logger.info("Estimating MBAR error with n_bootstraps={n_bootstraps} (0 is analytical estimate)", estimator_kwargs) elif estimator == "BAR": estimator_kwargs = kwargs.copy() estimator_kwargs.pop('n_bootstraps', None) ... self.estimator[estimator] = BAR(**kwargs).fit(u_nk)

i.e., modify the copy of kwargs as needed but keep everything in kwargs.

Sorry, what do you mean this will fail?

>>> kwargs = {"n_bootstraps": 0} >>> estimator_kwargs = {"n_bootstraps": 50, **kwargs} >>> print(estimator_kwargs) {'n_bootstraps': 0}

Gives the expected behaviour where kwargs overrides the default in estimator_kwargs?

You're right, I am a bit surprised. (I was in the process of trying it out but then wrote the review comment without finishing my testing.)

My concern remains that the default ought to be n_bootstraps=0 to initially have the same behavior.

orbeckst · 2023-06-06T18:54:03Z

src/alchemlyb/workflows/abfe.py

+                    "By default, n_bootstraps=50 is used to estimate the "
+                    "MBAR error. Supply n_bootstraps=0 to use analytic error."


We should not change the default without a deprecation. I'd issue a DeprecationWarning to state that the error estimate will change in release 2.3 to n_bootstraps=50.

Until then, keep

kwargs.setdefault('n_bootstraps', 0)

to keep the old behavior.

I'd also use a more informative status message (see other comment) that reports on n_bootstrap.

Use a DeprecationWarning for letting users know about coming changes.

Yes, I think for this PR I will just add a DeprecationWarning without changing the behaviour.

orbeckst · 2023-06-06T18:57:45Z

CHANGES

@@ -20,6 +20,7 @@ The rules for this file:
 Enhancements
  - Add a parser to read serialised pandas dataframe (parquet) (issue #316, PR#317).
  - workflow.ABFE allow parquet as input (issue #316, PR#317).
+  - Allow MBAR estimator to use bootstrap to compute error (issue #320, PR#322).



Add a deprecation for using analytical error estimate as the default in ABFE, will change to using 50 bootstraps in 2.3 (see other comments)

orbeckst

minor things

orbeckst · 2023-06-06T20:54:01Z

CHANGES

+
+DeprecationWarning
+  - The default MBAR error estimator in workflow.ABFE.estimate will change from
+  analytic to bootstrap=50 (issue #320, PR#322).


indentation

say when it will change (target release, current + 2 = 2.3 I think)

Or 2.2, if you want to be aggressive, but state the target release.

orbeckst · 2023-06-06T20:55:55Z

src/alchemlyb/workflows/abfe.py

@@ -403,6 +403,9 @@ def estimate(self, estimators=("MBAR", "BAR", "TI"), **kwargs):
            'MBAR']. Note that the estimators are in their original form where
            no unit conversion has been attempted.

+        .. versionchanged:: 2.1.0
+        DeprecationWarning for using analytic error for MBAR estimator.


orbeckst · 2023-06-06T20:57:54Z

src/alchemlyb/estimators/mbar_.py

@@ -67,13 +73,15 @@ def __init__(
        relative_tolerance=1.0e-7,
        initial_f_k=None,
        method="robust",
+        n_bootstraps=0,


I'd add a reminder comment

n_bootstraps=0, # release 2.2: change to 50 (see PR #322)

So my feeling is that for the MBAR estimator, we retain the current behaviour unless people manually turn it on as bootstrap does come with a computational cost.

It is only for the estimate method in workflow.ABFE where I know that the input is decorrelated and I'm prepared to pay the computational cost, I will set the default to 50.

Yes, that's ok, we just can't make the default behave differently without a minimal warning period.

Auto stash before checking out "upstream/301-using-loguru"

1a853ab

xiki-tempula linked an issue Jun 3, 2023 that may be closed by this pull request

enable bootstrap #320

Closed

update

8277280

xiki-tempula mentioned this pull request Jun 3, 2023

New dependency to be added to the 2.1.0 release #321

Closed

update

ee1d4fb

xiki-tempula requested a review from orbeckst June 3, 2023 20:55

orbeckst requested changes Jun 6, 2023

View reviewed changes

xiki-tempula added 3 commits June 6, 2023 20:37

add test

fb4b659

Merge branch 'master' into 320-enable-bootstrap

89f855f

update

9cf90c7

xiki-tempula force-pushed the 320-enable-bootstrap branch from ad50299 to 9cf90c7 Compare June 6, 2023 19:44

xiki-tempula added 3 commits June 6, 2023 20:49

update

18ded44

update

0ca40c4

update

d959393

xiki-tempula requested a review from orbeckst June 6, 2023 20:33

orbeckst requested changes Jun 6, 2023

View reviewed changes

xiki-tempula added 2 commits June 6, 2023 22:06

update

74df776

update

b887659

xiki-tempula requested a review from orbeckst June 6, 2023 21:22

orbeckst approved these changes Jun 6, 2023

View reviewed changes

xiki-tempula merged commit ae7f40b into master Jun 6, 2023

xiki-tempula deleted the 320-enable-bootstrap branch June 6, 2023 22:03

orbeckst mentioned this pull request Jun 6, 2023

WIP: bootstrapping submodule, parameter for estimators #94

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable MBAR to do bootstrap error estimation #322

Enable MBAR to do bootstrap error estimation #322

xiki-tempula commented Jun 3, 2023

codecov bot commented Jun 3, 2023 •

edited

Loading

orbeckst left a comment

orbeckst Jun 6, 2023

xiki-tempula Jun 6, 2023

orbeckst Jun 6, 2023

orbeckst Jun 6, 2023

orbeckst Jun 6, 2023

xiki-tempula Jun 6, 2023

orbeckst Jun 6, 2023

orbeckst left a comment

orbeckst Jun 6, 2023

orbeckst Jun 6, 2023

orbeckst Jun 6, 2023

orbeckst Jun 6, 2023

orbeckst Jun 6, 2023

xiki-tempula Jun 6, 2023

orbeckst Jun 6, 2023

		"By default, n_bootstraps=50 is used to estimate the "
		"MBAR error. Supply n_bootstraps=0 to use analytic error."

Enable MBAR to do bootstrap error estimation #322

Enable MBAR to do bootstrap error estimation #322

Conversation

xiki-tempula commented Jun 3, 2023

codecov bot commented Jun 3, 2023 • edited Loading

Codecov Report

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2023 •

edited

Loading