-
Notifications
You must be signed in to change notification settings - Fork 429
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
Changes from all commits
061fdaf
8a348fc
0f5d547
8bf1318
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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: | ||
|
@@ -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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(): | ||
|
@@ -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(): | ||
|
There was a problem hiding this comment.
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 thatself.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 setsself.affine_inv
in the docstring too.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#693