alchemistry / alchemlyb

the simple alchemistry library
https://alchemlyb.readthedocs.io
BSD 3-Clause "New" or "Revised" License
189 stars 49 forks source link

Fix the dE method for AMBER #300

Closed xiki-tempula closed 1 year ago

xiki-tempula commented 1 year ago

Fix #299

codecov[bot] commented 1 year ago

Codecov Report

Merging #300 (1d0c750) into master (045f85d) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #300   +/-   ##
=======================================
  Coverage   98.74%   98.74%           
=======================================
  Files          26       26           
  Lines        1747     1749    +2     
  Branches      380      380           
=======================================
+ Hits         1725     1727    +2     
  Misses          2        2           
  Partials       20       20           
Impacted Files Coverage Δ
src/alchemlyb/preprocessing/subsampling.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

xiki-tempula commented 1 year ago

I need some clarification: From the comments in the issue and PR, it sounds as if GROMACS data were correctly processed while others were not — correct me if I misunderstood. What does happen now to GROMACS data after this change?

GROMACS data should not be changed. The numerical tests with regard to gromacs is not changed.

For a simulation with lambda=0.5, the Gromacs has Lambda 0 0.5 1.0
1 -1 0 1

Previously, we just take data[lambda=1.0] = 1 We now do data[lambda=1.0] - data[lambda=0.5] = 1 - 0 = 1

xiki-tempula commented 1 year ago

Sorry, I just noticed that I'm wrong. For Gromacs, it is true that the column corresponding to the current lambda is 0. However, when we read in the reduced potential, it is the dE corrected by the pV, so this changes the gromacs result as well but to the good side.

xiki-tempula commented 1 year ago

@orbeckst Thanks for the review. I have fixed the doc.

orbeckst commented 1 year ago

Thanks @xiki-tempula !