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

WIP: Add baseline comparisons to tests. #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Member

Add baseline comparisons to tests.

Add the corresponding SHA512 hash files.

Add baseline comparisons to tests.

Add the corresponding SHA512 hash files.
@jhlegarreta
Copy link
Member Author

Folks, I had a look at the issue #36, and have some questions:

  • @phcerdan The three padding variants (Constant, Periodic, ZeroFluxNeumann) for the itkFFTPadPositiveIndexImageFilterTestConstant test produce the same hash. Havent dug further but looks weird.
  • @phcerdan @fbudin69500 @thewtex A number of tests use double/float pixel types, and hence hashing cannot be used. Using plain image comparisons could then be an option, but some of the tests do produce heavy images (e.g. itkWaveletFrequencyFilterBankGeneratorTest2D1.tiff: 1025 KB; itkWaveletFrequencyFilterBankGeneratorTest2D2.tiff 1025 KB). I'm wondering about the best strategy: I guess casting to unsigned char may be one, but I ignore whether by doing that we'd observe the desired effects? Pablo? Matt, François would writing to mha solve the issue? What about pixel types with complex parts?

The test at issue are

itkFrequencyBandImageFilterTest
itkFrequencyExpandAndShrinkTest
itkFrequencyExpandTest
itkFrequencyShrinkTest
itkMonogenicSignalFrequencyImageFilterTest
itkRieszFrequencyFilterBankGeneratorTest
itkRieszWaveletPhaseAnalysisTest
itkVectorInverseFFTImageFilterTest
itkWaveletFrequencyFilterBankGeneratorTest
itkVectorInverseFFTImageFilterTest
itkWaveletFrequencyForwardTest
itkWaveletFrequencyInverseTest
itkStructureTensorWithGeneralizedRieszTest
itkZeroDCImageFilterTest

On the other hand, itkIsotropicWaveletFrequencyFunctionTest writes its output information to a text file. Is there any better and faster solution than storing that file in the baseline folder and reading it to compare against the output?

Thanks.

@thewtex
Copy link
Member

thewtex commented Jun 7, 2017

@jhlegarreta good questions.

My general go-to format is MetaImage, .mha, because it supports all pixel components types (like float), it supports all multi-component pixel types (like storing complex pixels), it supports compression (set writer->SetUseCompression(true), and it also supports streamed reading and writing in ITK.

@phcerdan
Copy link
Collaborator

phcerdan commented Jun 7, 2017

Hey @jhlegarreta, you caught another one, thanks!
The FFTPadPositiveIndex wasn't propagating the BoundaryCondition to its internal m_FFTPad. see #49. Let me know if the outputs now are different? Not 100% sure about what to expect. It uses the original itkFFTPadImageFilter and just shifts the output.

About the itkIsotropicWaveletFrequencyFunctionTest writing txt files... I used those txt files to write the plots for the itk journal with a python script that is included in the submission.
These functions are tested using an image format with the itkWaveletFrequencyFilterBankGenerator, that basically takes the function and applies it to every pixel of a frequency image.
To simplify the regression test on this we can maybe just test 3 values for each function, like in both boundaries, and one in the middle?

@jhlegarreta
Copy link
Member Author

@phcerdan @thewtex I tried using metaimage format to save the images. Setting the compression to On drastically reduces the size (85 KB vs. non compression 1025 KB; vs. TIFF 1025 KB). However, the hashing issue with float/double pixel types remains unresolved.

Correct me if I'm wrong but we've got at least two possibilities:

  • Use plain mha compressed image comparison with float/double pixel types. The output image size would be yet increased using double, and would be beyond desirable (~ 50 KB) for test images [1], but I think that we may have to assume this payload.
  • Cast to int/uchar the output image and use the MD5/SHA512 hash for the image comparison. @phcerdan does casting involve an information loss that you'd like to avoid?

Any thoughts?

Thanks

[1] https://itk.org/ItkSoftwareGuide (Section 9.4)

@phcerdan
Copy link
Collaborator

@jhlegarreta that compression is great. I wonder, we can reduce the size of the 2D image from 512x512 to something smaller (128x128?) to fit under 50kb.

@jhlegarreta
Copy link
Member Author

@phcerdan I guess that's doable, but:

  • Depends on whether the output image is still meaningful at that size. Haven't looked into it, but looks like the output image size depends on the input image size as of the current implementation.

  • Depends on whether the folks at kitware tell that keeping the image size around 50 KB is absolutely necessary.

@thewtex @fbudin69500 for the second question?

@thewtex
Copy link
Member

thewtex commented Jun 11, 2017

@jhlegarreta regarding casting, I think setting the tolerance with the many option of the ITK test driver image comparison tool offers more flexibility.

@phcerdan
Copy link
Collaborator

@jhlegarreta yeah, I mean changing the input image, so the output will be shrinked as well.

@jhlegarreta
Copy link
Member Author

If reducing the input does not impact on the range of frequency and spatial analysis, I'm fine with it then.

Yep, Matt, you're right. We could use the appropriate tolerance option for the comparison command in
\Modules\Core\TestKernel\include\itkTestDriverInclude.h.

May be --compareIntensityTolerance? I ignore whether any other option fits better into our case.

@phcerdan
Copy link
Collaborator

phcerdan commented Aug 9, 2017

I searched for a smaller checker board image used in tests with no luck. We are using a 512x512 and 540x3--, right now.
When trying to shrink them to a smaller size with interpolation (I tried a few from ImageMagick), the borders don't look great.

@jhlegarreta
Copy link
Member Author

Then I guess keeping the existing ones is acceptable.

Now, we should figure out how to resolve the hashing issue with float/double pixel types. and how we can perform the regression on image baselines/add the tolerance for MetaImage output format files.

@fbudin69500
Copy link
Collaborator

fbudin69500 commented Aug 11, 2017 via email

@hjmjohnson
Copy link
Member

@jhlegarreta This is getting stale. It looks like important testing improvements. Should this be changed from a WIP and submitted?

@jhlegarreta
Copy link
Member Author

@hjmjohnson I do not have the necessary time to devote to this at this time unfortunately.

Sooner or later, I'd like to come back over this, but I'd leave it as it is for now.

But the present CI status will not be fixed by this. Commit 80fefd4 should have fixed the errors reported in discourse but CI shows that this has brought other consequences.

@phcerdan phcerdan added the test label Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants