[Insight-developers] Optimized ImageToImageMetric::SetFixedImageRegion signature

[Stephen Aylward]

Not a big fan since it does change backward compatibility a bit.  But
I don't feel strongly on this one.

s

On Wed, Aug 19, 2009 at 8:52 AM, Tom Vercauteren<tom.vercauteren at m4x.org> wrote:
> Thanks for the feedback Stephen. I have committed the patch:
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Review/itkOptImageToImageMetric.h?root=Insight&r1=1.29&r2=1.30&sortby=date
> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Review/itkOptImageToImageMetric.txx?root=Insight&r1=1.35&r2=1.36&sortby=date
>
> I have a similar but harder question on the same topic. The non
> optimized implementation has the following code:
>
> public:
>  itkSetObjectMacro( MovingImageMask, MovingImageMaskType );
>  itkSetObjectMacro( FixedImageMask, FixedImageMaskType );
> protected:
>  mutable FixedImageMaskPointer   m_FixedImageMask;
>  mutable MovingImageMaskPointer  m_MovingImageMask;
>
> whereas the optimized code has:
>
> public:
>  itkSetConstObjectMacro( MovingImageMask, MovingImageMaskType );
>  itkSetConstObjectMacro( FixedImageMask, FixedImageMaskType );
> protected:
>  FixedImageMaskConstPointer  m_FixedImageMask;
>  MovingImageMaskConstPointer m_MovingImageMask;
>
>
> For the same reasons as previously, this discrepancy may be
> problematic. In this case, the code from the optimized implementation
> seems to be the correct one to me. I would therefore like to modify
> the non-optimized version to match the optimized one.
>
> However, I understand that this is not strictly backward compatible. I
> have just tested the attached patch that deprecates the mutable masks
> and it works like a charm on the ITK tree:
> http://www.cdash.org/CDash/buildSummary.php?buildid=405762
>
> Do you think such a patch is appropriate to move forward?
>
> Tom
>
>
> On Tue, Aug 18, 2009 at 19:50, Stephen
> Aylward<stephen.aylward at kitware.com> wrote:
>> Hi,
>>
>> Yup - no backward compatibility issues apply to code in Review.
>>
>> The change you've suggested seems appropriate to me.
>>
>> Good catch - thanks!
>>
>> Stephen
>>
>> On Tue, Aug 18, 2009 at 1:40 PM, Tom Vercauteren<tom.vercauteren at m4x.org> wrote:
>>> Hi,
>>>
>>> In the non-optimized case, ImageToImageMetric::SetFixedImageRegion is
>>> defined as follows (through a itkSetMacro):
>>>
>>>  void SetFixedImageRegion( const FixedImageRegionType reg );
>>>
>>> In the optimized case, it is manually defined as:
>>>
>>>  void SetFixedImageRegion( FixedImageRegionType reg );
>>>
>>>
>>> This causes some warning in msvc when a class inherits from
>>> ImageToImageMetric and needs to override SetFixedImageRegion:
>>>
>>>    virtual function overrides
>>> 'itk::ImageToImageMetric<TFixedImage,TMovingImage>::SetFixedImageRegion',
>>> previous versions of the compiler did not override when parameters
>>> only differed by const/volatile qualifiers
>>>
>>>
>>> Just to make sure: Since the optimized registration code is in review,
>>> there is no backward compatibility problem if I change the signature
>>> of SetFixedImageRegion to match the one from the non-optimized code,
>>> right?
>>>
>>> Also, I realized that doxygen gets confused by the itkSetMacro.
>>> Looking at ImageToImageMetric::SetFixedImageRegion it reports a
>>> non-const argument:
>>>
>>>  http://www.itk.org/Doxygen/html/classitk_1_1ImageToImageMetric.html#7d192f58b6309c49ee01e47755faff03
>>>
>>> However the doxygen version is based on the non-optimized code:
>>>
>>>  http://www.itk.org/Doxygen/html/itkImageToImageMetric_8h-source.html
>>>
>>> and correctly shows the const in the itkSetMacro definition:
>>>
>>>  http://www.itk.org/Doxygen/html/itkMacro_8h.html#7ebdd33cc5e7d74720ced9099c034faa
>>>
>>> Regards,
>>> Tom
>>> _______________________________________________
>>> Powered by www.kitware.com
>>>
>>> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>>>
>>> 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
>>>
>>
>>
>>
>> --
>> 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
>>
>



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