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

Tue May 26 08:50:42 EDT 2009

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.
>
>