[Insight-developers] Optimized ImageToImageMetric::SetFixedImageRegion signature

Wed Aug 19 11:28:28 EDT 2009

Hi Brad,

To follow up on Stephen's reply.

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

I agree but this basically implies changing itkSetMacro to use a const
reference. This will change the public API of all ITK classes. This is
not backward compatible as any user-defined derived class may now hide
instead of override the functions defined by a itkSetMacro.

For things in Review (as with the patch I applied), this is not a
problem. But for ITK proper, we need to use the deprecation mechanism.
I don't think it's worth changing itkSetMacro this in ITK 3.x. I'll
add it the the ITK 4 wishlist.

For the second patch I am proposing, I think it's worth deprecating
the volatile masks by using itkSetConstObjectMacro instead of
itkSetObjectMacro because the volatile masks break const-corectness.

Tom

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