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

ENH: Add a parameter to test to improve itk::ResampleImageFilter testing. #447

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Jan 27, 2019

Add a parameter to the itkResampleImageFilterTest2.cxx to improve the
itk::ResampleImageFilter class testing.

Specifically, exercise the workflow when the UseReferenceImage boolean
is set to false. Add the corresponding baselines.

Rename the tests and old baselines to account for this new test.

Take advantage of the commit to improve the style of the test:

  • Use the TRY_EXPECT_NO_EXCEPTION in lieu of try/catch for the sake of
    readability/compactness/save typing boilerplate code.
  • Explicitly call Update() on filters with the help of the
    TRY_EXPECT_EXCEPTION macro.
  • Use the TEST_SET_GET_BOOLEAN macro to test the boolean members.
  • Move the std::cout messages before the filter calls to Update().

@jhlegarreta jhlegarreta changed the title ENH: Add a parameter to test to improve itk::ResampleImageFilter te… ENH: Add a parameter to test to improve itk::ResampleImageFilter testing. Jan 27, 2019
@jhlegarreta jhlegarreta force-pushed the ImproveItkResampleImageTest2 branch 2 times, most recently from d4b49da to 1d32259 Compare January 27, 2019 16:58
@jhlegarreta
Copy link
Member Author

jhlegarreta commented Jan 27, 2019

Looks like the Off case needs some more work to create a valid target IO region:

itk::ExceptionObject (0x12b0a20)
Location: "unknown" 
File: /home/vsts/work/1/s/Modules/IO/ImageBase/include/itkImageFileWriter.hxx
Line: 286
Description: itk::ERROR: ImageFileWriter(0x12a3820):
Largest possible region does not fully contain requested paste IO regionPaste IO region:
ImageIORegion (0x7fffa282bc30)

The corresponding baselines in the commit are just there to avoid CMake warning that they do not exist (since due to the above exception they were not generated).

However, I'm wondering about a use case where the target region would not be determined by the reference image. Any suggestions? Thanks.

@thewtex
Copy link
Member

thewtex commented Jan 28, 2019

CC: @romangrothausmann

@romangrothausmann
Copy link
Member

Thanks @jhlegarreta for these improvements and thanks to @thewtex for CCing me.

Yes there are use cases, for example if you want to use itkResampleImageFilter for mere down-sizing an image by non-integer scales (i.e. in a way that is not accessible with itkBinShrinkImageFilter). In such a case you just want to specify the desired spacing of the output without providing a reference image, see e.g. https://github.com/romangrothausmann/ITK-CLIs/blob/3153dcd98defe01ccec257336153ed860d494670/resample.cxx#L58-L80.
To my knowledge itkResampleImageFilter is the only available filter in ITK that can be used for such cases (I only learned later, that it is often used with a non-identity transform), which is my motivation to allow streaming at least for such a use case (#82).

…sting.

Add a parameter to the `itkResampleImageFilterTest2.cxx` to improve the
`itk::ResampleImageFilter` class testing.

Specifically, exercise the workflow when the `UseReferenceImage` boolean
is set to `false`. Add to the test arguments:
- A boolean to as a hint to use or discard the reference image.
- The spacing to be used when resampling when no reference image is used.

Add the corresponding baselines.

Rename the tests and old baselines to account for this new test.

Take advantage of the commit to improve the style of the test:
- Use the `TRY_EXPECT_NO_EXCEPTION` in lieu of `try/catch` for the sake of
  readability/compactness/save typing boilerplate code.
- Explicitly call `Update()` on filters with the help of the
  `TRY_EXPECT_EXCEPTION` macro.
- Use the `TEST_SET_GET_BOOLEAN` macro to test the boolean members.
- Remove unused/unnecessary `reader` declared variables.
- Move the `std::cout` messages before the filter calls to `Update()`.
@jhlegarreta
Copy link
Member Author

Thanks @romangrothausmann !

I re-worked the test to take the output spacing as an optional parameter in case the reference image is not to be used.

I thought repeatedly at getting rid of the boolean and just using the presence of the reference image as a signal to use it. Having the output spacing also as an optional argument made things less clear to me. If anyone has better suggestion, please go ahead.

Also, I could have made the spacing anisotropic, but the boilerplate code to handle a double-quote enclosed argument holding all spatial dimensions as an array put me off this time.

@jhlegarreta
Copy link
Member Author

CDash reported compiler warnings come from the Examples/DataRepresentation/Image/Image4.cxx file, and are thus unrelated.

The concerned tests are passing:

Line 2855: 1396/2727 Test #1393: itkResampleImageTest2UseRefImageOn 
.....................................................................
Passed    0.04 sec
Line 2859: 1398/2727 Test #1392: itkResampleImageTest2UseRefImageOff
....................................................................
Passed    0.05 sec

@jhlegarreta jhlegarreta merged commit 321e673 into InsightSoftwareConsortium:master Jan 31, 2019
@jhlegarreta jhlegarreta deleted the ImproveItkResampleImageTest2 branch January 31, 2019 04:35
@thewtex
Copy link
Member

thewtex commented Feb 1, 2019

There seems to been some baseline comparison issues with 32-bit systems:

https://open.cdash.org/testDetails.php?test=728564967&build=5731782

@jhlegarreta
Copy link
Member Author

I guess it has to do with this division being rounded differently in a 32-bit system vs. the 64-bit system I use.

Not sure I will be able to test any solution, but I'd guess that doing a ceil would make the test image dimension become 320 at the place of 319. I'll submit a patch set later.

@thewtex
Copy link
Member

thewtex commented Feb 1, 2019

Fantastic, thanks @jhlegarreta !

@jhlegarreta
Copy link
Member Author

Opened PR #462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants