Skip to content

ENH: Increase coverage#2286

Closed
jhlegarreta wants to merge 5 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreaseCoverage
Closed

ENH: Increase coverage#2286
jhlegarreta wants to merge 5 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreaseCoverage

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

@jhlegarreta jhlegarreta commented Feb 8, 2021 •
edited
Loading

Improvements concerning testing and a few related enhancements:

  • Fix an RTTI bug concerning a Superclass name.
  • Remove a duplicate include file.
  • Print all member variables in a number of classes.
  • Use itkBooleanMacro to set the boolean ivar values to On/Off.
  • Increase code coverage of a number of classes.

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)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)

@jhlegarreta jhlegarreta added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Enhancement Improvement of existing methods or implementation type:Documentation Documentation improvement or change type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module type:Coverage Code coverage impacts labels Feb 8, 2021
dzenanz
dzenanz reviewed Feb 8, 2021
View reviewed changes
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.

Looks good aside from a few compile errors on *nix:
https://open.cdash.org/viewBuildError.php?buildid=7030130

@jhlegarreta jhlegarreta removed the type:Documentation Documentation improvement or change label Feb 8, 2021
@jhlegarreta jhlegarreta changed the title ENH: Increase coverage WIP: ENH: Increase coverage Feb 8, 2021
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 8, 2021

Thanks for the review @dzenanz . I push forced the fix. However, I still need to add another commit which will contain the largest part of changes to the test files. Given the number of files I'm changing and the diversity of the commits I wanted to open a PR and test early to ensure that virtually each commit's builds' pass. So WIP/draft for now.

@jhlegarreta jhlegarreta force-pushed the IncreaseCoverage branch 9 times, most recently from 9147f92 to e31f003 Compare February 10, 2021 20:53
jhlegarreta
jhlegarreta commented Feb 11, 2021
View reviewed changes
Comment thread Modules/Filtering/BiasCorrection/test/itkN4BiasFieldCorrectionImageFilterTest.cxx Outdated
typename CorrecterType::Pointer correcter = CorrecterType::New();
correcter->SetSplineOrder(3);

EXERCISE_BASIC_OBJECT_METHODS(correcter, N4BiasFieldCorrectionImageFilter, ImageToImageFilter);
Copy link
Copy Markdown
Member Author

@jhlegarreta jhlegarreta Feb 11, 2021

Choose a reason for hiding this comment

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

CDash is failing here:
https://open.cdash.org/viewBuildError.php?buildid=7035321

The Superclass is correctly declared.

Not sure if the class is missing the ITK_TEMPLATE_EXPORT macro.

Copy link
Copy Markdown
Member Author

@jhlegarreta jhlegarreta Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Still being reported here after this comment was addressed in 155b729. All three related tests passing locally on my Windows/VS setup.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Feb 12, 2021

Choose a reason for hiding this comment

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

On line 66 it is SuperClass and on line 69 it is called Superclass (lowercase c).

But why does it occur only for a couple of classes?

Copy link
Copy Markdown
Member

@dzenanz dzenanz Feb 12, 2021

Choose a reason for hiding this comment

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

Latest commit fixes this, but exposes some other issues (e.g. protected member access from test).

Copy link
Copy Markdown
Member Author

@jhlegarreta jhlegarreta Feb 13, 2021

Choose a reason for hiding this comment

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

OK, I see. Thanks for catching this ! Now, as you say, it is confusing that this has worked so far for the rest of the classes. I think it deserves some investigation. I am reopening the conversation until we are able to find a convincing explanation. Thanks for your patience @dzenanz .

Copy link
Copy Markdown
Member Author

@jhlegarreta jhlegarreta Feb 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Update: so the error is still present despite 107d1cc.

Reading ITK_EXERCISE_BASIC_OBJECT_METHODS again with more care, I'd revert 107d1cc: the version in master is correct: the method's Class and SuperClass arguments are used in here and here as strings to compare to the ones the object retrieves. I grant it might be clearer to rename the arguments to ClassName and SuperclassName in a separate PR to avoid confusion with capitals.

But the call object->Superclass is actually calling the instance of the object's parent class to retrieve the latter's NameOfClass, which is legal and the original intent.

So there is something else with itk::N4BiasFieldCorrectionImageFilter/the issue is not in the definition of ITK_EXERCISE_BASIC_OBJECT_METHODS.

Even more intriguing, the test is passing in my local build (Windows 10/VS 2015/default CMake flags). Can't think of where to continue looking (ITK_TEMPLATE_EXPORT, CMakeLists.txt, CMake flags?). Obviously, adding explicitly the #include itkImageToImageFilter.h does not make a difference in my local environment, and not sure why that would be needed since itkN4BiasFieldCorrectionImageFilter.h does include it.

@thewtex @blowekamp @N-Dekker @hjmjohnson any help is appreciated.

@jhlegarreta jhlegarreta changed the title WIP: ENH: Increase coverage ENH: Increase coverage Feb 11, 2021
@jhlegarreta jhlegarreta marked this pull request as ready for review February 11, 2021 04:10
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 11, 2021 •
edited
Loading

Some failures are still expected until some aspects are better understood, and they will definitely benefit from reviews. I'll revise my notes concerning them and post the questions when I get some time.

dzenanz
dzenanz reviewed Feb 11, 2021
View reviewed changes
Comment thread Modules/Filtering/DisplacementField/include/itkDisplacementFieldJacobianDeterminantFilter.h
Comment thread Modules/Filtering/BiasCorrection/test/itkN4BiasFieldCorrectionImageFilterTest.cxx
Comment thread Modules/Filtering/BiasCorrection/test/itkN4BiasFieldCorrectionImageFilterTest.cxx Outdated
Comment thread Modules/Filtering/BiasCorrection/test/itkN4BiasFieldCorrectionImageFilterTest.cxx Outdated
Comment thread Modules/Filtering/ImageFeature/test/CMakeLists.txt Outdated
Comment thread Modules/IO/LSM/test/itkLSMImageIOTest.cxx Outdated
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 12, 2021

A few notes:

  • Have not yet uploaded the baseline images of the new tests since I have some questions to ask on them, and hence the configuration warnings.
    I still need to revise my notes, since I had a few questions while writing these. Will post them as I process them, thanks for the patience.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 12, 2021

I compiled this branch on my computer, and fixed some compile errors.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 16, 2021

Can you rebase on current master? The macro parameter rename PR was merged.

@jhlegarreta
BUG: Fix Superclass name in RTTI macro
f136e8c 
Fix Superclass name in RTTI macro.
@jhlegarreta
COMP: Remove duplicate include file
135141b 
Remove duplicate include file.
@jhlegarreta
ENH: Print all member variables.
f568835 
Print all member variables.

Take advantage of the commit to
- Use the `itk::NumericTraits::PrintType` definition, the `print_helper`-enabled
  overload for array-like types, and the `itkPrintSelfObjectMacro` macro
  to improve how the ivars and objects that can be null pointers are printed.
- Print the member variables in the same order they were declared for the
  sake of consistency.
- Print the member variable names verbatim to conform to the ITK SW Guide.
@jhlegarreta
ENH: Use itkBooleanMacro for ImageSpacing, `InterpolateSurfaceLoc…
84808d7 
…ation`

Use `itkBooleanMacro` to set `ImageSpacing` and
`InterpolateSurfaceLocation` boolean member variable values to On/Off:
- Deprecate `Set*(On/Off)` signatures.
- Test the methods.
@jhlegarreta
ENH: Increase code coverage
c19e8fa 
Increase the code coverage:
- Add new unit tests for untested classes, including the corresponding
  baseline hash files.
- 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.
- Add the corresponding test arguments to the appropriate
  `CMakeLists.txt` test driver command.
- Remove the global `try/catch` blocks and use the
  `ITK_TRY_EXPECT_NO_EXCEPTION` macro around the statement that is
  liable to throw an exception to avoid boilerplate code and to improve
  readability.
- Test the exceptions.
- Remove explicit calls to the `Print` method and rely on the
  basic method exercising macro call.
- Improve slightly the test style to make them more consistent (e.g. input
  argument checking, test finishing message, make the types and variables
  dwell closer to the place where they are instantiated, define the image
  dimension as a constant; use aliases for the pixel types, etc.).
- Remove unnecessary print messages.
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 17, 2021

c19e8fa rebased on master.

@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 17, 2021

Interestingly enough, only Linux reports the build failures related to the Superclass not being found when compiling the itkn4biasfieldcorrectionimagefiltertest.cxx file. Not sure what's happening.

Also, the itkPolygonGroupSpatialObjectXMLFileTest failure due to the superclass name mismatch is still confusing.

Suggestions are welcome. Thanks.

The rest of the failures/warnings are expected/will ask about them once we are able to find a solution to the above.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Feb 17, 2021

As this is a large improvement, maybe comment out the problematic tests now (or remove them) for now? After this is merged, start a new PR with just those two things being added? That will make it easier for others to pitch in. This 2500 line change is enough to discourage many from looking at it 😄

hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/ImageFeature/test/itkDerivativeImageFilterTest.cxx Outdated
hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/Smoothing/test/itkDiscreteGaussianImageFilterTest.cxx
hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/ImageFeature/test/itkDerivativeImageFilterTest.cxx
hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/ImageFeature/test/itkLaplacianImageFilterTest.cxx
hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/LabelMap/test/itkBinaryFillholeImageFilterTest1.cxx
hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/LabelMap/test/itkBinaryImageToLabelMapFilterTest.cxx
hjmjohnson
hjmjohnson reviewed Feb 17, 2021
View reviewed changes
Comment thread Modules/Filtering/LabelMap/test/itkBinaryNotImageFilterTest.cxx
@jhlegarreta jhlegarreta deleted the IncreaseCoverage branch February 18, 2021 03:41
@jhlegarreta jhlegarreta restored the IncreaseCoverage branch February 18, 2021 03:41
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 18, 2021

I inadvertently closed the PR. Sorry.

@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Feb 18, 2021

Opened PR #2327 with the same contents and having addressed the latest comments.

@jhlegarreta jhlegarreta mentioned this pull request Feb 18, 2021
Merged
5 tasks
@dzenanz dzenanz mentioned this pull request Aug 3, 2021
Closed
This was referenced Dec 28, 2021
Closed
Merged
@jhlegarreta jhlegarreta mentioned this pull request Jan 9, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hjmjohnson hjmjohnson hjmjohnson left review comments

@dzenanz dzenanz dzenanz left review comments

Assignees

No one assigned

Labels

area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Coverage Code coverage impacts type:Enhancement Improvement of existing methods or implementation 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jhlegarreta @dzenanz @hjmjohnson