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

Fix PEP8 in test_expectmax #918

Merged
merged 2 commits into from Feb 14, 2016

Conversation

Projects
None yet
3 participants
@ghoshbishakh
Member

ghoshbishakh commented Feb 13, 2016

fixes #863

expected[random_labels == 1, 2] = delta_field[random_labels == 1]*grad_G[random_labels == 1, 2]/sqnrm
#Pixels with gradient=0 and sigma_i_sq=0
expected[random_labels == 1, 0] = delta_field[

This comment has been minimized.

@omarocegueda

omarocegueda Feb 13, 2016

Contributor

Instead of breaking the line just after the square bracket [, I think something like:

    expected[random_labels == 1, 0] = (delta_field[random_labels == 1] *
                                       grad_G[random_labels == 1, 0] / sqnrm)
    expected[random_labels == 1, 1] = (delta_field[random_labels == 1] *
                                       grad_G[random_labels == 1, 1] / sqnrm)
    expected[raendom_labels == 1, 2] = (delta_field[random_labels == 1] *
                                       grad_G[random_labels == 1, 2] / sqnrm)

would be more readable and it still fits within the 79 character limit

sqnrm = sq_norm_grad_G[random_labels == 1]
sigma_i_sq[random_labels == 1] = 0
expected[random_labels == 1, 0] = delta_field[random_labels == 1]*grad_G[random_labels == 1, 0]/sqnrm
expected[random_labels == 1, 1] = delta_field[random_labels == 1]*grad_G[random_labels == 1, 1]/sqnrm
expected[random_labels == 1, 0] = delta_field[

This comment has been minimized.

@omarocegueda

omarocegueda Feb 13, 2016

Contributor

I think we shouldn't break the line just after the square bracket [. How about:

    expected[random_labels == 1, 0] = (delta_field[random_labels == 1] *
                                       grad_G[random_labels == 1, 0] / sqnrm)
    expected[random_labels == 1, 1] = (delta_field[random_labels == 1] *
                                       grad_G[random_labels == 1, 1] / sqnrm)
noise = np.random.ranf(np.size(true_quantization)).reshape(img_shape) * noise_amplitude
# make sure additive noise doesn't change the quantization result
noise_amplitude = np.min([delta / 4.0, min_positive / 4.0])
noise = np.random.ranf(np.size(true_quantization)).reshape(

This comment has been minimized.

@omarocegueda

omarocegueda Feb 13, 2016

Contributor

To shorten the line, we can define a new variable for the array length:

    sz = np.size(true_quantization)
    noise = np.random.ranf(sz).reshape(img_shape) * noise_amplitude

this way the product appears in the same line. A bit more readable, I think.

noise = np.random.ranf(np.size(true_quantization)).reshape(img_shape) * noise_amplitude
# make sure additive noise doesn't change the quantization result
noise_amplitude = np.min([delta / 4.0, min_positive / 4.0])
noise = np.random.ranf(np.size(true_quantization)).reshape(

This comment has been minimized.

@omarocegueda

omarocegueda Feb 13, 2016

Contributor

Same as above, we can avoid breaking the line with:

    sz = np.size(true_quantization)
    noise = np.random.ranf(sz).reshape(img_shape) * noise_amplitude
@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Feb 13, 2016

Hi @ghoshbishakh!, =)
thank you very much for your help!, I just verified that after your fix, Spyder did not report any more PEP8 issues. Apart from my comments above, this looks good to me.
Thanks!

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Feb 13, 2016

thank you for reviewing. Ill try to fix the issues pointed and update PR

@arokem

This comment has been minimized.

Member

arokem commented Feb 14, 2016

@omarocegueda - I am +1 for a merge here, if this addressed all your comments.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Feb 14, 2016

Oh! sorry I didn't notice it was done! merging now.

omarocegueda added a commit that referenced this pull request Feb 14, 2016

@omarocegueda omarocegueda merged commit 8c44f2c into nipy:master Feb 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Feb 14, 2016

Thank you @ghoshbishakh!

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