[Insight-developers] Rigid3DTransform

[Bill Lorensen]

The converted code should work with both ITK3.x and ITK4. Ifdef's are
OK, the preference would be to have changes that work with both
versions. For example, OStringStream replacements can use
std:;stringstream. Works on both ITK3.x and ITK4.

I am still not convinced that Rigid3DTransform should be abstract. It
can be used in a valid way. granted, it may have other problems that
need to be fixed.


On Sun, Jan 2, 2011 at 12:26 PM, 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/itkDiffusionTensor3DRigidTransform.txx
> 6) Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTransform.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/itkDiffusionTensor3DRigidTransform.txx
> 6) Applications/CLI/DiffusionApplications/ResampleDTI/itkDiffusionTensor3DRigidTransform.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
>>
>