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_distances.py #1541

Merged
merged 3 commits into from Jul 29, 2017

Conversation

utkbansal
Copy link
Member

Fixes #

Changes made in this Pull Request:

PR Checklist

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

assert_equal(contacts.shape, self.shape,
"wrong shape (should be {0})".format(self.shape))
contacts = MDAnalysis.analysis.distances.contact_matrix(self.coord, cutoff=1, returntype="numpy")
assert contacts.shape == self.shape, "wrong shape (should be {0})".format(self.shape)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like you break the 80 char line limit. Also these tests look like they could be parametrized.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kain88-de All the 4 methods have different check in the assert_equal statement. If you want to parametrize, I'll have to send both the values that will be compared. I don't think that's a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

called np.asarray(...) on the contacts should also work for the sparse case

[1, 0, 1, 1, 1]], dtype=np.bool)

class TestContactMatrix(object):
coord = np.array([[1, 1, 1],
Copy link
Member

Choose a reason for hiding this comment

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

The problem with declaring arrays in the class namespace like this is they can be accidentally modified by a test, and they will remain changed for all other tests (I think?)

del self.expected

class TestDist(object):
u = MDAnalysis.Universe(GRO)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, we shouldn't just be loading everything into the class namespace

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

Should use fixtures more

@richardjgowers richardjgowers self-assigned this Jul 25, 2017
@utkbansal
Copy link
Member Author

@richardjgowers Better now?

@utkbansal utkbansal changed the title [WIP]Pytest style analysis/test_distances.py Pytest style analysis/test_distances.py Jul 26, 2017
@kain88-de
Copy link
Member

@utkbansal the coverage went down with the latest changes

@utkbansal
Copy link
Member Author

@kain88-de I don't see how that happened, have restarted the build.

@kain88-de kain88-de merged commit a46cf63 into MDAnalysis:develop Jul 29, 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

3 participants