[Insight-developers] GetInverse is unusable -- patch attached

[Tom Vercauteren]

Luis and Paul,

Sorry if I wasn't clear there.

What I did was:

1) Take Pavel's patch for ITK 3.12 (i.e. implementation of "virtual
Base::Pointer GetInverse(void)", were I only put "void" to explicitly
say it does take an argument).

2) Extend it so that all transforms that already had either a
"GetInverse(Self*)" or a "CloneInverseTo(Pointer &)" method now also
have a "GetInverse(void)" method

3) Added a "GetInverse(Self*)" method for classes that did not have it
but whose parent had one.

4) Extended all unit tests that were checking or simply calling either
"GetInverse(Self*)" or "CloneInverseTo(Pointer &)" to also check (or
call respectively) "GetInverse(void)"


What might seem confusing is step 3. I did it for three reasons:

A) My compiler refuses to compile "d->GetInverse(invd)" if
       - "d" and "invd" are of type D
       - D inherits from B
       - D does not redefine the non-virtual method "GetInverse(Self*)"
       - "GetInverse(Self*)" is of course defined in the base class B
       - The virtual function "GetInverse(void)" is of course defined
in the base class B and possibly in D

B) Defining "GetInverse(Self*)" in thies derived class D allowed me to
avoid code duplication when implementing a "GetInverse(void)" the
returns a B::Pointer that actually points to a concrete object of the
derived type D

C) Having "GetInverse(Self*)" in more classes makes ITK more coherent
and adresses bug http://www.itk.org/Bug/view.php?id=3359


Hope this helps,
Tom

On Wed, Apr 8, 2009 at 18:53, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
> Paul,
>
> Thanks for the clarification...
>
> It seems that were are mixing patches here.
>
> My comments relate to the file
>
> file icon itk-getinverse.patch
>
> that Tom attached to bug:
> http://www.kwwidgets.org/Bug/bug_view_advanced_page.php?bug_id=7876
>
>
> Tom:
>
>  Where did you take this patch file from ?
>
>  Is it supposed to be equivalent to the one that Paul just posted ?
>
>
>
> Paul:
>
>  Regarding the objection. I failed to indicate that the objection
>  is not about the Self type but about the use of a raw pointer.
>  I'm suggesting to use a SmartPointer as argument so that we can
>  allocate (construct) the transform if necessary.
>
>  I agree with you that the "Self" type should be replaced with a
>  TransformBase type.
>
>
>
> ---
>
>
>
> Tom,
>
> let's make sure that we take the proper version of the patch...
>
>
>
>   Luis
>
>
>
>
> --------------------
> Paul Koshevoy wrote:
>>
>> Luis Ibanez wrote:
>>
>>> Hi Tom,
>>>
>>> Here are some observations about the patch: itk-getinverse.patch
>>>
>>> ----
>>>
>>> In line 184:
>>>
>>> CenteredRigid2DTransform<TScalarType>::
>>> +GetInverse( Self* inverse) const
>>> +{
>>> +  if(!inverse)
>>> +    {
>>> +    return false;
>>> +    }
>>>
>>>
>>> It is tempting to use a SmartPointer as argument
>>> so that we can do:
>>>
>>> +GetInverse( Self::Pointer inverse) const
>>> +{
>>> +  if( inverse.IsNull() )
>>> +    {
>>> +    inverse = Self::New();
>>> +    }
>>>
>>>
>>
>>
>>
>> Hi Luis,
>>
>> It seems the code you are objecting to is the existing ITK code.  Passing
>> a Self pointer (smart or not) restricts the inverse transform type to be the
>> same type as the forward transform.  That is broken, because only a few
>> transforms have an analytic inverse of the same type.  This is not what my
>> patch does, although I have followed the style and implemented GetInverse()
>> inline in the .h file.
>>
>> My current patch doesn't have the changes you are referring to:
>>
>> https://code.sci.utah.edu/svn/ImageReconstruction/trunk/code/itk-patch/InsightToolkit-3.12.0-GetInverse.patch
>>
>>   Pavel.
>>
>>
>