[Insight-developers] Change of behavior

[Stephen Aylward]

On Wed, Aug 12, 2009 at 4:12 AM, Tom
Vercauteren<tom.vercauteren at gmail.com> wrote:
> Hi Hans,
>
> Plese find some comments below.
>
>
>> A new boolean variable was introduced to the code on 2009-07-28 in the class
>> called: m_UseFixedImageMask
>>
>> This variable is used to determine if fixed image mask processing should be
>> done. In the past (and in the non-optimized version), fixed image mask
>> processing was contingent only upon the existence of a mask object.
>
> There is at least some use cases where having a fixed image mask does
> not mean it has to be necessarily used within the metric computation.
> Here is one:
>
> * The caller sets a fixed image mask
>
> * The caller asks to use only a subset of the fixed image indexes
> (e.g. take 20% at random)
>
> * The metric object generates the fixed image indexes to be used. Of
> course, only non-masked indexes are generated

Actually - I assume you mean only masked indexes are generated - a
mask defined the region from where points should be chosen.

>
> * The metric object computes the metric value based on the fixed image
> indexes. There is no need to check whether the indexes are within the
> mask since they have been designed this way.

This is already how it is done - the indexes are pre-computed using
the mask.  The mask is not then subsequently used.

>
> In this case it makes sense to me to have a non null fixed image mask
> and a boolean flag m_UseFixedImageMask set to false.

I still agree with Hans - it is not needed.

However, I prefer it, and backward compatibility of stuff in review
doesn't matter.

Yet, my initial implementation had a bug.  Hans is willing to fix it,
I don't feel strongly about this var (it is, however, consistent with
the other methods (e.g., threshold) used to modify the samples chosen
(e.g., threshold has a useThresholds var)).

>
>
>
>> This new version of the code now requires that you first set the mask, and
>> then indicate that you want to use the mask.
>
> I don't understand why the caller could have access to setting this
> boolean flag. I believe it should strictly be computed by the object
> based on the scenario at hand.

I don't understand why this would be hidden from the caller.   The
concept is that setting something to NULL in order to disable it is
somewhat obscure and a secondary effect - therefore you provide
methods for turning options on/off as well as methods for setting
those options' parameters.

I actually don't follow your comment - computed by the object - the
spatialObject?   The registration scenario?  Keep in mind this is an
option in the itkOptImageToImageMetric base class.

>
>
>> It is also possible to set the
>>
>> m_FixedImageMask to some real mask object, then set m_UseFixedImageMask to
>> true,
>> and subsequently set m_FixedImageMask back to NULL. This scenerio would
>> cause the following bit of code to fail.
>>
>>       if( m_UseFixedImageMask )
>>         {
>>         double val;
>>         if( m_FixedImageMask->ValueAt( inputPoint, val ) )
>>           {
>
> This should indeed be fixed.

Agreed.

>
>
>> I’ve removed all references to m_UseFixedImageMask  and replaced it with the
>> previous logical equivalent:
>>
>> < if( m_UseFixedImageMask )
>> ---
>>> if( m_FixedImageMask.IsNotNull() )
>>
>> This fixed all the failing cases in my code, and all the ITK test cases
>> still pass.
>
> I am not sure this is the best fix since it imposes some unnecessary
> computations in the use case I mentioned. It thus goes against the
> purpose of the optimized image metric.

No, it doesn't.

>
>
> My two cents,
> Tom
>



-- 
Stephen R. Aylward, Ph.D.
Director of Medical Imaging
Kitware, Inc. - North Carolina Office
http://www.kitware.com
stephen.aylward (Skype)
(919) 969-6990 x300