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

Wed May 27 05:54:31 EDT 2009

Hi all,

Sorry for the late reply but I am not in the same timezone...

Not that I would like to sound pessimistic but I would tend to think
that reverting to old-style rounding will not do much good in terms of
backward-compatibility. Indeed, before the update of vnl_math.h,
vnl_math_rnd was doing the following:
- round half-integers to the nearest even integer on windows 32 bits using msvc
- round half-integers away from zero on other platforms
- there is a hack in itkIndex.h
(http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkIndex.h?root=Insight&sortby=date&view=markup)
to make vnl_math_rnd round half-integers away from zero on windows 32
bits also (for this file only?)

With the update of vnl_math.h, vnl_math_rnd uses the most efficient
rule for half-integers which implies that it will
- round half-integers to the nearest even integer if sse2 is supported
or gcc on x86 is used or msvc on windows 32 bits is used
- round half-integers away from zero on other platforms
- the hack in itkIndex.h still only applies to msvc on windows 32 bits

(For simplicity I have omitted gcc-xml from the above discussion).

The platform dependence of vnl_math_rnd was one of the main reasons
for moving to a portable round within ITK. If we want to get closer to
backward-compatibility on half-integers rounding, we could maybe also
define the following functions:

inline int RoundHalfIntegerAwayFromZero(float  x) { return
x>=0.f?static_cast<int>(x+.5f):static_cast<int>(x-.5f); }
inline int RoundHalfIntegerAwayFromZero(double  x) { return
x>=0.?static_cast<int>(x+.5):static_cast<int>(x-.5); }

and use either
  itk::Math::RoundHalfIntegerAwayFromZero if  ITK_USE_PORTABLE_ROUND is off
or
  itk::Math::Round if  ITK_USE_PORTABLE_ROUND is on
in lieu of vnl_math_rnd in the entire toolkit.

This would also require to remove the hack in itkIndex.h and replace
the current uses of vnl_math_rnd_halfintup and
itk::Math::RoundHalfIntegerUp by
itk::Math::RoundHalfIntegerAwayFromZero

Maybe another option is to extend the hack in itkIndex.h to apply to
all platforms but I am not sure to understand how pervasive this hack
can be. A tentative backward-compatibility patch is attached. It adds
the vnl_math_rnd hack in itkMacro.h (instead of itkIndex.h) if
ITK_USE_PORTABLE_ROUND is off. Does this makes sense to any of you?

There is an experimental submission (OSX i386 build, default
parameters except build type which is set to Debug) corresponding to
this patch, it has three failing tests that I haven't investigated
yet:
http://www.cdash.org/CDash/buildSummary.php?buildid=341807

That being said, any of these two options seem like huge changes for a
(IMHO) small problem. It is because true backward compatibility on
half-integers appears to be very difficult to get that I was proposing
to get rid of the ITK_USE_PORTABLE_ROUND flag.

Thoughts?

Cheers,
Tom

On Tue, May 26, 2009 at 22:09, Wes Turner <wes.turner at kitware.com> wrote:
> All,
>
> We are closing in on the required changes for item (1).  There is one
> failing test that Luis is tracking down, at which point we should be able to
> check-in in time for the Nightlies.  Note that in support of backward
> compatibility, I did need to modify a few files that had been converted over
> to the new style, so the check in will be moderately larger than originally
> anticipated.  Please exercise the code to the extent you are able.  I would
> love to see lots of green Experimentals prior to the Nightlies kicking off
> at 9:00 tonight.
>
> - Wes
>
> On Tue, May 26, 2009 at 2:39 PM, Bill Lorensen <bill.lorensen at gmail.com>
> wrote:
>>
>> Sounds great to me.  Nice job guys...
>>
>> On Tue, May 26, 2009 at 1:35 PM, Wes Turner <wes.turner at kitware.com>
>> wrote:
>> > All,
>> >
>> > Just a little update on the status.  We decided that the least intrusive
>> > set
>> > of fixes is to divide Tom's excellent work into three sections
>> > corresponding
>> > to:
>> >
>> > the actual bug fix (Round will be defined in terms of
>> > RoundHalfIntegerUp()
>> > instead of RoundHalfIntegerToEven()),
>> > the addition of the ceil/floor functions, and
>> > the removal/disposition of the ITK_USE_PORTABLE_ROUND function.
>> >
>> > We will apply (1)  (the bug fix) to the code before cutting the release.
>> >
>> > We will apply (2)  (ceil and floor) upon reopening the repository.
>> >
>> > The disposition of the ITK_USE_PORTABLE_ROUND flag (3) needs to be
>> > addressed
>> > within the context of the backward compatibility policy.  We would like
>> > to
>> > change the default on the flag from OFF to ON following the reopening of
>> > the
>> > repository.  This will allow us to thoroughly test/exercise the change
>> > in
>> > the current code base.  Once the impact is understood and we have
>> > resolved
>> > dashboard issues, we will be better prepared to weigh the conflicting
>> > requirements of backward compatibility and platform independence.  We
>> > look
>> > forward to a vigorous debate from the concerned parties.
>> >
>> > - Wes
>> >
>> > On Tue, May 26, 2009 at 10:07 AM, Wes Turner <wes.turner at kitware.com>
>> > wrote:
>> >>
>> >> All,
>> >>
>> >> First, we would like to thank you for your patience, and ask for just a
>> >> little more forbearance.  We will not be tagging before tomorrow
>> >> afternoon
>> >> and ask that you continue to hold any changes to the repository until
>> >> we
>> >> have completed the release process.
>> >>
>> >> Tom Vercauteren has put in a lot of effort to make rounding work
>> >> consistently on all platforms and appears to have found and retired a
>> >> serious bug.  He submitted a patch this morning that list chatter
>> >> indicates
>> >> is both important and desirable.  Luis and I are running some
>> >> Experimental
>> >> builds rught now and if they look good, we will be comitting the patch
>> >> for
>> >> the Nightly build cycle.
>> >>
>> >> Thanks, Tom for all your effort.  With luck we will be wrapping up
>> >> tomorrow with an even more solid release.
>> >>
>> >> - Wes
>> >>
>> >> On Tue, May 26, 2009 at 8:50 AM, Tom Vercauteren
>> >> <tom.vercauteren at m4x.org>
>> >> wrote:
>> >>>
>> >>> Reposting without the patch to meet the size limit. The patch is now
>> >>> on the bug tracker:
>> >>> http://www.itk.org/Bug/file_download.php?file_id=2269&type=bug
>> >>>
>> >>> -----------------
>> >>> Folks,
>> >>>
>> >>> Attached is a patch that does the following:
>> >>> - handles the three points I initially mentioned
>> >>>   * remove ITK_USE_PORTABLE_ROUND
>> >>>   * check for x86 in gcc asm implementation of rounding
>> >>>   * add floor and ceil and use them in lieu of the buggy private
>> >>> fast floor function
>> >>> - adds a unit test for itk::Math function
>> >>> - fixes and enhances itkMathRoundProfileTest1
>> >>> - solves the small cmake issue of ResampleImageFilter3Test1
>> >>>
>> >>> Feel free to use only part of it. If you want I can also split it so
>> >>> as to cover only what you want to commit for the release.
>> >>>
>> >>> The experimental build corresponding to the entire patch with
>> >>> ITK_USE_CENTERED_PIXEL_COORDINATES_CONSISTENTLY turned off is green on
>> >>> my machine:
>> >>> http://www.cdash.org/CDash/buildSummary.php?buildid=341090
>> >>>
>> >>> Cheers,
>> >>> Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itk-portable-round-backward-compatibility-2009-05-27.patch
Type: text/x-diff
Size: 4416 bytes
Desc: not available
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090527/8544aaa4/attachment.patch>