[Insight-developers] Rounding functions in itkMacro.h

[Luis Ibanez]

Hi Tom,

Thansk for looking at this.
Please see my comments interleaved below.



Tom Vercauteren wrote:
> Hi Luis,
> 
> Thanks for checking in the rounding code in itkMacro.h. I just had a
> look at it and I saw several weird things.
> 
> First of all, it just happens that my patch for vnl has been committed
> by Amitha Perera 3 days ago :)
>   http://vxl.svn.sourceforge.net/viewvc/vxl?view=rev&sortby=date&revision=25053
> It therefore seems like we could just update the corresponding pieces
> of vnl now and wait for the next vxl release to update the rest of it.
> 


This sounds good.
I'll take a look at what files we need to move in.
(hopefully we will not have a domino effect...)


> 
>  Now, regarding itkMacro.h, it seems that this implementation should
> either be mostly independent from vnl or simply be a wrapper around
> it. Currently it is neither of these options. Below are some of the
> things that IMHO require second thought:
> 
> 1) The code uses the following preprocessor defines:
>   VNL_CONFIG_ENABLE_SSE2_ROUNDING
>   GCC_USE_FAST_IMPL
>   VC_USE_FAST_IMPL
>   VNL_CHECK_FPU_ROUNDING_MODE
> These are not even provided by the vnl version that ships with ITK. We
> should either use a newer version of vnl or provide some equivalent of
> these within ITK proper.
> 


Would this be an issue when we bring up the fix from VXL ?
Isn't it the case that the newer VXL files will have these symbols ?


> 2) The code in itkMacro.h that provides the new rounding methods is
> put within a  ifdef ITK_USE_PORTABLE_ROUND block. I don't think we
> need or want this. It seems that all code should be able to use these
> new rounding functions. Actually what is the rationale for having this
> ITK_USE_PORTABLE_ROUND define? Can't we just replace all calls to
> vnl_math_rnd by itk::Math::RoundHalfIntegerUp or
> itk::Math::RoundHalfIntegerBackwardCompatibility to maintain strict
> backward compatibility?
> 


We put the CMake symbol as a way of being able to fix the proper places
withtout affecting other developers in the meantime. The variable is not
intended to stay there.

You may have noticed the 6 failing tests in Zion and Redwall in the
Dashboard. These machines have the ITK_USE_PORTABLE_ROUND ON, and these
are the remaining failing tests. Once we fix them, we can remove the
variable and adopt the new rounding.



> 3) The MSVC ASM implementation won't compile as is. It indeed has some
> leftovers from the patch, e.g.:
>   __asm {
> @@ -218,10 +228,7 @@
>   }


This is most likely my mistake.

This may go away as we adopt the new VXL files.


> 
> 4) itkMacro.h only provides round but not floor and ceil. I think we
> should have them (itk::Math::Ceil and itk::Math::Floor).
> 


I fully agree.
We were going one step at a time here...



> 5) The itk::Math::XXX functions are not macros. Wouldn't it be better
> to put them in there own file (e.g. itkMath.h)? This header might in
> turn be included by itkMacro.h for convenience reasons.
> 


Yes, the final destination should be a file itkMath.h.
We were avoiding to commit a new file until we were sure
that the new functions would do the trick.


> 
> 
> Even though http://vxl.svn.sourceforge.net/viewvc/vxl?view=rev&sortby=date&revision=25053
> now provides good floating point to integer function, I understand
> your worry about not necessarily relying on vnl if we don't have to.
> What I suggest to fulfill most requirements is the following:
> 
> A) Update
>   Utilities/vxl/core/vnl/vnl_math.h
>   Utilities/vxl/core/vnl/tests/test_math.cxx
>   Utilities/vxl/core/vnl/vnl_config.h.in
>   Utilities/vxl/core/vnl/CMakeLists.txt
> to get the most up-to-date floating point to integer functions from vnl


Thanks for the list.
This is indeed very useful.
I'll run an Experimental build with these updated files.

> 
> B) Modify itkMacro.h so that
>   itk::Math::Round be only a wrapper on vnl_math_rnd (version as in
> current vxl svn)
>   itk::Math::RoundHalfIntegerUp be only a wrapper on vnl_math_rnd_halfintup
>   itk::Math::RoundHalfIntegerToEven be only a wrapper on
> vnl_math_rnd_halfinttoeven
> 



I think we still should do this.
It will give us a layer of independence from VXL.
and will allow us to make modifications without having to wait
for updates in VXL.



> C) Expand itkMacro.h so that
>   itk::Math::Ceil be only a wrapper on vnl_math_ceil
>   itk::Math::Floor be only a wrapper on vnl_math_floor
> 


This is still desirable.
Again, probably to be put in a itkMath.h file.



> D) If we need to be able to use the exact (platform-inconsistent) same
> behavior of vnl_math_rnd as currently in ITK, put
>   itk::Math::Round
> within a ifdef block and put the current ITK's vnl_math_rnd code in
> there. Another option might be to had a new function
>   itk::Math::RoundHalfIntegerBackwardCompatibility
> 


I would think that consistency across platforms
is a requirement, and that the previous inconsistency
is a bug, and not a feature that we want to be backwards
compatible with.


> E) Get rid of  ITK_USE_PORTABLE_ROUND and replace all calls to
> vnl_math_rnd by calls to itk::Math::Round (or
> itk::Math::RoundHalfIntegerBackwardCompatibility)
> 


Yeap, this we should do before cutting the release.
As soon as we fix the remining six failing tests.


> F) Bug fix that require a well defined half-integer rounding scheme
> can now use itk::Math::RoundHalfIntegerUp or
> itk::Math::RoundHalfIntegerToEven
> 
> 


Yeap, this is how we got here.
Michel and I were working on your patch for Bug #6558
and Simon reminded us that this also required a consistent
Round function to be available.



> Thoughts?
> 
> Regards,
> Tom
>