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

Fix vec2vec #334

Merged
merged 5 commits into from Apr 11, 2014

Conversation

Projects
None yet
3 participants
@arokem
Member

arokem commented Mar 11, 2014

This fixes a small bug in the calculation of rotation matrices.

The fix in the sims module follows, because test failures upon applying this fix indicated that this calculation was front-to-back. I think everything's alright now

arokem added some commits Jan 14, 2014

TST: This test currently fails.
Fix to follow immediately :-)
BF: Calculation of eigen-vectors was confused by sign reversal.
This should fix this. No other side effects, as far as I could tell.
rot_back = np.dot(Rp, v)
sign_reverser = np.sign((np.sign(rot_back) == np.sign(u)) - 0.5).squeeze()
# Multiply each line by it's reverser and reassmble the matrix:

This comment has been minimized.

@matthew-brett

matthew-brett Mar 11, 2014

Member
# Multiply each line by its reverser and reassemble the matrix:

(sorry)

# Multiply each line by it's reverser and reassmble the matrix:
Rp = np.array([np.array(Rp[i,:]) *
sign_reverser[i] for i in range(3)]).squeeze()

This comment has been minimized.

@matthew-brett

matthew-brett Mar 11, 2014

Member

Sorry - maybe not following - but can this be done by broadcasting maybe with a newaxis ?

This comment has been minimized.

@arokem

arokem Mar 12, 2014

Member

Yes - thanks for the suggestion. Applying that now.

On Tue, Mar 11, 2014 at 3:44 PM, Matthew Brett notifications@github.comwrote:

In dipy/core/geometry.py:

@@ -859,8 +862,17 @@ def vec2vec_rotmat(u, v):
R = np.array([[cosa, -sina, 0], [sina, cosa, 0], [0, 0, 1]])
Rp = np.dot(Pt, np.dot(R, P))

  • Everything's fine, up to a sign reversal:

  • rot_back = np.dot(Rp, v)
  • sign_reverser = np.sign((np.sign(rot_back) == np.sign(u)) - 0.5).squeeze()
  • Multiply each line by it's reverser and reassmble the matrix:

  • Rp = np.array([np.array(Rp[i,:]) *
  •               sign_reverser[i] for i in range(3)]).squeeze()
    

Sorry - maybe not following - but can this be done by broadcasting maybe
with a newaxis ?


Reply to this email directly or view it on GitHubhttps://github.com//pull/334/files#r10498203
.

# Multiply each line by it's reverser and reassmble the matrix:
Rp = np.array([np.array(Rp[i,:]) *
sign_reverser[i] for i in range(3)]).squeeze()
# make sure that you don't return any Nans
if np.sum(np.isnan(Rp)) > 0:
return np.eye(3)

This comment has been minimized.

@matthew-brett

matthew-brett Mar 11, 2014

Member

Forgive ignorance again - but is eye(3) a reasonable default for all possible nan cases?

This comment has been minimized.

@arokem

arokem Mar 12, 2014

Member

I don't know - I didn't put it there :-)

It would definitely cover the most typical nan case, which is when the
inputs are identical. But that case is already covered in the first lines
of this function, so I am not sure what other nan conditions might exist.

For what it's worth, commenting out those lines doesn't cause any test
failures, so at the very least we are not testing any of these other,
unknown nan conditions. Should I remove these lines?

On Tue, Mar 11, 2014 at 3:45 PM, Matthew Brett notifications@github.comwrote:

In dipy/core/geometry.py:

@@ -859,8 +862,17 @@ def vec2vec_rotmat(u, v):
R = np.array([[cosa, -sina, 0], [sina, cosa, 0], [0, 0, 1]])
Rp = np.dot(Pt, np.dot(R, P))

  • Everything's fine, up to a sign reversal:

  • rot_back = np.dot(Rp, v)
  • sign_reverser = np.sign((np.sign(rot_back) == np.sign(u)) - 0.5).squeeze()
  • Multiply each line by it's reverser and reassmble the matrix:

  • Rp = np.array([np.array(Rp[i,:]) *
  •               sign_reverser[i] for i in range(3)]).squeeze()
    

    make sure that you don't return any Nans

    if np.sum(np.isnan(Rp)) > 0:
    return np.eye(3)

Forgive ignorance again - but is eye(3) a reasonable default for all
possible nan cases?


Reply to this email directly or view it on GitHubhttps://github.com//pull/334/files#r10498227
.

# Everything's fine, up to a sign reversal:
rot_back = np.dot(Rp, v)
sign_reverser = np.sign((np.sign(rot_back) == np.sign(u)) - 0.5).squeeze()
# Multiply each line by it's reverser and reassmble the matrix:

This comment has been minimized.

@Garyfallidis
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 16, 2014

Thank you for the bug fix. Please fix this tiny typo and I will happily do the merge :)

@arokem

This comment has been minimized.

Member

arokem commented Mar 17, 2014

Done!

On Sun, Mar 16, 2014 at 4:50 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Thank you for the bug fix. Please fix this tiny typo and I will happily do
the merge :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/334#issuecomment-37775908
.

Garyfallidis added a commit that referenced this pull request Apr 11, 2014

@Garyfallidis Garyfallidis merged commit 911a6fc into nipy:master Apr 11, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment