[Insight-developers] Rounding functions in itkMacro.h

[Tom Vercauteren]

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.


 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.

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?

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 @@
  }

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

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.



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

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

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

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

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)

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


Thoughts?

Regards,
Tom