ENH: Improve Thresholding module filters' coverage.

[jhlegarreta]

Improve the ITKThresholding module filters' coverage:

• Exercise basic object methods. Remove redundant calls to print the
  filter.
• Exercise the Set/Get methods using the TEST_SET_GET_VALUE and On/Off
  methods using the TEST_SET_GET_BOOLEAN macro.
• Use the TRY_EXPECT_NO_EXCEPTION macro for all filter update calls to
  save typing try/catch blocks.
• Use TRY_EXPECT_EXCEPTION to try/catch expected exceptions when
  possible/appropriate.
• Refactor the tests to accept (more) input parameters to check the
  classes under a broader range of conditions. In many cases, the
  parameters accepted belong to inherited propertis (e.g.
  itk::HistogramThresholdImageFilter: m_NumberOfHistogramBins,
  m_AutoMinimumMaximum, m_MaskOutput, m_MaskValue). Keep the default
  values of the class for existing tests.
• Make the tests quantitative: compare the compute threshold to the
  expected one.
• Add the hash code to compare to the baseline for new tests.
• Remove unnecessary print messages.
• Style changes: define the image dimension as a constant; use a typedef
  for the pixel type; use a variable to set the output's inside/outside
  value so that its Set/Get methods can be clearly tested; try to use the
  same sequence of sentences across all tests; change '\n' to std::endl.
• Improve the messages in the threshold regression tests to make the
  consistent across the tests/the entire toolkit, and to provide rich
  information.
• Finished the tests with a "Test finished" message; helps telling in the
  the dashboard whether the failure has happened in the test or the
  comparison to the baseline.
• In itkBinaryThresholdImageFilterTest.cxx, set the functor before
  updating the filter.
• Correct the arguments passed to SetSigmaFactor and SetNumberOfIterations
  in itkKappaSigmaThresholdImageFilterTest.cxx (the expected sigmaFactor
  as defined by the order in the CMakeLists command/usage explanation was
  being set to the number of iterations of the filter and vice-versa).
  Update the corresponding baselines accordingly.

Take advantage of the commit to use C++11 features.

Recovered from gerrit:
Change-Id: I9d6050d9e474b17fe3639faa7a5b16c052440e21
https://review.source.kitware.com/%23/c/22134/

[jhlegarreta]

I still did not study @richardbeare 's last comment at the time the topic was being reviewed in gerrit. That will take some time.

And there will be a few tests whose baseline needs to be updated.

But this is a starting point as a refactoring and code coverage improvement.

[jhlegarreta]

Just as a follow-up, generating all baselines and updating/adding them will take me quite some time.

[jhlegarreta]

Rebased on master to fix the merge conflicts. Have not yet reproduced the test results.

[jhlegarreta]

Rebased on master to avoid further conflicts when I find the time to review the topic.

[jhlegarreta]

a6b148f rebased on master and addressed KWStyle warnings. Working on the baselines.

[jhlegarreta]

A few comments about 484290c:

• Undoing some style changes done in a6b148f that generated build errors.
• KW Style is generating a few error: Unnecessary semicolon false negatives. Still producing some locally at itkHistogramThresholdImageFilter:205 and itkHistogramThresholdImageFilter:207.
• Add missing baselines and fix the corresponding content links in the CMakeLists.txt file.
• Fix some expected values in the script tests.
• Change a TRY_EXPECT_EXCEPTION oversight to TRY_EXPECT_NO_EXCEPTION.
• Fix a test output filename.
• itkIntermodesThresholdImageFilterTestNoAutoMinMax failing locally because the itk::IntermodesThresholdCalculator instance is exceeding the maximum iterations for histogram smoothing.
• itkMaximumEntropyThresholdImageFilterTestNoAutoMinMax producing a vector subscript out of range error because the itk::Historam::GetMeasurement method's InstanceIdentifier parameter has a value of 18446744073709551615.
• itkShanbhagThresholdImageFilterTestNoAutoMinMax producing a vector subscript out of range at the itk::Historam::GetMeasurement method because the m_Min and m_Max variables are not initialized.
• Not sure about the itkOtsuThresholdImageFilterTestNoAutoMinMax test being deterministic any more.
• I have removed the null calculator exception test from itkOtsuThresholdImageFilterTestNoAutoMinMax because it no longer throws such an exception after 79148b6.

[jhlegarreta]

The remaining errors are due to the causes explained above, which dot not look evident, so it will take some time to find the appropriate fixes.

[fbudin69500]

Did a quick first pass review. Looks good. I will do a more in-depth review when the tests pass.

[jhlegarreta]

@fbudin69500 thanks for having a look.

I started to have another look at the IntermodesThresholdImageFilter failures.

Well, it seems to be trickier than what it looks like. I'd dare to say that there is some sort of inheritance mess involved.

When running the TRY_EXPECT_EXCEPTION( filter->Update() ); statement right after the call to SetMaximumSmoothingIterations in the test, the progress reporter std output shows the filter as being upcasted to its parent class.

If I reimplement the SetCalculator function in itk::IntermodesThresholdImageFilter, and lower the maximum number of iterations (it reaches 14 when finishing the process) and set it to 10 in the test), the exception is indeed caught, but the exception is not quite itkIntermodesThresholdCalculator.hxx:100, but the one in itkHistogramThresholdImageFilter.hxx:67.

So there is some sort of inheritance issue: the itk::IntermodesThresholdImageFilter class uses a specialized itk::itkIntermodesThresholdCalculator class, with an ivar of its own, but the former still uses its parent method's (e.g. GenerateData) which has no idea about the itk::itkIntermodesThresholdCalculator ivar.

To be honest, I've spent some time trying to find an accurate explanation in order to propose a solution, but the best explanation I've come up to is the above, and still have not found a fix.

[jhlegarreta]

Rebased on master after #999 has been merged.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jhlegarreta]

Folks, we are getting closer. A few comments:

• I could not make work two tests locally:
  • itkIntermodesThresholdImageFilterTestNoAutoMinMax gets stuck at progress 0.8 whatever the maximum number of smoothing iterations (I've gone up to 8000000). It is not related to the change in ENH: Avoid duplicated code in itk::HistogramThresholdImageFilter #1732 (I've checked that the same happens with the code prior to the commit in that PR).
  • itkOtsuThresholdCalculatorVersusOtsuMultipleThresholdsCalculatorTest fails when comparing the values of the values yielded by itk::OtsuThresholdsCalculator and the itk::OtsuMultipleThresholdsCalculator instances.

I had a look at both issues without success. To be honest, if nobody has the bandwidth to address them in the following days (I personally don't) I'd comment both tests and open two issues so that the PR can be merged (if the rest of the tests pass).

• When setting the autoMinimumMaximum flag to false, all thresholding filters return the same threshold value (provided that the input image is the same), e.g. -31744 for cthead1.png. Checking the range of the input image values, this expected threshold value seems to be in the lower end of the dynamic range. I have not dug more than that, but my impression is that it is like a floor value of the histogram. Also, the output image contains a single value (they appear completely black). I'd dare to say that this makes sense since the histogram filter was told not to compute any limits. Now to solve this we should be able to set those limits manually (e.g. find the filtering thresholds between the itk::HistogramThresholdImageFilter::m_Minimum and itk::HistogramThresholdImageFilter::m_Maximum arbitrary values e.g. 100 and 1340 -the filter should internally make sure they are not out of bounds). This means that we should be able to set those numbers or that filter to our thresholding filter (e.g. itk::MaximumEntropyThresholdImageFilter), instead of the autoMinimumMaximum flag maybe or using some logic in case the flag is set to false. And we should be able to set those values in the tests where the flag is set to false. That is not possible now, and requires some work both on the classes and the tests. Honestly, I would open an issue and let whoever has the bandwith to fix it.
• Note that I did not have a close look at whether all tests had nice parameters so as to have a meaningful output (e.g. itkOtsuThresholdImageFilterTestShort generates a thresholded output which is not really meaningful/useful). So maybe an issue could be opened to identify those tests so that anybody who has time can play with the parameters to get nice thresholding results and modify the tests accordingly.

TLDR: If only itkIntermodesThresholdImageFilterTestNoAutoMinMax itkOtsuThresholdCalculatorVersusOtsuMultipleThresholdsCalculatorTest fail, I will comment the tests and open the appropriate issues.

Sorry for the length of the message.

[jhlegarreta]

CI results are as anticipated in this comment. Please consider reading it and comment as appropriate. Thanks !

[dzenanz]

Even with those two tests commented in CMakeLists.txt, this is a big improvement. Hopefully it will nudge somebody to look into those two tests too.

[jhlegarreta]

So I commented the two failing tests and push-forced.

If CIs still pass, this will be ready to be merged. I will open the related issues once it gets merged.

Thanks.

[jhlegarreta]

Not sure why Azure builds have not been reported. Anybody with the appropriate permissions to trigger those? Thanks.

[dzenanz]

Occasionally they don't report. A force push with updated time should trigger them.

[dzenanz]

Thanks for this improvement!

[thewtex]

Amazing work, Jon Haitz!

[jhlegarreta]

Thank you all your bearing with the multiple unsuccessful commits and my intermittent dedication. This was team work.