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: test_nuclinfo.py #1469

Merged
merged 4 commits into from Jul 13, 2017

Conversation

Projects
None yet
5 participants
@utkbansal
Copy link
Member

utkbansal commented Jul 10, 2017

Fixes #

Changes made in this Pull Request:

PR Checklist

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

@orbeckst orbeckst added the testing label Jul 11, 2017

@orbeckst
Copy link
Member

orbeckst left a comment

Make some of the methods static (to be consistent with what you did elsewhere), otherwise looks good.

seg1 = self.universe.residues[3].atoms.segids[0]
seg2 = self.universe.residues[19].atoms.segids[0]
wc = nuclinfo.wc_pair(self.universe, 4, 20, seg1, seg2)
def test_wc_pair(self, universe):

This comment has been minimized.

@orbeckst

orbeckst Jul 11, 2017

Member

could be static, does not need self

seg1 = self.universe.residues[3].atoms.segids[0]
seg2 = self.universe.residues[19].atoms.segids[0]
maj = nuclinfo.major_pair(self.universe, 4, 20, seg1, seg2)
def test_major_pair(self, universe):

This comment has been minimized.

@orbeckst

orbeckst Jul 11, 2017

Member

could be static, does not need self

def test_minor_pair(self):
seg1 = self.universe.residues[3].atoms.segids[0]
seg2 = self.universe.residues[19].atoms.segids[0]
def test_minor_pair(self, universe):

This comment has been minimized.

@orbeckst

orbeckst Jul 11, 2017

Member

could be static, does not need self

@utkbansal

This comment has been minimized.

Copy link
Member

utkbansal commented Jul 11, 2017

@orbeckst pytest will not collect the tests if I make them static. So I can't do those changes.
@kain88-de @richardjgowers Any changes needed?

@orbeckst

This comment has been minimized.

Copy link
Member

orbeckst commented Jul 11, 2017

@@ -41,57 +42,57 @@
from nose.plugins.attrib import attr

This comment has been minimized.

@kain88-de

kain88-de Jul 11, 2017

Member

Please also remove the attr import and usage as well as the TestCase import


class TestNuclinfo(object):

This comment has been minimized.

@kain88-de

kain88-de Jul 12, 2017

Member

there is no need to keep this a class.


# There is not really a baseflip, just testing the code...
flip = nuclinfo.pseudo_dihe_baseflip(universe, 4, 20, 5, seg1=seg1, seg2=seg2, seg3=seg1)
assert_almost_equal(flip, 322.0826, 4,

This comment has been minimized.

@richardjgowers

richardjgowers Jul 12, 2017

Member

I'd rather we defined PREC = 4 in the module namespace, and used it in this function, it's very confusing what that 4 is doing there otherwise.

This comment has been minimized.

@utkbansal

utkbansal Jul 12, 2017

Member

We could do a keyword argument. I'm not generally in favour of adding variables to the global namespace.

This comment has been minimized.

@jbarnoud

jbarnoud Jul 12, 2017

Contributor

PREC = 4 would be a constant 😉

This comment has been minimized.

@utkbansal

utkbansal Jul 12, 2017

Member

IMO I would favour having a class if we do PREC = 4.

This comment has been minimized.

@richardjgowers

richardjgowers Jul 12, 2017

Member

Yeah using the keyword explicitly is better, but then it's not immediately obvious that they all share the same precision, so defining it at module level is better for that. I just don't want a random 4 floating around that I have to go look up the signature to figure out.

@utkbansal

This comment has been minimized.

Copy link
Member

utkbansal commented Jul 12, 2017

@richardjgowers Good to go now?

@richardjgowers richardjgowers merged commit 26e6332 into MDAnalysis:develop Jul 13, 2017

3 checks passed

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