Fixing broken top reader #110

jandom · 2017-03-22T17:00:19Z

Re-enable test cases,
Exclusions-related bug,
Handling improper overrides in scaling code,
Atom naming to match conf.gro file in amber03w tests.

- Re-enable test cases, - Exclusions-related bug, - Handling improper overrides in scaling code, - Atom naming to match conf.gro file in amber03w tests.

orbeckst · 2017-03-22T17:14:34Z

Does this PR replace #97 ?

jandom · 2017-03-22T17:15:21Z

Negative, it just fixes the current develop branch and hopefully resolves #61

orbeckst · 2017-03-22T17:16:15Z

Ok.

I'll review when tests pass. Please ping me when ready.

jandom · 2017-03-22T17:19:40Z

hmm, it's strange the base branch develop passes but these tests fail in ways that are not related to my code, i think

@orbeckst i'm no longer super-familiar with this code maybe you can shed some light on the failures?

jandom · 2017-03-22T17:25:21Z

Maybe i'm doing sth wrong but on my end everything passes on development and fix/61-top-reader-broken

$ py.test gromacs
=============================================================================================== test session starts ================================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /media/domanski/STORAGE_4/workspace/GromacsWrapper/gromacs, inifile: 
collected 78 items 

gromacs/tests/test_cbook.py .
gromacs/tests/test_run.py .........
gromacs/tests/test_setup.py .
gromacs/tests/test_tools.py ......................
gromacs/tests/test_utilities.py ........................
gromacs/tests/test_version.py ...
gromacs/tests/fileformats/top/test_amber03star.py xx....
gromacs/tests/fileformats/top/test_amber03w.py xxx...
gromacs/tests/fileformats/top/test_charmm22.py x.....

====================================================================================== 72 passed, 6 xfailed in 13.78 seconds =======================================================================================
domanski@dcd:~/workspace/GromacsWrapper$ git checkout fix/61-top-reader-broken
Switched to branch 'fix/61-top-reader-broken'
domanski@dcd:~/workspace/GromacsWrapper$ py.test gromacs
=============================================================================================== test session starts ================================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /media/domanski/STORAGE_4/workspace/GromacsWrapper/gromacs, inifile: 
collected 78 items 

gromacs/tests/test_cbook.py .
gromacs/tests/test_run.py .........
gromacs/tests/test_setup.py .
gromacs/tests/test_tools.py ......................
gromacs/tests/test_utilities.py ........................
gromacs/tests/test_version.py ...
gromacs/tests/fileformats/top/test_amber03star.py ......
gromacs/tests/fileformats/top/test_amber03w.py ......
gromacs/tests/fileformats/top/test_charmm22.py ......

============================================================================================ 78 passed in 19.95 seconds ============================================================================================

jandom · 2017-03-22T17:33:37Z

@orbeckst this failure mode seems to be occurring also on other PRs, such as
#101 and in related to numkit integration testing?

jandom · 2017-03-22T18:07:00Z

@orbeckst @dotsdl broken the travis.ci for develop branch just by restarting the build
https://travis-ci.org/Becksteinlab/GromacsWrapper/jobs/198105261

This is also described here
rbgirshick/py-faster-rcnn#481

It is some sort of numpy version problem

dotsdl · 2017-03-22T18:33:17Z

@jandom do you still have issues with tmpdirs? Is there anything here you'd like from me?

jandom · 2017-03-22T21:19:09Z

@dotsdl yeah i do – i'd like to understand how they can be used if a test fails? maybe i'm doing something inefficiently, but i suspect many other old-geezers do it the same :P I basically had to engineer the failed test case in a separate directory copying the input files into that location.

I imagine that there is a way where I can go to the tmpdir used by the failed test and inspect what's going on there?

codecov-io · 2017-03-22T22:50:33Z

Codecov Report

Merging #110 into develop will decrease coverage by 0.22%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##           develop    #110      +/-   ##
==========================================
- Coverage    44.32%   44.1%   -0.23%     
==========================================
  Files           26      26              
  Lines         4823    4834      +11     
  Branches       695     699       +4     
==========================================
- Hits          2138    2132       -6     
- Misses        2568    2583      +15     
- Partials       117     119       +2

Impacted Files	Coverage Δ
gromacs/fileformats/top.py	`86.57% <50%> (-0.54%)`	⬇️
gromacs/scaling.py	`91.39% <72.22%> (-2.94%)`	⬇️
gromacs/core.py	`53.87% <0%> (-3.45%)`	⬇️

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 9fe10ee...173ed56. Read the comment docs.

dotsdl · 2017-03-22T22:51:39Z

@jandom for your test to use a tmpdir at all, you need to explicitly use it in your test. Best way is to wrap the body of your test in a with tmpdir.as_cwd(): context manager as it's done here. These tmpdirs on a Linux system will show up by default in /tmp, and pytest won't delete them outright (it keeps the most recent 3).

jandom · 2017-03-22T23:02:24Z

@dotsdl many thanks! all clear now :) 👍

dotsdl · 2017-03-22T23:14:20Z

@jandom are you ready for review?

jandom · 2017-03-22T23:17:14Z

@orbeckst ready for review here

orbeckst · 2017-03-22T23:41:24Z

@dotsdl please feel free to review, I don't have time today.

orbeckst

Just minor format things and comment clean up.

orbeckst · 2017-03-23T05:36:25Z

gromacs/tests/fileformats/top/test_amber03star.py

@@ -17,10 +17,10 @@ class TestAmber03star(TopologyTest):
        conf = datafile('fileformats/top/amber03star/conf.gro')
        molecules = ['Protein', 'SOL', 'IB+', 'CA', 'CL', 'NA', 'MG', 'K', 'RB', 'CS', 'LI', 'ZN']

-        @pytest.mark.xfail(raises=ValueError, reason="Not currently maintained. See #61.")
+        #@pytest.mark.xfail(raises=ValueError, reason="Not currently maintained. See #61.")


remove commented out xfail

orbeckst · 2017-03-23T05:36:30Z

gromacs/tests/fileformats/top/test_amber03star.py

        def test_read_write(self, tmpdir):
                super(TestAmber03star, self).test_read_write(tmpdir)

-        @pytest.mark.xfail(raises=GromacsError, reason="Not currently maintained. See #61.")
+        #@pytest.mark.xfail(raises=GromacsError, reason="Not currently maintained. See #61.")


remove commented out xfail

orbeckst · 2017-03-23T05:37:23Z

gromacs/scaling.py

-        for k in top.dict_molname_mol.keys():
-                if k in ["Protein", "SOL", "Ion" ]: continue
-                del top.dict_molname_mol[k]
+        #for k in top.dict_molname_mol.keys():


Can this be removed or is there a good reason to keep the commented-out code around?

orbeckst · 2017-03-23T05:38:09Z

gromacs/scaling.py

-                mol = scale_dihedrals(mol, dihedraltypes, args.scale_protein, banned_lines)
-                mol = scale_impropers(mol, impropertypes, 1.0, banned_lines)
+        for molname_mol in top.dict_molname_mol.keys():
+            if not 'Protein' in molname_mol: continue


I prefer putting continue on a separate line (as well as having explicit for blocks).

orbeckst · 2017-03-23T05:38:46Z

gromacs/scaling.py

-                for at in mol.atoms: at.charge *= math.sqrt(args.scale_protein)
-                mol = scale_dihedrals(mol, dihedraltypes, args.scale_protein, banned_lines)
-                mol = scale_impropers(mol, impropertypes, 1.0, banned_lines)
+        for molname_mol in top.dict_molname_mol.keys():


YOu don't need .keys() here. in will do this automatically for a dict.

orbeckst · 2017-03-23T05:39:39Z

gromacs/scaling.py

+        for molname_mol in top.dict_molname_mol.keys():
+            if not 'Protein' in molname_mol: continue
+            mol = top.dict_molname_mol[molname_mol]
+            for at in mol.atoms: at.charge *= math.sqrt(args.scale_protein)


- break if..continue and for loop into separate blocks, - remove commented lines and annotations, - handle impropers and regular dihedrals in the same way.

jandom · 2017-03-23T08:58:39Z

@orbeckst this is now all resolved - thank you for all the comments

orbeckst · 2017-03-23T10:31:11Z

Thanks a lot!!

Fixing broken top reader

66bb3e2

- Re-enable test cases, - Exclusions-related bug, - Handling improper overrides in scaling code, - Atom naming to match conf.gro file in amber03w tests.

orbeckst self-requested a review March 22, 2017 17:15

dotsdl mentioned this pull request Mar 22, 2017

Small fix for recent changes in numpy. #114

Merged

orbeckst mentioned this pull request Mar 22, 2017

fix numkit float/int indexing #115

Closed

orbeckst requested a review from dotsdl March 22, 2017 23:41

orbeckst approved these changes Mar 23, 2017

View reviewed changes

Review-related comments resolved

173ed56

- break if..continue and for loop into separate blocks, - remove commented lines and annotations, - handle impropers and regular dihedrals in the same way.

orbeckst merged commit 7d3ffef into Becksteinlab:develop Mar 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing broken top reader #110

Fixing broken top reader #110

jandom commented Mar 22, 2017

orbeckst commented Mar 22, 2017

jandom commented Mar 22, 2017

orbeckst commented Mar 22, 2017

jandom commented Mar 22, 2017 •

edited

jandom commented Mar 22, 2017

jandom commented Mar 22, 2017

jandom commented Mar 22, 2017 •

edited

dotsdl commented Mar 22, 2017

jandom commented Mar 22, 2017

codecov-io commented Mar 22, 2017 •

edited

dotsdl commented Mar 22, 2017

jandom commented Mar 22, 2017

dotsdl commented Mar 22, 2017

jandom commented Mar 22, 2017

orbeckst commented Mar 22, 2017

orbeckst left a comment

orbeckst Mar 23, 2017

orbeckst Mar 23, 2017

orbeckst Mar 23, 2017

orbeckst Mar 23, 2017

orbeckst Mar 23, 2017

orbeckst Mar 23, 2017

jandom commented Mar 23, 2017

orbeckst commented Mar 23, 2017

Fixing broken top reader #110

Fixing broken top reader #110

Conversation

jandom commented Mar 22, 2017

orbeckst commented Mar 22, 2017

jandom commented Mar 22, 2017

orbeckst commented Mar 22, 2017

jandom commented Mar 22, 2017 • edited

jandom commented Mar 22, 2017

jandom commented Mar 22, 2017

jandom commented Mar 22, 2017 • edited

dotsdl commented Mar 22, 2017

jandom commented Mar 22, 2017

codecov-io commented Mar 22, 2017 • edited

Codecov Report

dotsdl commented Mar 22, 2017

jandom commented Mar 22, 2017

dotsdl commented Mar 22, 2017

jandom commented Mar 22, 2017

orbeckst commented Mar 22, 2017

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Mar 23, 2017

Choose a reason for hiding this comment

orbeckst Mar 23, 2017

Choose a reason for hiding this comment

orbeckst Mar 23, 2017

Choose a reason for hiding this comment

orbeckst Mar 23, 2017

Choose a reason for hiding this comment

orbeckst Mar 23, 2017

Choose a reason for hiding this comment

orbeckst Mar 23, 2017

Choose a reason for hiding this comment

jandom commented Mar 23, 2017

orbeckst commented Mar 23, 2017

jandom commented Mar 22, 2017 •

edited

jandom commented Mar 22, 2017 •

edited

codecov-io commented Mar 22, 2017 •

edited