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

GradientTable mask does not account for nan's in b-values #93

Closed
stefanv opened this Issue Nov 6, 2012 · 9 comments

Comments

Projects
None yet
3 participants
@stefanv
Contributor

stefanv commented Nov 6, 2012

The b0s_mask is not correctly generated for nan b-values. Worse, when nan's are present, both the b0s_mask and the opposite mask must be false in those positions. It seems the only way to do that would be to add back a "signal_mask" or equivalent.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Nov 6, 2012

What's the meaning of nan b-value?
On Nov 6, 2012 2:29 AM, "Stefan van der Walt" notifications@github.com
wrote:

The b0s_mask is not correctly generated for nan b-values. Worse, when
nan's are present, both the b0s_mask and the opposite mask must be false in
those positions. It seems the only way to do that would be to add back a
"signal_mask" or equivalent.


Reply to this email directly or view it on GitHubhttps://github.com//issues/93.

@stefanv

This comment has been minimized.

Contributor

stefanv commented Nov 6, 2012

I constructed a gradient-table using the data in small_64D based on bvals[:, None] * bvecs. But the first line of bvecs is nan, so boom!

@MrBago

This comment has been minimized.

Contributor

MrBago commented Nov 6, 2012

I specifically wrote a function to handle all the weirdness of nans in the bvecs, bvals. Try this, I believe it should work:

grad_table = gradient_table_from_bvals_bvecs(bvals, bvecs)
or
gradient_table(bvals, bvecs)

I understand the advantage of being able to write the following and have it work, especially because new users might go to it first before they find the above function:

gradients = bvals[:, None] * bvecs
GradientTable(gradients)

But i'm not sure that we should support nans in gradients. Nans in gradients is meaningless and nans in bvecs is pretty rare. Assuming that nans == 0 is a bit of a hack, and do we really want to set gradient = [0, 0, 0] if bval == 2000 and bvec == [nan, nan, nan]? What if bvec == [nan, nan, .4]? Let me know what you guys think.

@stefanv

This comment has been minimized.

Contributor

stefanv commented Nov 7, 2012

We could also just handle nan's as missing values. That's something I'd like to extend to the models as well. E.g., I often do simulations where I "drop" measurements, so there I coded the kernel model to simply ignore the fitting matrix columns corresponding to missing elements in the data.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Nov 7, 2012

Would we need to change the GradientTable to make that happen?

Bago

On Tue, Nov 6, 2012 at 4:02 PM, Stefan van der Walt <
notifications@github.com> wrote:

We could also just handle nan's as missing values. That's something I'd
like to extend to the models as well. E.g., I often do simulations where I
"drop" measurements, so there I coded the kernel model to simply ignore the
fitting matrix columns corresponding to missing elements in the data.


Reply to this email directly or view it on GitHubhttps://github.com//issues/93#issuecomment-10133196.

@stefanv

This comment has been minimized.

Contributor

stefanv commented Nov 7, 2012

No, I just meant a consistent treatment of nan's throughout DiPy would be good. And I think this is an example of where it is needed.

@arokem

This comment has been minimized.

Member

arokem commented Jan 11, 2014

Are we still having this discussion? @stefanv? @MrBago ? I actually recently ran into something like this myself (nan's in bvecs in some of the test data-sets), so maybe we should :-)

@stefanv

This comment has been minimized.

Contributor

stefanv commented Jan 12, 2014

I think the GradientTable problem could be somewhat ameliorated by adding a class method for construction (as Bago also recommended in another recent PR)--that should at least make the nan-aware constructor more discoverable.

But in the long run, we need to discuss how we want to handle NaN values--both in data, b-values and directions.

@arokem

This comment has been minimized.

Member

arokem commented Jan 27, 2016

This ancient one may be resolved with #817. Closing for now.

@arokem arokem closed this Jan 27, 2016

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