Our current element does not build the ball by alternating the application of crosses and boxes. Perhaps we should have an element that does it that way. For speed, I think old school morphology would not build a large structuring element but apply small elements in a particular sequence to the image (dilate with triangle, dilate with rotated triangle, ...).
I like the idea of introducing a new structuring element that implements Dirk's design. I'd also be happy with a mode on the current element to switch between the behaviors as long as the default was the current behavior. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Bradley Lowekamp Sent: Thursday, October 17, 2013 1:44 PM To: Padfield, Dirk R (GE Global Research) Cc: Richard Beare; Johnson, Hans J; [email protected] Subject: Re: [Insight-developers] BUG in BinaryBallStructuringElement Dirk, First, I have been using the FlatStructuringElement class as opposed to the BinaryBallStructuring element. Does the resulting shape differ? I think they should be consistent. Please, please do not change the shape of the current structuring elements. Many optimized usage and implementation of morphology rely on a specific decomposition of the binary ball structuring element. To create a ball, you are alternate between a 1-cross and a 1-box. I believe your change is simple switching for the cross be for the box. Which would make such decomposition optimization inconsistent. I would suggest you create a new structuring element class to meet you needs. I think this would make a easier transition of we wish to deprecate the current version in favor of yours interpretation of correctness. I would like to hear from Richard Beare and/or Gaetan on this issue. They are the resident morphology experts in our community. Brad On Oct 17, 2013, at 1:14 PM, "Padfield, Dirk R (GE Global Research)" <[email protected]> wrote: > Hi Ho, > > Thanks for looking into this more closely. Your agreement that this is a bug > is encouraging! > > Now, what to do...This is the perennial battle between backward compatibility > and future correctness! > > Hans' suggestion to add a member function to enable the correct behavior > makes sense. But I am concerned that it adds additional complexity and that > these methods will be ignored if not set as default. > > Hans, what do you think about the idea of adding such a method and then > marking the old method as deprecated and setting an ITK_FUTURE_LEGACY_REMOVE > flag? > > Thanks, > Dirk > > ________________________________ > From: Ho Cheung [[email protected]] > Sent: Thursday, October 17, 2013 12:22 PM > To: Padfield, Dirk R (GE Global Research) > Subject: Re: [Insight-developers] BUG in BinaryBallStructuringElement > > I totally agree that the corrected version is more accurate. > > Upon examining the code more closely and further thought, it also does seem > to be an oversight by the original author. > > Furthermore, my suggestion that it was simply a discretization problem turns > out to be a non-sequiter. It turns out that the additional pixel on the axes > length was approximating the "rounding discretization process" for the radius > sizes I was looking at. > > I do share the same concerns about backwards compatibility as Hans and Wes, > but definitely I'll change my opinion of this to being an actual bug. > > Regards, > > Ho Cheung > (775) 388-2368 > > On Oct 17, 2013, at 8:09 AM, Padfield, Dirk R (GE Global Research) > <[email protected]<mailto:[email protected]>> wrote: > > Hi Ho, > > Thanks for your feedback and insight! I agree that discretizing continuous > functions is always a tricky thing. Luckily, we have the spatial objects to > help with this since they define their own inside-outside tests. The Ellipse > spatial object is used in the BinaryBallStructuringElement implementation, > but the problem is that the spatial object itself is used incorrectly. By > definition, the axes should be "radius*2" rather than "radius*2+1". Defining > the axes of an ellipse/circle to be "radius*2+1" is simply an error. > > We can also attack this question by considering the area of the continuous > function versus the discretized version by counting the number of "on" pixels > in the kernel as follows: > > For radius=1, the true area is pi = 3.14 Using the old version, we get > 9 Using the correction, we get 5 > > For radius=5, the true area is 25*pi = 78.5 Using the old version, we > get 97 (24% error) Using the correction, we get 81 (3% error) > > For radius=11, the true area is 121*pi = 380 Using the old version, we > get 421 (11% error) Using the correction, we get 377 (1% error) > > For radius=21, the true area is 21*21*pi = 1385 Using the old version, > we get 1457 (5% error) Using the correction, we get 1373 (1% error) > > As expected, as the radius increases, the discretized version better > approximates the continuous function. We can also see that the corrected > version is always more accurate than the old version. > > What do you think? > > Thanks, > Dirk > > > ________________________________ > From: Ho Cheung [[email protected]<mailto:[email protected]>] > Sent: Wednesday, October 16, 2013 6:26 PM > To: Padfield, Dirk R (GE Global Research) > Cc: ITK > Subject: Re: [Insight-developers] BUG in BinaryBallStructuringElement > > Dirk, > > As a counterpoint, I do not agree that there is a bug but rather just an > ambiguity in the way we have defined whether or not a pixel is to be included. > > If you take a protractor and plotted a unit circle, then superimpose a grid > on it (this this case, 3x3), and then shaded in the nearest pixels to the > circle, it would look like the "original" example. The same applies to the > radius 5 circle. > > Technically, if you look at the parametric definition of a circle, then yes, > those pixels would not be included, as their physical space points fall > outside the circle. > > However, I believe (anecdotal) in graphics rendering, it is common practice > to include those pixels which are nearest to the actual physical space point. > > Regards, > > Ho Cheung > (775) 388-2368 > > On Oct 16, 2013, at 1:05 PM, Padfield, Dirk R (GE Global Research) > <[email protected]<mailto:[email protected]><mailto:[email protected]>> > wrote: > > Hi All, > > I am writing to ask your advice about a bug I found in > BinaryBallStructuringElement. > > For a while, I have been bothered by the fact that the > BinaryBallStructuringElement return balls that are larger than the specified > radius. For example, when given a radius of 1, it returns the structuring > element: > 1 1 1 > 1 1 1 > 1 1 1 > > But this structuring element has a radius that is more than 1! If it truly > had a radius of 1, it would be a cross shape in this case. > > When choosing a larger radius, the problem is more obvious. Setting radius = > 5 (leading to a structuring element size of 11x11) results in: > 0 0 0 1 1 1 1 1 0 0 0 > 0 0 1 1 1 1 1 1 1 0 0 > 0 1 1 1 1 1 1 1 1 1 0 > 1 1 1 1 1 1 1 1 1 1 1 > 1 1 1 1 1 1 1 1 1 1 1 > 1 1 1 1 1 1 1 1 1 1 1 > 1 1 1 1 1 1 1 1 1 1 1 > 1 1 1 1 1 1 1 1 1 1 1 > 0 1 1 1 1 1 1 1 1 1 0 > 0 0 1 1 1 1 1 1 1 0 0 > 0 0 0 1 1 1 1 1 0 0 0 > > This is clearly not an ellipse/circle with radius 5 because the interior > ellipse/circle is touching each image border at five points rather than just > one. As it turns out, the code is actually defining a radius that is "X + > 0.5", where X is the radius that is requested! > > The problem is in the specification of the ellipse axes on lines 70-76 of > itkBinaryBallStructuringElement.hxx: > // Define and set the axes lengths for the ellipsoid typename > EllipsoidType::InputType axes; for ( i = 0; i < VDimension; i++ ) { > axes[i] = this->GetSize(i); } > spatialFunction->SetAxes(axes); > > In this case, "this->GetSize()" is equal to radius*2+1. But, an > ellipse/circle with radius X should have axes length 2X, not 2X+1! In the > implementation, the center of the ellipse is properly accounted for by > setting it to "this->GetRadius+1", but the size of the ellipse is not correct! > > To correct this, we can make a simple change either axes[i] = > this->GetSize(i) - 1; or axes[i] = this->GetRadius(i) * 2; > > The second is probably more intuitive. > > With this fix, we get for radius=1: > 0 1 0 > 1 1 1 > 0 1 0 > > and for radius=5: > 0 0 0 0 0 1 0 0 0 0 0 > 0 0 1 1 1 1 1 1 1 0 0 > 0 1 1 1 1 1 1 1 1 1 0 > 0 1 1 1 1 1 1 1 1 1 0 > 0 1 1 1 1 1 1 1 1 1 0 > 1 1 1 1 1 1 1 1 1 1 1 > 0 1 1 1 1 1 1 1 1 1 0 > 0 1 1 1 1 1 1 1 1 1 0 > 0 1 1 1 1 1 1 1 1 1 0 > 0 0 1 1 1 1 1 1 1 0 0 > 0 0 0 0 0 1 0 0 0 0 0 > > This is a true circle with radius 5! > > My questions are: > 1) Is anyone else bothered by this bug? I imagine that many users expect the > corrected version and don't realize they are getting the incorrect one. > 2) Do others agree with this fix? > 3) Can we make this fix given the number of filters/applications that will > change slightly as a result of this fix? > > Many thanks, > Dirk > > > > _______________________________________________ > Powered by > www.kitware.com<http://www.kitware.com><http://www.kitware.com> > > Visit other Kitware open-source projects at > http://www.kitware.com/opensource/opensource.html > > Kitware offers ITK Training Courses, for more information visit: > http://kitware.com/products/protraining.php > > Please keep messages on-topic and check the ITK FAQ at: > http://www.itk.org/Wiki/ITK_FAQ > > Follow this link to subscribe/unsubscribe: > http://www.itk.org/mailman/listinfo/insight-developers > > > _______________________________________________ > Powered by www.kitware.com > > Visit other Kitware open-source projects at > http://www.kitware.com/opensource/opensource.html > > Kitware offers ITK Training Courses, for more information visit: > http://kitware.com/products/protraining.php > > Please keep messages on-topic and check the ITK FAQ at: > http://www.itk.org/Wiki/ITK_FAQ > > Follow this link to subscribe/unsubscribe: > http://www.itk.org/mailman/listinfo/insight-developers _______________________________________________ Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Kitware offers ITK Training Courses, for more information visit: http://kitware.com/products/protraining.php Please keep messages on-topic and check the ITK FAQ at: http://www.itk.org/Wiki/ITK_FAQ Follow this link to subscribe/unsubscribe: http://www.itk.org/mailman/listinfo/insight-developers _______________________________________________ Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Kitware offers ITK Training Courses, for more information visit: http://kitware.com/products/protraining.php Please keep messages on-topic and check the ITK FAQ at: http://www.itk.org/Wiki/ITK_FAQ Follow this link to subscribe/unsubscribe: http://www.itk.org/mailman/listinfo/insight-developers
