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

dipy.align.vector_fields.simplify_warp_function_3d: Wrong equation in docstring #1213

Closed
quakberto opened this Issue Apr 4, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@quakberto

quakberto commented Apr 4, 2017

Hi,

the docstring of the function simplify_warp_function_3d says that its result will be a deformation field satisfying:
(1) T[i] = W * d[U * i] + V * i

However, I think this equation should be:
(1) T[i] = W * d[U * i] + V * i - i or
(1) T[i] = W * d[U * i] + (V - I)* i,
since in lines 1585, 1587 and 1589 the coordinate is subtracted after the applying the affine transform V:
_apply_affine_3d_x0(k, i, j, 1, affine_idx_out) - k

Hope this is the right place for this comment and thanks for the great tool!
Best,
quakberto

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Apr 4, 2017

Hi @quakberto!,
thank you very much for reporting this!, we are taking a closer look to make sure it's just a documentation error.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Apr 4, 2017

Hi @quakberto,
The documentation is correct. I think the confusion is that the user may expect out = T, but that would be inconsistent with the rest of the align module, since all non-linear transforms are represented using deformation fields instead of actual mapped values for accuracy reasons.

More precisely, if the output of simplify_warp_function_3d is out then, in order to compute T in (1) we need to do:

T[i] = i + out[i] 

I'll make a PR to state this more precisely.
Thanks!

@quakberto

This comment has been minimized.

quakberto commented Apr 4, 2017

Hi @omarocegueda,
that makes sense! Indeed I thought that T was the intended output.
I'm only starting to get familiar with nipy/dipy's conventions, so thanks a lot for the clarification!
Best

@arokem

This comment has been minimized.

Member

arokem commented Apr 5, 2017

(hopefully) Closed through #1214

@arokem arokem closed this Apr 5, 2017

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