[Insight-developers] Portable round - Is it still time to commit?
Wes Turner
wes.turner at kitware.com
Tue May 26 13:35:48 EDT 2009
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:
1. the actual bug fix (Round will be defined in terms of
RoundHalfIntegerUp() instead of RoundHalfIntegerToEven()),
2. the addition of the ceil/floor functions, and
3. 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090526/db2995cb/attachment.htm>
More information about the Insight-developers
mailing list