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

RF: Convert nan values in bvectors to 0's #817

Merged
merged 4 commits into from Jan 18, 2016

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Dec 21, 2015

This get's rid of warnings of the following type that currently show up
in our tests:

./Users/arokem/source/dipy/dipy/core/gradients.py:132: RuntimeWarning: invalid value encountered in less_equal

bvecs_close_to_1 = abs(vector_norm(bvecs) - 1) <= atol

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Dec 21, 2015

This changes the API, no? What do people use NaN bval values for?

Needs a test.

@arokem arokem force-pushed the arokem:warning-nan-bvecs branch from c6f0d1f to 2cfdad1 Dec 21, 2015

@arokem

This comment has been minimized.

Member

arokem commented Dec 21, 2015

Added a test to clarify the case in which this is happening. In the data set that we commonly use for our own tests, a nan bvector is used to mark a 0 b-value.

Either way, I don't think that this is a change in API, because I think that it's exactly equivalent to what was previously happening on line 137

I have now also removed that line, and rebased to change the order of things: first the test and then the fix.

@arokem arokem force-pushed the arokem:warning-nan-bvecs branch from 5287096 to 57b4712 Jan 11, 2016

@arokem

This comment has been minimized.

Member

arokem commented Jan 11, 2016

Just rebased this one on master. Anyone care to review/merge?

bvecs_close_to_1 = abs(vector_norm(bvecs) - 1) <= atol
if bvecs.shape[1] != 3 or not np.all(bvecs_close_to_1[dwi_mask]):
raise ValueError("bvecs should be (N, 3), a set of N unit vectors")

bvecs = np.where(bvecs_close_to_1[:, None], bvecs, 0)

This comment has been minimized.

@MrBago

MrBago Jan 11, 2016

Contributor

@arokem this line is here because in some data sets a low b-value (~5-30) is used for the b-0 directions. There was a little back and forth about this, but I decided to squash these values because they don't come with a direction vector and a gradient with no direction vector doesn't really make sense. I think we should leave this line in, I can make a test for this kind of data.

@@ -134,11 +134,11 @@ def gradient_table_from_bvals_bvecs(bvals, bvecs, b0_threshold=0, atol=1e-2,
"respectively, where N is the number of diffusion "
"gradients")

bvecs[np.isnan(bvecs)] = 0

This comment has been minimized.

@MrBago

MrBago Jan 11, 2016

Contributor

Please use something like bvecs = np.where(np.isnan(bvecs), 0, bvecs), bvecs is an argument to the function, we don't want to silently modify the function arguments.

@@ -243,7 +243,7 @@ def gradient_table(bvals, bvecs=None, big_delta=None, small_delta=None,
" array containing both bvals and bvecs")
else:
bvecs = np.asarray(bvecs)
if (bvecs.shape[1] > bvecs.shape[0]) and bvecs.shape[0] > 1:
if (bvecs.shape[1] > bvecs.shape[0]) and bvecs.shape[0] > 1:

This comment has been minimized.

@MrBago

MrBago Jan 11, 2016

Contributor

what did you change here?

This comment has been minimized.

@arokem

arokem Jan 11, 2016

Member

whitespace

@arokem

This comment has been minimized.

Member

arokem commented Jan 11, 2016

Thanks for taking a look @MrBago - fixed according to your suggestions. Would be good to have test-cases for those bvecs_close_to_1 cases you mentioned.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 17, 2016

Is this ready for a merge or we are waiting for the close to 1 case?

@arokem

This comment has been minimized.

Member

arokem commented Jan 17, 2016

Ready from my point of view, unless @MrBago wants to add the test-cases discussed. He can also do this on a separate PR. At the very least, to my understanding, this now gets rid of those unnecessary warnings, and doesn't change any other behavior.

Garyfallidis added a commit that referenced this pull request Jan 18, 2016

Merge pull request #817 from arokem/warning-nan-bvecs
RF: Convert nan values in bvectors to 0's

@Garyfallidis Garyfallidis merged commit c3a86ac into nipy:master Jan 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment