[Insight-developers] Rigid3DTransform

[Hans Johnson]

Luis,

I am working on removing it from BRAINSTools.  It was never actually used,
and was just a copy and paste inclusion.

========
As you point out, there are several places where this was used improperly,
and at a minimum I'd like to be able to force compile time errors when
Rigid3DTransform was used improperly.

Hans


On 1/2/11 11:26 AM, "Luis Ibanez" <luis.ibanez at kitware.com> wrote:

> Hi Bill,
> 
> 
> The class itk::Rigid3DTransform  is used in Slicer in the following files:
> 
> 
> 1) Base/Logic/vtkSlicerTransformLogic.cxx
> 2) Applications/CLI/BRAINSTools/BRAINSCommonLib/GenericTransformImage.cxx
> 3) Applications/CLI/ResampleVolume2.cxx
> 4) Applications/CLI/DiffusionApplications/ResampleDTI/ResampleDTI.cxx
> 5) 
> Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTr
> ansform.txx
> 6) 
> Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTr
> ansform.h
> 
> 
> 
> The uses in (1) and (2) are now obsolete.
> 
> They were registering all the Transforms factories in order to
> deal with the absence of the ScaleVersor3DTransform in ITK 3.18.
> Now that this transform is in ITK 3.20 (and ITKv4) the factory
> registration is no longer needed.
> 
> So, lines 170-232 of  (1)
> Base/Logic/vtkSlicerTransformLogic.cxx
> should be removed.
> 
> The same goes for lines 410-476 in (2)
> Applications/CLI/BRAINSTools/BRAINSCommonLib/GenericTransformImage.cxx
> 
> 
> -----
> 
> The uses in (3) lines 890-925 is actually an improper use of
> polymorphism. The code is using dynamic_casts to uncover
> the type of a derived class, just to call on it a function that was
> virtual in the first place. This one actually calls for a refactoring
> of the GetInverse() method in ITK. We should discuss that one
> with Brian Avants as part of the Image Registration refactoring
> effort.
> 
> 
> The use in (3) lines 228 can be replaced with MatrixOffsetTransformBase,
> since, the code is using only the SetMatrix() and SetTranslation() methods
> that are actually defined in this base class.
> 
> 
> The use in (3) 287 and 318 can also be replaced with
> MatrixOffsetTransformBase. It is actually casting the rigid tranform
> to that base class anyways in line 323.
> 
> Same goes for the uses in (3) in lines 328 and 330.
> 
> The declaration in (3) line 501 is not used at all,
> so it should just be deleted.
> 
> ----
> 
> The uses in (4)
> Applications/CLI/DiffusionApplications/ResampleDTI/ResampleDTI.cxx
> 
> The use in ines 471, does not really requires instantiation with New()
> The code is using a dynamic_cast in order to discover the type
> of an abstract pointer, in order to assign the same pointer to a
> MatrixOffsetTransformBase in line 478, and register a code "rt"
> in line 476. The check could have been done with a raw pointer.
> 
> The same applies in 483-491.
> 
> ----
> 
> The uses in (5,6)
> 
> 5) 
> Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTr
> ansform.txx
> 6) 
> Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTr
> ansform.h
> 
> The file (6) only has declarations,
> The file (5) uses the type in:
> 
> Lines 47-56, where it uses the transform as a MatrixOffsetTransformBase.
> That is, it only uses the SetMatrix(), SetTranslation() API.
> 
> In lines 32-41, the transform is also used just as a
> MatrixOffsetTransformBase.
> 
> So, essentially, the uses of Rigid3DTransform in (5,6) can be replaced
> with MatrixOffsetTransformBase.
> 
> 
> ------
> 
> I'll be happy to help convert all the code above.
> 
> ------
> 
> However,
> the only unsolved case is:
> 
>                                       IO
> 
> since the transform writers put the class names in the files *AND*
> we do not use a file format version (which is the real problem)...
> 
> Then,
> previously generated files, that contain the code "Rigid3DTransform"
> can't be read with ITKv4.
> 
> The lesson to learn there is that we should introduce versioning
> in all the file formats over which we have some control.
> 
> ----
> 
> Of course,
> it is easier to restore the NewMacro in Rigid3DTransform,
> but then...
> 
> We have to find an alternative way of preventing new users
> from instantiating that transform and trying to use it in their
> first registration example.
> 
> 
> Any suggestions ?
> 
> - Throwing exceptions when they call SetParameters ?
> - Displaying warnings ?
> 
> 
> 
>      Luis
> 
> 
> ------------------------------------------------------------------
> On Sat, Jan 1, 2011 at 12:23 PM, Bill Lorensen <bill.lorensen at gmail.com>
> wrote:
>> Luis,
>> 
>> As part of our Slicer4/ITK4 experiment, there are a number of compile
>> errors because Slicer4(3) instantiate Rigid3dTransform.
>> 
>> Recall that we removes the itkNewMacro for the class in response to
>> bug 8320: http://public.kitware.com/Bug/view.php?id=8320
>> 
>> The corresponding gerrit topic is here:
>> http://review.source.kitware.com/#change,504
>> 
>> I believe that Slicer4(3)'s use of Rigid3DTransform is valid.
>> Slicer4(3) have saved transforms that are part of the test suite. Note
>> that in your comments on the bug tracker you state:
>> "It doesn't hurt to have a valid implementation of Get/SetParamters at
>> the level of the Rigid3DTransform. However, this class simply
>> shouldn't be used as a full fledged transform. (e.g. for example as
>> part of an image registration framework). "
>> 
>> Note the word "shouldn't". Well, Slicer4(3) did.
>> 
>> I suggest that we restore the itkNewMacro and resolve bug 8320 another
>> way. I think it is hard to justify such an API change in light of a
>> valid use in Slicer4(3).
>> 
>> Bill
>> 

-- 
Hans J. Johnson, Ph.D.
Assistant Professor
200 Hawkins Drive
T205 BT, The University of Iowa
Iowa City, IA 52242

hans-johnson at uiowa.edu
PHONE: 319 353 8587