Introduced the "extract" function to all the parsers, with relative tests

[DrDomenicoMarson]

Ok, I hope not to have broken anything with this PR :-) I added the extract(file, T) function to all the existing parsers, with a relative test for each parser.

The only parser that was heavily hit by this PR is the AMBER parser, as was mentioned in issue #222 . Here I had to make different adjustments, on my machine all tests are passing through. I took the opportunity to make also some small, cosmetic, changes to the code in the amber parser (just a couple of them, like removing the unnecessary 'class SomeThing(Object').

PS: brace yourself, I now have a copy of Amber2022, and it seems the way dHdl/MBAR data is written has changed, so the parser will need an urgent update to handle AMBER <2022 | >=2022)

[codecov[bot]]

Codecov Report

Merging #240 (71020ee) into master (cb965ad) will increase coverage by 0.11%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
+ Coverage   98.44%   98.56%   +0.11%     
==========================================
  Files          25       25              
  Lines        1673     1672       -1     
  Branches      357      353       -4     
==========================================
+ Hits         1647     1648       +1     
+ Misses          5        3       -2     
  Partials       21       21              

Impacted Files	Coverage Δ
src/alchemlyb/postprocessors/units.py	100.00% <ø> (ø)
src/alchemlyb/parsing/__init__.py	95.00% <100.00%> (-5.00%)	:arrow_down:
src/alchemlyb/parsing/amber.py	98.64% <100.00%> (+1.16%)	:arrow_up:
src/alchemlyb/parsing/gmx.py	98.20% <100.00%> (+0.02%)	:arrow_up:
src/alchemlyb/parsing/gomc.py	93.33% <100.00%> (+0.12%)	:arrow_up:
src/alchemlyb/parsing/namd.py	99.18% <100.00%> (+0.01%)	: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]

These are great changes. This is an initial review. Do you mind bumping the coverage a bit? Thanks,

This is unfortunately impossible right now. I made a detailed check, the lines that are not covered by the tests are unaffected/not created by my modifications (so the coverage is the same as before, just the total number of lines is reduced as I removed an entire function in the amber parser). I would like not to touch the other parsers, as I'm not an expert in doing TI/FEP with other software. In issue #241 I pointed out a place where we could increase coverage.

In the amber parser, I could easily reach a coverage of 99%, but the issues in alchemtest have to be addressed before, as I need 3 "properly" created files. Currently, in the amber parser, we have 4 lines that are not covered by the tests (and were not covered also before this PR), 3 of which are easily addressable with the 3 new files, while for the 4th I have to go deeper in understanding a function that was already there and that I don't currently understand 100% :-).

[DrDomenicoMarson]

# pragma: no cover added to AMBER parser, it feels like cheating though :-)

[xiki-tempula]

@DrDomenicoMarson It would be best that all lines are covered. Obviously, some lines are more difficult to cover and offer little value to cover, so it is fine that those are not being covered. What we don't want is a surprise where we thought that some lines of code are being covered but they are not, thus, # pragma: no cover will acknowledge that this line is not covered.

[xiki-tempula]

Have you checked that sphinx builds? As RTD is failing?

[DrDomenicoMarson]

Have you checked that sphinx builds? As RTD is failing?

I just re-checked, in my computer if I go inside alchemtest/docs and run make html it creates a perfectly fine HTML file, with the extract methods reported. sphinx gives me some warning, but concerning alchemlyb.workflows.* stuff, so I supposed those were already there!

[DrDomenicoMarson]

Ok, digging deeper the problem was unrelated to this PR. sphinx was complaining about the first lines in `postprocessors/units.py' my sphinx (which is v4.3.2 vs the v5 here) didn't complain about that. For now I tried just commenting the offending lines (maybe a better solution exists?)

[DrDomenicoMarson]

Ok, it's a good thing this happened for us, as I fixed also a couple of other things in the docs regarding extract, but the problem is not on us, it is from the sphinx side, as [https://github.com/readthedocs/sphinx_rtd_theme/issues/1343]

[xiki-tempula]

Ok, probably give it a day or two and see if it got fixed by sphinx.

[orbeckst]

PS: brace yourself, I now have a copy of Amber2022, and it seems the way dHdl/MBAR data is written has changed, so the parser will need an urgent update to handle AMBER <2022 | >=2022)

Argh. Sigh. Thanks for raising #242 , that's the right approach!

[orbeckst]

Regarding coverage: Are you saying that if we merged https://github.com/alchemistry/alchemtest/pull/71 then you could increase the test coverage here and avoid no-cover pragmas?

[DrDomenicoMarson]

@orbeckst Yeah, I can cover all lines of parsers/amber.py except 1 line, but maybe it's better not in this PR, I'd prefer to merge this with the pragma, then merge https://github.com/alchemistry/alchemtest/pull/71 and I'll submit a PR that removes pragma.

@xiki-tempula they fixed the bug in sphinx, so now all checks are passed here!

[DrDomenicoMarson]

@xiki-tempula what do you think about the #Note in the namd.py parser? Should we keep it like that, with just returning u_nk, or should we also add the item dHdl: None to be consistent with the other parsers?

[xiki-tempula]

@DrDomenicoMarson I think at the moment both of them are good. I mean it is either if 'dhdl' in extract() or if extract()['dhdl'] is None. Both of them are quite intuitive and both of them are ok with me.

[DrDomenicoMarson]

Ok, so we can just leave it like it's now, I remove the commented line in the test then, before the final merging!