Correct TI error estimation II #61
Conversation
Codecov Report
@@ Coverage Diff @@
## master #61 +/- ##
==========================================
+ Coverage 98.17% 98.18% +0.01%
==========================================
Files 9 9
Lines 493 497 +4
Branches 100 100
==========================================
+ Hits 484 488 +4
Misses 4 4
Partials 5 5
Continue to review full report at Codecov.
|
|
So, it does reproduce the values in alchemical-analysis? Specifically, what data sets were used to test, not just that the comparison was done. Can you post the output snippets before/after? Does it work for uneven lambda? |
|
I'm currently working on a command line "frontend" for alchemlyb that I can use similarly to alchemical-analysis: https://github.com/harlor/flamel and I used that to compare to alchemical-analysis. I used different input files for testing (from NVT and NPT simulations) and compared the results for single steps, segments and in total. I show example outputs in the bottom of https://github.com/harlor/flamel/blob/master/README.md that were made with these modifications. Of course I can also show outputs without these modifications - if this is helpful. Yes, it should work with an even and uneven number of lambdas. |
"Different input files" is not really sufficient. The locations of the specific files and the commands really are what are needed to be reproducible.
OK, make sure to link output like that in the PR description, so people can see what is being referred to.
I would HIGHLY recommend getting in contact with @davidlmobley about coordinating. The original plan was for alchemical-analysis to be a front end to alchemlyb. Even if it makes sense to rewrite from scratch, it would be useful to you to be able to copy over any of the graphing functionality for alchemical-analysis. I.e. better that such improvements involve everyone that is working on them, rather than duplicating effort. |
|
Thanks for putting this together @harlor! I'll be able to take some time this weekend to review, I think. I'll need to step through the changes to make sure I understand what's being done here, because I'm not sure I understand what's wrong with the existing implementation. |
True. Assuming you have a proper python environment with pip, numpy, pymbar (I use a miniconda enviroment for that) and you start in an empty directory, a reproducible procedure is: 0. Download and install current alchemlyb 1. Download flamel: 2. Get some input files: 3. Test alchemlyb TI estimator using flamel: You should get an output like: 4. Download and install alchemlyb with this PR 5. Test alchemlyb TI estimator using flamel agian: You should now get an output like: 6. Compare with alchemical analysis: You should get an output like: |
I tried to explain this in: #58 |
|
Understood, will review. The gist as I understand it is that since we are actually summing integrals between |
No. Summing integrals between <dH/dl>s can always be written as a weighted sum of <dH/dl>s. There is no need to include correlations (assuming the dH/dl come from different simulations - if expanded ensemble, a bit more complicated). I think that @harlor's work fixed, this, though (if he ended up matching the alchemical-analysis code) results. |
Agree - That's how I derived this code. |
|
@harlor many thanks for your contribution. But would you mind opening an actual issue for the bug and then reference the issue in the PR? This is important for properly keeping track of problems, both for the CHANGELOG and for the broader community – people tend to browse issues for problems, not so much the PRs. Thanks a lot! |
@mrshirts then please do a formal review and accept it or request changes. You can always add comments and ping @dotsdl or myself with questions regarding implementation details. But ultimately someone has to take the responsibility for agreeing to take the code. You seem to be in the best position to do so. |
|
@harlor you don't have a variety of different datasets on hand? We don't usually do TI here so I don't have many handy, but OTOH GROMACS usually writes the data so probably we can find some. Presumably data from any code will serve if the issue is analysis; perhaps @sukanyasasmal or @hannahbaumann have some TI data handy. |
Was already discussed what https://github.com/alchemistry/alchemtest should contain to have a reasonable test coverage for different scenarios? I guess that's the right place to collect some data sets. |
|
Yes, exactly, we should end up with adequate data in |
|
The TI tests now also check the uncertainty. I also compared the values for the water particle datasets with alchemical-analysis using:
|
|
@mrshirts By skipping the autocorrelation analysis in alchemical-analysis it is also possible to compare the uncertainty results. What I did for the water particle datasets. @orbeckst The uncertainties are also tested now - the reference values in the tests can also be reproduced by alchemical-analysis |
|
I take @mrshirts comment #61 (comment)
as his approval. |
Sorry for messing up #58.
In summary: I believe, that the TI error estimation in alchemlyb does not reproduce the value that alchemical-analysis calculates.
This PR should change the TI error estimation such that it reproduces the value alchemical-analysis calculates.