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

[Luis Ibanez]

Hi Tom,


Backward Compatibility Alert!
============================

Adding new methods must be done with careful consideration.
If these new methods made it to a release (ITK 3.14 in May
for example), then we are condemned to maintain them *forever*,
even if we discover later that they were not the best approach.


-----

The fact that we are having to take workarounds for the
API deficiencies of the original GetInverse() method
(e.g. like adding a new method name, and presumably
deprecating the original GetInverse() method) is a sign
that the original design may not be worth maintaining.


What is the motivation for insisting in the method with
signature

       BaseTransform GetInverse(void)    ?

instead of turning to the more consistent API:

       bool GetInverse( BaseTransform::Pointer inverse );


If we are going to rename the GetInverse() method as
GetInverseTransform() anyways, then we are essentially
deprecating the "TransfromBase * GetInverse(void)" method.

If we are going to deprecate this method, it will be more
valuable to adopt an API that prevents the ambiguities of
the previous GetInverse() method.

Overall, the main concern is that we will end up writing
applications where a lot of introspection is needed in order
to manage the inverse Transforms properly.


I will suggest that we provide the methods:


virtual bool GetInverse( TransfromBase * inverse) const;
         bool GetInverse( InverseType   * inverse) const;


                   and not the method:

virtual InverseTransformBasePointer GetInverseTransform() const;



     Luis



----------------------
Tom Vercauteren wrote:
> 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.
>>>>>
>>>>>
>>>>
>