Skip to content

ENH: Increase coverage for miscellaneous classes#3003

Merged
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreaseMiscClassesCoverage
Dec 30, 2021
Merged

ENH: Increase coverage for miscellaneous classes#3003
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreaseMiscClassesCoverage

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

Increase coverage for miscellaneous classes:

  • Exercise the basic object methods using the
    ITK_EXERCISE_BASIC_OBJECT_METHODS macro.
  • Test the Set/Get methods using the ITK_TEST_SET_GET_VALUE macro.
  • Test the boolean ivars using the ITK_TEST_SET_GET_BOOLEAN macro, and
    rely on it to set the boolean ivar values.
  • Make the calls to the Get methods be quantitative where they were just
    being called without being compared to the expected value.
  • Test exceptions using the ITK_TRY_EXPECT_EXCEPTION macro.
  • Convert existing hard-coded parameters to test input parameters and
    add parameters to the tests where appropriate so that different
    valued-tests can be added.
  • Remove explicit calls to the Print method and rely on the basic
    method exercising macro call.
  • Remove explicit calls to the GetNameOfClass and rely on the basic
    method exercising macro call.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)

@github-actions github-actions Bot added area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Coverage Code coverage impacts type:Data Changes to testing data type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Dec 28, 2021
@jhlegarreta jhlegarreta force-pushed the IncreaseMiscClassesCoverage branch from 8921311 to cba0777 Compare December 28, 2021 02:50
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to uncover some FFT failures. Maybe once #3002 is integrated, they will go away? Otherwise awesome!

@jhlegarreta jhlegarreta force-pushed the IncreaseMiscClassesCoverage branch from cba0777 to b36f5a8 Compare December 28, 2021 17:57
@jhlegarreta
Copy link
Copy Markdown
Member Author

This seems to uncover some FFT failures. Maybe once #3002 is integrated, they will go away?

I think they were more of my mistake than actual bugs: when registering the FFT factory, the superclass name of the class seems to be ImageToImageFilter, rather than the superclass name in the itkTypeMacro of the class at issue. @tbirdso might confirm if this behavior is the expected one.

@tbirdso
Copy link
Copy Markdown
Contributor

tbirdso commented Dec 28, 2021

Thanks for doing this @jhlegarreta ! Yes, the superclass name for each distinct FFT backend implementation should be the superclass name in its itkTypeMacro, for example the hierarchy for a VnlForwardFFTImageFilter would be

itk::ImageToImageFilter -> itk::ForwardFFTImageFilter -> itk::VnlForwardFFTImageFilter

It looks like FFT failures stem from class name failures rather than superclass name failures at the moment:

Class name provided does not match object's NameOfClass

My guess is that this because the object factory upcasts each backend implementation to the base FFT class rather than implementation class (Vnl, FFTW, etc). If an FFT test doesn't explicitly request a certain backend we usually don't worry about which backend is being used, so it would be appropriate to pass in to ITK_EXERCISE_BASIC_OBJECT_METHODS the name of the base FFT class with ImageToImageFilter as its superclass.

EDIT: See discussion in review below. GetNameOfClass is in fact overridden for different object factory backends for a given class, which can result in inconsistent naming for classes relying on object factory instantiation. To be addressed in a different PR.

Comment thread Modules/Filtering/FFT/test/itkComplexToComplex1DFFTImageFilterTest.cxx Outdated
Comment thread Modules/Filtering/FFT/test/itkComplexToComplexFFTImageFilterTest.cxx Outdated
@jhlegarreta jhlegarreta force-pushed the IncreaseMiscClassesCoverage branch 3 times, most recently from 027520e to 5643fdb Compare December 29, 2021 16:54
Increase coverage for miscellaneous classes:
- Exercise the basic object methods using the
  `ITK_EXERCISE_BASIC_OBJECT_METHODS` macro.
- Test the Set/Get methods using the `ITK_TEST_SET_GET_VALUE` macro.
- Test the boolean ivars using the `ITK_TEST_SET_GET_BOOLEAN` macro, and
  rely on it to set the boolean ivar values.
- Make the calls to the Get methods be quantitative where they were just
  being called without being compared to the expected value.
- Test exceptions using the `ITK_TRY_EXPECT_EXCEPTION` macro.
- Convert existing hard-coded parameters to test input parameters and
  add parameters to the tests where appropriate so that different
  valued-tests can be added.
- Remove explicit calls to the `Print` method and rely on the basic
  method exercising macro call.
- Remove explicit calls to the `GetNameOfClass` and rely on the basic
  method exercising macro call.
@jhlegarreta jhlegarreta force-pushed the IncreaseMiscClassesCoverage branch from 5643fdb to ddd6563 Compare December 29, 2021 23:44
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Dec 29, 2021

Folks, the tests were passing on Windows, but not for Linux/macOS when uncommenting the ITK_EXERCISE_BASIC_OBJECT_METHODS statements in the relevant Vnl/FFTW backend-powered tests in commit 5643fdb:
https://dev.azure.com/itkrobotwindow/ITK.Windows/_build/results?buildId=7352&view=results

I had tested locally using a Windows/MSVC build before posting #3003 (comment), so I do not understand how these statements or the instance types are interpreted differently across different compilers.

I do not have a Linux/macOS machine at hand to be able to try to investigate this.

Also, note that in the #2286 (comment) I cross-referenced in the issue #3008, I was experiencing this very same behavior (passing on Windows/MSVC, other CIs reporting errors) for another class.

And maybe related, in #2991 (comment) I also noticed some inconsistent behavior across platforms/compilers.

I have push forced again in ddd6563 the files commenting out the concerned statements.

I think best is to have it merged as it is, and investigate the issue separately. Otherwise, this is blocking changes to the other 90 files.

@dzenanz dzenanz merged commit 748ddf7 into InsightSoftwareConsortium:master Dec 30, 2021
@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Dec 30, 2021

Thanks for this large contribution!

@jhlegarreta jhlegarreta deleted the IncreaseMiscClassesCoverage branch December 30, 2021 15:19
@tbirdso
Copy link
Copy Markdown
Contributor

tbirdso commented Dec 30, 2021

Thanks for testing @jhlegarreta , I agree with the decision to go ahead and merge. I'll investigate different behavior among platforms as this may relate to #2233 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Coverage Code coverage impacts type:Data Changes to testing data type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants