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

[Tom Vercauteren]

Oops, sorry for being so noisy. I feel a bit stupid not to have
thought about it before.

The easiest solution to workaround the "hiding rule" would be to
choose a different name for
  GetInverse(void)

GetInverse seems so natural, I can't really find a better name though.
What about one of the following?
  - GetInverseTransform
  - CreateInverse

Cheers,
Tom


On Wed, Apr 8, 2009 at 19:53, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
> To add some information about my reason A to do the step 3 below. I
> believe it is related to the so-called hiding rule:
> http://www.parashift.com/c++-faq-lite/strange-inheritance.html#faq-23.9
>
> Another option besides implementing "GetInverse(Self*)" in the derived
> class would have been to add
>  using Superclass::GetInverse;
> in the header of the derived classes.
>
> Since I already gave Luis a hard time with my previous commits that
> broke either MSVC or Sun CC builds. I am not sure I am ready to commit
> "advanced" c++ code such as this "using Superclass::GetInverse" :)
>
> Tom
>
>
>
> On Wed, Apr 8, 2009 at 19:22, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
>> 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.
>>>>
>>>>
>>>
>>
>