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

PEP8 in reconst #881 #984

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@theaverageguy
Contributor

theaverageguy commented Mar 21, 2016

Fixes #881

@@ -139,7 +140,8 @@ def __init__(self, gtab, response, reg_sphere=None, sh_order=8, lambda_=1,
else:
self.sphere = reg_sphere
r, theta, phi = cart2sphere(self.sphere.x, self.sphere.y, self.sphere.z)
r, theta, phi = cart2sphere(
self.sphere.x, self.sphere.y, self.sphere.z)

This comment has been minimized.

@arokem

arokem Mar 21, 2016

Member

I prefer the breakdown you did on line 294 below, with each one of these on its own line.

npa = np.sqrt((psi0 - psi1) ** 2 + (psi1 - psi2) ** 2 + (psi2 - psi0) ** 2) / np.sqrt(2 * (psi0 ** 2 + psi1 ** 2 + psi2 ** 2))
#print 'tom >>>> ',t0,t1,t2,npa
npa = np.sqrt((psi0 - psi1) ** 2 + (psi1 - psi2) ** 2 + (psi2 - psi0)
** 2) / np.sqrt(2 * (psi0 ** 2 + psi1 ** 2 + psi2 ** 2))

This comment has been minimized.

@arokem

arokem Mar 21, 2016

Member

I prefer not to have a line break right before a binary operator (**)

@arokem

This comment has been minimized.

Member

arokem commented Mar 21, 2016

Thanks! Just a couple of small comments.

For future reference, please use "rebase", instead of merging from the master branch:

http://nipy.org/dipy/devel/gitwash/development_workflow.html#rebasing-on-trunk

theaverageguy added some commits Mar 21, 2016

@theaverageguy

This comment has been minimized.

Contributor

theaverageguy commented Mar 21, 2016

Please check @arokem :)

if self.model.method == 'standard':
self.gqi_vector = np.real(np.sinc(np.dot(self.model.b_vector, sphere.vertices.T) * self.model.Lambda / np.pi))
self.gqi_vector = np.real(np.sinc(np.dot(self.model.b_vector,
sphere.vertices.T) * self.model.Lambda / np.pi))

This comment has been minimized.

@arokem

arokem Mar 21, 2016

Member

This line is still too long (>80 characters). Do you have a text editor that tells you about PEP8 issues? I use Atom with a PEP8 linter: https://atom.io/packages/linter-pep8

@@ -181,7 +185,7 @@ def equatorial_zone_vertices(vertices, pole, width=5):
finds the 'vertices' in the equatorial zone conjugate
to 'pole' with width half 'width' degrees
"""
return [i for i,v in enumerate(vertices) if np.abs(np.dot(v,pole)) < np.abs(np.sin(np.pi*width/180))]
return [i for i, v in enumerate(vertices) if np.abs(np.dot(v, pole)) < np.abs(np.sin(np.pi * width / 180))]

This comment has been minimized.

@arokem

arokem Mar 21, 2016

Member

Same here - line's too long

@@ -189,21 +193,22 @@ def polar_zone_vertices(vertices, pole, width=5):
finds the 'vertices' in the equatorial band around
the 'pole' of radius 'width' degrees
"""
return [i for i,v in enumerate(vertices) if np.abs(np.dot(v,pole)) > np.abs(np.cos(np.pi*width/180))]
return [i for i, v in enumerate(vertices) if np.abs(np.dot(v, pole)) > np.abs(np.cos(np.pi * width / 180))]

This comment has been minimized.

@arokem

arokem Mar 21, 2016

Member

And here

"""
find 'vertices' within the cone of 'width' degrees around 'pole'
"""
return [i for i,v in enumerate(vertices) if np.abs(np.dot(v,pole)) > np.abs(np.cos(np.pi*width/180))]
return [i for i, v in enumerate(vertices) if np.abs(np.dot(v, pole)) > np.abs(np.cos(np.pi * width / 180))]

This comment has been minimized.

@arokem

arokem Mar 21, 2016

Member

And here

@theaverageguy

This comment has been minimized.

Contributor

theaverageguy commented Mar 22, 2016

Why did the checks fail though @arokem?

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2016

Looks like a typo that I missed in my review: https://travis-ci.org/nipy/dipy/jobs/117482040#L2334

Thank providence for those tests :-)

npa = np.sqrt((psi0 - psi1) ** 2 + (psi1 - psi2) ** 2 + (psi2 - psi0) ** 2) / np.sqrt(2 * (psi0 ** 2 + psi1 ** 2 + psi2 ** 2))
#print 'tom >>>> ',t0,t1,t2,npa
npa = np.sqrt((psi0 - psi1) ** 2 + (psi1 - psi2) ** 2 + (psi2 - psi0) ** 2)
/ np.sqrt(2 * (psi0 ** 2 + psi1 ** 2 + psi2 ** 2))

This comment has been minimized.

@arokem

arokem Mar 22, 2016

Member

These two lines need another set of parentheses. Also, the / is a binary operator, so please don't start the line with it.

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2016

You should run the tests locally on your machine, before pushing up to Github.

@arokem

This comment has been minimized.

Member

arokem commented Apr 11, 2016

@theaverageguy : any progress here?

@arokem

This comment has been minimized.

Member

arokem commented Oct 21, 2016

I assume this PR has been abandoned. Feel free to reopen if you still want to work on this.

@arokem arokem closed this Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment