Remove implicit conversion from a single scalar value to itk::Vector

[N-Dekker]

Follow-up to commit a890962 "ENH: Make Vector construction from scalar value explicit" by Brian Helba (@brianhelba), January 26, 2015

Analog to: pull request #3237 commit 8825834 "ENH: Declare converting Point(v) constructors explicit"

[N-Dekker]

@dzenanz Do you have any suggestion how to fix https://open.cdash.org/viewBuildError.php?buildid=7780689 ?

Wrapping/Modules/ITKLevelSets/itkVectorThresholdSegmentationLevelSetImageFilterPython.cpp:3906:44: error: no matching function for call to 'itk::VectorThresholdSegmentationLevelSetImageFilter<itk::Image<float, 2>, itk::Image<itk::Vector<float, 2>, 2>, float>::SetMean(const double&)'

   (arg1)->SetMean((double const &)*arg2);

I can't find that cpp file (itkVectorThresholdSegmentationLevelSetImageFilterPython.cpp), how is it generated?

[dzenanz]

That file is automatically generated by SWIG, from itkVectorThresholdSegmentationLevelSetImageFilter.wrap.

[dzenanz]

I guess that making the constructor explicit is preventing some SetMean call from compiling. The reported lines do not match my local file. I guess local python compilation is needed for hunting this down.

[N-Dekker]

The error message does seem to make sense, because SetMean expects a Vector as argument, not just a single double value:

ITK/Modules/Segmentation/LevelSets/include/itkVectorThresholdSegmentationLevelSetImageFilter.h

Line 122 in 4f30980

SetMean(const MeanVectorType & mean)

Of course, we could add an extra SetMean(double) overload, which would fill the Vector of mean values by the specified double value. But would that be desirable?

[dzenanz]

add an extra SetMean(double)

Sounds reasonable. And it does not force us to find where the offending call is being made 😄

[dzenanz]

This still fails to compile. Matt suggested to revert in #3258.

[Leengit]

I just deleted my previous comment.

[N-Dekker]

This still fails to compile. Matt suggested to revert in #3258.

@dzenanz I just hope 8825834 doesn't get reverted (as suggested by PR #3258). But what can I do?

[dzenanz]

Finding bugs exposed by the explicit constructor are a great attempt, which might work if there aren't too many of them 😄

[Leengit]

https://github.com/N-Dekker/ITK/blob/9ecef41cd9a44086050b5e62f5c4eb9b993cf8d7/Modules/Segmentation/LevelSets/include/itkVectorThresholdSegmentationLevelSetImageFilter.h#L128-L129
is failing because the compiler thinks that double might be an example of a MeanVectorType but then chokes on MeanVectorType::ValueType when that turns out not be the case. We might change

  void
  SetMean(const typename MeanVectorType::ValueType meanValue)

to

  template <typename TMeanValue>
  std::enable_if_t<std::is_same<TMeanValue, typename MeanVectorType::ValueType>::value, void>
  SetMean(const TMeanValue meanValue)

[N-Dekker]

the compiler thinks that double might be an example of a MeanVectorType

Thanks @Leengit but if I understand correctly, MeanVectorType should be an itk::Vector, not a double!

MeanVectorType is defined at

ITK/Modules/Numerics/Statistics/include/itkMahalanobisDistanceMembershipFunction.h

Lines 84 to 87 in 865d0b0

	/** Type of the mean vector. RealType on a vector-type is the same
	* vector-type but with a real element type. */
	using MeasurementVectorRealType = typename itk::NumericTraits<MeasurementVectorType>::RealType;
	using MeanVectorType = MeasurementVectorRealType;

My guess it now that the itk::NumericTraits specialization for itk::Vector was not available at those particular lines of code. (Without this specialization, NumericTraits::RealType might just be double.) That's why I'm now hoping to fix the errors by an #include "itkNumericTraitsVectorPixel.h". With commit 563256a

Fingers crossed please 🤞

[N-Dekker]

I think this pull request is ready now 😃 The missing #include "itkNumericTraitsVectorPixel.h", fixed with 563256a might be a serious bug, because MeanVectorType appears defined as double, instead of a itk::Vector, when the specific NumericTraits<Vector> specialization is not included. Problem solved! 😄

[hjmjohnson]

building AnisotropicDiffusionLBR now fails:

/Users/johnsonhj/Dashboard/src/AnisotropicDiffusionLBR/include/itkLinearAnisotropicDiffusionLBRImageFilter.hxx:480:27: error: reference to type 'const itk::Vector<double, 3>' could not bind to an rvalue of type 'double'
  m_NextImage->FillBuffer(0.);
                          ^~

NOTE: ITK_FUTURE_LEGACY_REMOVE:BOOL=ON. Some of the remote modules probably will need some TLC .

[N-Dekker]

Thanks for the notice, @hjmjohnson

m_NextImage->FillBuffer(0.);

What you would need here is a zero-initialized itk::Vector. In general, this can be achieved by the syntax VectorType(), or VectorType{}. (Assuming that there is a VectorType alias. Otherwise I guess something like ImageType::PixelType) But probably an empty pair of curly braces will do as well:

 m_NextImage->FillBuffer({});

HTH, Niels

PS FYI The specific Vector(r) constructor has already been explicit for more than seven years, with ITK_FUTURE_LEGACY_REMOVE=ON: a890962

[hjmjohnson]

FYI: InsightSoftwareConsortium/ITKAnisotropicDiffusionLBR#51