BAR Estimator #60

harlor · 2018-10-17T15:59:57Z

This version seems to reproduce results i get using alchemical-analysis.

codecov-io · 2018-10-17T16:03:37Z

Codecov Report

Merging #60 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   98.18%   98.33%   +0.14%     
==========================================
  Files           9       10       +1     
  Lines         497      539      +42     
  Branches      100      105       +5     
==========================================
+ Hits          488      530      +42     
  Misses          4        4              
  Partials        5        5

Impacted Files	Coverage Δ
src/alchemlyb/estimators/__init__.py	`100% <100%> (ø)`	⬆️
src/alchemlyb/estimators/bar_.py	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd70f41...d572e9e. Read the comment docs.

mrshirts · 2018-10-17T17:38:27Z

Can you explain a little more what is different? Also, be specific as to what tests you did to compare the alchemical-analysis and alchemlyb

Note that the error estimate generated by BAR ARE correlated and so it's not a great idea to compute the overall uncertainty by adding them in the square. Correlations are very hard to compute between neighboring BAR calculations. Bootstrapping the entire data set is almost certainly the best way to do this.

harlor · 2018-10-17T21:19:22Z

I used different input files to test this (As explained in #61 (comment))

Indeed - unfortunately I am currently just calculating the sum of the squares. I guess bootstrapping just requires to have a sufficient amount of uncorrelated samples in u_kln? As suggested in choderalab/pymbar#304 (comment) do you think 10 samples are sufficient to do an error estimation? Of course this is something that should be adjustable but to have a default value for this would still be nice.

mrshirts · 2018-10-17T23:26:52Z

I used different input files to test this (As explained in #61 (comment))

Again, document which input files used so that it's reproducible.

Indeed - unfortunately I am currently just calculating the sum of the squares.

There's no other way to do it with BAR currently that I know if.

As suggested in choderalab/pymbar#304 (comment) do you think 10 samples are sufficient to do an error estimation? Of course this is something that should be adjustable but to have a default value for this would still be nice.

It probably makes sense to do bootstrapping as a general utility (perhaps inside alchemlyb, as perhaps a wrapper for all methods?), since it's useful for ALL calculations, not just BAR.

200 is much better for bootstrap than 10. At least 50 is needed.

Note that bootstrapping is NOT running 10 independent runs. See http://www.alchemistry.org/wiki/Analyzing_Simulation_Results#Bootstrap_Sampling

It's less accurate than running 200 independent samples, and includes more bias, but is statistically well behaved.

dotsdl · 2018-10-19T02:05:01Z

@harlor, thanks for this contribution! We've been wanting a BAR implementation for some time (#28), so this will be great to finally land. I'll have time to review sometime this weekend.

harlor · 2018-10-19T14:08:48Z

Thank you @dotsdl!

@mrshirts

It probably makes sense to do bootstrapping as a general utility (perhaps inside alchemlyb, as perhaps a wrapper for all methods?), since it's useful for ALL calculations, not just BAR.

ok, maybe we can then just return nan-values for unknown uncertainties in the bar esimator here? Returning uncertainties that are known to be underestimated is not a good idea I guess.

dotsdl · 2018-11-03T22:19:31Z

@harlor to move this one forward, we'll need some tests added to test_fep_estimators.py. You can create a class TestBAR similar to TestMBAR that tests the results from this estimator, which at the moment is an almost one-liner. Feel free to add additional tests as needed to help define the correct functionality of the BAR estimator.

harlor · 2018-11-26T12:55:35Z

@dotsdl I added tests for the BAR estimator's value and its uncertainties - I also added tests for the uncertainty of the MBAR estimator and compared the results with alchemical-analysis where I find exact same values by using:
alchemical_analysis.py -g -p lambda_ -t 300 -i 0 -u kBT -r 6

@mrshirts Instead of calculating underestimated uncertainties, the estimator now just gives NaN for unknown uncertainties.

orbeckst · 2018-12-14T20:08:01Z

The predict() method only contains pass and is not tested. Can't we just drop it? Or does it have to be there – @dotsdl ?

orbeckst

I am covering the code/interface aspects.

I only have a minor issue related to predict() and coverage.

@mrshirts do you sign off on the science?

orbeckst · 2018-12-14T20:08:44Z

src/alchemlyb/estimators/bar_.py

+
+        return self
+
+    def predict(self, u_ln):


Can we drop it?

(It's neither documented nor tested.)

If you keep then there should be a test that shows that it does nothing.

This is comes from the implementation of MBAR which I used as a starting point. I don't find any usages of this method in the project so let's drop it...

If it's in the MBAR class then we should also remove it from there (in a different PR).

orbeckst · 2018-12-14T20:23:13Z

src/alchemlyb/estimators/bar_.py

+                    dout.append(d_deltas[i:i + j + 1].sum())
+                # Other uncertainties are unknown at this point
+                else:
+                    dout.append(float('NaN'))


Or use np.NaN directly, given that we already use numpy. Not sure what's more Pythonic although float("nan") seems to be the way to go without numpy.

Bogus performance "argument" in favor of numpy:

In [17]: %time float("NaN") CPU times: user 4 µs, sys: 0 ns, total: 4 µs Wall time: 9.06 µs Out[17]: nan In [18]: %time np.NaN CPU times: user 4 µs, sys: 0 ns, total: 4 µs Wall time: 7.87 µs Out[18]: nan

Ok i changed it to the numpy version. Mainly because it avoids the explicit data type conversion.

orbeckst · 2018-12-17T20:37:07Z

Looks good to me from the pure code/api side of things.

Quick approval from @mrshirts (and/or @davidlmobley ) regarding the veracity of the implementation would be highly appreciated! – Essentially, are you satisfied with the evidence that @harlor has shown to demonstrate that this implementation produces correct answers (i.e., same answer as alchemical-analysis)?

orbeckst · 2019-01-07T22:57:35Z

Ping @mrshirts @davidlmobley .

orbeckst · 2019-01-11T20:19:11Z

I haven't heard anything against merging this PR so I am now merging it. If anything else comes up later, please open a new issue.

Thanks @harlor !

davidlmobley · 2019-01-14T22:07:55Z

Yes, I think this is fine. Sorry for the delay; huge series of deadlines.

@hbaumann do you have a set of data from equilibrium free energy calculations you can run through this to doublecheck results seem correct?

harlor added 5 commits October 17, 2018 17:44

Add BAR

fd26742

Correct BAR description

ebff629

BAR Cleanup

e03efc7

Fix typo

408ae05

Remove theta from description

90d6179

harlor added 4 commits November 24, 2018 22:24

Check uncertainties in MBAR tests, add new water particle datasets

2f09798

Add BAR estimator tests

cb27296

Merge branch 'master' into bar_pr

1a3ddaa

Don't use underestimated uncertainties

2d91430

Remove MBAR protocols from BAR estimator

970f18d

orbeckst mentioned this pull request Dec 4, 2018

Nonequilibrium calculations/BAR estimator? #70

Closed

harlor mentioned this pull request Dec 5, 2018

BAR estimator #28

Closed

Merge branch 'master' into bar_pr

bb813e3

orbeckst requested changes Dec 14, 2018

View reviewed changes

orbeckst requested a review from mrshirts December 14, 2018 20:30

harlor added 2 commits December 15, 2018 14:20

Some cleanup

b110f16

Merge branch 'bar_pr' of github.com:harlor/alchemlyb into bar_pr

d572e9e

orbeckst approved these changes Dec 17, 2018

View reviewed changes

orbeckst merged commit 4f1509d into alchemistry:master Jan 11, 2019

dotsdl mentioned this pull request Mar 11, 2020

python script to calculate the free energy #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BAR Estimator #60

BAR Estimator #60

harlor commented Oct 17, 2018

codecov-io commented Oct 17, 2018 •

edited

Loading

mrshirts commented Oct 17, 2018

harlor commented Oct 17, 2018

mrshirts commented Oct 17, 2018

dotsdl commented Oct 19, 2018

harlor commented Oct 19, 2018

dotsdl commented Nov 3, 2018

harlor commented Nov 26, 2018

orbeckst commented Dec 14, 2018

orbeckst left a comment

orbeckst Dec 14, 2018

orbeckst Dec 14, 2018

harlor Dec 15, 2018

orbeckst Dec 17, 2018

orbeckst Dec 14, 2018

harlor Dec 15, 2018

orbeckst commented Dec 17, 2018

orbeckst commented Jan 7, 2019

orbeckst commented Jan 11, 2019

davidlmobley commented Jan 14, 2019

BAR Estimator #60

BAR Estimator #60

Conversation

harlor commented Oct 17, 2018

codecov-io commented Oct 17, 2018 • edited Loading

Codecov Report

mrshirts commented Oct 17, 2018

harlor commented Oct 17, 2018

mrshirts commented Oct 17, 2018

dotsdl commented Oct 19, 2018

harlor commented Oct 19, 2018

dotsdl commented Nov 3, 2018

harlor commented Nov 26, 2018

orbeckst commented Dec 14, 2018

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Dec 14, 2018

Choose a reason for hiding this comment

orbeckst Dec 14, 2018

Choose a reason for hiding this comment

harlor Dec 15, 2018

Choose a reason for hiding this comment

orbeckst Dec 17, 2018

Choose a reason for hiding this comment

orbeckst Dec 14, 2018

Choose a reason for hiding this comment

harlor Dec 15, 2018

Choose a reason for hiding this comment

orbeckst commented Dec 17, 2018

orbeckst commented Jan 7, 2019

orbeckst commented Jan 11, 2019

davidlmobley commented Jan 14, 2019

codecov-io commented Oct 17, 2018 •

edited

Loading