InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.41k stars 663 forks source link

Overload the stream insertion operator for types that are missing it #513

Open jhlegarreta opened 5 years ago

jhlegarreta commented 5 years ago

Description

Overload the stream insertion operator for types that are missing it.

Expected behavior

To be able to print any data structure with a single line, e.g.:

os << indent << "MyStructure : " << m_MyStructure << std::endl;

Actual behavior

Developer's are forced to iterate over such structures to get them printed (usually in the PrintSelf method), e.g.:

  while( it != m_CirclesList.end() )
    {
    os << indent << "[" << i << "]: " << *it << std::endl;
    ++it;
    ++i;
    }

Additional information

Some fundamental ITK types (e.g. IndexTypes, SizeType, SpacingType, OffsetType, DirectionType, OffsetTable, RegionType) either seem not to have such an overload or it is just because some filters (itkImageConstIteratorWithIndex.h, itkImageIORegion.h, itkNeighborhood.h, itkImportImageFilter.h, itkConstNeighborhoodIterator.h, itkImportImageFilter.hxx, itkDisplacementFieldJacobianDeterminantFilter.h, itkGaussianBlurImageFunction.h, itkEllipsoidInteriorExteriorSpatialFunction.h) that declare ivars of such types are not using the correct types, since it looks weird that such fundamental types do not have the stream insertion operator overload.

Other more exotic type aliases (GradientType, StrideTable, SeedsContainerType, WeightsType, ErrorArrayType, ExtentArrayType) would also deserve some investigation, as well as other aliases that are not consistent with the toolkit convention for the intended use (e.g. InputType in itkEllipsoidInteriorExteriorSpatialFunction.h).

Finally, other data structures that are also missing such an overload are regular arrays, e.g.

double m_Sigma[ImageDimension2];

in itkBSplineControlPointImageFunction.h or itkGaussianDerivativeImageFuncton.h

or

double          m_Scale[NDimensions];

in itkScalableAffineTransform.h.

std::list types (e.g. itkHoughTransform2DCirclesImageFilter.h or itkHoughTransform2DLinesImageFilter.h).

Or vnl_matrix matrix (e.g. itkBSplineScatteredDataPointSetToImageFilter.h).

This is related to #512.

ChristianJacobsen commented 3 years ago

@dzenanz, is this issue still relevant? If so I'd like to have a go at it. After listening to @thewtex talk about the library on cppcast today I got inspired to help out.

dzenanz commented 3 years ago

Yes, this is still relevant.

dzenanz commented 3 years ago

Looking forward to reviewing the contribution!

ChristianJacobsen commented 3 years ago

Hi again @dzenanz!

As far as I am able to see the problems with printing fundamental types from itkIntTypes.h and itkFloatTypes.h no longer exist. Compiler seemed happy printing ImageConstIteratorWithIndex::GetIndex() returning an IndexType.

As for native arrays and std::list, I have so far added support to itkPrintHelper.h for those.

vnl_matrix already has an operator<< overload, so I'm leaving that one as well.

W.r.t.

Other more exotic type aliases

can you give me some examples of where they are printed currently and how you would like them printed/formatted?

dzenanz commented 3 years ago

Example of an exotic type: GradientType: 1 2 3

A large part of the job is finding instances of these types and replacing custom printing operators by the new specialized overloads.

dzenanz commented 3 years ago

And of course printing instances of these types as ivars now that it is convenient to print them.

jhlegarreta commented 3 years ago

@ChristianJacobsen contributions are always welcome :100:, and as @dzenanz says, this is still relevant. It would be awesome to finally have all necessary overloads in ITK ! So thanks for considering this :+1:.

Besides what @dzenanz pointed, section C.21 of the ITK SW Guide gives some more guidance about ivar printing in ITK. Essentially we would like to overload the << operator or for all non-trivial types so that we can potentially print any type in ITK with a simple one-liner, e.g.

os << indent << "OurNonTrivialType: " << m_OurNonTrivialType << std::endl

That would allow us to be able to:

PR #512 is related to this. Essentially, all ivars are expected to be printed in a method's PrintSelf method. You will see that this is not always the case: https://open.cdash.org/viewCoverage.php?buildid=7432448

And some other times we may still encounter loops to print some ivars due to a lacking support for the overload.

Some other times ivars are serialized but the lack of appropriate support makes their address to be printed instead of their content (or some more meaningful information), or the itk::print_helper namespace is still missing, etc. e.g. PR #2226 never made it to master, for example, but I had it in my ToDo list to revisit it. So you can also take it if you are willing to.

Booleans have not yet got the corresponding treatment either (cf. this comment), and at times their are directly serialized to their integer values, other times an if/else is used to print the On/Off or True/False strings, etc.

A nice side benefit of printing all ivars is that by doing so at times we identify find some ivars that have not been initialized, which is an undesirable situation, and may lead to a bug. So printing all ivars, hopefully using such useful overloads, is important.

Many of the PRs that have as subject ENH: Improve coverage (...) or a similar text add statements to print the related classes' ivars, so you can get some inspiration from them as well.

Hope this helps. Thanks @ChristianJacobsen .

jhlegarreta commented 2 years ago

@ChristianJacobsen Another good candidates to get this enhancement are these two: https://github.com/InsightSoftwareConsortium/ITK/blob/f81578bbb6242032b611d7e3e4dca3b7557fbbe0/Modules/Core/Common/include/itkNeighborhood.hxx#L170

m_StrideTable and m_OffsetTable.

Or the m_MembershipFunctions vector here: https://github.com/InsightSoftwareConsortium/ITK/blob/047fa6934ac32711d1d7b84fa6eb81c72db01960/Modules/Segmentation/Classifiers/include/itkClassifierBase.hxx#L51

One would call itkPrintSelfObjectMacro(MemberThanCanBeNull) for objects that can be null pointers; however in this case, we have a vector, where each of the values can be so.

Or still, removing the need for the boilerplate code to check for null raw pointers, as in: https://github.com/InsightSoftwareConsortium/ITK/blob/966c9e56f70b7c60ecb05cccb93a3463d4fd5852/Modules/Filtering/Convolution/include/itkConvolutionImageFilterBase.hxx#L118

Thanks.

ChristianJacobsen commented 2 years ago

Hi @jhlegarreta! Sorry for the late reply. Things at work are just so hectic at the moment that I barely have any spare time left. I will continue to look at this when my schedule clears up. Feel free to reassign responsibility should someone else want to do it instead. Thank you for the tip about itkPrintSelfObjectMacro! 😄

jhlegarreta commented 2 years ago

@ChristianJacobsen no worries. Take your time. Thanks.