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

[Review Needed]Pytest Style analysis/test_rms.py #1610

Merged
merged 6 commits into from Aug 23, 2017

Conversation

Projects
None yet
4 participants
@utkbansal
Copy link
Member

utkbansal commented Aug 8, 2017

@kain88-de @richardjgowers @jbarnoud Need help with test_with_superposition_smaller and test_with_superposition_equal

Error for test_with_superposition_equal

MDAnalysisTests/analysis/test_rms.py:156 (Testrmsd.test_with_superposition_equal)
self = <testsuite.MDAnalysisTests.analysis.test_rms.Testrmsd object at 0x10da1ca90>
p_first = <AtomGroup with 3341 atoms>, p_last = <AtomGroup with 3341 atoms>

    def test_with_superposition_equal(self, p_first, p_last):
        align.alignto(p_first, p_last)
        A = p_first.positions
        B = p_last.positions
        rmsd = rms.rmsd(A, B)
        rmsd_superposition = rms.rmsd(A, B, center=True, superposition=True)
>       assert_almost_equal(rmsd, rmsd_superposition)
E       AssertionError: 
E       Arrays are not almost equal to 7 decimals
E        ACTUAL: 3.3291879348562332e-08
E        DESIRED: 1.2660351092866358e-06

test_rms.py:163: AssertionError 
@jbarnoud

This comment has been minimized.

Copy link
Contributor

jbarnoud commented Aug 10, 2017

I did not read the all thing, but you have two fixtures called u2 in the same class (just following each other), it may cause issues.

def u(self):
u = mda.Universe(PSF, DCD)
u.trajectory[2]
u.trajectory[0]

This comment has been minimized.

@kain88-de

kain88-de Aug 10, 2017

Member

This has no affect. The trajectory will be at frame 0 no matter what.

def u2(self):
u = mda.Universe(PSF, DCD)
u.trajectory[-2]
u.trajectory[-1]

This comment has been minimized.

@kain88-de

kain88-de Aug 10, 2017

Member

This will just set you to the last frame. I think you can remove this fixture.

@kain88-de

This comment has been minimized.

Copy link
Member

kain88-de commented Aug 14, 2017

build fails

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 14, 2017

To fix the failures, use decimal=6. It seems that it worked to machine precision before but I have no problem specifying it explicitly or dialing down the precision. Mismatches at the level of 1e-06 do not worry me for the RMSD.

EDIT: ... or decimal=5 if needed, but try to err on the side of higher (more demanding) precision.

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 14, 2017

FYI: If PR #1581 is merged before this one, then you will need to rebase. Otherwise, #1581 will need a rebase (and adjusting to to new pytest style).

@orbeckst orbeckst referenced this pull request Aug 15, 2017

Merged

analysis.rms fixes and deprecation removals #1581

4 of 4 tasks complete

utkbansal added some commits Aug 8, 2017

@utkbansal utkbansal force-pushed the utkbansal:analysis-rms branch from 2866681 to 9935afb Aug 19, 2017

@utkbansal

This comment has been minimized.

Copy link
Member Author

utkbansal commented Aug 19, 2017

@kain88-de @orbeckst One test is failing

    def test_with_superposition_smaller(self, p_first, p_last):
        A = p_first.positions
        B = p_last.positions
        rmsd = rms.rmsd(A, B)
        rmsd_superposition = rms.rmsd(A, B, center=True, superposition=True)
        print(rmsd, rmsd_superposition)
        # by design the super positioned rmsd is smaller
        assert rmsd > rmsd_superposition

pytest error output -

    def test_with_superposition_smaller(self, p_first, p_last):
        A = p_first.positions
        B = p_last.positions
        rmsd = rms.rmsd(A, B)
        rmsd_superposition = rms.rmsd(A, B, center=True, superposition=True)
        print(rmsd, rmsd_superposition)
        # by design the super positioned rmsd is smaller
>       assert rmsd > rmsd_superposition
E       assert 0.0 > 1.0884445076843497e-06
@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 20, 2017

@utkbansal , PR #1581 was merged. Did you already rebase this branch against develop? If not, please do this first.

It only makes sense to debug after this rebase.

@utkbansal

This comment has been minimized.

Copy link
Member Author

utkbansal commented Aug 20, 2017

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 20, 2017

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 21, 2017

I can reproduce the failure locally (Python 3.6, macOS 10.12.6)

>       assert rmsd > rmsd_superposition
E       assert 0.0 > 1.0884445076843497e-06
@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 21, 2017

@utkbansal I pushed a fix to your branch (! – this works on GitHub, see Committing changes to a pull request branch created from a fork).

The problem was twofold:

  • p_last needs to refer to the last frame in the trajectory and there needs to be a universe.trajectory[-1] to move it to the last frame
  • The underlying universes for p_first and p_last must be independent but if we just use u as a fixture then this is the same for each function and therefore p_first and p_last will always refer to the same frame.

I added a test test_p_frames that is not strictly necessary for MDAnalysis but makes sure that the tests are not broken in the future by someone who wants to optimize away "redundant" code.

The test_rms.py now pass locally for me. Hopefully also on Travis.

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Aug 21, 2017

I fixed a few other things that caught my eye. Please feel free to squash all my commits into yours.

@orbeckst
Copy link
Member

orbeckst left a comment

LGTM, I will merge.

@orbeckst orbeckst merged commit 56776ef into MDAnalysis:develop Aug 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 90.603%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment