WIP: Added HoughTransform2D Circle class, simplified GetCircles() ret…

[hjmjohnson]

…urn type

Added a very simple Circle class to represent a circle, detected by
itk::HoughTransform2DCirclesImageFilter. Changed the return type of
HoughTransform2DCirclesImageFilter::GetCircles() from
std::list<EllipseSpatialObject<2>::Pointer>& to std::vector&,
to ease retrieval of the center and the radius of detected circles.

Added convenience conversion function from Circle to EllipseSpatialObject<2>,
to ease upgrading legacy code that depended on the old (<= ITK 4.13) interface.

Removed redundant local variable 'circles' (equal to m_CirclesList.size())
from GetCircles().

Triggered by comments from Tim Evain, starting at:
https://discourse.itk.org/t/hough-transform-2d-circles-image-filter-getcircles-patch/350/46

[hjmjohnson]

http://review.source.kitware.com/#/c/23139/

Niels Dekker
Uploaded patch set 1.Feb 1, 2018

Kitware Build Robot
Patch Set 1: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 1, 2018

Niels Dekker
Uploaded patch set 2.Feb 2, 2018

Niels Dekker
Patch Set 2: For the record, Tim Evain commented the following at https://discourse.itk.org/t/hough-transform-2d-circles-image-filter-getcircles-patch/350/53 I think this is a great improvement over the previous system :-). Some thoughts: On “ToDo 3” : maybe start with the definition of Circle into the Hough file, and see afterward if there is really a need for other filters to use it ? Circles are found by an index center, but defined as a point afterward, leading to a confusing representation. An user that doesn’t check the code will think that the circle is defined in world coordinates, but the radius and center are in fact in continuous indexes. I think it should be harmonized, either by making the center an index, or putting all values in world coordinates (I think the latter is preferable). On “ToDo 2” : it depends on the previous choice. If the circle is in index system, there is no need for template. Since the return type of GetCircles() will change, I wonder if a vector is not preferable over a list. The syntax to access circles with lists is heavier in my opinion. Anyway thanks for the time you put into making this!Feb 2, 2018

Kitware Build Robot
Patch Set 2: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 2, 2018

Niels Dekker
Uploaded patch set 3.Feb 2, 2018

Niels Dekker
Uploaded patch set 4.Feb 2, 2018

Kitware Build Robot
Patch Set 3: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 2, 2018

Kitware Build Robot
Patch Set 4: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 2, 2018

Niels Dekker
Patch Set 4: With the next Patch Set, I'll try to fix the following GCC error: ITK-src/Modules/Filtering/ImageFeature/include/itkHoughTransform2DCirclesImageFilter.hxx:370:3: error: need 'typename' before 'itk::HoughTransform2DCirclesImageFilter::CirclesListType:: const_iterator' because 'itk::HoughTransform2DCirclesImageFilter::CirclesListType' is a dependent scope", at https://open.cdash.org/viewBuildError.php?buildid=5245393Feb 2, 2018

Niels Dekker
Uploaded patch set 5.Feb 2, 2018

Kitware Build Robot
Patch Set 5: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-5&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 2, 2018

Niels Dekker
Patch Set 5: With the next Patch Set, I'll try to fix MacOSX-Clang warnings, "itkHoughTransform2DCirclesImageFilter.hxx:304:25: warning: unused variable 'circles'". Note that Visual Studio would produce such warnings as well at Warning Level 4: "warning C4189: 'circles': local variable is initialized but not referenced".Feb 3, 2018

Niels Dekker
Uploaded patch set 6.Feb 3, 2018

Kitware Build Robot
Patch Set 6: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-6&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 3, 2018

Niels Dekker
Patch Set 6: With my next Patch Set, I'll try to fix a Clang error on Dash5.kitware.com (but not on misty.kitware.com): "itkHoughTransform2DCirclesImageFilter.hxx:44:13: error: no viable conversion from 'const CenterType' (aka 'const Index<2>') to 'IndexValueType' (aka 'long')". It appears that this is a C++11/compiler issue of Clang, which is fixed somewhere between AppleClang version 6.0.0.6000056 (the version on Dash5.kitware.com) and 8.0.0.8000042 (the version on misty.kitware.com).Feb 4, 2018

Niels Dekker
Uploaded patch set 7.Feb 4, 2018

Kitware Build Robot
Patch Set 7: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-7&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 4, 2018

Niels Dekker
Patch Set 7: FYI, Tim Evain commented "I’m not on Gerrit, but your patch seems fine to me.", at https://discourse.itk.org/t/hough-transform-2d-circles-image-filter-getcircles-patch/350/55Feb 5, 2018

Niels Dekker
Patch Set 7: With the next Patch Set, I'll try to fix the Merge Conflict.Feb 5, 2018

Niels Dekker
Uploaded patch set 8: Patch Set 7 was rebased.Feb 5, 2018

Kitware Build Robot
Patch Set 8: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-8&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 5, 2018

Niels Dekker
Patch Set 8: Does this patch make any chance for ITK 5.0?Feb 8, 2018

Matt McCormick
Patch Set 8: Code-Review-1 (5 comments) Thanks for working on this. Please see inline comments.Feb 8, 2018

Niels Dekker
Patch Set 8: Thanks for your review, Matt. You wrote: "To ensure compatibility and functionality with the rest of the toolkit, a CircleSpatialObject should be created instead." You would like such a class instead of the lightweight Circle class I proposed, right? Do you still like such a CircleSpatialObject class to be nested inside the Filter class, or would you prefer to have it at ITK/Modules/Core/SpatialObjects ?Feb 13, 2018

Matt McCormick
Feb 13, 2018
↩
Patch Set 8:

Thanks, Neils. Good question -- a CircleSpatialObject should be its own class along with the other SpatialObject's.

Niels Dekker
Patch Set 8: Do you think such a CircleSpatialObject class should have number-of-dimensions as template argument? Personally I think a circle is fundamentally 2-D. So I would prefer not to support 1-D, 3-D, 4-D, circles, etc. It's far beyond the scope of HoughTransform2DCirclesImageFilter anyway. But then... itk::EllipseSpatialObject is actually templated over the number-of-dimensions. Having 3-D as default!Feb 13, 2018

Niels Dekker
Patch Set 8: Instead of introducing another SpatialObject derived class, what would you think of an extra GetCenterPoint() member function, added to EllipseSpatialObject, basically returning just this->GetObjectToParentTransform()->GetOffset()?Feb 15, 2018

Niels Dekker
Patch Set 8: With the following patch set, I'll remove the ITK_TEMPLATE_EXPORT, replaced two 'auto' keywords by explicit types, and replaced two 'long int' by IndexValueType, as suggested by Matt. Also did a rebase, hoping to fix the Merge error.Feb 19, 2018

Niels Dekker
Uploaded patch set 9.Feb 19, 2018

Kitware Build Robot
Patch Set 9: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=23139-9&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 19, 2018

Niels Dekker
Patch Set 9: Personally I still prefer the patch that adds a simple HoughTransform2D Circle class, http://review.source.kitware.com/#/c/23139 which is much clearer about the fact that GetCircles() produces pixel grid coordinates, rather than world coordinates, by using Index<2> as CenterType.Feb 19, 2018

Niels Dekker
Patch Set 9: Sorry, my previous comment was meant for "ENH: Added GetCenterPoint and SetCenterPoint to EllipseSpatialObject", http://review.source.kitware.com/#/c/23188Feb 19, 2018

Hans J. Johnson
Patch Set 9: Niels, When trying to move this PR to GitHub, there are some merge conflicts that I think you are best suited to resolve.Nov 5 5:33 PM

Niels Dekker
4:40 AM
↩
Patch Set 9:

@hans, thanks for trying to move this to GitHub. Maybe I should just abandon the issue, as I could not convince Matt (or anyone else, I guess) that filter->GetCircles() should return std::vector& ('Circle' being a simple aggregate of a radius and a center point), instead of the more complicated std::list<EllipseSpatialObject<2>::Pointer>&.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.