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

[Bill Lorensen]

Tom,

We are not concerned about backward compatibility here. We consider
the inconsistent rounding to be a bug. However, for this release, we
want to validate the consistent rounding. Next release, assuming all
goes well, we'll remove the rounding flag.

Bill

On Wed, May 27, 2009 at 5:54 AM, Tom Vercauteren
<tom.vercauteren at m4x.org> wrote:
> 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
>