choderalab / pymbar

Python implementation of the multistate Bennett acceptance ratio (MBAR)
http://pymbar.readthedocs.io
MIT License
235 stars 91 forks source link

problems with gromacs parser in alchemical-free-energy #181

Open vvoelz opened 9 years ago

vvoelz commented 9 years ago

We have found a problem in pymbar/examples/alchemical-free-energy/parsers/parser_gromacs.py with parsing large dhdl.xvg files. In slice_data(), there's a line:

f.len_first, f.len_last = (len(line.split()) for line in unixlike.tailPy(f.filename, 2))

the tailPy routine here is supposed to do a "tail" on the last two lines, and compare the number of fields; if they disagree, then the last line of the xvg file is corrupt and should be ignored.

In our case, we have found that unixlike.tailPy() gives incorrect results, truncating the last line. We suspect this may be because our xvg files have many alchemical intermediates (40 or more), all written in high-precision scientific notation, and unixlike.tailPy() has a character limit (?!)

def tailPy(f, nlines, LENB=1024): <------ ?

I think this could easily be fixed either rewriting unixlike.tailPy or just using the normal python read() or readlines(). --VInce

jchodera commented 9 years ago

@mrshirts: Can you guys take a look at this?

kyleabeauchamp commented 9 years ago

I think there is an outstanding pull request already waiting for review. We should see if that fixes the problem.

On 4/18/15, John Chodera notifications@github.com wrote:

@mrshirts: Can you guys take a look at this?


Reply to this email directly or view it on GitHub: https://github.com/choderalab/pymbar/issues/181#issuecomment-94174240

mrshirts commented 9 years ago

I emailed pavel (who wrote the code) to take a look at it.

On Sat, Apr 18, 2015 at 12:57 PM, Kyle Beauchamp notifications@github.com wrote:

I think there is an outstanding pull request already waiting for review. We should see if that fixes the problem.

On 4/18/15, John Chodera notifications@github.com wrote:

@mrshirts: Can you guys take a look at this?


Reply to this email directly or view it on GitHub: https://github.com/choderalab/pymbar/issues/181#issuecomment-94174240

— Reply to this email directly or view it on GitHub https://github.com/choderalab/pymbar/issues/181#issuecomment-94181011.

kyleabeauchamp commented 9 years ago

We still need someone to merge the pull request. I am not sufficiently familiar with the Gromacs code to review the changes.

wesbarnett commented 9 years ago

Hey I think the pull requests you mention were partly authored by me. Those were just a small fix to the graph output in the alchemical analysis script, not the actual gromacs parser, so I think this is a different issue.

jchodera commented 9 years ago

@mrshirts: Who has primary responsibility for the gromacs parser at this point?

vvoelz commented 9 years ago

@wesbarnett -- not to add more confusion, but are you referring to an output problem where the "vdWaals" free energy is always zero? This is something we consistently get.

--- Coulomb: 654.143 +- 5.676 643.565 +- 6.049 35.005 +- 0.171 9.977 +- 0.165 21.819 +- 0.128 18.144 +- 0.293 vdWaals: 0.000 +- 5.676 0.000 +- 6.049 0.000 +- 0.000 0.000 +- 0.000 0.000 +- 0.000 0.000 +- 0.000 TOTAL: 654.143 +- 8.027 643.565 +- 8.554 35.005 +- 0.171 9.977 +- 0.165 21.819 +- 0.128 18.144 +- 0.293
wesbarnett commented 9 years ago

@vvoelz Yes, that's one of the fixes in the pull request.

mrshirts commented 9 years ago

Who has primary responsibility for the gromacs parser at this point?

I don't think we've entirely decided. Pavel, would you be able do the maintenance for now?

davidlmobley commented 9 years ago

Pavel should be handling this. I'm talking with him today. Sorry for the delay here. We're new to GitHub and I was not monitoring issues on here until now. @wesbarnett - worlds collide. :)

davidlmobley commented 9 years ago

@FEPanalysis (Pavel) and I talked. He is working through issue #179 and #180 and may need test data from @wesbarnett. He also needs permissions to handle merging of pull requests (or we need to appoint someone else who will do it) and we're (@mrshirts @jchodera) sorting that out via e-mail now.

He also has a fix to @vvoelz 's issue #181 and will submit it as a pull request once it's sorted out who is handling merges.

davidlmobley commented 9 years ago

@vvoelz - we think this is resolved by https://github.com/MobleyLab/alchemical-analysis/pull/11. Can you check that it fixes your issue?