AMBER parser now compare user-provided T with what's read from AMBER files

[DrDomenicoMarson]

This PR addresses issue #225, now:

• we require T in input for the parser
• we still read temp0 from AMBER output
• the WARNING about non-constant T simulation is now a WARNING (the check could also be removed)
• the parser raises a WARNING if temp0 != T (two new tests now assert that)
• now the tests in test_amber.py use the correct T=298.0

[codecov[bot]]

Codecov Report

Merging #232 (af57e5d) into master (97ac3d9) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #232   +/-   ##
=======================================
  Coverage   98.17%   98.18%           
=======================================
  Files          25       25           
  Lines        1646     1655    +9     
  Branches      347      349    +2     
=======================================
+ Hits         1616     1625    +9     
  Misses          7        7           
  Partials       23       23           

Impacted Files	Coverage Δ
src/alchemlyb/parsing/amber.py	97.52% <100.00%> (+0.09%)	:arrow_up:

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

[DrDomenicoMarson]

a ValueError should be raised

So do you want I change the WARNING to a proper ValueError that stops the parsing completely?

I agree this will be a more robust behavior, I just raised a warning to not break any existing script other users could be using right now!

We can wait for some input from @orbeckst regarding the rising of a WARNING/ERROR maybe?

[xiki-tempula]

Thanks for the comments and the commits. I think it is more like what kind of use cases that we want to handle. There are two user cases that which the user-supplied temperature could be different from the temperature in the file.

1. If the user supplied the wrong temperature and the user didn't know about this.
2. The user supplies the temperature that is not the same as the one that is in the file and the user wishes to overwrite the temperature in the file.

My personal opinion is that I think for case 1, we raise an exception and ask the user to input the right temperature as we want the user to know what they are doing before going forward. For case 2, I think we might not want the extract_ functions to handle this as the users who want to do this are expert users and they could modify the dataframe by themselves. For novice users, this might do more harm than good.

It is concerning that the other parsers (like the gmx one) will just use the user-supplied temperature without checking if the user supplied temperature matches the temperature in the file or not.

By the way, it might be good to not resolve the conversation and allow the reviewer to resolve the conversations. It might be my habit but I kind of use open conversation to track whether a comment has been resolved or not. Thanks for the contributions.

[DrDomenicoMarson]

1. If the user supplied the wrong temperature and the user didn't know about this.
2. The user supplies the temperature that is not the same as the one that is in the file and the user wishes to overwrite the temperature in the file.

My personal opinion is that I think for case 1, we raise an exception and ask the user to input the right temperature as we want the user to know what they are doing before going forward. For case 2, I think we might not want the extract_ functions to handle this as the users who want to do this are expert users and they could modify the dataframe by themselves. For novice users, this might do more harm than good.

Yeah, these are the two cases I was thinking about! So you say we raise an exception, so the beginner will be notified he has to check T, while the advanced user could just provide the same temperature as the input file and THEN modify the obtained dataframe according to its needs, right?

I'm sorry, I didn't know I had to leave the conversations unresolved :-)

[xiki-tempula]

So you say we raise an exception, so the beginner will be notified he has to check T, while the advanced user could just provide the same temperature as the input file and THEN modify the obtained dataframe according to its needs, right?

Yes.

I'm sorry, I didn't know I had to leave the conversations unresolved :-)

No worry, I didn't know this as well until other people told me this.

[DrDomenicoMarson]

Done, now using a wrong T raises the correct warning, and I updated the two tests to catch that.

[DrDomenicoMarson]

Now lots of tests are failing because they used the wrong T :-) I'm on it.... :-)

[DrDomenicoMarson]

While fixing other tests I catched these warnings in alchemlyb/tests/test_ti_estimators.py

../../../usr/lib/python3/dist-packages/joblib/backports.py:7
  /usr/lib/python3/dist-packages/joblib/backports.py:7: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives
    from distutils.version import LooseVersion

src/alchemlyb/tests/test_ti_estimators.py::test_TI_separate_dhdl_no_pertubed
  /home/domenico/sources/alchemlybDM/src/alchemlyb/tests/test_ti_estimators.py:160: FutureWarning: The 'inplace' keyword in DataFrame.set_index is deprecated and will be removed in a future version. Use `df = df.set_index(..., copy=False)` instead.
    dHdl.set_index('bound-lambda', append=True, inplace=True)

-- Docs: https://docs.pytest.org/en/stable/warnings.html

with pandas version 1.5.0rc0

[DrDomenicoMarson]

Now all tests should work, I changed T to match what was in the AMBER files, and changed E/err expected values accordingly!

[DrDomenicoMarson]

LGTM. Thanks for addressing the comments and fixing the tests as well. Will merge once all the tests passes.

All passed now :-)