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

[Tom Vercauteren]

Thanks Luis for the prompt reply and your comments.


> 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();
> +    }

I don't think I can change this since GetInverse( Self* inverse) has
been in itk::Transform for ages. This looks like a non-backward
compatible change.


> Regarding form: The implementation of methods
> should go into the .txx files, not the .h files.
> I know that many ITK files don't follow this rule, but
> as we go about the toolkit we should clean what we can.

Ok, I'll change that.


> The major issue:
>
>      Some of the  tests exercise the methods but do not
>      verify the value of the expected inverse transforms.
>
>      We should insert regression  testing in all of them
>     (numerical   verification).
>
>      Otherwise we will pay for this later...

I do agree with that. Right now, this patch simply extends the
existing tests of the other inversion methods. It would definitely be
nice to enhance the coverage of these tests. I'll see if I can add
some easy real tests in there.


> Please feel free to go ahead with the patch.
> Let us know as soon as you committed, so that we
> can track issues in any other platforms.

Thanks, I'll see if I have time to do it tonight. Otherwise, I'll try tomorrow.

Tom

>
>
>       Thanks
>
>
>             Luis
>
>
> ------------------------------------------------------------------------------
> On Wed, Apr 8, 2009 at 9:00 AM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>> Hi Tom,
>>
>> Does the patch includes new classes ?
>>
>> If so, the new classes will have to go through the Insight Journal.
>>
>> Let me look at the patch from the file that was attached to bug 7876.
>> I'll get back to you soon.
>>
>>
>>    Luis
>>
>>
>> ---------------------------------------------------------------------
>> On Wed, Apr 8, 2009 at 8:53 AM, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
>>> Hi Luis,
>>>
>>> As pointed out by Pavel, the current GetInverse function in
>>> itk::Transform is very limited. He provided an initial backward
>>> compatible patch which is in the bug tracker
>>>  http://www.kwwidgets.org/Bug/bug_view_advanced_page.php?bug_id=7876
>>> and discussed on this wiki page:
>>>  http://www.itk.org/Wiki/Proposals:InverseTransform
>>>
>>> I'd like to "adopt this bug". As a matter of fact, I just spent some
>>> time working on this issue and have a new patch that extends the one
>>> from Pavel. All unit tests that were using either
>>>  GetInverse(Self *)
>>> or
>>>  CloneInverseTo(Pointer &)
>>> now also cover
>>>  GetInverse()
>>>
>>> This means that most (if not all) transforms that had one of those
>>> inversion methods or were depending on an inversion method in their
>>> superclass now have a working GetInverse() method.
>>>
>>> Since no new class need to be added and since two independent
>>> developers have been working on this, I don't think it needs to go
>>> through the IJ paper step.
>>>
>>> May I just go and commit the enhanced patch?
>>>
>>> Tom
>>>
>>> P.S.: The new patch is too large to be attached here. I have attached
>>> it to bug 7876
>>> http://www.itk.org/Bug/view.php?id=7876
>>>
>>>
>>> On Tue, Apr 7, 2009 at 20:19, Paul Koshevoy <koshevoy at sci.utah.edu> wrote:
>>>> Paul Koshevoy wrote:
>>>>>
>>>>> Hi Tom,
>>>>>
>>>>> It's been 2 years since I left the SCI Institute at the University of
>>>>> Utah.  I have a full time job at Sorenson Media, and occasional consulting
>>>>> for SCI Institute CRCNS group, maintaining the ir-tools, ir-tweak,
>>>>> ir-mosaic, etc... that I've created for them years ago.  I can't bring
>>>>> myself to spend the time on writing a technical report to justify these
>>>>> simple changes, as well as coming up with test cases and sample datasets.
>>>>>  Patching ITK with each new release is so much easier -- it's just adding
>>>>> 5-10 to 5 ITK transforms.
>>>>>
>>>>
>>>> I meant to say it's just 5-10 lines added to just a few ITK files --
>>>>
>>>> itkIdentityTransform.h
>>>> itkMatrixOffsetTransformBase.h
>>>> itkScaleTransform.h
>>>> itkTransform.h
>>>> itkTranslationTransform.h
>>>>
>>>>
>>>>
>>>>
>>>>> What I am saying is that to contribute these changes to the ITK I have to
>>>>> get over a significant (artificial) barrier.  I don't feel right spending my
>>>>> own time on this, and I can't charge the time it would take me to do this to
>>>>> SCI/CRCNS because that is not a priority for them.
>>>>>
>>>>> I have the latest patch, as well as the fully patched files here:
>>>>> https://code.sci.utah.edu/svn/ImageReconstruction/trunk/code/itk-patch/
>>>>> Their SSL cert expired last weekend, so you may need to add a security
>>>>> exception to be able to view the page.
>>>>>
>>>>>   Pavel.
>>>>>
>>>>
>>>>
>>>
>>
>