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 for test files #947

Merged
merged 9 commits into from Mar 11, 2016

Conversation

Projects
None yet
4 participants
@souravsingh
Contributor

souravsingh commented Feb 29, 2016

This PR fixes the files mentioned in Issues #865 #866 #867 #870 and #871.

disp1, assign1 = vfu.create_random_displacement_2d(
np.array(
input_shape, dtype=np.int32), input_grid2world, np.array(
tgt_sh, dtype=np.int32), target_grid2world)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

I think it was easier to read before...

Did it go over 80 characters?

target_grid2world)
disp2, assign2 = vfu.create_random_displacement_2d(
np.array(
input_shape, dtype=np.int32), input_grid2world, np.array(

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

Same comment here

disp1, assign1 = vfu.create_random_displacement_3d(
np.array(
input_shape, dtype=np.int32), input_grid2world, np.array(
tgt_sh, dtype=np.int32), target_grid2world)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

And here

disp2, assign2 = vfu.create_random_displacement_3d(
np.array(
input_shape, dtype=np.int32), input_grid2world, np.array(
tgt_sh, dtype=np.int32), target_grid2world)

This comment has been minimized.

@arokem

arokem Mar 1, 2016

Member

And here

This comment has been minimized.

@omarocegueda

omarocegueda Mar 1, 2016

Contributor

I think something like this would be much more readable:

    disp2, assign2 = vfu.create_random_displacement_3d(
        np.array(input_shape, dtype=np.int32),
        input_grid2world,
        np.array(tgt_sh, dtype=np.int32),
        target_grid2world)

Putting more spaces makes pep8 complain about over-indenting.

This comment has been minimized.

@arokem

arokem Mar 2, 2016

Member

+1 to this. @souravsingh - could you please change this to match the suggestion from @omarocegueda ?

@arokem

This comment has been minimized.

Member

arokem commented Mar 1, 2016

Just a couple of small comments. Otherwise, this looks good to me. But it's a lot of changes, so I would be happy if someone else could also take a look here.

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 1, 2016

I think those parts were going over 80 characters since I checked the script using pep8 and flake8.

@arokem

This comment has been minimized.

Member

arokem commented Mar 1, 2016

I think there were some other issues there (indentation level didn't match across lines). Would you mind putting these cases back as they were? I think they were easier to read in their previous form. Thanks!

Updated script
Updated the script based on the comments on the PR
@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 2, 2016

@arokem I have made the updates to the file.

codomain_grid2world)
np.array(domain_shape, dtype=np.int32),
domain_grid2world, np.array(codomain_shape,
dtype=np.int32),

This comment has been minimized.

@omarocegueda

omarocegueda Mar 2, 2016

Contributor

It's not necessary to break this line, this is just 68 characters long:

        domain_grid2world, np.array(codomain_shape, dtype=np.int32),
moved.astype(np.float64), grid_to_space,
mgrad, smask, mmask)
parzen_hist.update_gradient_dense(
id, transform, static.astype(

This comment has been minimized.

@omarocegueda

omarocegueda Mar 2, 2016

Contributor

I think in general it is more readable when each parameter is contained within one single line (when possible). For example, it is not necessary to break static.astype(np.float64) nor moved.astype(np.float64), what about:

        parzen_hist.update_gradient_dense(
            id, transform, static.astype(np.float64),
            moved.astype(np.float64), grid_to_space,
            mgrad, smask, mmask)
@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Mar 2, 2016

Hi @souravsingh!,
this was a lot of work, thank you very much for taking the time to do all these fixes! I ran pep8 and flake8 on the affected files and there are only very few minor issues for this to be perfect!:

test_imwarp.py:3:1: F401 'npt' imported but unused
test_transforms.py:3:1: F401 'assert_almost_equal' imported but unused
test_transforms.py:182:9: F841 local variable 'n' is assigned to but never used
test_parzenhist.py:324:80: E501 line too long (82 > 79 characters)
test_parzenhist.py:334:80: E501 line too long (80 > 79 characters)
test_parzenhist.py:335:80: E501 line too long (81 > 79 characters)
test_parzenhist.py:337:80: E501 line too long (83 > 79 characters)
test_parzenhist.py:417:80: E501 line too long (80 > 79 characters)
test_parzenhist.py:418:80: E501 line too long (81 > 79 characters)
test_parzenhist.py:420:80: E501 line too long (83 > 79 characters)
test_parzenhist.py:473:80: E501 line too long (80 > 79 characters)
test_parzenhist.py:474:80: E501 line too long (82 > 79 characters)
test_parzenhist.py:609:80: E501 line too long (80 > 79 characters)
test_parzenhist.py:610:80: E501 line too long (80 > 79 characters)
test_parzenhist.py:611:80: E501 line too long (84 > 79 characters)
test_parzenhist.py:612:80: E501 line too long (90 > 79 characters)
test_parzenhist.py:613:80: E501 line too long (84 > 79 characters)
test_parzenhist.py:614:80: E501 line too long (85 > 79 characters)
test_parzenhist.py:615:80: E501 line too long (84 > 79 characters)
test_parzenhist.py:616:80: E501 line too long (85 > 79 characters)
test_vector_fields.py:956:47: W291 trailing whitespace
test_vector_fields.py:957:26: W291 trailing whitespace
test_vector_fields.py:958:42: W291 trailing whitespace
test_vector_fields.py:964:47: W291 trailing whitespace
test_vector_fields.py:965:26: W291 trailing whitespace
test_vector_fields.py:966:42: W291 trailing whitespace
test_vector_fields.py:1091:47: W291 trailing whitespace
test_vector_fields.py:1092:60: W291 trailing whitespace

Appart from this and my two comments above, this is ready to merge for me. Again thanks a lot for the hard work @souravsingh !

souravsingh added some commits Mar 2, 2016

Fix test_parzenhist
Fix test_parzenhist to take care of a few PEP8 errors.
Another Fix
Fix the format of a function in the script
@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 2, 2016

The commit is able to fix a few PEP8 coming from test_parzenhist.py.The PEP8 errors from Line 324-420 are arising from block comments and from 609-616 is coming from a for statement,which I am wondering whether it needs to be changed

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Mar 3, 2016

Hi @souravsingh!,
I agree with you. At the beginning I thought that following PEP8 rules everywhere even in comment blocks was kind of silly (there are many other issues that are much more important than this), but now that I have seen the tremendous effort @arokem is doing to improve the quality of our code, I realize that being as consistent as possible has a lot of value, especially for automation: it allows us to find real PEP8 issues automatically without being distracted by these unimportant issues appearing in the PEP8 report, which leaves more time for us to work on other things. So, I think it would be great if we can make pep8 and flake8's reports clean.

What about:

    # Compare the analytical and numerical (finite differences) gradient of
    # the joint distribution (i.e. derivatives of each histogram cell) w.r.t.
    # the transform parameters. Since the histograms are discrete partitions
    # of the image intensities, the finite difference approximation is
    # normally not very close to the analytical derivatives. Other sources of
    # error are the interpolation used when transforming the images and the
    # boundary intensities introduced when interpolating outside of the image
    # (i.e. some "zeros" are introduced at the boundary which affect the
    # numerical derivatives but is not taken into account by the analytical
    # derivatives). Thus, we need to relax the verification. Instead of
    # looking for the analytical and numerical gradients to be very close to
    # each other, we will verify that they approximately point in the same
    # direction by testing if the angle they form is close to zero.
    h = 1e-4

    # Make sure dictionary entries are processed in the same order regardless
    # of the platform. Otherwise any random numbers drawn within the loop
    # would make the test non-deterministic even if we fix the seed before
    # the loop. Right now, this test does not draw any samples, but we still
    # sort the entries to prevent future related failures.

and

    # Make sure dictionary entries are processed in the same order regardless
    # of the platform. Otherwise any random numbers drawn within the loop
    # would make the test non-deterministic even if we fix the seed before
    # the loop. Right now, this test does not draw any samples, but we still
    # sort the entries to prevent future related failures.

and

        C = [(invalid_vals, valid_vals, valid_points, valid_grad),
             (valid_vals, invalid_vals, valid_points, valid_grad),
             (valid_vals, valid_vals, invalid_points_dim, valid_grad),
             (valid_vals, valid_vals, invalid_points_dim, invalid_grad_dim),
             (valid_vals, valid_vals, invalid_points_len, valid_grad),
             (valid_vals, valid_vals, valid_points, invalid_grad_type),
             (valid_vals, valid_vals, valid_points, invalid_grad_dim),
             (valid_vals, valid_vals, valid_points, invalid_grad_len)]

        for s, m, p, g in C:
            assert_raises(ValueError, H.update_gradient_sparse,
                          theta, transform, s, m, p, g)

?
Again, thank you very much for your help and patience! =)

@omarocegueda

This comment has been minimized.

This is weird, all these lines have a trailing white space (PEP8 rule W291). What editor are you using? Some editors allow you to automatically trim the trailing spaces on saving (Sublime and Spyder can do that for you, and in Windows I think Notepad++ also allows you to do that automatically). This is the output of runing flake8 on /dipy/align/test/test_parzenhist.py:

test_parzenhist.py:3:1: F401 'ndimage' imported but unused
test_parzenhist.py:319:76: W291 trailing whitespace
test_parzenhist.py:320:78: W291 trailing whitespace
test_parzenhist.py:321:77: W291 trailing whitespace
test_parzenhist.py:322:71: W291 trailing whitespace
test_parzenhist.py:323:78: W291 trailing whitespace
test_parzenhist.py:324:76: W291 trailing whitespace
test_parzenhist.py:325:78: W291 trailing whitespace
test_parzenhist.py:326:73: W291 trailing whitespace
test_parzenhist.py:327:76: W291 trailing whitespace
test_parzenhist.py:328:72: W291 trailing whitespace
test_parzenhist.py:329:77: W291 trailing whitespace
test_parzenhist.py:330:75: W291 trailing whitespace
test_parzenhist.py:334:78: W291 trailing whitespace
test_parzenhist.py:335:74: W291 trailing whitespace
test_parzenhist.py:336:75: W291 trailing whitespace
test_parzenhist.py:337:77: W291 trailing whitespace
test_parzenhist.py:373:54: W291 trailing whitespace
test_parzenhist.py:374:53: W291 trailing whitespace
test_parzenhist.py:417:78: W291 trailing whitespace
test_parzenhist.py:418:74: W291 trailing whitespace
test_parzenhist.py:419:75: W291 trailing whitespace
test_parzenhist.py:420:76: W291 trailing whitespace
test_parzenhist.py:609:1: W293 blank line contains whitespace
test_parzenhist.py:618:1: W293 blank line contains whitespace
dtype=np.int32),
codomain_grid2world)
np.array(domain_shape, dtype=np.int32),
domain_grid2world, np.array(codomain_shape,dtype=np.int32),

This comment has been minimized.

@omarocegueda

omarocegueda Mar 7, 2016

Contributor

This is much more readable! it just needs a space after the comma:

domain_grid2world, np.array(codomain_shape, dtype=np.int32),

and remove unused line 3:

 import numpy.testing as npt

This is what flake8 reports on test_imwarp.py:

test_imwarp.py:3:1: F401 'npt' imported but unused
test_imwarp.py:72:51: E231 missing whitespace after ','
dtype=np.int32),
target_grid2world)
disp1, assign1 = vfu.create_random_displacement_2d(
np.array(input_shape, dtype=np.int32),

This comment has been minimized.

@omarocegueda

omarocegueda Mar 7, 2016

Contributor

We have some trailing white spaces in these lines too, this is what flake8 reports on test_vector_fields.py:

test_vector_fields.py:956:47: W291 trailing whitespace
test_vector_fields.py:957:26: W291 trailing whitespace
test_vector_fields.py:958:42: W291 trailing whitespace
test_vector_fields.py:964:47: W291 trailing whitespace
test_vector_fields.py:965:26: W291 trailing whitespace
test_vector_fields.py:966:42: W291 trailing whitespace
test_vector_fields.py:1091:47: W291 trailing whitespace
test_vector_fields.py:1092:60: W291 trailing whitespace
@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Mar 7, 2016

We are really close @souravsingh! just some evil trailing spaces went unnoticed!

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 7, 2016

I don't know why, But flake8 is not reporting any whitespace errors when I run it on test_parzenhist.py. I use the vim editor.

@manu-tej

This comment has been minimized.

Contributor

manu-tej commented Mar 7, 2016

@souravsingh Use the latest version of PEP8. For downloading use 'pip' as it will get you the latest version.

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 7, 2016

@manu-tej flake8 downloads PEP8 as a dependency.I have done a pip freeze to check the list of packages.Here are the version of pep8 and flake8-
pep8==1.7.0
flake8==2.5.4

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Mar 8, 2016

I have the exact same version of pep8 and flake8, that's really weird!, by any chance do you have any local changes that you have not commited/pushed to the remote branch? If you look at the code as shown in github you can see the trailing spaces there. Can you see the trailing spaces in your local test_parzenhist.py file (just to make sure this is flake8 bug)?

souravsingh added some commits Mar 11, 2016

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 11, 2016

@omarocegueda I have fixed the files.Hopefully the scripts should show no errors in flake8.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Mar 11, 2016

Great! this looks good to me, I just verified that pep8 doesn't report any more issues. @arokem, this is ready to merge for me (as soon Travis finishes its tests). Thank you very much @souravsingh !

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 11, 2016

A question- I noticed a few pep8 errors in test_crosscorr and test_imaffine.Should I fix them?

arokem added a commit that referenced this pull request Mar 11, 2016

Merge pull request #947 from souravsingh/pep8
FIX: PEP8 for test files

@arokem arokem merged commit 8836cfa into nipy:master Mar 11, 2016

1 check passed

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

This comment has been minimized.

Member

arokem commented Mar 11, 2016

Thanks! And thanks @omarocegueda for the review.

@souravsingh

This comment has been minimized.

Contributor

souravsingh commented Mar 11, 2016

Thanks for merging the patch @arokem

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