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

Tue May 26 14:39:05 EDT 2009

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
>>>
>>>
>>> On Tue, May 26, 2009 at 14:28, Michel Audette
>>> <michel.audette at kitware.com> wrote:
>>> > Nice recovery, Tom.
>>> > I don't know if this is the case yet, but I'm thinking that unit
>>> > testing
>>> > should alternate between data that has even and odd dimensions. Perhaps
>>> > a
>>> > pattern emerges sometime when there is a bias in failing tests towards
>>> > one
>>> > or the other.
>>> >
>>> > Cheers,
>>> >
>>> > Michel
>>> > On Tue, May 26, 2009 at 8:19 AM, Bill Lorensen
>>> > <bill.lorensen at gmail.com>
>>> > wrote:
>>> >>
>>> >> No problem. The repository is frozen. Wes or Luis will have to make
>>> >> the changes to itkMacro.h.
>>> >>
>>> >> On Tue, May 26, 2009 at 7:43 AM, Tom Vercauteren
>>> >> <tom.vercauteren at m4x.org> wrote:
>>> >> > Hi Bill,
>>> >> >
>>> >> > It seems that I am the culprit :(
>>> >> > This apparently stems from the patch I proposed on the bug tracker.
>>> >> > I
>>> >> > am really really sorry for the mess. This is definitely the result
>>> >> > of
>>> >> > insufficient unit testing, I am preparing a test for
>>> >> > itk::Math::Round,
>>> >> > it will mimic the test from vnl_math. I'll post it ASAP.
>>> >> >
>>> >> > Tom
>>> >> >
>>> >> > On Tue, May 26, 2009 at 13:37, Bill Lorensen
>>> >> > <bill.lorensen at gmail.com>
>>> >> > wrote:
>>> >> >> Tom,
>>> >> >>
>>> >> >> I believe that you are correct and that the consensus was to round
>>> >> >> up.
>>> >> >> This
>>> >> >> inline int Round(float   x) { return RoundHalfIntegerUp(x); }
>>> >> >> inline int Round(double  x) { return RoundHalfIntegerUp(x); }
>>> >> >>
>>> >> >> should be fixed before tagging. Looks like we'll need another day
>>> >> >> if
>>> >> >> it's not too late.
>>> >> >>
>>> >> >> Bill
>>> >> >>
>>> >> >> On Tue, May 26, 2009 at 4:53 AM, Tom Vercauteren
>>> >> >> <tom.vercauteren at m4x.org> wrote:
>>> >> >>> Hi all,
>>> >> >>>
>>> >> >>> Guess I did the code review of ITK_USE_PORTABLE_ROUND a little too
>>> >> >>> fast. I didn't spot another problem with the portable rounding
>>> >> >>> function.
>>> >> >>>
>>> >> >>> I was so sure that it::Math::Round was rounding half-integers
>>> >> >>> upward
>>> >> >>> that I read it that way in itkMacro.h. Actually, what we have in
>>> >> >>> this
>>> >> >>> file is
>>> >> >>>
>>> >> >>> inline int Round(float   x) { return RoundHalfIntegerToEven(x); }
>>> >> >>> inline int Round(double  x) { return RoundHalfIntegerToEven(x); }
>>> >> >>>
>>> >> >>> instead of what I understood the consensus would have led to
>>> >> >>>
>>> >> >>> inline int Round(float   x) { return RoundHalfIntegerUp(x); }
>>> >> >>> inline int Round(double  x) { return RoundHalfIntegerUp(x); }
>>> >> >>>
>>> >> >>> Changing this fixes the two additional failing tests that appeared
>>> >> >>> with the previous patch (ResampleImageFilter3Test and
>>> >> >>> ResampleImageFilter9Test) when
>>> >> >>> ITK_USE_CENTERED_PIXEL_COORDINATES_CONSISTENTLY was off.
>>> >> >>>
>>> >> >>> For your consideration, I have attached an updated patch. Also as
>>> >> >>> shown in this patch, there is a small cmake issue in
>>> >> >>> ResampleImageFilter3Test1 when
>>> >> >>> ITK_USE_CENTERED_PIXEL_COORDINATES_CONSISTENTLY is on.
>>> >> >>>
>>> >> >>> Hope this helps,
>>> >> >>>
>>> >> >>> Tom
>>> >> >>>
>>> >> >>>
>>> >> >>> On Tue, May 26, 2009 at 02:02, Michel Audette
>>> >> >>> <michel.audette at kitware.com> wrote:
>>> >> >>>> Hi gents,
>>> >> >>>>
>>> >> >>>> I've isolated the 2 failing tests to something in itkMacro.h part
>>> >> >>>> of
>>> >> >>>> the
>>> >> >>>> patch.
>>> >> >>>>
>>> >> >>>> Cheers,
>>> >> >>>>
>>> >> >>>> Michel
>>> >> >>>>
>>> >> >>>> On Mon, May 25, 2009 at 4:47 PM, Luis Ibanez
>>> >> >>>> <luis.ibanez at kitware.com>
>>> >> >>>> wrote:
>>> >> >>>>>
>>> >> >>>>> Thanks,
>>> >> >>>>>
>>> >> >>>>>
>>> >> >>>>>      Luis
>>> >> >>>>>
>>> >> >>>>> ------------------------------
>>> >> >>>>> On Mon, May 25, 2009 at 4:42 PM, Michel Audette
>>> >> >>>>> <michel.audette at kitware.com> wrote:
>>> >> >>>>>>
>>> >> >>>>>> Thanks for pointing that out. I missed those. Let me try a
>>> >> >>>>>> clean
>>> >> >>>>>> build
>>> >> >>>>>> again then.
>>> >> >>>>>>
>>> >> >>>>>> Michel
>>> >> >>>>>>
>>> >> >>>>>> On Mon, May 25, 2009 at 4:40 PM, Luis Ibanez
>>> >> >>>>>> <luis.ibanez at kitware.com>
>>> >> >>>>>> wrote:
>>> >> >>>>>>>
>>> >> >>>>>>> Hi Michel,
>>> >> >>>>>>>
>>> >> >>>>>>> Please note that the submission with two failing tests,
>>> >> >>>>>>> has link errors:
>>> >> >>>>>>>
>>> >> >>>>>>> http://www.cdash.org/CDash/viewBuildError.php?buildid=340518
>>> >> >>>>>>>
>>> >> >>>>>>> We may not want to draw conclusions about failing/passing
>>> >> >>>>>>> tests until the build itself is clean of errors.
>>> >> >>>>>>>
>>> >> >>>>>>>
>>> >> >>>>>>>     Luis
>>> >> >>>>>>>
>>> >> >>>>>>>
>>> >> >>>>>>>
>>> >> >>>>>>>
>>> >> >>>>>>> --------------------------------------------------------------------
>>> >> >>>>>>> On Mon, May 25, 2009 at 4:37 PM, Michel Audette
>>> >> >>>>>>> <michel.audette at kitware.com> wrote:
>>> >> >>>>>>>>
>>> >> >>>>>>>> Hi Luis,
>>> >> >>>>>>>>
>>> >> >>>>>>>> I'm trying to find out more. I went back to the presently
>>> >> >>>>>>>> committed
>>> >> >>>>>>>> version of Insight, and ascertained that no ctest -V Resample
>>> >> >>>>>>>> does not fail
>>> >> >>>>>>>> there.
>>> >> >>>>>>>>
>>> >> >>>>>>>> I'm currently adding each of Tom's patched files one by one,
>>> >> >>>>>>>> compiling
>>> >> >>>>>>>> and trying to determine which of these is the culprit. I
>>> >> >>>>>>>> don't
>>> >> >>>>>>>> know of a
>>> >> >>>>>>>> quicker way to do it.
>>> >> >>>>>>>>
>>> >> >>>>>>>> Feel free to call me if you want to discuss things.
>>> >> >>>>>>>>
>>> >> >>>>>>>> Cheers,
>>> >> >>>>>>>>
>>> >> >>>>>>>> Michel
>>> >> >>>>>>>>
>>> >> >>>>>>>>
>>> >> >>>>>>>> On Mon, May 25, 2009 at 4:33 PM, Luis Ibanez
>>> >> >>>>>>>> <luis.ibanez at kitware.com>
>>> >> >>>>>>>> wrote:
>>> >> >>>>>>>>>
>>> >> >>>>>>>>> Hi Michel,
>>> >> >>>>>>>>>
>>> >> >>>>>>>>> Why are these two test not showing up
>>> >> >>>>>>>>> as failings in the Continuous builds ?
>>> >> >>>>>>>>>
>>> >> >>>>>>>>>
>>> >> >>>>>>>>> Thanks for any hint,
>>> >> >>>>>>>>>
>>> >> >>>>>>>>>
>>> >> >>>>>>>>>      Luis
>>> >> >>>>>>>>>
>>> >> >>>>>>>>>
>>> >> >>>>>>>>> -------------------------------------------------
>>> >> >>>>>>>>> On Mon, May 25, 2009 at 3:05 PM, Michel Audette
>>> >> >>>>>>>>> <michel.audette at kitware.com> wrote:
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> Hi Tom,
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> you were bang on. I have done a make clean and re-built the
>>> >> >>>>>>>>>> code, and
>>> >> >>>>>>>>>> it looks like it's doing better.
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> On another note, two tests appear to fail with the Portable
>>> >> >>>>>>>>>> Round,
>>> >> >>>>>>>>>> Pixel-centered, and Region-validation flags off.
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> http://www.cdash.org/CDash/viewTest.php?onlyfailed&buildid=340518
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> These two tests also were failing with the flags on. Is
>>> >> >>>>>>>>>> there a
>>> >> >>>>>>>>>> part
>>> >> >>>>>>>>>> of the new code that is not #ifdef'ed with the flags?
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> The ctest -Experimental with flags on is still running.
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> Cheers,
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> Michel
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> On Mon, May 25, 2009 at 2:05 PM, Tom Vercauteren
>>> >> >>>>>>>>>> <tom.vercauteren at gmail.com> wrote:
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>> _______________________________________________
>>> >> >>>>>>>>>> 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
>>> >> >>>>>>>>>>
>>> >> >>>>>>>>>
>>> >> >>>>>>>>
>>> >> >>>>>>>>
>>> >> >>>>>>>>
>>> >> >>>>>>>> --
>>> >> >>>>>>>> Michel Audette, Ph.D.
>>> >> >>>>>>>> R & D Engineer,
>>> >> >>>>>>>> Kitware Inc.,
>>> >> >>>>>>>> Chapel Hill, N.C.
>>> >> >>>>>>>>
>>> >> >>>>>>>
>>> >> >>>>>>
>>> >> >>>>>>
>>> >> >>>>>>
>>> >> >>>>>> --
>>> >> >>>>>> Michel Audette, Ph.D.
>>> >> >>>>>> R & D Engineer,
>>> >> >>>>>> Kitware Inc.,
>>> >> >>>>>> Chapel Hill, N.C.
>>> >> >>>>>>
>>> >> >>>>>
>>> >> >>>>
>>> >> >>>>
>>> >> >>>>
>>> >> >>>> --
>>> >> >>>> Michel Audette, Ph.D.
>>> >> >>>> R & D Engineer,
>>> >> >>>> Kitware Inc.,
>>> >> >>>> Chapel Hill, N.C.
>>> >> >>>>
>>> >> >>>>
>>> >> >>>
>>> >> >>> _______________________________________________
>>> >> >>> 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
>>> >> >>>
>>> >> >>>
>>> >> >>
>>> >> >
>>> >
>>> >
>>> >
>>> > --
>>> > Michel Audette, Ph.D.
>>> > R & D Engineer,
>>> > Kitware Inc.,
>>> > Chapel Hill, N.C.
>>> >
>>> >
>>> _______________________________________________
>>> 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
>>
>>
>>
>> --
>> Wesley D. Turner, Ph.D.
>> Kitware, Inc.
>> R&D Engineer
>> 28 Corporate Drive
>> Clifton Park, NY 12065-8662
>> Phone: 518-371-3971 x120
>
>
>
> --
> Wesley D. Turner, Ph.D.
> Kitware, Inc.
> R&D Engineer
> 28 Corporate Drive
> Clifton Park, NY 12065-8662
> Phone: 518-371-3971 x120
>
> _______________________________________________
> 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
>
>