Fix the dE method for AMBER

[xiki-tempula]

Fix #299

[codecov]

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.

[orbeckst]

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?

Also, if it's a Change then there should be a versionchanged in u_nk2series(). However, it sounds more like a fix to me to bring the code in line with the documentation. What do you think it should be classified as?

[orbeckst]

Is this really a change or shouldn't this be listed as a Fix because it now does what is advertised?

[xiki-tempula]

Yes, this should be a fix. Sorry.

[xiki-tempula]

versionchanged added

[xiki-tempula]

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]

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.

[orbeckst]

Thank for the explanation and digging into what effect the fix will have.

Could you please improve the documentation a little bit? I just want to give users a fair chance to understand where results are changing.

[orbeckst]

Can you be more specific what bug was fixed — instead of the word "Change", something like "fixed to now use .... (note: will lead to different results than before) (issue ....)"

If I am a user and look through CHANGES I would want to understand if anything affects me.

[orbeckst]

add "... instead of ..." so that people have an idea what behavior changed

[xiki-tempula]

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

[orbeckst]

Thanks @xiki-tempula !