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

Fixes #931 : checks if nb_points=0 #932

Merged
merged 1 commit into from Feb 24, 2016

Conversation

Projects
None yet
4 participants
@sahmed95
Contributor

sahmed95 commented Feb 19, 2016

Fixed

@arokem

This comment has been minimized.

Member

arokem commented Feb 19, 2016

LGTM - I don't mind the PEP8 fix that's here, but please take additional ones on another PR. Thanks!

Anyone else have any comments on this?

@@ -240,6 +240,9 @@ def set_number_of_points(streamlines, nb_points=3):
if len(streamlines) == 0:
return []

if (nb_points == 0):

This comment has been minimized.

@MarcCote

MarcCote Feb 20, 2016

Contributor

Parentheses aren't needed here.

@@ -240,6 +240,9 @@ def set_number_of_points(streamlines, nb_points=3):
if len(streamlines) == 0:
return []

if (nb_points == 0):
raise ValueError("nb_points canot be 0")

This comment has been minimized.

@MarcCote

MarcCote Feb 20, 2016

Contributor

canot => cannot

@@ -296,6 +296,10 @@ def test_set_number_of_points():
assert_equal(len(set_number_of_points(streamlines_readonly, nb_points=42)),
len(streamlines_readonly))

# Test if nb_points is 0
assert_raises(ValueError, set_number_of_points, [np.ones((10,3)),

This comment has been minimized.

@MarcCote

MarcCote Feb 20, 2016

Contributor

According to PEP8 a space is needed after the comma in tuple.

@@ -296,6 +296,10 @@ def test_set_number_of_points():
assert_equal(len(set_number_of_points(streamlines_readonly, nb_points=42)),
len(streamlines_readonly))

# Test if nb_points is 0
assert_raises(ValueError, set_number_of_points, [np.ones((10,3)),
np.ones((10,3))], nb_points=0)

This comment has been minimized.

@MarcCote

MarcCote Feb 20, 2016

Contributor

Same thing here.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 20, 2016

Thanks for the fix,

Hmm. reading the code I realised that nb_points needs to be 2 or greater. If nb_points is 1, you'll end up with a division by zero!

Do you mind changing the test condition to include such case? And while at it, changing the error message accordingly and adding a new unit test.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Feb 20, 2016

on it. Should there be two separate cases ? for nb_points = 0 and nb_points < 2 or should it be just one case checking nb_points < 2 ?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 20, 2016

check for nb point > 2 always, negative is nonsense and good marc-alexandre saw the 1 point thing, a quick look did not make raise a brow.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 20, 2016

+1 for simply checking nb_points >= 2 and also casting nb_points to integer at the beginning of the function (see my #931 (comment)).

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Feb 21, 2016

Test error if I check nb_points < 2 using :

    if nb_points < 2:
        raise ValueError("nb_points should be greater than or equal to 2")

The error is happening because of this :
File "/home/travis/build/nipy/dipy/venv/lib/python2.7/site-packages/dipy/segment/tests/test_feature.py", line 79, in test_feature_resample
assert_array_almost_equal(features, set_number_of_points(s, nb_points))

for nb_points in [1, 5, 2*max_points]:
        for feature in [dipymetric.ResampleFeature(nb_points), ResampleFeature(nb_points)]:
            for s in [s1, s2, s3, s4]:
                # Test method infer_shape
                assert_equal(feature.infer_shape(s), (nb_points, s.shape[1]))

                # Test method extract
                features = feature.extract(s)
                assert_equal(features.shape, (nb_points, s.shape[1]))
                assert_array_almost_equal(features, set_number_of_points(s, nb_points))

Any idea why nb_points is 1 here ?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 22, 2016

@sahmed95, you are right, it should have been 2 instead of 1. You can go ahead and change it.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Feb 22, 2016

I have changed it and all test cases are passing. Please have a look.

Non-negative integers (<2) are then taken care of. For points like 2.3, numpy gives a warning. Instead of casting nb_points to an integer why not let numpy raise the error? If the user has made a mistake which is making the nb_points non integer then he should know of it right ?

@arokem

This comment has been minimized.

Member

arokem commented Feb 22, 2016

+1, as long as it doesn't segfault

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Feb 22, 2016

Ok, so is this enough to close this? Or some more tests are necessary ?

if nb_points == 0:
raise ValueError("nb_points cannot be 0")

if nb_points < 2:

This comment has been minimized.

@samuelstjean

samuelstjean Feb 22, 2016

Contributor

This already encompass the check for 0 so you could just remove it altogether.

raise ValueError("nb_points cannot be 0")

if nb_points < 2:
raise ValueError("nb_points must atleast be 2")

This comment has been minimized.

@samuelstjean

samuelstjean Feb 22, 2016

Contributor

and space in at least

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 22, 2016

After you fixed the typo at line 247 and remove the test at line 243, all will be good. 👍

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 23, 2016

I was about to merge this PR but it seems master as changed and now there are conflicts. Can you do I quick rebase from master and I will merge it. Again thanks for the fix.

Shahnawaz Ahmed
@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Feb 24, 2016

Done. :)

MarcCote added a commit that referenced this pull request Feb 24, 2016

Merge pull request #932 from sahmed95/nb
Fixes #931 : check that nb_points > 2 in `dipy.tracking.streamline.set_number_of_points`

@MarcCote MarcCote merged commit 82142f3 into nipy:master Feb 24, 2016

1 check passed

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

@sahmed95 sahmed95 referenced this pull request Mar 2, 2016

Open

it's finally here #1

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