[Insight-developers] Rounding functions in itkMacro.h

[Tom Vercauteren]

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