[Insight-developers] Portable round - Is it still time to commit?
Wes Turner
wes.turner at kitware.com
Wed May 27 09:48:13 EDT 2009
Whew ... with that in mind, I am awaiting a few more dashboards. If they
continue to come in as expected and the same as yesterday, I will be tagging
in the next hour or so.
- Wes
On Wed, May 27, 2009 at 7:53 AM, Tom Vercauteren <tom.vercauteren at m4x.org>wrote:
> 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
> >>
> >
>
--
Wesley D. Turner, Ph.D.
Kitware, Inc.
R&D Engineer
28 Corporate Drive
Clifton Park, NY 12065-8662
Phone: 518-371-3971 x120
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090527/b504fd87/attachment.htm>
More information about the Insight-developers
mailing list