FIX: The GradientTable object should be immutable; the bvalues aren't

[kesshijordan]

being passed into the object correctly, so I changed the function used
to create the GradientTable object

[kesshijordan]

I was using this with a .bvecs and .bvals format input to the GradientTable object, and the units were off by quite a bit when I fit the tensor. It turns out, the bvalues weren't being used by the GradientTable object because they were being set as an attribute after the object was created. They should be input using one of the factory functions to ensure that they are handled correctly.

[coveralls]

Coverage increased (+0.02%) to 72.055% when pulling f41d0f6 on kesshijordan:master into b323622 on nipy:master.

[oesteban]

One question @arokem: is that possible that older versions of dipy only implement the GradientTable?

If I'm not wrong, I implemented it that way because dipy.core.gradient_table didn't seem to be available in older versions of dipy.

Once confirmed, may I ask you @kesshijordan to go back to a try ... except structure, but using the recommended gradient_table as default?. Something like:

try:
    from dipy.core.gradients import gradient_table
    gtab = gradient_table(bval, bvec)
except NameError:
    from dipy.data import GradientTable
    gtab = GradientTable(bvec)
    gtab.bvals = bval

[arokem]

This has been in place since version 0.6 (2012):
dipy/dipy@1714e7c

On Wed, May 25, 2016 at 9:05 PM, Oscar Esteban notifications@github.com
wrote:

One question @arokem https://github.com/arokem: is that possible that
older versions of dipy only implement the GradientTable?

If I'm not wrong, I implemented it that way because
dipy.core.gradient_table didn't seem to be available in older versions of
dipy.

Once confirmed, may I ask you @kesshijordan
https://github.com/kesshijordan to go back to a try ... except
structure, but using the recommended gradient_table as default?.
Something like:

try:
from dipy.core.gradients import gradient_table
gtab = gradient_table(bval, bvec)
except NameError:
from dipy.data import GradientTable
gtab = GradientTable(bvec)
gtab.bvals = bval

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1484 (comment)

[kesshijordan]

The way that is written, the bvals you are setting will not be reflected by the GradientTable object, and everything will by off by a factor of the bvalue. We could put just the vectors in, and let the user combine their bvals&bvecs in the except case (@MrBago, perhaps you could comment?)

[oesteban]

Well, looking at the old code: dipy/dipy@1714e7c#diff-3d2aafe93b8f41be9cf3521aa9d5d194L5, maybe this would do:

try:
    from dipy.core.gradients import gradient_table
    gtab = gradient_table(bval, bvec)
except NameError:
    from dipy.data import GradientTable
    gtab = GradientTable(bval, bvecs=bvec)

Don't you think?

[MrBago]

@oesteban GradientTable and gradient_table were added in the same release, the older version you're looking at was a WIP that was never made public. I just checked and dipy 0.5.0 does not have a dipy.core.gradients file at all.

The issue may have been because GradientTable is being imported from dipy.data instead of dipy.core.gradients (I'm not sure when we started importing GradientTable into dipy.data). I believe if you want a GradientTable from a bvec/bval pair, you should be using the helper functions.

Just for context, the helper functions do a bunch of checking for errors and corner cases then call GradientTable(bval[:, None] * bvec).

[oesteban]

@MrBago thanks for the details :). What do you think we should implement for nipype? Should we get rid of the try ... catch, and go use gradient_table?

[MrBago]

I'm not sure about the try catch, why is it catching a NameError?

[oesteban]

I included it because I was having that exception in earlier versions of dipy. It should be an ImportError , correct?

EDIT: I mean, maybe the try catch is not necessary at all.

[MrBago]

In that case I don't think you need the try catch.

On Thu, May 26, 2016, 12:08 PM Oscar Esteban notifications@github.com
wrote:

I included it because I was having that exception in earlier versions of
dipy. It should be an ImportError , correct?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1484 (comment)

[arokem]

I think that what @MrBago means is that since the code proposed here imports from the right place (from dipy.core.gradients, this should work on older versions of Dipy as well. I am +1 for merging this PR.