[Insight-developers] Change of behavior

[Tom Vercauteren]

Thanks for the clarification Stephen.

I didn't realize that the path I described was the only possible one
in the optimized metrics. I still had in mind the non-optimized
behavior where during the metric computation, one checks whether the
pixel is masked or not...

Hans proposal totally makes sense to me now.


>>> 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.

Well that's just a matter of definition ("to mask" equivalent to "to
hide" is in common language).


>> * 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.

Ok, I thought it was still used in the case where no subsampling was
asked for. This is where I initially got confused.


>> 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.

Got it now.


> 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.

Well, I usually prefer to have only one way to specify thing. Putting
something to NULL to disable it looks fine to me :)
But then as long as it is well documented, any option is fine to me.


> 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.

I meant the ImageToImageMetric object. Anyhow, as you said,
m_UseFixedImageMask is not necessary so my comment didn't make sense.


>>> 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.

Got it now.

Tom