Hough Transform 2D Circles Image Filter GetCircles patch.


(Niels Dekker) #81

@matt.mccormick In order to ensure that world space supports is added to GetCircles() the way you have in mind, can you please submit a patch for this issue?

I think it would be nice if a spatial object created by GetCircles() would store both the world coordinates and the original grid index coordinates of the center of the circle. Or if it would store the transformation that was applied from grid index to world space. Do you think that’s possible?


(Matt McCormick) #82

Yes, I will create a patch.

With the circle world coordinates, the index coordinates can be obtained with TransformPhysicalPointToIndex or TransformPhysicalPointToContinuousIndex. This will result in the correct index coordinates, which are dependent on the image’s metadata.


(Niels Dekker) #83

@matt.mccormick I guess you don’t have time anymore to create such a patch for ITK 5.0.0 alpha (estimating circles, based on world coordinates, instead of pixel coordinates), right…?

Do you think the new EllipseSpatialObject member functions ‘GetCenterPoint’ + ‘SetCenterPoint’ should still be renamed to ‘GetCenter’ + ‘SetCenter’, before ITK 5.0.0 alpha? The current ‘GetCenterPoint’ and ‘SetCenterPoint’ are fine to me, but I’d rather not have them deprecated immediately with the next release! :open_mouth:


(Matt McCormick) #84

This is currently on by todo list, but I do not have time to work on it at the moment.

There is no rush to get these changes into the first ITK 5.0 alpha. There will be more ITK 5 alpha’s and API / breaking changes are expected between alphas – deprecation should not be a concern.


(Niels Dekker) #85

Hereby I would like to draw your attention to STYLE: Removed HoughTransform2DCircles default for TRadiusPixelType

The current HoughTransform2DCirclesImageFilter template has TRadiusPixelType = TOutputPixelType, by default. This patch just removes the default argument, hoping to trigger ITK users to make the right choice themselves.

In general (also as ITK user), I do appreciate when a template has default arguments, if those correspond to the most common (or recommended) use case. However, with the HoughTransform2DCircles filter, I think it’s often preferable to use different types for TRadiusPixelType and TOutputPixelType.

TOutputPixelType is the type of the accumulator values calculated by the Hough transform. These values are always whole numbers. They are calculated by incrementing repetitively (starting at zero), at https://github.com/Kitware/ITK/blob/v5.0a02/Modules/Filtering/ImageFeature/include/itkHoughTransform2DCirclesImageFilter.hxx#L146

TRadiusPixelType is the pixel type of the radius image; radius image pixels are computed by averaging: https://github.com/Kitware/ITK/blob/v5.0a02/Modules/Filtering/ImageFeature/include/itkHoughTransform2DCirclesImageFilter.hxx#L173

So for TOutputPixelType, an unsigned integer is usually the best choice for this pixel type, whereas for TRadiusPixelType, a floating point type usually more appropriate.

Please review: http://review.source.kitware.com/#/c/23518/

Update (14 June 2018): The patch is merged now :smile: Thanks for your review, @matt.mccormick & @dzenanz


(Niels Dekker) #86

Another proposed patch for HoughTransform2DCirclesImageFilter! The original code checked if the gradient (Vx, Vy) at input pixel location (x, y) is not flat by the following code:

  // if the gradient is not flat
  if ( ( std::fabs(Vx) > 1 ) || ( std::fabs(Vy) > 1 ) )

This test appears quite arbitrary, as Vx and Vy are floating point numbers, and for some images, (0.9, 0.9) might not be such a flat gradient. For other images, the gradient may still appear rather flat when Vx and Vy are significantly greater than one. Moreover, it often seems to make more sense to look at the norm of the gradient, instead of its individual (Vx, Vy) components. This is why I’m proposing to allow the user to specify their own (application specific) MinimumGradientNorm. Please review:

ENH: Added MinimumGradientNorm to HoughTransform2DCirclesImageFilter

I already did some code improvement based on suggestions by @jhlegarreta

Note: The patch should be backward-compatible, because it still checks if (std::fabs(Vx) > 1) || (std::fabs(Vy) > 1) when the user does not set the value of MinimumGradientNorm.

Update: For the record, the MinimumGradientNorm proposal has been superseded by “ENH: Added GradientNormThreshold to HoughTransform2DCirclesImageFilter”: