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

Remove dependency from nose #1574

Merged
merged 4 commits into from Aug 5, 2017

Conversation

Projects
None yet
4 participants
@utkbansal
Member

utkbansal commented Aug 2, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?
@kain88-de

This comment has been minimized.

Member

kain88-de commented Aug 2, 2017

Can you remove nose as well from the Travis config. Then we can see if this really is the last thing that uses nose.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 2, 2017

@kain88-de This is not the last thing that uses nose. I'm currently investigating what is still dependent on nose. Will update as I know more.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 3, 2017

@kain88-de https://github.com/MDAnalysis/mdanalysis/pull/1574/files#diff-416947382aa4e650975916732248fef5R519 is failing. I tried using the debugger and the value of dihedrals[0] comes out to be 0.0 (float64).

@kain88-de

This comment has been minimized.

Member

kain88-de commented Aug 3, 2017

The diff you are citing is to big.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 3, 2017

@kain88-de testsuite/MDAnalysisTests/utils/test_distances.py line 519. Method is test_dihedrals in TestCythonFunctions class.

@richardjgowers

This comment has been minimized.

Member

richardjgowers commented Aug 3, 2017

Not sure why that would have started working, but it looks like the tests pass now? (Ie it raises the right Error again). This just needs a rebase now

@mnmelo

This comment has been minimized.

Member

mnmelo commented Aug 3, 2017

This is weird indeed. @nestorwendt's recent PR #1566 went over the behavior of those lines (the one in question seems to be the check whether an undefined dihedral returns a nan).
There should be little numerical instability in this particular check, since it's the case where all four dihedral atoms' positions are [0., 0., 0.].

utkbansal added some commits Aug 2, 2017

@utkbansal utkbansal changed the title from [WIP]Get rid of assert_raises to Remove dependency from nose Aug 5, 2017

@utkbansal utkbansal changed the title from Remove dependency from nose to [WIP]Remove dependency from nose Aug 5, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 5, 2017

@kain88-de @richardjgowers This works on Travis, not on my system. Have tried reinstalling the package and testusite, but the tests still fail.

@kain88-de

Looks good besides of the issue with marking a test when the import doesn't work. Please change all of those tests to use pytest.importorskip

@dec.skipif(module_not_found('sklearn'),
"Test skipped because sklearn is not available.")
@dec.slow
@pytest.mark.skipif(module_not_found('sklearn'), reason="Test skipped because sklearn is not available.")

This comment has been minimized.

@kain88-de

kain88-de Aug 5, 2017

Member

correct here is pytest.importorskip. Note you need to use it inside of the function.

@kain88-de

This comment has been minimized.

Member

kain88-de commented Aug 5, 2017

@utkbansal I don't know why the test don't work on your local laptop. It's hard to tell without being in front of the computer.

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 5, 2017

@kain88-de

    def chk_same_position(x_id, y_id, hasval='nan'):
        """Handling nan/inf: check that x and y have the nan/inf at the same
            locations."""
        try:
            assert_array_equal(x_id, y_id)
        except AssertionError:
            msg = build_err_msg([x, y],
                                err_msg + '\nx and y %s location mismatch:'
                                % (hasval), verbose=verbose, header=header,
                                names=('x', 'y'), precision=precision)
>           raise AssertionError(msg)
E           AssertionError: 
E           Arrays are not almost equal to 5 decimals
E           Cython dihedrals didn't match numpy calculations
E           x and y nan location mismatch:
E            x: array([ 0.     ,  0.     ,  3.14159,  0.50714])
E            y: array([     nan,      nan,  3.14159, -0.50714], dtype=float32)
@kain88-de

This comment has been minimized.

Member

kain88-de commented Aug 5, 2017

Are your cython generated files up do date? We recently changed them so maybe you need to compile the cython modules against the current package version you are testing.

@utkbansal utkbansal changed the title from [WIP]Remove dependency from nose to Remove dependency from nose Aug 5, 2017

@utkbansal

This comment has been minimized.

Member

utkbansal commented Aug 5, 2017

@kain88-de Can we merge this soon? Would save me from a lot of merge conflicts.

@kain88-de kain88-de merged commit 0b9066c into MDAnalysis:develop Aug 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 90.575%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment