[Insight-developers] Rounding functions in itkMacro.h

[Luis Ibanez]

Hi Tom,


Thanks for the detailed instructions

following your guidance,
I have patched the following files:

* vxl/core/vnl/CMakeLists.txt
* vxl/core/vnl/vnl_config.h.in
* vxl/core/vnl/vnl_math.h
* vxl/core/vnl/tests/test_math.cxx


I'm running an Experimental build,
and if things go well, I'll be committing
these changes after the repository closes
at 9pm. In this way we will see the effects
on the first continuous builds tomorrow.

At that point, we could take the new
functions out of the itkMacro.h file
and move them into a file

             itkMath.h

To be put in Code/Common.



     Thanks


         Luis



-------------------
Tom Vercauteren wrote:
> Hi Luis,
> 
> I didn' t understand that ITK_USE_PORTABLE_ROUND was meant to be
> temporary. It makes more sense now!
> 
> Below are some comments.
> 
> 
>>>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...)
> 
> 
> The difference should be rather minimal.:
> - Utilities/vxl/core/vnl/vnl_math.h can be replaced by its vxl svn counterpart
> - Utilities/vxl/core/vnl/tests/test_math.cxx can be replaced by its
> vxl svn counterpart
> - Utilities/vxl/core/vnl/vnl_config.h.in can be replaced by its vxl
> svn counterpart
> - Utilities/vxl/core/vnl/CMakeLists.txt needs to be modified. We needs
> to be taken from its vxl  svn counterpart is everything that touches
> VNL_CONFIG_ENABLE_SSE2_ROUNDING
> 
> 
> 
>>>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 ?
> 
> 
> This won't be an issue once Utilities/vxl/core/vnl/vnl_config.h.in and
> Utilities/vxl/core/vnl/CMakeLists.txt are patched.
> 
> 
> 
>>>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.
> 
> 
> Ok, I get it now. This makes sense.
> 
> 
> 
>>>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...
> 
> 
> Ok, got it.
> 
> 
> 
>>>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.
> 
> 
> Again it makes sense now that I understand it is a progressive approach :)
> 
> 
> 
>>>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.
> 
> 
> This indeed seems a reasonable approach.
> 
> 
> 
>>>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.
> 
> 
> Ok, thanks,
> 
> 
> 
>>>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.
> 
> 
> I do agree.
> 
> 
> 
>>>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.
> 
> 
> Great!
> 
> 
> 
>>>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.
> 
> 
> Thanks again for looking at this!
> 
> Tom
>