ENH: Increase Common operators' code coverage

[jhlegarreta]

Increase Common operator classes' code coverage:

• 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.
• Make the calls to the Get methods be quantitative where they were just
  being called without being compared to the expected value.
• 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, etc.)

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)

[jhlegarreta]

This PR will be accompanied by several separate PRs where I'll be adding RTTI to the operators that are missing it, using the Set/Get macros to set/get the ivar values, etc.

[jhlegarreta]

Not sure about this compiler failure:

Modules/Core/Common/test/itkGaussianDerivativeOperatorTest.cxx:47:36:
error: 'using GaussianOp = class itk::GaussianDerivativeOperator<double, 1u>
{aka class itk::GaussianDerivativeOperator<double, 1u>}' has no member named 'GetMaximumKernelWidth';
did you mean 'SetMaximumKernelWidth'?

   ITK_TEST_SET_GET_VALUE(width, op.GetMaximumKernelWidth());
                                                             ^

since the method exists in https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/include/itkGaussianOperator.h#L129

[Leengit]

Looks good.

[dzenanz]

Looks good

[issakomi]

Not sure about this compiler failure:

Modules/Core/Common/test/itkGaussianDerivativeOperatorTest.cxx:47:36:
error: 'using GaussianOp = class itk::GaussianDerivativeOperator<double, 1u>
{aka class itk::GaussianDerivativeOperator<double, 1u>}' has no member named 'GetMaximumKernelWidth';
did you mean 'SetMaximumKernelWidth'?

   ITK_TEST_SET_GET_VALUE(width, op.GetMaximumKernelWidth());
                                                             ^

since the method exists in https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/include/itkGaussianOperator.h#L129

This might be a minor bug, comment in itkGaussianDerivativeOperator.h mentions Get kernel max width, but there is no function, only Set.
itk:: GaussianDerivativeOperator doesn't inherit methods from itk::GaussianOperator, BTW.

  /** Sets/Get a limit for growth of the kernel.  Small maximum error values with
   *  large variances will yield very large kernel sizes.  This value can be
   *  used to truncate a kernel in such instances.  A warning will be given on
   *  truncation of the kernel. */
  void
  SetMaximumKernelWidth(unsigned int n)
  {
    m_MaximumKernelWidth = n;
  }

ITK/Modules/Core/Common/include/itkGaussianDerivativeOperator.h

Line 182 in fc10011

/** Sets/Get a limit for growth of the kernel. Small maximum error values with

[Leengit]

Thank you for the edits. Looks good to me, pending figuring out one way or another what is up with the GitHub actions failures.

[jhlegarreta]

🤦 : #2536 (comment) Thanks @issakomi !!

Have not looked into it in detail, but itk::GaussianDerivativeOperator could inherit also from itk::DerivativeOperator , isn't it? Not sure what the policy about multiple inheritance is. But if that should be done, a separate PR is welcome.

PRs #2544 and #2545 should address the failures. Will rebase on master once these get merged.

[jhlegarreta]

d0d6804 has been rebased on master.

[jhlegarreta]

Cross-referencing #2547.

Also, this has uncovered the interesting:

Modules/Core/Common/test/itkAnnulusOperatorTest.cxx:122:32: 
error: 'using PixelType = using PixelType = float' is protected within this context

across a few lines:
https://open.cdash.org/viewBuildError.php?buildid=7234727

Many Operator classes declare a using PixelType = typename Superclass::PixelType; inside their protected section. However, they receive the pixel type as a template parameter. Hence, am I wrong when saying that we should prefer using PixelType = TPixel; in the public section and the use PixelType everywhere in their methods?

[dzenanz]

using PixelType = TPixel; in the public section

Yes, I think this is correct.

By the way, almost each time you make "increase X coverage" PR some inconsistency in the library is discovered 😄

[jhlegarreta]

Yes, I think this is correct.

Will make a PR tonight.

By the way, almost each time you make "increase X coverage" PR some inconsistency in the library is discovered 😄

👍.

[jhlegarreta]

Yes, I think this is correct.

Will make a PR tonight.

PR #2550. Finally left TPixel type for the method parameter/return types as I was not sure how consistent are we being on this or whether specific criteria are applicable. I can add an additional commit if PixelType is preferred.

[jhlegarreta]

Not sure what to think about the failures:
https://open.cdash.org/viewTest.php?onlyfailed&buildid=7251375

Detail:
https://open.cdash.org/test/410774528
https://open.cdash.org/test/410774620

I'd say that since itk::DerivativeOperator does not have its RTTI implementation, it's probably calling its parent's, and hence the Name of Class found using GetNameOfClass is the parent's (?). If that hypothesis is true, #2556 should fix it.

But the same should happen to the itk::ForwardDifferenceOperator, the itk::BackwardDifferenceOperator, and the itk::LaplacianOperator, it seems not be happening. These ones' basic object methods are not being exercised, and hence the the class name is not being checked.

[N-Dekker]

Just a small comment here: std::cerr does flush automatically with each << insertion. (For details, check: https://en.cppreference.com/w/cpp/io/cerr) This means that explicit flushing of the stream by << std::endl is unnecessary. So in this case, I would suggest:

std::cerr << "Test failed!\nError in coefficients\n";

The C++ Core Guidelines also suggest to avoid std::endl: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rio-endl

[jhlegarreta]

@N-Dekker thanks for the review. At some point we started preferring std::endl over \n a few years ago. Cannot remember the reasons, but it dates back to the Gerrit days, so I have been unable to find the related comments/discussion.

Also, although at times many of my own changes are not extensive throughout the whole code base, in this case I'm reluctant to change this only for this test: this looks to me like a major style change, involving the SW Guide as well, and virtually all test argument check blocks, and error blocks. Unless a sweeping change is made (and I'm well aware of the effort this involves), I'd sacrifice performance for consistency in this case.

[jhlegarreta]

#2556 fixes these: #2536 (comment)

But at the same time, as noted in the comment, it worries me 😟 because failures were not consistent for all classes missing their RTTIs. The other classes missing their RTTI do not have their basic object methods being exercised, so all is OK 👌 (except that in a future PR they should get them tested).

Edit: Confirmed debugging and following the calls locally that the hypothesis in the comment was right: the parent's RTTI method was being called. So all is OK 👌

Also, @Leengit not sure whether there is room for improvement involving these comments: #2536 (comment)
#2547 (comment)
And what I was forced to do because the pointers/variable types were not matching for the DerivativeScalings:
https://github.com/InsightSoftwareConsortium/ITK/pull/2536/files#diff-802c29f5c013b90694382e6078ba14f99dd3cae19d83f10dbcef6753d4459457R150

I'll push a separate PR switching to getter/setter macros, but I ignore whether the above is just the result of a poor design for the setter/getters regardless of using the macros or poor variable definition, and hence whether they should be addressed now.

[dzenanz]

itkPyBufferMemoryLeakTest (Failed) is a fluke. Good to merge, I think.