[Insight-developers] Bug 6558. PhysicalPoint to Index consisten conversions

[Luis Ibanez]

Simon,

Thanks a lot for the feedback.

1) Fully agree about the use of round functions.

    From Tom's email of April 21:
http://www.itk.org/mailman/private/insight-developers/2009-April/012341.html

    It seems that the most reliable option will be
    to create our own ITK rounding halfintup method.
    It is true that it would duplicate the functionality
    of VNL, but it just doesn't make sense to have ITK
    stuck depending on a small change in a third party
    library for 100 lines of code that we can write
    ourselves.


2) Agree that we should fix the other interpolators.


3) About the new method IsStrictlyInside(), we struggled
    with this one.

    A central part of the patch is to have

                IsInside( ContinuousIndex )

    return true for indices located at a band of
    0.5 pixels wide all around the image grid.

    However, in certain classes, when IsInside() is called,
    (e.g. BSplines) the expectation is really that a point
    in that external band will be considered to be outside
    of the image grid. In that particular code, IsInside()
    is used to compute the support of a BSpline in the
    BSpline grid (which is itself represented as an image).


    Alternative options could be:


      A.1) Add a second parameter to IsInside()
           maybe something like

         IsInside( ContinuousIndex, Width )

         where Width will have a default of 0.5,
         but uses (such as the BSpline) could invoke it
         with a band-width of Zero.


      A.2) Use a clearer name in replacement of

                IsStrictlyInside()

           ...  Suggestions ?




    Thanks,


       Luis



---------------------
Simon Warfield wrote:
> insight-users-request at itk.org wrote:
> 
>> Date: Tue, 5 May 2009 17:57:50 -0400
>> From: Michel Audette <michel.audette at kitware.com>
>> Subject: [Insight-users] Significant changes coming to address bug
>>         6558
>> To: insight-developers at itk.org, insight-users at itk.org
>> Message-ID:
>>         <b16f21b00905051457v68e2a634hbe83fa20a293c07f at mail.gmail.com>
>> Content-Type: text/plain; charset="iso-8859-1"
>>
>> Dear members of the Insight community,
>>
>> we are currently at work to solve bug 6558: Physical coordinates of a 
>> pixel
>> - Severe inconsistency and bug in ImageBase. We've found that the
>> implementation of pixel-centered positions had unintended and adverse
>> effects on other classes, which we have also been correcting for the past
>> two days. This fix appears to be a priority of many members of the
>> community, so we feel it's important to provide the required 
>> functionality
>> integrally.
>>
>> We will make available a patch to the Insight community to apply and 
>> comment
>> on.
>> This patch can be found at: 
>> http://public.kitware.com/Bug/view.php?id=6558.
>> Feel free to comment on its validity either by email or in the SecondLife
>> discussions on Friday afternoons.
>>   
> 
> 
> Dear Michel,
> 
>  Thanks for working on correcting and improving the representation of 
> physical coordinates in ITK.
> 
>  The patch you reference has several good additions to the previous 
> work.  I want to note a few potential problems:
> 1. It references vnl_math_round( ) and vnl_math_rnd()  . You should 
> consistently use one rounding implementation, and it should be the one 
> Tom suggested.
> 
> 2. The patch has a fix for 
> Code/Common/itkLinearInterpolateImageFunction.txx, the same fix needs to 
> be applied to the VectorLinearInterpolateImageFunction.txx file,
>       but in fact with some straightforward modifications, the 
> VectorLinearInterpolateImageFunction implementation could be an empty 
> shell that delegates to the regular 
> itkLinearInterpolateImageFunction.txx  . See the earlier email from Tom 
> on the list about that too.
>      You might also notice that all of the interpolators that have an 
> interpolation window size > 0 can make invalid accesses, by deliberate 
> design. If you are fixing some of the interpolators, you might consider 
> to fix the others also.
> 
> 3. The patch introduces a new concept IsStrictlyInside() . I am not sure 
> what it is for.  I suggest to have IsInside() actually test whether or 
> not a point is exactly inside the region, and I suggest to not introduce 
> IsStrictlyInside().
> 
>> We plan on making the fix permanent in the upcoming release.
>>
>> Best wishes,
>>
>> Michel
>>
>> -- 
>> Michel Audette, Ph.D.
>> R & D Engineer,
>> Kitware Inc.,
>> Chapel Hill, N.C.
>>   
> 
>