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

Port utils module #1419

Closed
wants to merge 12 commits into from
Closed

Conversation

utkbansal
Copy link
Member

Fixes #

Changes made in this Pull Request:

PR Checklist

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

from MDAnalysisTests import parser_not_found
from nose.plugins.attrib import attr
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the attr decorator. I don't think we use it as indented.

@@ -325,7 +355,7 @@ def test_selfdist(self):

def test_distarray(self):
from MDAnalysis.lib.distances import distance_array
from MDAnalysis.lib.distances import transform_StoR, transform_RtoS
from MDAnalysis.lib.distances import transform_StoR
Copy link
Member

Choose a reason for hiding this comment

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

these modules should already be imported anyway. Do the tests fail when you remove the imports? Just keep in mind that they are in the MDAnalysis.lib.distances namespace globally in that file.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are most probably removed automatically by my IDE because they are not referenced anywhere. Should I put them back?

Copy link
Member

Choose a reason for hiding this comment

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

your IDE just sorted the imports. It can't actually remove them. When you remove them just make sure the tests still pass.

Copy link
Member

Choose a reason for hiding this comment

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

you likely have to change some code for that when the functions where called directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like all the tests pass just fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a side note, please deactivate the import sorting from your IDE. It is not a big deal in this specific file, but we do not just alphabetize imports, we order them by categories which is completely missed by your IDE.

(Just to ramble a bit more, having your IDE changing parts of the code you did not touch is not the best idea. When it does not mess with the imports it can adds small bits of diff that dilute your changes and make the blame more difficult to read. This may just be my current obsession, though.)

@utkbansal
Copy link
Member Author

@kain88-de @richardjgowers This can be merged now.

@@ -55,12 +48,12 @@ def capping(ref, ace, nma, output):
# TODO consider a case when the protein resid is 1 and all peptide has to be shifted by +1, put that in docs as a
# post-processing step
alignto(ace, ref, select={
"mobile": "resid {0} and backbone".format(resid_min),
"reference": "resid {0} and backbone".format(resid_min)},
"mobile": "resid {0} and backbone".format(resid_min),
Copy link
Member

Choose a reason for hiding this comment

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

This alignment is wrong. It should start on the opening brace. What alignment does yapf produce here?

writer = MDAnalysis.selections.jmol.SelectionWriter
filename = "CA.spt"
ref_name, ref_indices = spt2array(
( '@~ca ({4 21 45 64 83 102 121 128 140 152 159 169 176 198 205 219 236'
' 246 263 283 302 319 334 356});')
Copy link
Member

Choose a reason for hiding this comment

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

Please configure your ide to stop these automated corrections. They are meant well but add noise to the diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree they add noise to the diff, but this is what PEP8 - the official style guide for Python recommends.

Copy link
Member

Choose a reason for hiding this comment

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

Yes and we try to satisfy PEP8 in the code we touch and not the whole file. It's noise and in an old code base not everything is perfect following a coding style

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay

@utkbansal
Copy link
Member Author

@richardjgowers @kain88-de What code style do we follow. yapf is quite different from autopep8. I will just configure my IDE to do the style we want.

@utkbansal
Copy link
Member Author

@kain88-de @richardjgowers Except the formatting, everything looks good?

@kain88-de
Copy link
Member

@jbarnoud & @mnmelo could you have a look please.

Copy link
Contributor

@jbarnoud jbarnoud left a comment

Choose a reason for hiding this comment

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

Most of my comments are about style, which @kain88-de mostly addressed already.

Looking into more details, it seems that test_distances.test_case_insensitivity does appear in the pytest run with this PR; but it appears only once when all the other tests generators appear in the verbose log as often as they produce tests. Since it is a very easy test to move to parametrize I would be more comfortable if it were converted.

Against my expectations, some test are discovered by pytest and do not appear so in nose for some reason. It is the case of:

  • test_persistence.py::TestAtomGroupPickle::test_pickle_unpickle_empty
  • test_persistence.py::TestAtomGroupPickle::test_unpickle
  • apparently some tests in test_transformations

Just to be clear, this PR makes the module pytest-compatible, but not yet pytest-styles; right?

Anyway, good job on this.

@@ -325,7 +355,7 @@ def test_selfdist(self):

def test_distarray(self):
from MDAnalysis.lib.distances import distance_array
from MDAnalysis.lib.distances import transform_StoR, transform_RtoS
from MDAnalysis.lib.distances import transform_StoR
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a side note, please deactivate the import sorting from your IDE. It is not a big deal in this specific file, but we do not just alphabetize imports, we order them by categories which is completely missed by your IDE.

(Just to ramble a bit more, having your IDE changing parts of the code you did not touch is not the best idea. When it does not mess with the imports it can adds small bits of diff that dilute your changes and make the blame more difficult to read. This may just be my current obsession, though.)

from MDAnalysis.core.groups import AtomGroup
from MDAnalysis import NoDataError
from MDAnalysis.tests.datafiles import PSF, DCD, capping_input, capping_output, capping_ace, capping_nma, \
Copy link
Contributor

Choose a reason for hiding this comment

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

If this has to change, replace the new-line-escape by parentheses.

import shutil
import warnings
assert_equal)
from six.moves import cPickle
Copy link
Contributor

@jbarnoud jbarnoud Jun 23, 2017

Choose a reason for hiding this comment

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

Please, put back the imports in the previous order here. Python 2/3 compatibility imports (basically anuyjing from __future__ or from six should be at the beginning). See https://github.com/MDAnalysis/mdanalysis/wiki/Style-Guide#importing-modules.

@jbarnoud
Copy link
Contributor

I figured out why some tests appeared like being only discovered by pytest: I forgot to account for nose displaying the docstring instead of the name when available. Seems OK on that point; the others remain, though.

@utkbansal
Copy link
Member Author

@jbarnoud Yes, this PR is only to make tests discoverable by pytest. The next phase will be trying to incorporate the pytest goodness.

I'll fix all the formatting problems right away and then we can merge.

@utkbansal
Copy link
Member Author

@jbarnoud I have tried to fix most of the changes.

@@ -0,0 +1,8 @@

class TestFoo(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably not stay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I was playing around and mistakenly checked this in. Removing now.

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.

Can you pull my changes from #1423 rather than have the "compromise" versions of these files. The files I touched should run fine under pytest and be a nicer style

@utkbansal
Copy link
Member Author

Okay, looks like I've completely messed up the commit history.

@jbarnoud
Copy link
Contributor

testsuite/MDAnalysisTests/utils/failure.txtshould not be included in the repo.

@jbarnoud
Copy link
Contributor

If I squash the whole thing somebody is going to loose the credit for its changes. @utkbansal Could you squash your commits together? The easiest would be if you could have your two commits before @richardjgowers's commits squashed in one commit, and your changes after Richard's commits squashed in an other. This would make for a neater history.

Please, try first in a copy of the branch and make sure you keep Richard's authorship on his commits. And since you're at it, fix the conflict in .travis.yml. Thanks.

@richardjgowers
Copy link
Member

@jbarnoud sounds like a good plan

@utkbansal
Copy link
Member Author

@jbarnoud I'm unable to do the squash, it always gets messed up.

@kain88-de
Copy link
Member

@utkbansal squashing should be fine. You can squash your last commits that have been continuously from you into one. As long as you don't touch any of Richards commits you should be fine.

@utkbansal
Copy link
Member Author

utkbansal commented Jun 24, 2017

I need help with squashing, see the mess I've made at #1434.

EDIT: It works now, I think.

@kain88-de
Copy link
Member

I see why you get a messed up history when you squash. There is a merge commit with develop in this branch.

@kain88-de
Copy link
Member

What you have to do is. Remove all the merge commits in this branch. Squash your commits in the branch. Afterwards you can cherry-pick Richards changes again into your branch. Actually I'm surprised git isn't warning you that your rebase contains a merge commit.

@utkbansal utkbansal closed this Jun 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants