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

Wed May 27 07:53:58 EDT 2009

Bill,

I had over-interpreted Wes' comments "Note that in support of backward
compatibility, I did need to modify a few files that had been
converted over to the new style". Given your feedback, the current
state of things seems fine to me for the release.

Tom

On Wed, May 27, 2009 at 13:37, Bill Lorensen <bill.lorensen at gmail.com> wrote:
> 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
>>
>