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

Test DTI eigenvectors #661

Merged
merged 6 commits into from Aug 1, 2015

Conversation

Projects
None yet
3 participants
@arokem
Member

arokem commented Jun 6, 2015

As pointed out by @RafaelNH here: #582, there seems to be some inconsistency about whether eigenvectors of tensors should be arranged column-wise or row-wise in 3-by-3 arrays. I am trying to enforce something uniform here. At the moment, this triggers several other test failures, but I wonder whether these are because of patching around this inconsistency, and would just require some additional fixing to make all parts consistent. I am continuing to investigate.

@arokem

This comment has been minimized.

Member

arokem commented Jun 6, 2015

OK - I think that I have patched up all the subsequent test failures, but I'll wait to see if Travis comes up with something I've missed.

If there's one thing I've learned from this, it is how poorly our simulation module is covered. I had to rely on tests from all kinds of other modules (e.g. SHORE) to tell me that the "ground truth" simulation was doing it wrong.

@arokem

This comment has been minimized.

Member

arokem commented Jun 6, 2015

Also just confirmed that this will work fine when we merge #648

@arokem arokem force-pushed the arokem:evecs-to-columns branch from d5b15ff to 26dde28 Jun 8, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jun 8, 2015

This PR was made a bit obsolete with the merging of #582, but there is still some useful stuff here. In particular, more testing of DTI. Changing the title.

@arokem arokem changed the title from Evecs to columns to Test DTI eigenvectors Jun 8, 2015

@arokem arokem force-pushed the arokem:evecs-to-columns branch from 26dde28 to 84af594 Jun 24, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jun 24, 2015

This one's now rebased. This one would be a good opportunity to gain experience reviewing code for someone who's interested in pitching in, but doesn't have a lot of time to go over large PRs.

@arokem arokem force-pushed the arokem:evecs-to-columns branch from 84af594 to 9a153bb Jul 3, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jul 3, 2015

Just rebased again. With 26 lines of added testing, this one should be uncontroversial and a real quick review. Would someone please take a look?

# Check that eigenvalues and eigenvectors are properly sorted through
# that previous operation:
for i in range(3):
assert_array_equal(np.dot(tensor, evecs[:, i]),

This comment has been minimized.

@omarocegueda

omarocegueda Jul 3, 2015

Contributor

In general, we should avoid comparing floating point values for equality, because even if the operations are equivalent, the result will in general depend on the order of evaluation, for example:

x = (np.float64(3.1) / np.float64(2.1)) * (np.float64(2.1) / np.float64(3.1))
y = (np.float64(3.1) * np.float64(2.1)) / (np.float64(3.1) * np.float64(2.1))
x == y

Which evaluates to False.
I think assert_array_almost_equal with a precision of about 1e-9 would be better.

@arokem

This comment has been minimized.

Member

arokem commented Jul 3, 2015

Thanks for the comment! Changed accordingly.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jul 3, 2015

Thanks Ariel!, it looks good to me =)

@arokem

This comment has been minimized.

Member

arokem commented Jul 9, 2015

Any more comments here?

@arokem arokem force-pushed the arokem:evecs-to-columns branch from d317627 to 1030481 Jul 15, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jul 15, 2015

Just rebased this on top of master. Would anyone please give this a review and merge this?

@arokem arokem force-pushed the arokem:evecs-to-columns branch from 1030481 to e3fa651 Jul 27, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jul 27, 2015

Um, hullo? Is this thing on?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 1, 2015

Thx @arokem. 👍

Garyfallidis added a commit that referenced this pull request Aug 1, 2015

@Garyfallidis Garyfallidis merged commit 2f11a74 into nipy:master Aug 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment