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 sims #884 #960

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@theaverageguy
Contributor

theaverageguy commented Mar 2, 2016

Fixes #884

@@ -55,32 +55,32 @@ def test_phantom():
FA[np.isnan(FA)] = 0
# 686 -> expected FA given diffusivities of [1500, 400, 400]
l1, l2, l3 = 1500e-6, 400e-6, 400e-6
expected_fa = (np.sqrt(0.5) * np.sqrt((l1 - l2)**2 + (l2-l3)**2 + (l3-l1)**2 )/np.sqrt(l1**2 + l2**2 + l3**2))
expected_fa = np.sqrt(0.5) * np.sqrt((l1 - l2)**2 + (l2 - l3)**2 + (l3 - l1)**2) / np.sqrt(l1**2 + l2**2 + l3**2))

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

This line is probably still >80 characters long. A suggestion:

    expected_fa = (np.sqrt(0.5) * np.sqrt((l1 - l2)**2 + 
                   (l2 - l3)**2 + (l3 - l1)**2) / 
                   np.sqrt(l1**2 + l2**2 + l3**2)))

assert_array_almost_equal(FA.max(), expected_fa, decimal=2)
assert_array_almost_equal(FA.max(), expected_fa, decimal = 2)

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

No space needed here.

def test_add_noise():
np.random.seed(1980)
N = 50
S0 = 100
N=50

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

But space would be nice here.

scale = (3, 3, 3),
angles = np.linspace(0, 2 * np.pi, 16),
radii = np.linspace(0.2, 2, 6),
S0 = S0)

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

No spaces around "=" sign when assigning to a dict or when passing arguments to a function, please.

vol = orbital_phantom(gtab, **options)
vol=orbital_phantom(gtab, **options)

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

But space around "=" when used in assignment!

for snr in [10, 20, 30, 50]:
vol_noise = orbital_phantom(gtab, snr=snr, **options)
vol_noise=orbital_phantom(gtab, snr = snr, **options)

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

Same as above.

sigma = S0 / snr
sigma=S0 / snr

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

And same here.

@arokem

This comment has been minimized.

Member

arokem commented Mar 2, 2016

Thanks for taking a look! Just a couple of small comments.

@theaverageguy

This comment has been minimized.

Contributor

theaverageguy commented Mar 2, 2016

Fixing em in 5! :)

On Thu, Mar 3, 2016 at 2:16 AM, Ariel Rokem notifications@github.com
wrote:

Thanks for taking a look! Just a couple of small comments.


Reply to this email directly or view it on GitHub
#960 (comment).

@theaverageguy

This comment has been minimized.

Contributor

theaverageguy commented Mar 2, 2016

Can you check now @arokem ?

# r=fvtk.ren()
# fvtk.add(r,fvtk.volume(vol234[...,0]))
# fvtk.show(r)
# vol234n=add_rician_noise(vol234,20)

This comment has been minimized.

@arokem

arokem Mar 3, 2016

Member

Somebody might want to clean this up at some point, but maybe not on this PR.

@arokem

This comment has been minimized.

Member

arokem commented Mar 3, 2016

LGTM. Someone else want to take a look here?

expected_fa = (np.sqrt(0.5) * np.sqrt((l1 - l2)**2 + (l2-l3)**2 + (l3-l1)**2 )/np.sqrt(l1**2 + l2**2 + l3**2))
expected_fa = (np.sqrt(0.5) * np.sqrt((l1 - l2)**2 +
(l2 - l3)**2 + (l3 - l1)**2) /
np.sqrt(l1**2 + l2**2 + l3**2)))

This comment has been minimized.

@manu-tej

manu-tej Mar 3, 2016

Contributor

np.sqrt(l1**2 + l2**2 + l3**2))
remove extra brackets.

This comment has been minimized.

@arokem

arokem Mar 5, 2016

Member

Yup. That's probably my bad for suggesting this. See failing tests:

https://travis-ci.org/nipy/dipy/jobs/113234014#L2306

evecs=all_tensor_evecs(
sticks[i]), snr=None)
S = S + fractions[i] * single_tensor(gtab, S0=S0, evals=mevals[i],
evecs=all_tensor_evecs(

This comment has been minimized.

@samuelstjean

samuelstjean Mar 3, 2016

Contributor

put the function and its param on the same line, it's a bit hard to read like that

@arokem

This comment has been minimized.

Member

arokem commented Mar 18, 2016

Closing for now.

@arokem arokem closed this Mar 18, 2016

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