amber mbar parser #51
Conversation
|
I'll review once you have added the data files to alchemtest alchemistry/alchemtest#20 |
|
@orbeckst done |
|
Thanks, I reviewed alchemistry/alchemtest#21 – if you can quickly address my comments then we can merge the data set in a few minutes. |
|
@orbeckst just updated my alchemistry PR |
|
alchemistry/alchemtest#21 was merged. I will restart the tests so that the new data files are picked up and then we'll see. |
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
- Coverage 98.57% 98.11% -0.47%
==========================================
Files 9 9
Lines 422 478 +56
Branches 78 94 +16
==========================================
+ Hits 416 469 +53
- Misses 3 4 +1
- Partials 3 5 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Tests pass but coverage decreased. Looking at https://codecov.io/gh/alchemistry/alchemlyb/src/713871daad3834ea5895a8d1560d57e6ebbaeadd/src/alchemlyb/parsing/amber.py the lines that are not tested all relate to catching problems with the file format or the data themselves.
Ideally, add additional tests that exercise the checks (see what @shuail did with invalid_files for the TI parser.)
If want to indicate that something is not tested, add # pragma: no cover on the same line as the if statement that leads to the exception or warning.
|
|
||
| file_datum.mbar_energies[lmbda].append(E - E_ref) | ||
|
|
||
| if high_E_cnt: |
There was a problem hiding this comment.
add test file or remove from coverage with #pragma: no cover
src/alchemlyb/parsing/amber.py
Outdated
| if not file_validation(outfile): | ||
| return None | ||
| if not file_datum.have_mbar: | ||
| logging.error('ERROR: No MBAR energies found! Cannot parse file.') |
There was a problem hiding this comment.
SHouldn't this raise an exception? At this point it shouldn't continue, should it?
There was a problem hiding this comment.
It should behave in the same way as the TI parser.
|
@orbeckst alchemistry/alchemtest#22 must be approved before the additional tests I have added can be tested properly |
This PR adds Bace solvated vdw output files from a perturbation that was improperly prepared and has large energies and NaN's at endpoints. The purpose of this example is to increase coverage of tests in Alchemlyb alchemistry/alchemlyb#51. * Added Bace CAT-13d~CAT-17a perturbation with mbar from JACS set and load_bace_example function to access.py * Added load_bace_example to access.py * bzip2 .out files in bace example directory and add to amber.rst file in docs/ * add new bace_improper example for testing of amber MBAR parsing in alchemlyb
|
I merged alchemistry/alchemtest#22 and restarted the Travis tests. Will have a look once the tests are done. |
| file_datum.mbar_lambdas = mbar_lambdas | ||
| clambda_str = '%6.4f' % clambda | ||
|
|
||
| if clambda_str not in mbar_lambdas: |
There was a problem hiding this comment.
This block is not tested. See https://codecov.io/gh/alchemistry/alchemlyb/pull/51/src/src/alchemlyb/parsing/amber.py?before=src/alchemlyb/parsing/amber.py#L218
Put a #pragma: no cover to indicate that we exclude it from test.
There was a problem hiding this comment.
Should be addressed in alchemistry/alchemtest#23
|
@orbeckst should be good to go after alchemistry/alchemtest#24 merge |
|
Merged PR alchemistry/alchemtest#24 and waiting for Travis. |
|
Travis has issues https://www.traviscistatus.com/incidents/c1s4dlyxd4lf so might take a while. |
|
Yea I saw it hanging and was hoping it wasn't my code that caused it... ha |
|
Thanks @brycestx ; I will take it from here. I will rebase and then merge. (Probably also add another |
orbeckst
force-pushed
the
amber-mbar-parse
branch
from
497f0e8
to
5cbca71
Compare
|
I rebased against master and excluded the I'll wait for the tests to finish and then squash & merge. |
- fixes #42 - Added functions to extract u_nk mbar energies from Amber files if they exist. - added test to determine if file was parsed correctly. ix test_amber.py to conform to new load_bace_example added to alchemtest. - small formatting adjustments. - Adding another test "load_bace_improper" to increase coverage - Adding support for exception thrown by improper clambda value for test case. - removed untested portion of amber parser from coverage The "file_validation(outfile)" step in extract_dHdl() is not covered by our tests. I am flagging the "not tested" status explicitly in the code with `#pragma: no cover`.
orbeckst
force-pushed
the
amber-mbar-parse
branch
from
5cbca71
to
9117cc6
Compare
|
I squashed everything into a single commit. |
|
@brycestx merged – thanks for all the work. (The coverage diff was "only" 91% but I looked at the code and it seemed ok to me so I overrode the merge warning.) |
|
@orbeckst hmm any idea why that is. Looking here it looks much better than before.. https://codecov.io/gh/alchemistry/alchemlyb/src/ab1b6573670ab13d09b92ee291e9159a1e3ca860/src/alchemlyb/parsing/amber.py |
|
The relative diff is also 91.66% in the page that you linked – I think we've been looking at the same thing. The overall coverage of the file is still 97.35%. If you want to test the lines that are omitted or marked |
Fixes #42
This pull request adds support for amber mbar parsing for amber .out files by using the extract_u_nk function, which I wrote to primarily mimic the output format of the extract_u_nk gromacs function.
A test_u_nk function was also added to test_amber.py. I am now working on adding a minimal test example to alchemtest.