[Insight-users] Nasty bug in Canny level set + another nasty bug??

[Zachary Pincus]

Hi Josh et al.,

I agree with your suggestion, as changing what various functions do can 
lead to trouble for everyone.

I would just like to make it clear that as far as I can tell, it should 
*never* be advantageous to have a negative advection coefficient 
(within the ITK level set formalism), unless the original advection 
image is broken in the first place, as in the Canny level set function. 
(Well, if you have some application where you want to drive the level 
set away from the image edges, then a negative coefficient is 
correct...) I believe this to be correct -- do people dis/agree?

Anyhow, I don't mean to cause a ruckus; it just seems like some users 
of ITK might be getting shortchanged if they're trying to use 
ReverseExpansionDirection or the Canny level set segmentation and not 
seeing the results that they ought to. (But, miracle of miracles, if 
they use both together, with a positive-inside level set, the errors 
cancel each other out! Alternatively, if they figured out to use a 
negative advection coefficient in these cases, they should be in the 
clear too...)

A related point is that if there is a good reason to not go ahead and 
fix ReverseExpansionDirection, then there might be the same good reason 
to not fix the error I identified in the Canny level set segmentation. 
If people have adapted to this problem (by using negative advection 
coefficients, or by using positive-inside level sets plus the erroneous 
ReverseExpansionDirection), then fixing the bug could break their code.

Also, does anyone remember why ReverseExpansionDirection was changed to 
negate the advection coefficient? (I wonder if it wasn't to "fix" a 
problem similar to the Canny problem I outlined...)

Sorry for the trouble,

Zach




On Feb 16, 2005, at 1:38 PM, Joshua Cates wrote:

> Hi Zachary,
>
> Thanks for your work and analysis on this issue.  Rather than modifying
> "ReverseExpansionDirection", my suggestion is to deprecate its use
> entirely and (eventually) force users to explicitly reverse the
> propagation and/or advection signs where appropriate for their sign
> conventions and methods.  The bottom line is that this method adds
> unneccessary complexity and confusion to the level-set function. This 
> is
> not the first time it has been an issue.  I think, for example, that 
> when
> it was first introduced, "ReverseExpansionDirection" existed in the 
> form
> you suggest, i.e.  without the flip of the advection sign.
>
> Note that some groups currently do make use of
> "ReverseExpansionDirection", so we will have to be careful in phasing 
> this
> method out or in making any changes in the short term.
>
> What do others think about this?
>
> Josh.
>
> ______________________________
>  Josh Cates			
>  Scientific Computing and Imaging Institute
>  University of Utah
>  http://www.sci.utah.edu/~cates
>
>
> On Wed, 16 Feb 2005, Zachary Pincus wrote:
>
>> Hello,
>>
>> I think I've discovered another problematic bug in the ITK level sets
>> implementation. In answering Hiraki Hideaki's question about whether
>> the advection direction problems were the result of whether inside
>> values were assumed to be positive or negative (it's not; see below), 
>> I
>> think I found a problem with ReverseExpansionDirection().
>>
>> Now, the advection direction is uninfluenced by whether the "inside" 
>> of
>> the level set is assumed to be the positive or negative values of the
>> phi function. One can verify this by referring to the figures I made (
>> http://www.stanford.edu/~zpincus/GeodesicLevelSet.pdf and
>> http://www.stanford.edu/~zpincus/CannyLevelSet.pdf ) and noting that
>> nowhere does the question of "which side is inside" come into play. 
>> The
>> Geodesic case works and the Canny case does not regardless of your
>> convention of positive and negative.
>>
>> You can also verify this by inverting the input level sets in the test
>> examples I provided and noting that they have the exact same behavior
>> to the un-inverted version. When advection is considered alone, apart
>> from propagation, the question of inside versus outside isn't 
>> relevant:
>> the only matter of importance is which way the zero-level set moves
>> with respect to image edges.
>>
>> However, calling ReverseExpansionDirectionOn() on the level set filter
>> "fixed" the problem, which puzzled me, since in my test examples I had
>> set the propagation coefficient to zero, so which way the expansion
>> direction is set should have no influence. I investigated, and found
>> that this function causes the advection term to be negated.
>>
>> I believe this behavior is entirely incorrect. As above, the advection
>> direction is entirely decoupled from whether the inside is assumed
>> positive or negative. The speed image (and hence the "expansion"
>> direction) is *not* decoupled from this assumption. Thus, to reverse
>> the expansion direction, it is correct to negate the propagation
>> coefficient, but it is *not* correct to negate the advection
>> coefficient. Negating the advection coefficient ONLY serves to drive
>> the zero-level set away from edges (provided the advection image is
>> properly defined!)
>>
>> I thus strongly believe that this is a second nasty bug in the ITK
>> level sets implementation. ReverseExpansionDirectionOn() called on the
>> level set segmentation filter (which causes 
>> ReverseExpansionDireciton()
>> of the level set function object to be called) should not cause the
>> advection term to be negated.
>>
>> As such, lines 24 - 30 of itkSegmentationLevelSetFunction.txx should 
>> be
>> changed from:
>>
>> template <class TImageType, class TFeatureImageType>
>> void SegmentationLevelSetFunction<TImageType, TFeatureImageType>
>> ::ReverseExpansionDirection()
>> {
>>    this->SetPropagationWeight( -1.0 * this->GetPropagationWeight() );
>>    this->SetAdvectionWeight( -1.0 * this->GetAdvectionWeight() );
>> }
>>
>> to
>>
>> template <class TImageType, class TFeatureImageType>
>> void SegmentationLevelSetFunction<TImageType, TFeatureImageType>
>> ::ReverseExpansionDirection()
>> {
>>    this->SetPropagationWeight( -1.0 * this->GetPropagationWeight() );
>> }
>>
>> Zach
>>