Skip to content
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

Merged
merged 4 commits into from
Mar 21, 2019
Merged

NAMD parser for FEP files #72

merged 4 commits into from
Mar 21, 2019

Conversation

vtlim
Copy link
Collaborator

@vtlim vtlim commented Feb 28, 2019

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 in alchemtest.

@vtlim
Copy link
Collaborator Author

vtlim commented Feb 28, 2019

For issue #7

@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #72 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/alchemlyb/estimators/bar_.py 100% <ø> (ø) ⬆️
src/alchemlyb/parsing/namd.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9857cd6...d668819. Read the comment docs.

@orbeckst
Copy link
Member

@vtlim sorry, been busy, and didn't see your PR. Will have a look at it, eventually (unless anyone else beats me to it). Feel free to ping me @orbeckst

Copy link
Member

@orbeckst orbeckst left a 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:

  1. add a file to docs/parsing/ (follow the examples), name it alchemlyb.parsing.namd.rst
  2. add "namd" to the bottom of docs/parsing.rst
  3. 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 level index.html)




def extract_dHdl(fepout):
Copy link
Member

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.

Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

@orbeckst orbeckst merged commit 4ed712e into alchemistry:master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants