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

Improve check of collinearity in vec2vec_rotmat #648

Merged
merged 3 commits into from Jun 24, 2015

Conversation

Projects
None yet
4 participants
@oesteban
Contributor

oesteban commented May 13, 2015

  • More strict check of collinearity of vectors.
  • Fixed check for nans in the end.
norm_u_v = np.linalg.norm(u - v)
# return eye when u is the same with v
if np.linalg.norm(u - v) < np.finfo(float).eps:
w = np.cross(u, v)

This comment has been minimized.

@stefanv

stefanv May 14, 2015

Contributor

Documenting the logic behind the code would be helpful

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented May 30, 2015

@oesteban thank you for your PR as @stefanv suggests it would be nice to explain a bit the logic of your change. I would like to see a comment in the code or here explaining why your change is a more strict check of collinearity. After that I will merge this PR. Sounds good?

@oesteban

This comment has been minimized.

Contributor

oesteban commented Jun 1, 2015

I basically tried to replace workarounds by directly checking the cross product (which is at the core of finding the rotation matrix).

@arokem

This comment has been minimized.

Member

arokem commented Jun 2, 2015

I am +1 for this change. The previous implementation would only work for unit vectors, but despite the documentation stating that the inputs should be unit vectors, there is nothing enforcing that they are. I think that this is more robust for distinguishing the antipodal vs. colinear case.

@arokem arokem referenced this pull request Jun 6, 2015

Merged

Test DTI eigenvectors #661

@arokem

This comment has been minimized.

Member

arokem commented Jun 24, 2015

Since there were no further objections here, I am merging this.

arokem added a commit that referenced this pull request Jun 24, 2015

Merge pull request #648 from oesteban/fix/ColinearUVvec2vec_rotmat
Improve check of collinearity in vec2vec_rotmat

@arokem arokem merged commit b8b62cb into nipy:master Jun 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@oesteban oesteban deleted the oesteban:fix/ColinearUVvec2vec_rotmat branch Jun 25, 2015

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