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

Fixed PEP8 in utils, denoise , tracking and testing #939

Closed
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@manu-tej
Contributor

manu-tej commented Feb 23, 2016

issues #875 #885 #889 #887 have been fixed

32: 0.5641772300866416,
64: 0.5455611840489607,
128: 0.5322811923303339}
16: 0.5900487123737876,

This comment has been minimized.

@samuelstjean

samuelstjean Feb 24, 2016

Contributor

Those were intentionally not aligned, so that the decimal point aligns instead of the blank space.

@@ -103,7 +105,8 @@ def piesno(data, N, alpha=0.01, l=100, itermax=100, eps=1e-5, return_mask=False)
q = 0.5
# Initial estimation of sigma
initial_estimation = np.percentile(data, q * 100) / np.sqrt(2 * _inv_nchi_cdf(N, 1, q))
initial_estimation = np.percentile(data, q * 100) / \

This comment has been minimized.

@samuelstjean

samuelstjean Feb 24, 2016

Contributor

Same here, line was too long to not break up the math, how much character does it take in the old form?

This comment has been minimized.

@manu-tej

manu-tej Feb 24, 2016

Contributor

92 characters

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

I prefer:

(np.percentile(data, q * 100) / 
 np.sqrt(2 * _inv_nchi_cdf(N, 1, q)))

With an enclosing parenthesis breaking the line. Since this is a fraction, it makes it even easier to read IMHO.

@@ -117,7 +120,8 @@ def piesno(data, N, alpha=0.01, l=100, itermax=100, eps=1e-5, return_mask=False)
itermax=itermax,
eps=eps,
return_mask=True,
initial_estimation=initial_estimation)

This comment has been minimized.

@samuelstjean

samuelstjean Feb 24, 2016

Contributor

Same here, I found that this decreases readability to not have the parameter on the same line.

This comment has been minimized.

@manu-tej

manu-tej Feb 24, 2016

Contributor

here it is taking 181 characters. Can I ignore these kind of PEP8 issues?

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

If you are going to have parameters spread over multiple lines, I prefer to have one line per parameter:

_piesno_3D(data[..., idx, :],
                    N,
                    alpha=alpha,
                    l=l,
                    itermax=itermax,
                    eps=eps,
                    return_mask=True,
                    initial_estimation=initial_estimation)

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

But with the indentation correctly matched for continuation lines...

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

And agreed with @samuelstjean on not breaking the line to move the parameter to the next line.

24: 0.49736907650825657,
32: 0.49803177052530145,
64: 0.49901964176235936}
12: 0.4946862482541263,

This comment has been minimized.

@samuelstjean

samuelstjean Feb 24, 2016

Contributor

Also reminded me we can now use scipy 0.17 instead of precomputed values.

@manu-tej

This comment has been minimized.

Contributor

manu-tej commented Feb 24, 2016

@samuelstjean Can you please tell me how to resolve these issues?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 24, 2016

Well, for the first two, I'd say to put them back as they were, the third one look at the length of the line (any decent text editor will give you that) and the last one is rather a friendly reminder that I need to update it, as the real formula allows us to use whatever value instead of those fixed in the table.

manu-tej added some commits Feb 24, 2016

@manu-tej manu-tej changed the title from Fixed PEP8 in utils, denoise and testing to Fixed PEP8 in utils, denoise , tracking and testing Feb 27, 2016

manu-tej added some commits Feb 27, 2016

@manu-tej manu-tej referenced this pull request Feb 27, 2016

Closed

PEP8 in tracking #887

@@ -35,7 +36,8 @@ class TripWire(object):
... import silly_module_name
... except ImportError:
... silly_module_name = TripWire('We do not have silly_module_name')
>>> silly_module_name.do_silly_thing('with silly string') #doctest: +IGNORE_EXCEPTION_DETAIL
>>> silly_module_name.do_silly_thing('with silly string') \
#doctest: +IGNORE_EXCEPTION_DETAIL

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

Please put that one back on the same line as it was. This line-break is not needed.

reconstruction (Siemens and GE). Use 0 to disable the correction factor,
as for example if the noise is Gaussian distributed. See [1] for more information.
reconstruction (Siemens and GE). Use 0 to disable the correction factor
, as for example if the noise is Gaussian distributed. See [1] for more

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

Please delete the space between the comma and the word "factor"

assert_equal(sigma, np.array([0., 0., 0.]))
arr = np.zeros((3, 3, 3))
arr[0, 0, 0] = 1
sigma = estimate_sigma(arr, disable_background_masking=False, N=1)
assert_array_almost_equal(sigma, 0.10286889997472792 / np.sqrt(0.42920367320510366))
assert_array_almost_equal(sigma, 0.10286889997472792 /

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

You might want to break the line after the comma:

(sigma, 
 0.10286889997472792 / np.sqrt(0.42920367320510366))

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

Or if there's not enough space:
(sigma,
(0.10286889997472792 /
np.sqrt(0.42920367320510366)))

Examples
----------
>>> import numpy as np
>>> import dipy.tracking.learning as tl
>>> A=np.array([[0,0,0],[1,1,1],[2,2,2]])
>>> B=np.array([[1,0,0],[2,0,0],[3,0,0]])
>>> C=np.array([[0,0,-1],[0,0,-2],[0,0,-3]])
>>> C=np.array([[0,0,-1],[0,0,-2],[0,0,-3]])

This comment has been minimized.

@arokem

arokem Feb 27, 2016

Member

Whitespace still needed in these examples (e.g. around equal signs, after commas)

This comment has been minimized.

@manu-tej

manu-tej Feb 28, 2016

Contributor

Completed it

eps=eps,
return_mask=True,
initial_estimation=initial_estimation)
sigma[idx], mask_noise[..., idx] = _piesno_3D(data[..., idx, :], N, alpha=alpha, l=l, itermax=itermax, eps=eps, return_mask=True, initial_estimation=initial_estimation)

This comment has been minimized.

@arokem

arokem Feb 28, 2016

Member

This is no good. Either put it back as it was, or rewrite this as:

_piesno_3D(data[..., idx, :],
           N,
           alpha=alpha,
           l=l,
           itermax=itermax,
           eps=eps,
           return_mask=True,
           initial_estimation=initial_estimation)
eps=eps,
return_mask=True,
initial_estimation=initial_estimation)
sigma, mask_noise = _piesno_3D(data, N, alpha=alpha, l=l, itermax=itermax, eps=eps, return_mask=True, initial_estimation=initial_estimation)

This comment has been minimized.

@arokem

arokem Feb 28, 2016

Member

Same comment here.

@arokem

This comment has been minimized.

Member

arokem commented Feb 28, 2016

For future reference, we prefer that contributors rebase on master, rather than merging from master. See here: http://nipy.org/dipy/devel/gitwash/development_workflow.html#rebasing-on-trunk

@arokem

This comment has been minimized.

Member

arokem commented Feb 29, 2016

And this branch now needs a rebase on top of the recent master branch. Please let me know if you need guidance on the rebase process.

@manu-tej manu-tej closed this Mar 1, 2016

@manu-tej manu-tej deleted the manu-tej:master branch Mar 1, 2016

@manu-tej manu-tej restored the manu-tej:master branch Mar 1, 2016

@manu-tej manu-tej reopened this Mar 1, 2016

manu-tej added some commits Mar 1, 2016

@arokem

This comment has been minimized.

Member

arokem commented Mar 1, 2016

Why did you close this one?

@manu-tej manu-tej deleted the manu-tej:master branch Mar 1, 2016

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