Skip to content
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

BF: nan entries cause segfault #690

Merged
merged 4 commits into from Aug 1, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions dipy/align/imaffine.py
Expand Up @@ -143,6 +143,8 @@ def set_affine(self, affine):
if self.affine is None:
self.affine_inv = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to do small suggestions not relevant to this PR - but how about putting this line after self.affine = affine so that self.affine_inv is always defined (as None or an affine) even if the inverse raises an error. Otherwise it's set to whatever it was before, which could be confusing. Probably also worth noting that the method sets self.affine_inv in the docstring too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, don't worry, I'll do a PR for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return
if np.any(np.isnan(affine)):
raise AffineInversionError('Affine contains invalid elements')
try:
self.affine_inv = npl.inv(affine)
except npl.LinAlgError:
Expand Down
17 changes: 14 additions & 3 deletions dipy/align/tests/test_imaffine.py
Expand Up @@ -172,7 +172,13 @@ def test_align_origins_3d():

def test_affreg_all_transforms():
# Test affine registration using all transforms with typical settings
for ttype in factors.keys():

# Make sure dictionary entries are processed in the same order regardless of
# the platform. Otherwise any random numbers drawn within the loop would make
# the test non-deterministic even if we fix the seed before the loop.
# Right now, this test does not draw any samples, but we still sort the entries
# to prevent future related failures.
for ttype in sorted(factors):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment to explain why the factor keys must be sorted (in order to preserve relationship of random numbers to dict key / values)? Ditto for other instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments added. Actually only one of the tests needed the sort, but I think it is ok to still sort the keys in the other three places, just in case we extend the tests in the future.

dim = ttype[1]
if dim == 2:
nslices = 1
Expand Down Expand Up @@ -265,7 +271,12 @@ def test_mi_gradient():
np.random.seed(2022966)
# Test the gradient of mutual information
h = 1e-5
for ttype in factors:
# Make sure dictionary entries are processed in the same order regardless of
# the platform. Otherwise any random numbers drawn within the loop would make
# the test non-deterministic even if we fix the seed before the loop:
# in this case the samples are drawn with `np.random.randn` below

for ttype in sorted(factors):
transform = regtransforms[ttype]
dim = ttype[1]
if dim == 2:
Expand Down Expand Up @@ -304,4 +315,4 @@ def test_mi_gradient():
enorm = np.linalg.norm(expected)
anorm = np.linalg.norm(actual)
nprod = dp / (enorm * anorm)
assert(nprod >= 0.999)
assert(nprod >= 0.99)
17 changes: 15 additions & 2 deletions dipy/align/tests/test_parzenhist.py
Expand Up @@ -330,7 +330,13 @@ def test_joint_pdf_gradients_dense():
# they approximately point in the same direction by testing if the angle
# they form is close to zero.
h = 1e-4
for ttype in factors:

# Make sure dictionary entries are processed in the same order regardless of
# the platform. Otherwise any random numbers drawn within the loop would make
# the test non-deterministic even if we fix the seed before the loop.
# Right now, this test does not draw any samples, but we still sort the entries
# to prevent future related failures.
for ttype in sorted(factors):
dim = ttype[1]
if dim == 2:
nslices = 1
Expand Down Expand Up @@ -406,7 +412,14 @@ def test_joint_pdf_gradients_dense():

def test_joint_pdf_gradients_sparse():
h = 1e-4
for ttype in factors:

# Make sure dictionary entries are processed in the same order regardless of
# the platform. Otherwise any random numbers drawn within the loop would make
# the test non-deterministic even if we fix the seed before the loop.
# Right now, this test does not draw any samples, but we still sort the entries
# to prevent future related failures.

for ttype in sorted(factors):
dim = ttype[1]
if dim == 2:
nslices = 1
Expand Down
16 changes: 12 additions & 4 deletions dipy/align/tests/test_vector_fields.py
Expand Up @@ -793,15 +793,19 @@ def test_affine_transforms_2d():

# Test exception is raised when the affine transform matrix is not valid
invalid = np.zeros((2, 2), dtype=np.float64)
invalid_nan = np.zeros((3, 3), dtype=np.float64)
invalid_nan[1, 1] = np.nan
shape = np.array(codomain_shape, dtype=np.int32)
# Exceptions from warp_2d
# 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)
# Exceptions from warp_2d_nn
assert_raises(ValueError, vfu.transform_2d_affine, circle, shape, invalid_nan)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this fix, this assert will reproduce the segfault.

# Exceptions from transform_2d_nn
assert_raises(ValueError, vfu.transform_2d_affine_nn, circle, shape, invalid)
assert_raises(ValueError, vfu.transform_2d_affine_nn, circle, shape, invalid)
assert_raises(ValueError, vfu.transform_2d_affine_nn, circle, shape, invalid)
assert_raises(ValueError, vfu.transform_2d_affine_nn, circle, shape, invalid_nan)


def test_affine_transforms_3d():
Expand Down Expand Up @@ -880,15 +884,19 @@ def test_affine_transforms_3d():

# Test exception is raised when the affine transform matrix is not valid
invalid = np.zeros((3, 3), dtype=np.float64)
invalid_nan = np.zeros((4, 4), dtype=np.float64)
invalid_nan[1, 1] = np.nan
shape = np.array(codomain_shape, dtype=np.int32)
# Exceptions from transform_2d
# Exceptions from transform_3d_affine
assert_raises(ValueError, vfu.transform_3d_affine, sphere, shape, invalid)
assert_raises(ValueError, vfu.transform_3d_affine, sphere, shape, invalid)
assert_raises(ValueError, vfu.transform_3d_affine, sphere, shape, invalid)
# Exceptions from transform_2d_nn
assert_raises(ValueError, vfu.transform_3d_affine, sphere, shape, invalid_nan)
# Exceptions from transform_3d_affine_nn
assert_raises(ValueError, vfu.transform_3d_affine_nn, sphere, shape, invalid)
assert_raises(ValueError, vfu.transform_3d_affine_nn, sphere, shape, invalid)
assert_raises(ValueError, vfu.transform_3d_affine_nn, sphere, shape, invalid)
assert_raises(ValueError, vfu.transform_3d_affine_nn, sphere, shape, invalid_nan)


def test_compose_vector_fields_2d():
Expand Down
2 changes: 2 additions & 0 deletions dipy/align/vector_fields.pyx
Expand Up @@ -17,6 +17,8 @@ def is_valid_affine(double[:, :] M, int dim):
return True
if M.shape[0] < dim or M.shape[1] < dim + 1:
return False
if np.any(np.isnan(M)):
return False
return True


Expand Down