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

STYLE: Make PrintSelf implementation style consistent #3872

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Jan 14, 2023

Make PrintSelfimplementation style consistent following the ITK SWG
coding style guideline and the available ITK macros:

  • Always print the superclass first.
  • Print only the class instance variables.
  • Use the os << indent << "{ivarName}: " << m_ivarName << std::endl
    recipe: do not print the leading m_ of the ivar; use a colon after
    printing its name; leave a whitespace between the colon and the
    printed value of the ivar.
  • For boolean ivars print On/Off according to their values using the
    ternary operator.
  • Use itkPrintSelfObjectMacro to print objects inheriting from
    itk::LightObject that can be null.
  • For pointers that do not inherit from itk::LightObject check whether
    they are non-null before attempting to print them and print the
    (empty) string otherwise.
  • Rely on the self printing ability of the ITK classes to avoid
    boilerplate code when printing the corresponding ivars in other
    classes' PrintSelf methods.
  • Take advantage of the output stream << insertion operator overload
    for std::vector types in the itk::print_helper namespace defined
    in itkPrintHelper.h to print such types consistently.
  • Cast custom types to their numerical values using
    itk::NumericTraits::PrinType so that they are printed as expected.
  • Remove statements printing the hard-coded class name and its own
    pointer, e.g.
    os << indent << "ArrowSpatialObject(" << this << ")" << std::endl;
    
    PrintSelf does this without requiring the removed line.
  • Save typing this-> to point to the instance of the current class.

Apply the same principles to the << operator overloads.

Overload the ostream operator for the FixedImageSamplePoint class used
by the itk::ImageToImageMetric class.

Overload the ostream operator for the ImageVoxel class used by the
itk::DeformableSimplexMesh3DGradientConstraintForceFilter class.

Overload the ostream operator for the ImageVoxel class used by the
itk::DeformableSimplexMesh3DGradientConstraintForceFilter class.

PR Checklist

@github-actions github-actions bot added area:Core Issues affecting the Core module area:Examples Demonstration of the use of classes 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:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Jan 14, 2023
@jhlegarreta jhlegarreta force-pushed the MakePrintSelfMethodStyleConsistent branch 4 times, most recently from 8ba8047 to a53a4cc Compare January 14, 2023 23:51
Copy link
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 great! There are still a few compile errors.

@jhlegarreta
Copy link
Member Author

jhlegarreta commented Jan 16, 2023

There are still a few compile errors.

I know. Still WIP/draft. I have other changes locally, but unsure whether I should split it across at least 2 PRs, as it is becoming difficult to deal with. Also it is not intended to be comprehensive, as unfortunately, this is not an automated change.

I also grant that some changes belong to ENH, while others are more STYLE, but it is not evident to me that they should be split/how to split them without creating dozens of commits/PRs then even more difficult to deal with locally.

@dzenanz
Copy link
Member

dzenanz commented Jan 16, 2023

Having it as a single commit is fine. I know that splitting into themed commits requires extra effort.

@blowekamp
Copy link
Member

Amazing work getting this more consistent!

Use itkPrintSelfObjectMacro to print objects that can be null.
It may be clearer to say that this should be used for all pointer like objects, as pointers could be null.

One other issue that some time occurs with the print self methods is when "pixel types" are char. When they are a char then they are printed as an ASCII char and not a numeric value as expected. This is a case where the NumbericTraits::PrintType must be used. This is clearly separate work.

@jhlegarreta
Copy link
Member Author

Amazing work getting this more consistent!

Thanks Brad.

Use itkPrintSelfObjectMacro to print objects that can be null.
It may be clearer to say that this should be used for all pointer like objects, as pointers could be null.

Will rework the commit message as time permits and as I revise all the changes contained.

One other issue that some time occurs with the print self methods is when "pixel types" are char. When they are a char then they are printed as an ASCII char and not a numeric value as expected. This is a case where the NumbericTraits::PrintType must be used. This is clearly separate work.

Yes, had that in mind, but did not take the chance to make the changes. Will try to do that for the ivars that I'll be streaming, but will not be checking the existing ones as a general rule.

Note that this process is not automated, and I do not intend it to be comprehensive across the entire code base, but just a first step: I have mainly looked at some given patterns (like containing equal signs, etc.). Also, it will take me some time until I can get a working version. For now, it is still WIP.

@jhlegarreta jhlegarreta force-pushed the MakePrintSelfMethodStyleConsistent branch 5 times, most recently from db723f1 to 19557f6 Compare January 26, 2023 03:10
@jhlegarreta jhlegarreta force-pushed the MakePrintSelfMethodStyleConsistent branch 6 times, most recently from 96bc1cd to 1edc5e3 Compare January 27, 2023 14:33
@jhlegarreta jhlegarreta force-pushed the MakePrintSelfMethodStyleConsistent branch 6 times, most recently from 1a7fbfd to d257b97 Compare February 4, 2023 21:59
@jhlegarreta jhlegarreta force-pushed the MakePrintSelfMethodStyleConsistent branch 10 times, most recently from 361f692 to 4aee9a9 Compare February 5, 2023 16:11
Make `PrintSelf`implementation style consistent following the ITK SWG
coding style guideline and the available ITK macros:
- Always print the superclass first.
- Print only the class instance variables.
- Use the `os << indent << "{ivarName}: " << m_ivarName << std::endl`
  recipe: do not print the leading `m_` of the ivar; use a colon after
  printing its name; leave a whitespace between the colon and the
  printed value of the ivar.
- For boolean ivars print `On`/`Off` according to their values using the
  ternary operator.
- Use `itkPrintSelfObjectMacro` to print objects inheriting from
  `itk::LightObject` that can be null.
- For pointers that do not inherit from `itk::LightObject` check whether
  they are non-null before attempting to print them and print the
  `(empty)` string otherwise.
- Rely on the self printing ability of the ITK classes to avoid
  boilerplate code when printing the corresponding ivars in other
  classes' `PrintSelf` methods.
- Take advantage of the output stream `<<` insertion operator overload
  for `std::vector` types in the `itk::print_helper` namespace defined
  in `itkPrintHelper.h` to print such types consistently.
- Cast custom types to their numerical values using
  `itk::NumericTraits::PrinType` so that they are printed as expected.
- Remove statements printing the hard-coded class name and its own
  pointer, e.g.
  ```
  os << indent << "ArrowSpatialObject(" << this << ")" << std::endl;
  ```
  `PrintSelf` does this without requiring the removed line.
- Save typing `this->` to point to the instance of the current class.

Apply the same principles to the `<<` operator overloads.

Overload the ostream operator for the `FixedImageSamplePoint` class used
by the `itk::ImageToImageMetric` class.

Overload the ostream operator for the `ImageVoxel` class used by the
`itk::DeformableSimplexMesh3DGradientConstraintForceFilter` class.
@jhlegarreta jhlegarreta force-pushed the MakePrintSelfMethodStyleConsistent branch from 4aee9a9 to 173752f Compare February 5, 2023 17:27
Copy link
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 on a glance.

@dzenanz dzenanz merged commit c47ed1c into InsightSoftwareConsortium:master Feb 6, 2023
@jhlegarreta jhlegarreta deleted the MakePrintSelfMethodStyleConsistent branch February 6, 2023 15:39
@issakomi
Copy link
Member

issakomi commented Feb 9, 2023

M:\Dashboard\ITK\Modules\Numerics\FEM\include\itkImageToRectilinearFEMObjectFilter.hxx(302,3): error C3861: 'itkPrintSelfObject': identifier not found [M:\Dashboard\ITK-build\Modules\Registration\FEM\test\ITKFEMRegistrationTestDriver.vcxproj]

  itkPrintSelfObject(Material);
  itkPrintSelfObjectMacro(Element);

Does itkPrintSelfObject exist? S. this line

@jhlegarreta
Copy link
Member Author

Again, thanks for the heads-up @issakomi. You're right, it should be itkPrintSelfObjectMacro. I'll push another patch set.

@issakomi
Copy link
Member

issakomi commented Feb 9, 2023

Thanks @jhlegarreta.
BTW, there are many other errors from this PR in Modules/Registration/FEM, s.
https://open.cdash.org/viewBuildError.php?buildid=8462352

@jhlegarreta
Copy link
Member Author

BTW, there are many other errors from this PR in Modules/Registration/FEM, s.
https://open.cdash.org/viewBuildError.php?buildid=8462352

Oh, my bad. I did not have the FEM modules turned on and blindly relied on my local build/CI results, and did not spot those in the yesterday dashboard result. Thanks for the heads-up @issakomi.

@jhlegarreta
Copy link
Member Author

Again, thanks for the heads-up @issakomi. You're right, it should be itkPrintSelfObjectMacro. I'll push another patch set.

Oh, my bad. I did not have the FEM modules turned on and blindly relied on my local build/CI results, and did not spot those in the yesterday dashboard result. Thanks for the heads-up @issakomi.

PR #3918.

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:Examples Demonstration of the use of classes 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:Style Style changes: no logic impact (indentation, comments, naming) 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.

4 participants