-
-
Notifications
You must be signed in to change notification settings - Fork 665
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
BUG: Fix failing isAffine check due to insuffcient precision in calculations #3339
Changes from all commits
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 | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -12,6 +12,7 @@ itkNiftiImageIOTest9.cxx | |||||||||||||||||||||||||||||||
itkNiftiImageIOTest10.cxx | ||||||||||||||||||||||||||||||||
itkNiftiImageIOTest11.cxx | ||||||||||||||||||||||||||||||||
itkNiftiImageIOTest12.cxx | ||||||||||||||||||||||||||||||||
itkNiftiImageIOTest13.cxx | ||||||||||||||||||||||||||||||||
itkNiftiReadAnalyzeTest.cxx | ||||||||||||||||||||||||||||||||
itkNiftiReadWriteDirectionTest.cxx | ||||||||||||||||||||||||||||||||
itkExtractSlice.cxx | ||||||||||||||||||||||||||||||||
|
@@ -96,3 +97,7 @@ itk_add_test(NAME itkNiftiReadWriteDirectionSmallVoxelTest | |||||||||||||||||||||||||||||||
DATA{Input/SmallVoxels_nosform.nii.gz} | ||||||||||||||||||||||||||||||||
${ITK_TEST_OUTPUT_DIR} | ||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
itk_add_test(NAME itkNiftiSmallVoxelsAffinePrecisionTest | ||||||||||||||||||||||||||||||||
COMMAND ITKIONIFTITestDriver itkNiftiImageIOTest13 | ||||||||||||||||||||||||||||||||
DATA{Input/SmallVoxels_AffinePrecision.nii.gz}) | ||||||||||||||||||||||||||||||||
Comment on lines
+101
to
+103
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. What about using itkNiftiImageIOTest2 instead, like this:
Would that also work? 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. That test with the original code does not fail. 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.
Why not? Originally you added your SmallVoxels_AffinePrecision.nii.gz to an existing 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. I don't get why itkNiftiImageIOTest2 doesn't fail, because the test file 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. Reverting the change, and running both tests.
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. So itkNiftiImageIOTest2 just cannot be used for SmallVoxels_AffinePrecision.nii.gz, for some unknown reason, right? 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.
Correct. The difference between the two sets of code appears to be
Where the NiftiimageIO ImageIO is explicitly set in itkNiftiImageIOTest2 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. @gdevenyi Indeed, itkNiftiImageIOTest2 does explicitly set ImageIO, but it does also do a "read" without explicitly setting ImageIO. By calling
Anyway, it seems to me that the CMake code of itkNiftiIOShouldSucceed was already incorrect. It says:
ITK/Modules/IO/NIFTI/test/CMakeLists.txt Lines 55 to 57 in 6fc6164
The arguments of itk_add_test don't seem to match correctly with arg1, arg2, and prefix at ITK/Modules/IO/NIFTI/test/itkNiftiImageIOTest2.cxx Lines 32 to 42 in 8c2c654
When I run it locally, I think itkNiftiSmallVoxelsAffinePrecisionTest should be more like the following, if you would still like to use the existing itkNiftiImageIOTest2:
I don't know what exactly the prefix ("SomeSpecificPrefix") should be. But it should be the last argument to My two cents, Niels |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
5afad4b7b6fe872a896ed7eb555a572dc893e8074555973aca1bff3d2c12f6549a6d79b88d8c869c4e0b514de8b049a92b74fc09765fa82cbb21494f4e6608d6 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/*========================================================================= | ||
* | ||
* Copyright NumFOCUS | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0.txt | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
*=========================================================================*/ | ||
|
||
#include "itkNiftiImageIOTest.h" | ||
|
||
// Test to read small voxel NIFTI file which was triggering numerical instability | ||
// in IsAffine loading code | ||
int | ||
itkNiftiImageIOTest13(int argc, char * argv[]) | ||
{ | ||
if (argc != 2) | ||
{ | ||
return EXIT_FAILURE; | ||
} | ||
|
||
constexpr unsigned int Dimension = 3; | ||
|
||
using PixelType = float; | ||
using ImageType = itk::Image<PixelType, Dimension>; | ||
|
||
try | ||
{ | ||
using ReaderType = itk::ImageFileReader<ImageType>; | ||
ReaderType::Pointer reader = ReaderType::New(); | ||
reader->SetFileName(argv[1]); | ||
reader->Update(); | ||
ImageType::Pointer image = reader->GetOutput(); | ||
Comment on lines
+38
to
+42
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. Please consider using 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. Maybe some extra checks here to see that the read image is as expected...? Or at least, check that it is a valid image? 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.
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. 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.
I guess you could still add some checks that the read image is valid, for example:
Right? 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. @gdevenyi Did you still consider adding the extra checks that I suggested here? In order to ensure that the reading does actually yield a valid image?
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. This can be condiserably abbreviated via 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. Both done in #3448. |
||
} | ||
catch (const itk::ExceptionObject & err) | ||
{ | ||
std::cerr << "ExceptionObject caught !" << std::endl; | ||
std::cerr << err << std::endl; | ||
return EXIT_FAILURE; | ||
} | ||
|
||
return EXIT_SUCCESS; | ||
} |
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.
is float epsilon several lines below (line 1769) intentional?
I thought that it might have been forgotten.
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.
@issakomi Good question. But I wonder, isn't
std::numeric_limits<T>::epsilon()
rather arbitrary anyway? I just don't know, but ifmat[3][i]
must be close to zero (fori
= 0, 1, 2), then those matrix elements might as well be compared againststd::numeric_limits<T>::min()
, which if much smaller thanstd::numeric_limits<T>::epsilon()
.Honestly I'm just wondering what is intended as well. I have no suggestion here.
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.
I think the bottom row should be precisely (0,0,0,1) - but I guess it's possible there might be some epsilon error when the inverse is calculated
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.
I think it was just forgotten.
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.
OK, but
std::numeric_limits<T>::epsilon()
isn't just some epsilon error. It is specifically the difference between 1 and the least value greater than 1 that is representable by floating point typeT
. https://www.cplusplus.com/reference/cfloat/ That's why I wonder if this specific epsilon should also be used for values that should be close to zero (rather than close to one). But I just don't know, honestly.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.
One further idea here, currently the test is made with the sum as each entry added. Should we instead test the individual entries?
Changing to
::min()
, or testing the individual entries will be substantially stricter than the existing implementation as well.