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

Replace raise by auto normalization when creating a gradient table with un-normalized bvecs. #452

Closed
matthieudumont opened this Issue Oct 28, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@matthieudumont
Contributor

matthieudumont commented Oct 28, 2014

When using gradients.gradient_table_from_bvals_bvecs with non-normalized bvecs, an exception is currently raised.

I was wondering if auto normalizing the bvecs was a viable alternative to that. This could be either triggered by an 'auto_normalize' parameter or just done if abs(norm - 1) > atol.

If you like the idea I dont mind doing the pull request for this.

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 28, 2014

I personally think this feature would cause more harm than good. bvecs, as
we use them, are unit vectors. If they are not unit vectors their meaning
becomes ambiguous. Take for example these bval/bvec values:

2000 [1., 0, 0]
2000 [.5, 0, 0](this could mean [2000, 0, 0] or [1000, 0, 0])
1000 [.5, 0, 0](this could mean [1000, 0, 0] or [500, 0, 0])

Also it might not be obvious to someone else reading the code that the
vectors are being normalized in the function. I think it is more clear to
use something like:

gtab = gradient_table_from_bvals_bvecs(bvals, normalize(vertors))

It's obvious to anyone reading the code that the directions of vectors
are being used as the bvecs. I believe we have some version of normalize
in dipy.core.geometry, if we don't we should add it either there or in
dipy.core.gradients.

Bago

On Tue, Oct 28, 2014 at 9:08 AM, matthieudumont notifications@github.com
wrote:

When using gradients.gradient_table_from_bvals_bvecs with non-normalized
bvecs, an exception is currently raised.

I was wondering if auto normalizing the bvecs was a viable alternative to
that. This could be either triggered by an 'auto_normalize' parameter or
just done if (norm - 1) > atol.

If you like the idea I dont mind doing the pull request for this.


Reply to this email directly or view it on GitHub
#452.

@rfdougherty

This comment has been minimized.

rfdougherty commented Oct 28, 2014

I also think auto-normalizing bvecs is a bad idea. On GE scanners, multi-shell diffusion is achieved by scaling the bvec magnitude in the scanner's tensor.dat file, which is a look-up table of gradient directions to be played out for any particular number of prescribed diffusion directions. And these scaled bvecs are what end up in the DICOM header, along with the prescribed bvalue (which is actually not the correct bvalue for non-unit length bvecs). E.g., the effective bvalue for a particular measurement is the prescribed bvalue stored in the DICOM header * bvec_norm**2. So a typical multi-shell scan on GE will have bvecs with non-unit norms, and auto-normalizing them will create an incorrect gradient table. I suggest making the normalization explicit (as suggested by Bago). If the user has GE vectors (and knows what they are doing) they might use:

gtab = gradient_table_from_bvals_bvecs(bvals_norm(vectors)_*2, normalize(vertors))

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Oct 28, 2014

Allright!

Thanks for quick replies.

@rfdougherty

This comment has been minimized.

rfdougherty commented Oct 28, 2014

Maybe it would be useful to add this info (with examples) to the help text of gradient_table_from_bvals_bvecs, and perhaps to the exception error message?

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 28, 2014

On Tue, Oct 28, 2014 at 11:11 AM, Bob Dougherty notifications@github.com
wrote:

gtab = gradient_table_from_bvals_bvecs(bvals_norm(vectors)_*2,
normalize(vertors))

Just a quick aside, if you do not already have the gradients expressed as
bvec/bval it's often easier to use the GradientTable class directly instead
of using the above factory function, for example in the GE case I think it
would be:

gtab = GradientTable( bvals[:, np.newaxis] * bvecs )


Reply to this email directly or view it on GitHub
#452 (comment).

@rfdougherty

This comment has been minimized.

rfdougherty commented Oct 28, 2014

Cool-- thanks for the tip!

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