[Insight-developers] Optimized ImageToImageMetric::SetFixedImageRegion signature

Wed Aug 19 10:44:38 EDT 2009

Hello,

	Ideally this method should be:

void SetFixedImageRegion( const FixedImageRegionType &reg);

Please note the reference. As this is  not a pointer type, so it  
prevents an unnecessary copy of a complex type. I actually don't even  
see a set macro for this case though.

To:
>>>>  void SetFixedImageRegion( const FixedImageRegionType reg );
From:
>
>>>>  void SetFixedImageRegion( FixedImageRegionType reg );

Stephen,

As for backwards compatibility, I fail to see any potential issues  
with the applied patch. How would such a change not be compatible?!?

On Aug 19, 2009, at 9:43 AM, Stephen Aylward wrote:

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

========================================================
Bradley Lowekamp
Lockheed Martin Contractor for
Office of High Performance Computing and Communications
National Library of Medicine
blowekamp at mail.nih.gov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090819/27bf72a6/attachment.htm>