[Insight-developers] GetInverse is unusable -- patch attached
Tom Vercauteren
tom.vercauteren at m4x.org
Thu Apr 9 09:54:45 EDT 2009
Hi Luis,
Thanks a lot for your help and feedback.
The current patch is definitively backward compatible. What has been
in ITK is only
bool GetInverse( Self * inverse) const;
which cannot be made virtual because Self is obviously not the same
type in all transforms.
Your proposal to implement
virtual bool GetInverse( TransfromBase * inverse) const;
bool GetInverse( InverseType * inverse) const;
is not practical because of the hiding rule
http://www.parashift.com/c++-faq-lite/strange-inheritance.html#faq-23.9
The easiest thing to do to get pass the hiding rule is to have
different names depending on the signature so your proposition would
become to implement
virtual bool GetInverseTransform( TransfromBase * inverse) const;
bool GetInverse( InverseType * inverse) const;
Your other proposition, which is to implement
bool GetInverse( BaseTransform::Pointer inverse );
has both the hiding issue and is a confusing since inheritance is not
understood when a smart pointer is used. So this won't work:
InverseType::Pointer inverse = InverseType::New();
bool ok = transform->GetInverse(inverse);
The user will have to call
InverseType::Pointer inverse = InverseType::New();
bool ok = transform->GetInverse(inverse.GetPointer());
Which means that a raw pointer could have been used in the first place.
So we are left with only two options, both maintain
bool GetInverse( Self * inverse) const;
as is for obvious backward compatible reasons
Option A) implement
virtual InverseTransformBasePointer GetInverseTransform() const;
Option B) implement
virtual bool GetInverseTransform( TransfromBase * inverse) const;
I don't see any issue with option A) but I see several issues with option B)
Let's refer to
virtual InverseTransformBasePointer GetInverseTransform() const;
as
GetInverseTransformA
and
virtual bool GetInverseTransform( TransfromBase * inverse) const;
as
GetInverseTransformB
Even though GetInverseTransformB take a TransfromBase * as argument
the caller has to know what concrete type is required by
GetInverseTransformB. Indeed inverse cannot be reallocated and a
dynamic_cast is required in GetInverseTransformB.
So this won't work:
TransformBase::Pointer inverse = TransfromBase::New();
bool ok = transform->GetInverseTransformB(inverse);
This is a real problem if you want to be generic. On the other hand
this will work smoothly:
TransformBase::Pointer inverse = transform->GetInverseTransform();
if( !inverse.isNull() ) inverse->TransformPoint(point);
Let me know if this clarifies things or if I need to better advocate my option.
Cheers,
Tom
On Thu, Apr 9, 2009 at 15:09, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
> 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.
>>>>>>
>>>>>>
>>>>>
>>
>
More information about the Insight-developers
mailing list