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: add tests for AffineMap #700

Merged
merged 9 commits into from Oct 6, 2015

Conversation

Projects
None yet
4 participants
@omarocegueda
Contributor

omarocegueda commented Aug 13, 2015

This PR just adds tests for the AffineMap class.

@omarocegueda omarocegueda referenced this pull request Aug 13, 2015

Closed

WIP: affine map tests #693

def create_affine_transforms(dim, translations, rotations, scales, rot_axis=None):
r""" Creates a list of affine transforms with all combinations of params
This function is intended to be used for testing only. It generates

This comment has been minimized.

@arokem

arokem Aug 27, 2015

Member

Why is this function only for testing? I can see this being useful elsewhere as well.

This comment has been minimized.

@omarocegueda

omarocegueda Aug 27, 2015

Contributor

Oh! really!? I don't imagine an application other than generating some random transforms for testing. Where do you think we should put it? some utilities module?

This comment has been minimized.

@arokem

arokem Sep 2, 2015

Member

Yes - I would think something like that. Anyone else have strong opinions about this one?

This comment has been minimized.

@matthew-brett

matthew-brett Sep 7, 2015

Member

No strong opinion, but why not leave it in the test module for now, and see if anyone has use for it?

codomain_grid2world = affine
grid2grid_transform = affine
# Evaluate the transform with vector_fields module (already tested)

This comment has been minimized.

@arokem

arokem Aug 27, 2015

Member

What does it mean that it's "already tested"? Does that mean that we treat this as ground truth here?

This comment has been minimized.

@omarocegueda

omarocegueda Aug 27, 2015

Contributor

Yes, exactly, transform_2d_affine and transform_3d_affine have their own tests, which compare against scipy's map_coordinates. In this case, the test is much cleaner by using transform_2d_affine and transform_3d_affine as oracles instead of creating the full coordinate arrays to call map_coordinates.

assert(affine_map.affine_inv is None)
else:
assert_array_equal(affine, affine_map.affine)
actual = affine_map.affine.dot(affine_map.affine_inv)

This comment has been minimized.

@arokem

arokem Aug 27, 2015

Member

This is just to show that these properties really are the inverses of each other?

This comment has been minimized.

@omarocegueda

omarocegueda Aug 27, 2015

Contributor

Yes. Too paranoid? =)

@@ -798,13 +798,9 @@ def test_affine_transforms_2d():
shape = np.array(codomain_shape, dtype=np.int32)
# Exceptions from transform_2d
assert_raises(ValueError, vfu.transform_2d_affine, circle, shape, invalid)
assert_raises(ValueError, vfu.transform_2d_affine, circle, shape, invalid)
assert_raises(ValueError, vfu.transform_2d_affine, circle, shape, invalid)

This comment has been minimized.

@arokem

arokem Aug 27, 2015

Member

These were just repeated here? Curious.

This comment has been minimized.

@omarocegueda

omarocegueda Aug 27, 2015

Contributor

Yes... in Notepad++, Ctrl+d means "duplicate current line" and 'd' is just besides 's'... I guess I accidentally pressed 'd' (twice?! =S) attempting to save...

This comment has been minimized.

@arokem

arokem Sep 2, 2015

Member

Notepad++?! You are a strange individual :-)

This comment has been minimized.

@omarocegueda

omarocegueda Sep 7, 2015

Contributor

Thank you?... I guess... =P

@@ -19,6 +19,8 @@ def is_valid_affine(double[:, :] M, int dim):
return False
if np.any(np.isnan(M)):
return False
if np.any(np.isinf(M)):
return False

This comment has been minimized.

@arokem

arokem Aug 27, 2015

Member

Nice one. You could use np.isfinite to catch both of these.

This comment has been minimized.

@omarocegueda

omarocegueda Aug 27, 2015

Contributor

Thanks! fixed.

@arokem

This comment has been minimized.

Member

arokem commented Sep 2, 2015

Except for the controversy around whether that one function belongs in the tests, or somewhere else, I am pleased with all this, but I have a hard time determining whether this covers all we need. Would be good if someone else with a knowledge of this part of the codebase (e.g., @matthew-brett ) took a look at this.

return
if np.any(np.isnan(affine)):
if np.any(np.isnan(affine)) or np.any(np.isinf(affine)):

This comment has been minimized.

@matthew-brett

matthew-brett Sep 2, 2015

Member

if not np.all(np.isfinite(affine)) ?

This comment has been minimized.

@omarocegueda

omarocegueda Sep 7, 2015

Contributor

Fixed, thanks!

@@ -222,8 +225,9 @@ def _apply_transform(self, image, interp='linear', image_grid2world=None,
shape = np.array(sampling_grid_shape, dtype=np.int32)
# Verify valid image dimension
if dim < 2 or dim > 3:
raise ValueError('Undefined transform for dimension: %d' % (dim,))
img_dim = len(image.shape)

This comment has been minimized.

@matthew-brett

matthew-brett Sep 2, 2015

Member

Sorry to be lazy - so this was previously a bug right? In that you were checking the output dims rather than the input dims? Do the current tests check for this?

This comment has been minimized.

@omarocegueda

omarocegueda Sep 7, 2015

Contributor

Yes, actually I found this bug while writing the tests for the exceptions. Here I verify that the exception is raised when attempting to transform an image of dimension 1 and 4:
https://github.com/nipy/dipy/pull/700/files#diff-5a512c70f85666010a0c8c3d13db8f3aR508

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Sep 14, 2015

@omarocegueda rebase to remove the rbf error. Then you have some other tests failing. Keep it up!

@omarocegueda omarocegueda force-pushed the omarocegueda:imaffine_tests branch from e5aa8da to 49ad2f3 Sep 14, 2015

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Sep 14, 2015

Rebased, thanks!

@arokem

This comment has been minimized.

Member

arokem commented Oct 2, 2015

This looks ready to merge. If no one objects, I will go ahead and do that some time next week.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Oct 2, 2015

Great! =) thanks Ariel!

arokem added a commit that referenced this pull request Oct 6, 2015

Merge pull request #700 from omarocegueda/imaffine_tests
TEST: add tests for AffineMap

@arokem arokem merged commit 8434726 into nipy:master Oct 6, 2015

1 check passed

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

This comment has been minimized.

Contributor

omarocegueda commented Oct 6, 2015

Thanks Ariel! I'm so happy that buildbots didn't complain this time =)

@arokem

This comment has been minimized.

Member

arokem commented Oct 6, 2015

Yeah - it's like a small miracle when that happens...

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