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

Pytest Style analysis/test_lineardensity.py #1548

Merged
merged 6 commits into from Aug 5, 2017

Conversation

utkbansal
Copy link
Member

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

ld = LinearDensity(self.selection, binsize=5).run()
assert_allclose(self.xpos, ld.results['x']['pos'], rtol=1e-6, atol=0)

# TODO: Remove this?!
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you activate the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbarnoud It fails. LinearDensity doesn't have a parameter universe and ld.run() doesn't take any parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be removed then.

Copy link
Member

Choose a reason for hiding this comment

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

This can still be fixed @utkbansal

@utkbansal
Copy link
Member Author

@richardjgowers @kain88-de @jbarnoud I need help with coverage! The drop doesn't make sense to me. This file has one and only one function, I don't see what changed. Interesting to note that before I dropped the class, the coverage remained the same. The coverage dropped in the last commit where all I did was drop the class.

@kain88-de
Copy link
Member

@utkbansal are you able to point to the line that is not covered anymore?

@kain88-de
Copy link
Member

weird. Looks like we introduced more randomness in our testsuite.

@utkbansal
Copy link
Member Author

@kain88-de @richardjgowers What should I do to fix this?

@utkbansal utkbansal changed the title [WIP]Pytest Style analysis/test_lineardensity.py Pytest Style analysis/test_lineardensity.py Jul 29, 2017
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.

I don't think that the minute change in coverage should hold up this PR. LGTM.


# TODO: Remove this?!
Copy link
Member

Choose a reason for hiding this comment

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

We said this can be removed.

xpos = np.array([0., 0., 0., 0.0072334, 0.00473299, 0.,
0., 0., 0., 0.])
ld = LinearDensity(selection, binsize=5).run()
assert_allclose(xpos, ld.results['x']['pos'], rtol=1e-6, atol=0)
Copy link
Member

Choose a reason for hiding this comment

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

please us e assert_almost equal like everywhere else

Copy link
Member Author

Choose a reason for hiding this comment

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

The precision would be 6, right? assert_almost_equal(xpos, ld.results['x']['pos'], 6)

Copy link
Member

Choose a reason for hiding this comment

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

Try using the default precision and see if you need to reduce it.

@kain88-de kain88-de merged commit 1f69ba0 into MDAnalysis:develop Aug 5, 2017
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

5 participants