-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NAMD parser for FEP files #72
Conversation
For issue #7 |
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 98.33% 98.42% +0.09%
==========================================
Files 10 11 +1
Lines 539 572 +33
Branches 105 109 +4
==========================================
+ Hits 530 563 +33
Misses 4 4
Partials 5 5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very nice already. Apart from the one comment that I found on a quick parse, please also integrate your code into the docs:
- add a file to docs/parsing/ (follow the examples), name it
alchemlyb.parsing.namd.rst
- add "namd" to the bottom of docs/parsing.rst
- Try building the docs with
python setup.py build_sphinx
and look at the locally generated HTML files (see output to find path to the top levelindex.html
)
src/alchemlyb/parsing/namd.py
Outdated
|
||
|
||
|
||
def extract_dHdl(fepout): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not implemented then leave it out, better than a pass
and weird failures down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, removing it will improve your coverage ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I'll take out this placeholder function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
This pull request parses NAMD .fepout files to extract the work values (
dE
column) into a pandas dataframe in line with the overall API proposal. I also added a few tests to check the dimensions of the imported data and verify the results of BAR analysis on the data inalchemtest
.