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

DrDomenicoMarson · 2022-09-23T13:35:19Z

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 · 2022-09-23T13:45:58Z

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%)`	⬇️
src/alchemlyb/parsing/amber.py	`98.64% <100.00%> (+1.16%)`	⬆️
src/alchemlyb/parsing/gmx.py	`98.20% <100.00%> (+0.02%)`	⬆️
src/alchemlyb/parsing/gomc.py	`93.33% <100.00%> (+0.12%)`	⬆️
src/alchemlyb/parsing/namd.py	`99.18% <100.00%> (+0.01%)`	⬆️

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

xiki-tempula

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

src/alchemlyb/parsing/amber.py

xiki-tempula · 2022-09-25T11:00:15Z

src/alchemlyb/parsing/amber.py

@@ -252,63 +248,94 @@ def extract_u_nk(outfile, T):
        Temperature in Kelvin at which the simulations were performed;
        needed to generated the reduced potential (in units of kT)

-    Returns
+    Returns a dictionary with elements:


Please don't change this as this would break sphinx.
Just say return a dictionary. Might also be good to check if the doc builds correctly.

Returns ------- Dict A dictionary with keys of 'u_nk', which is a pandas DataFrame ....

I'm sorry, I didn't know about that :-)

I edited the docstring for every new extract functions, and added to the docs, now the extract function appears correctly when the docs are built.

src/alchemlyb/parsing/gmx.py

src/alchemlyb/parsing/gomc.py

src/alchemlyb/parsing/namd.py

src/alchemlyb/tests/parsing/test_amber.py

src/alchemlyb/tests/parsing/test_gomc.py

DrDomenicoMarson · 2022-09-25T15:22:40Z

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% :-).

xiki-tempula

The current one looks reasonable to me. Just some comments.

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 do understand that some of the code are not being covered by the previous tests, do you mind adding # pragma: no cover to bump the coverage?

DrDomenicoMarson · 2022-09-25T16:15:40Z

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

xiki-tempula · 2022-09-25T16:29:36Z

@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 · 2022-09-25T16:37:47Z

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

DrDomenicoMarson · 2022-09-25T16:58:32Z

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 · 2022-09-25T17:05:20Z

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?)

src/alchemlyb/tests/parsing/test_namd.py

DrDomenicoMarson · 2022-09-25T21:01:27Z

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

I'm happy with the content but we cannot merge it when the RTD fails.

src/alchemlyb/parsing/namd.py

src/alchemlyb/parsing/gomc.py

src/alchemlyb/parsing/gmx.py

src/alchemlyb/parsing/amber.py

xiki-tempula · 2022-09-25T21:07:29Z

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

orbeckst · 2022-09-27T03:52:14Z

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 · 2022-09-27T03:53:31Z

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

DrDomenicoMarson · 2022-09-27T07:36:14Z

@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 alchemistry/alchemtest#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!

xiki-tempula

Do you mind add the versionadded and this PR is ready to go.

xiki-tempula

LGTM will merge if all tests passes.

DrDomenicoMarson · 2022-09-27T07:59:15Z

@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 · 2022-09-27T08:01:33Z

@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 · 2022-09-27T08:09:07Z

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

DrDomenicoMarson added 11 commits September 23, 2022 14:06

merged the extract functions

1c18bbb

amber merged parsers pass all old tests

4e62919

added extract to gmx parser, tests passed

bc77e98

added extract to gomc

745690e

added extract to namd parser

fe34171

added test for the new extract function

d897c69

removed reduntant test for T

bd81563

added test for extract in for gmx

3d6a827

cleaned amber tests

c7d58c6

added extract test for gomc and namd

57dafa5

added to CHANGES

e592104

DrDomenicoMarson mentioned this pull request Sep 25, 2022

AMBER: are we too permissive in reading the files? (and also we are doing a double-reading? #238

Closed

xiki-tempula requested changes Sep 25, 2022

View reviewed changes

DrDomenicoMarson added 4 commits September 25, 2022 17:24

removed support for python<3.6

954effe

emoved type hint to be consistent with the rest

a102a69

fixed the doscring of extract for sphinx

d4b2355

fix testing extract of gomc parser

b879407

xiki-tempula reviewed Sep 25, 2022

View reviewed changes

added # pragma: no cover

b7d18d5

commented two lines in units.py that ruined sphinx

107b282

added description for extract in docs

df95656

xiki-tempula reviewed Sep 25, 2022

View reviewed changes

src/alchemlyb/tests/parsing/test_namd.py Show resolved Hide resolved

xiki-tempula requested changes Sep 25, 2022

View reviewed changes

src/alchemlyb/parsing/namd.py Show resolved Hide resolved

src/alchemlyb/parsing/gomc.py Show resolved Hide resolved

src/alchemlyb/parsing/gmx.py Show resolved Hide resolved

src/alchemlyb/parsing/amber.py Show resolved Hide resolved

DrDomenicoMarson added 2 commits September 25, 2022 23:08

removed a redundant try/except in the test

99f3f69

uncommented the docstring of units.py

448c648

DrDomenicoMarson mentioned this pull request Sep 25, 2022

Why test with exception in test_import? #241

Closed

just to force docs rebuilding

fcbe559

orbeckst linked an issue Sep 27, 2022 that may be closed by this pull request

Why test with exception in test_import? #241

Closed

xiki-tempula requested changes Sep 27, 2022

View reviewed changes

DrDomenicoMarson added 2 commits September 27, 2022 09:40

Added versionadded:: 1.0.0

eaf8f3d

fixed versionadded

71020ee

xiki-tempula approved these changes Sep 27, 2022

View reviewed changes

xiki-tempula merged commit 48a719b into alchemistry:master Sep 27, 2022

DrDomenicoMarson deleted the add-the-single-extract-function-in-parsers branch September 27, 2022 08:13

DrDomenicoMarson mentioned this pull request Sep 27, 2022

Amber parser should be improved #222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DrDomenicoMarson commented Sep 23, 2022

codecov bot commented Sep 23, 2022 •

edited

xiki-tempula left a comment

xiki-tempula Sep 25, 2022

DrDomenicoMarson Sep 25, 2022

DrDomenicoMarson Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

xiki-tempula left a comment

DrDomenicoMarson commented Sep 25, 2022

xiki-tempula commented Sep 25, 2022 •

edited

xiki-tempula commented Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

xiki-tempula left a comment

xiki-tempula commented Sep 25, 2022

orbeckst commented Sep 27, 2022

orbeckst commented Sep 27, 2022

DrDomenicoMarson commented Sep 27, 2022

xiki-tempula left a comment

xiki-tempula left a comment

DrDomenicoMarson commented Sep 27, 2022

xiki-tempula commented Sep 27, 2022

DrDomenicoMarson commented Sep 27, 2022

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

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

Conversation

DrDomenicoMarson commented Sep 23, 2022

codecov bot commented Sep 23, 2022 • edited

Codecov Report

xiki-tempula left a comment

Choose a reason for hiding this comment

xiki-tempula Sep 25, 2022

Choose a reason for hiding this comment

DrDomenicoMarson Sep 25, 2022

Choose a reason for hiding this comment

DrDomenicoMarson Sep 25, 2022

Choose a reason for hiding this comment

DrDomenicoMarson commented Sep 25, 2022

xiki-tempula left a comment

Choose a reason for hiding this comment

DrDomenicoMarson commented Sep 25, 2022

xiki-tempula commented Sep 25, 2022 • edited

xiki-tempula commented Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

DrDomenicoMarson commented Sep 25, 2022

xiki-tempula left a comment

Choose a reason for hiding this comment

xiki-tempula commented Sep 25, 2022

orbeckst commented Sep 27, 2022

orbeckst commented Sep 27, 2022

DrDomenicoMarson commented Sep 27, 2022

xiki-tempula left a comment

Choose a reason for hiding this comment

xiki-tempula left a comment

Choose a reason for hiding this comment

DrDomenicoMarson commented Sep 27, 2022

xiki-tempula commented Sep 27, 2022

DrDomenicoMarson commented Sep 27, 2022

codecov bot commented Sep 23, 2022 •

edited

xiki-tempula commented Sep 25, 2022 •

edited