alchemistry / alchemlyb

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

AMBER: temperature reading in parsers and tests. #225

Closed DrDomenicoMarson closed 2 years ago

DrDomenicoMarson commented 2 years ago

Temperature is required in input for both AMBER parsers (extract_dHdl and extract_u_nk), but also system temperature is already automatically read from the output file (reading temp0). I understand making T optional create a problem with the decorator that adds T to the argument of the output from the parsers, so I think still requiring T as a parameter is fine.

Currently, if temp0 is not found we log that 'Non-constant temperature MD not currently supported.' I don't think that this is right, as temp0 is set even in non-constant temperature MD, so I think that the check as it is is meaningless. As a note, it could be possible to check if the simulation was performed not at constant T, but it's not as easy because this information is written at the end of the file in a format that it's not easy to catch with current SectionParser. This shouldn't be a big problem I think, as the user should know if the simulation he/she performed was done at constant T, and I don't know if other parsers make this check at all (maybe it was a left-over from copying from alchemical-analysis?).

Still, we can benefit from reading temp0, and issue a WARNING to the user if the T in input is different from what is read from the file.

Related to that, I noticed we set T=300 in test_amber.py, but the simulations are run at T=298.0 K, so this should be fixed as well.

RECAP of what can be done:

Let me know if this is OK, I can do it as soon as PR #224 is successfully closed.

orbeckst commented 2 years ago

Sounds all sensible to me. I'd be happy to review a PR that addresses this issue.

DrDomenicoMarson commented 2 years ago

Great, I will address all these issues in some order, one at a time, to make the review process easier!