[Insight-developers] Bug 6558: pixel coordinates

[Tom Vercauteren]

Hi Luis,

This really is good news!

I'll try to recap the information I have about this issue. I didn't
really commit any changes w.r.t. to that bug but proposed a few
patches. The plan I had in mind is the following:


Step 1:
----------
We need a good real to integer rounding function that meets our needs.
The consensus we arrived at is that we need
- a correct, testable and tested function
- half-integers needs to be rounded consistently across platforms -
round up would apparently be OK for everyone
- it needs to be relatively fast

Note the current implementation of vnl_math_round in ITK does not meet
any of these requirements, cf. for example the hidden, not directly
testable, workaround used here:
  http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkIndex.h?root=Insight&r1=1.56&r2=1.57

Plan A to solve step 1 is to add a new vnl_math_rnd_halfintup to vnl.
A patch for that is already available on VXL's bug tracker:
  https://apps.sourceforge.net/trac/vxl/ticket/23
It is however waiting for the approval of Amitha Perera. Since Amitha
is at Kitware, maybe Michel could go and check with him if anything
more is required to commit my patch. I don't feel like bugging him any
more myself since I already spent a lot of time pushing for this on
the VXL's mailing list :)

Plan B to solve step 1 is to move the code from
https://apps.sourceforge.net/trac/vxl/ticket/23 within ITK proper.
This really doesn't seem as nice to me as it would duplicate the
functionalities in vnl_math.h but it would be doable.


Step 2:
----------
The new function (let's call it vnl_math_rnd_halfintup) needs to be
used in ITK. This means replacing all calls to  vnl_math_rnd by
vnl_math_rnd_halfintup and removing all hidden hacks used for real to
integer rounding/ceiling/flooring:
  http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkIndex.h?root=Insight&r1=1.56&r2=1.57
and
  http://www.itk.org/Bug/view.php?id=2078
but there might be others


Step 3:
----------
Decide whether http://public.kitware.com/Bug/view.php?id=6558 is
considered as a simple bug that needs patching or if a backward
compatible solution is necessary. As this will potentially have an
impact on every registration result, I suppose that we need a backward
compatibility scheme. Given the importance of this issue I think that
adding a new CMake variable for that makes sense.

If a cmake variable is added, we also need to decide what its default
value is. I would vote for switching to the correct behavior by
default.


Step 4:
----------
Fix the core of ITK, this should be achieved by the patch I attached
to ITK's bug tracker:
  http://www.itk.org/Bug/file_download.php?file_id=1876&type=bug
Note that in that patch also, vnl_math_rnd_halfintup needs to be used
instead of vnl_math_rnd


Step 5:
----------
Fix the code that uses the patched world <-> physical space
transformation function and bound checking. This would at least
require to modify the interpolators to correctly handle the border
cases.

You might want to check with Simon Warfield for that. He told me he
already had an initial patch to include bounds checking inside the
interpolator.


Step 6:
----------
Enhance code coverage to make sure that the patch works correctly with
oriented images for example


I guess that's it. Note that steps 3 to 5 can be done in parallel with
step 1 if step 2 is performed between step 5 and 6.

Hope this helps,
Tom


On Mon, Apr 20, 2009 at 22:44, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> Hi Tom,
>
> Michel is going to take a look at what can be done with this bug:
> http://public.kitware.com/Bug/view.php?id=6558
>
> From the notes, it seems that you have already
> committed some changes.
>
> Could you please remind us of what needs to be
> done ?
>
>    Thanks
>
>
>         Luis
>