[Insight-developers] Portable round - Is it still time to commit?

Mon May 25 08:41:28 EDT 2009

Hi Luis and Bill,

Thanks for the feedback. I most certainly understand that it can be
too late to commit, which is why I was asking.

Just regarding the ITK_USE_PORTABLE_ROUND flag, I really don't get how
it can modify the behavior of the code.The only place where it is
actually used is in itkIndex and itkMacro. Let me know if I am missing
something.

Here is how ITK_USE_PORTABLE_ROUND is used in itkMacro,
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkMacro.h?root=Insight&sortby=date&view=markup:

#ifdef ITK_USE_PORTABLE_ROUND
#define itkForLoopRoundingAndAssignmentMacro(DestinationType,Sourcrnd_halfintup,DestinationElementType,DestinationArray,SourceArray,NumberOfIterations)
\
    for(unsigned int i=0;i < NumberOfIterations; ++i) \
      { \
      DestinationArray[i] = static_cast< DestinationElementType >(
itk::Math::Round( SourceArray[i] ) ); \
      }
#else
#define itkForLoopRoundingAndAssignmentMacro(DestinationType,SourceType,DestinationElementType,DestinationArray,SourceArray,NumberOfIterations)
\
    for(unsigned int i=0;i < NumberOfIterations; ++i) \
      { \
      DestinationArray[i] = static_cast< DestinationElementType >(
itk::Math::RoundHalfIntegerUp( SourceArray[i] ) ); \
      }
#endif

However, from the same itkMacro we have:

inline int Round(float   x) { return RoundHalfIntegerToEven(x); }
inline int Round(double  x) { return RoundHalfIntegerToEven(x); }

This implies that in this file, the flag has no visible effect.

Here is how ITK_USE_PORTABLE_ROUND is used in itkIndex,
http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkIndex.h?root=Insight&sortby=date&view=markup:

#ifdef ITK_USE_PORTABLE_ROUND
      m_Index[i] = static_cast< IndexValueType>( itk::Math::Round( point[i] ) );
#else
      m_Index[i] = static_cast< IndexValueType>(
vnl_math_rnd_halfintup( point[i] ) );
#endif

However, from itkMacro we also have:

inline int RoundHalfIntegerUp(float   x) { return vnl_math_rnd_halfintup(x); }
inline int RoundHalfIntegerUp(double  x) { return vnl_math_rnd_halfintup(x); }
inline int Round(float   x) { return RoundHalfIntegerToEven(x); }
inline int Round(double  x) { return RoundHalfIntegerToEven(x); }

This means that in this file also the flag has no visible effect.

The only thing that ITK_USE_PORTABLE_ROUND will change is what
vnl_math_rnd_halfintup does on some platforms. Indeed, in itkIndex,
for some reason vnl_math_rnd_halfintup is made platform-dependent if
ITK_USE_PORTABLE_ROUND is undefined. Indeed, the following hack for
vnl_math_rnd was not removed when moving from a platform-dependent
vnl_math_rnd to a platform-consistent  vnl_math_rnd_halfintup but was
wrongly "ported" to vnl_math_rnd_halfintup:

#ifndef ITK_USE_PORTABLE_ROUND
  // The Windows implementaton of vnl_math_rnd() does not round the
  // same way as other versions. It has an assembly "fast" implementation
  // but with the drawback of rounding to the closest even number.
  // See: http://www.musicdsp.org/showone.php?id=170
  // For example 0.5 is rounded down to 0.0.
  // This conditional code replaces the standard vnl implementation that uses
  // assembler code. The code below will be slower for windows but will
  // produce consistent results. This can be removed once vnl_math_rnd is
  // fixed in VXL.
#if (defined (VCL_VC) && !defined(__GCCXML__)) || (defined(_MSC_VER)
&& (_MSC_VER <= 1310))
#define vnl_math_rnd_halfintup(x) ((x>=0.0)?(int)(x + 0.5):(int)(x - 0.5))
#endif
#endif

This basically means that on some windows machines,
vnl_math_rnd_halfintup does rounding with half-integers being rounded
away from zero instead of being rounded upwards.

This is why I still vote for removing ITK_USE_PORTABLE_ROUND before
the 3.14 release. My other two points are less important and even I
wouldn't necessarily vote for fixing them before the release.

Hope this clarifies things a little bit.

Tom

On Mon, May 25, 2009 at 13:55, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
> Hi Tom,
>
>
> Thanks for looking at this.
> Please see my comments interleaved.
>
>
>
>     Luis
>
>
> ---------------------
> Tom Vercauteren wrote:
>>
>> Hi all,
>>
>> It seems I have been away at a critical time. Before leaving, I
>> thought I would have some time at my return (today) to commit a few
>> patches. However apparently the deadline for 3.14 is today...
>>
>> There are a few things that bother me in the current state of things
>> with respect to the new rounding functions:
>>
>> 1) The code still relies on ITK_USE_PORTABLE_ROUND. As mentioned by
>> Luis, this variable was intended to be temporary. I really think that
>> the release should not have this variable. The patch for that is
>> straightforward since  ITK_USE_PORTABLE_ROUND is only used in a few
>> files and does not change the behavior of the code at all.
>>
>
>
>
>    Well,
>    the flag does change the behavior of the code.
>    Otherwise there will be no point on it.
>
>    Please note that most builds in the dashboard are using
>    ITK_USE_PORTABLE_ROUND  OFF
>
>    We have not tested this flag ON extensively, but we have
>    managed to reduce the number of failing test related to it.
>
>
>
>> 2) The current implementation of rounding on gcc (when sse2 is not
>> available) works only on x86. However the code checks that it is not
>> compiling for ppc nor for ppc64. As discussed with Sean McBride, It
>> would make more sense to really check that we are on a a x86 platform.
>>
>
>
>           This makes a lot of sense.
>
>
>
>> 3) itkMacro.h only wraps the rounding function in vnl but not the ceil
>> and floor. It would be nice to wrap them and fix bugs 2078 and 5692 by
>> using itk::Math::Floor. The patch for this is also rather
>> straightforward.
>>
>
>
>           Yes, this is desirable functionality.
>
>
>
>> Do you think that any of these points merit attention before the
>> release or should I wait until the reopening of the repository to
>> handle this?
>>
>
>
>              This is indeed the big question.
>
>
>           We are on day before the tagging date.
>
>     We could, of course postpone the release and try to get
>     these changes in, if more developers agree that this is
>     worth the delay.
>
>
>     Given that these changes affect very fundamental pieces of
>     the toolkit, such as the itk::Index, the itk::Interpolators,
>     and the conversions from Physical Point to Indexes, I'm
>     skeptical that we could introduce more changes and still
>     stabilize the Dashboard in just a couple of days.
>
>
>
>> For your consideration, I have attached a patch for these three
>> points. I can of course split it if deemed necessary.
>>
>
>
>   Have you submitted Experimental builds to the Dashboard with
>   all the changes in the patch ?
>
>   Have you tested them both with the PIXEL consistency
>   flag ON and OFF ?
>
>
>
>
>> Cheers,
>> Tom Vercauteren
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Please keep messages on-topic and check the ITK FAQ at:
>> http://www.itk.org/Wiki/ITK_FAQ
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>