[Insight-developers] Portable round - Is it still time to commit?
Michel Audette
michel.audette at kitware.com
Mon May 25 12:18:28 EDT 2009
Hi again,
Sorry, just noticed that you have it in your original message. I can give it
a try.
Michel
On Mon, May 25, 2009 at 12:13 PM, Michel Audette <michel.audette at kitware.com
> wrote:
> Hi Tom,
>
> I could suggest that you make the patch available to a few of us, we could
> each apply it, and do a ctest Experimental with flags on and off prior to
> coming to a decision. I have some regression test data that I was thinking
> of committing as well.
>
> Best wishes,
>
> Michel
>
>
> On Mon, May 25, 2009 at 9:34 AM, Tom Vercauteren <tom.vercauteren at m4x.org>wrote:
>
>> Hi Luis,
>>
>> Please don't get me wrong. I am certainly not saying that things needs
>> to be done in one way or another! If I were to decide, I would
>> definitely not release 3.14 today if a similar patch were committed
>> today. I am simply trying to highlight the issues I have seen so far
>> and I apologize for mentioning them so late... The goal of starting
>> this discussion now was just to help make an informed decision about
>> the release.
>>
>> Given the potential impact of such a patch I indeed see no other good
>> plan than the two you just proposed and option B seems good to me.
>>
>> Regarding the experimental build: I just started them this morning and
>> did not have the results at the time of my previous email. I should
>> have waited for them but thought that the earlier I would start this
>> discussion, the better! It takes quite a while on my computer to get
>> a complete Experimental build. Currently, the only results I have are
>> with ITK_USE_PORTABLE_ROUND and PIXEL consistency turned off:
>> http://www.cdash.org/CDash/buildSummary.php?buildid=340427
>>
>> There are two failing tests on this build, I haven't had the time to
>> look into it yet.
>>
>> Cheers,
>>
>> Tom
>>
>> On Mon, May 25, 2009 at 15:08, Luis Ibanez <luis.ibanez at kitware.com>
>> wrote:
>> >
>> >
>> > Hi Tom
>> >
>> >
>> > A change in the itk::Index and the itkMacro.h will affect
>> > *all* the toolkit. These files are the very core of ITK.
>> >
>> >
>> > I understand and appreciate your interest for getting the
>> > code to behaver correctly. We are all for it. However, we
>> > are also constrained by a backward compatibility requirement,
>> > and we are hours away from cutting a release.
>> >
>> >
>> > The perspective of making untested changes to files that
>> > are at the core of the library doesn't seem to be a wise
>> > thing to do.
>> >
>> >
>> >
>> > I couldn't find in your email the answer to the question:
>> >
>> >
>> > Have you submitted patched Experimental builds
>> > with this flag ON and with this flag OFF ?
>> >
>> > Were these Experimental builds Green ?
>> >
>> > In the absence of the minimal reassurance of an
>> > Experimental build there is little foundation,
>> > other that wishful thinking, to expect that committing
>> > changes to essential files will proceed smoothly.
>> >
>> > Should I bring up the Sun-CC affair ?
>> > to remind us of how an otherwise innocuous-looking change
>> > can have vastly time-consuming consequences.
>> >
>> >
>> >
>> > Let me propose two options:
>> >
>> >
>> >
>> > A) We commit your patch, and post-pone the release
>> > until next Saturday.
>> >
>> > This will give us time to verify the all builds
>> > are correct
>> >
>> >
>> >
>> > B) We hold on the patch. Cut the release as scheduled.
>> > and commit the patch immediately after the repository
>> > reopens.
>> >
>> > Once the dashboard stabilizes, we cut a patch release
>> > ITK 3.14.1.
>> >
>> >
>> >
>> >
>> > If we are going to proceed with (A),
>> > we should at least see an Experimental build first.
>> >
>> >
>> >
>> > What other developers think ?
>> >
>> >
>> > Luis
>> >
>> >
>> >
>> >
>> > ----------------------
>> > Tom Vercauteren wrote:
>> >>
>> >> Hi Luis and Bill,
>> >>
>> >> Thanks for the feedback. I most certainly understand that it can be
>> >> too late to commit, which is why I was asking.
>> >>
>> >>
>> >> Just regarding the ITK_USE_PORTABLE_ROUND flag, I really don't get how
>> >> it can modify the behavior of the code.The only place where it is
>> >> actually used is in itkIndex and itkMacro. Let me know if I am missing
>> >> something.
>> >>
>> >>
>> >> Here is how ITK_USE_PORTABLE_ROUND is used in itkMacro,
>> >>
>> >>
>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkMacro.h?root=Insight&sortby=date&view=markup
>> :
>> >>
>> >> #ifdef ITK_USE_PORTABLE_ROUND
>> >> #define
>> >>
>> itkForLoopRoundingAndAssignmentMacro(DestinationType,Sourcrnd_halfintup,DestinationElementType,DestinationArray,SourceArray,NumberOfIterations)
>> >> \
>> >> for(unsigned int i=0;i < NumberOfIterations; ++i) \
>> >> { \
>> >> DestinationArray[i] = static_cast< DestinationElementType >(
>> >> itk::Math::Round( SourceArray[i] ) ); \
>> >> }
>> >> #else
>> >> #define
>> >>
>> itkForLoopRoundingAndAssignmentMacro(DestinationType,SourceType,DestinationElementType,DestinationArray,SourceArray,NumberOfIterations)
>> >> \
>> >> for(unsigned int i=0;i < NumberOfIterations; ++i) \
>> >> { \
>> >> DestinationArray[i] = static_cast< DestinationElementType >(
>> >> itk::Math::RoundHalfIntegerUp( SourceArray[i] ) ); \
>> >> }
>> >> #endif
>> >>
>> >> However, from the same itkMacro we have:
>> >>
>> >> inline int Round(float x) { return RoundHalfIntegerToEven(x); }
>> >> inline int Round(double x) { return RoundHalfIntegerToEven(x); }
>> >>
>> >> This implies that in this file, the flag has no visible effect.
>> >>
>> >>
>> >>
>> >> Here is how ITK_USE_PORTABLE_ROUND is used in itkIndex,
>> >>
>> >>
>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/Common/itkIndex.h?root=Insight&sortby=date&view=markup
>> :
>> >>
>> >> #ifdef ITK_USE_PORTABLE_ROUND
>> >> m_Index[i] = static_cast< IndexValueType>( itk::Math::Round(
>> point[i]
>> >> ) );
>> >> #else
>> >> m_Index[i] = static_cast< IndexValueType>(
>> >> vnl_math_rnd_halfintup( point[i] ) );
>> >> #endif
>> >>
>> >> However, from itkMacro we also have:
>> >>
>> >> inline int RoundHalfIntegerUp(float x) { return
>> >> vnl_math_rnd_halfintup(x); }
>> >> inline int RoundHalfIntegerUp(double x) { return
>> >> vnl_math_rnd_halfintup(x); }
>> >> inline int Round(float x) { return RoundHalfIntegerToEven(x); }
>> >> inline int Round(double x) { return RoundHalfIntegerToEven(x); }
>> >>
>> >> This means that in this file also the flag has no visible effect.
>> >>
>> >>
>> >> The only thing that ITK_USE_PORTABLE_ROUND will change is what
>> >> vnl_math_rnd_halfintup does on some platforms. Indeed, in itkIndex,
>> >> for some reason vnl_math_rnd_halfintup is made platform-dependent if
>> >> ITK_USE_PORTABLE_ROUND is undefined. Indeed, the following hack for
>> >> vnl_math_rnd was not removed when moving from a platform-dependent
>> >> vnl_math_rnd to a platform-consistent vnl_math_rnd_halfintup but was
>> >> wrongly "ported" to vnl_math_rnd_halfintup:
>> >>
>> >> #ifndef ITK_USE_PORTABLE_ROUND
>> >> // The Windows implementaton of vnl_math_rnd() does not round the
>> >> // same way as other versions. It has an assembly "fast"
>> implementation
>> >> // but with the drawback of rounding to the closest even number.
>> >> // See: http://www.musicdsp.org/showone.php?id=170
>> >> // For example 0.5 is rounded down to 0.0.
>> >> // This conditional code replaces the standard vnl implementation that
>> >> uses
>> >> // assembler code. The code below will be slower for windows but will
>> >> // produce consistent results. This can be removed once vnl_math_rnd
>> is
>> >> // fixed in VXL.
>> >> #if (defined (VCL_VC) && !defined(__GCCXML__)) || (defined(_MSC_VER)
>> >> && (_MSC_VER <= 1310))
>> >> #define vnl_math_rnd_halfintup(x) ((x>=0.0)?(int)(x + 0.5):(int)(x -
>> 0.5))
>> >> #endif
>> >> #endif
>> >>
>> >>
>> >> This basically means that on some windows machines,
>> >> vnl_math_rnd_halfintup does rounding with half-integers being rounded
>> >> away from zero instead of being rounded upwards.
>> >>
>> >>
>> >> This is why I still vote for removing ITK_USE_PORTABLE_ROUND before
>> >> the 3.14 release. My other two points are less important and even I
>> >> wouldn't necessarily vote for fixing them before the release.
>> >>
>> >> Hope this clarifies things a little bit.
>> >>
>> >> Tom
>> >>
>> >> On Mon, May 25, 2009 at 13:55, Luis Ibanez <luis.ibanez at kitware.com>
>> >> wrote:
>> >>
>> >>> Hi Tom,
>> >>>
>> >>>
>> >>> Thanks for looking at this.
>> >>> Please see my comments interleaved.
>> >>>
>> >>>
>> >>>
>> >>> Luis
>> >>>
>> >>>
>> >>> ---------------------
>> >>> Tom Vercauteren wrote:
>> >>>
>> >>>> Hi all,
>> >>>>
>> >>>> It seems I have been away at a critical time. Before leaving, I
>> >>>> thought I would have some time at my return (today) to commit a few
>> >>>> patches. However apparently the deadline for 3.14 is today...
>> >>>>
>> >>>> There are a few things that bother me in the current state of things
>> >>>> with respect to the new rounding functions:
>> >>>>
>> >>>> 1) The code still relies on ITK_USE_PORTABLE_ROUND. As mentioned by
>> >>>> Luis, this variable was intended to be temporary. I really think that
>> >>>> the release should not have this variable. The patch for that is
>> >>>> straightforward since ITK_USE_PORTABLE_ROUND is only used in a few
>> >>>> files and does not change the behavior of the code at all.
>> >>>>
>> >>>
>> >>>
>> >>>
>> >>> Well,
>> >>> the flag does change the behavior of the code.
>> >>> Otherwise there will be no point on it.
>> >>>
>> >>> Please note that most builds in the dashboard are using
>> >>> ITK_USE_PORTABLE_ROUND OFF
>> >>>
>> >>> We have not tested this flag ON extensively, but we have
>> >>> managed to reduce the number of failing test related to it.
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>> 2) The current implementation of rounding on gcc (when sse2 is not
>> >>>> available) works only on x86. However the code checks that it is not
>> >>>> compiling for ppc nor for ppc64. As discussed with Sean McBride, It
>> >>>> would make more sense to really check that we are on a a x86
>> platform.
>> >>>>
>> >>>
>> >>>
>> >>> This makes a lot of sense.
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>> 3) itkMacro.h only wraps the rounding function in vnl but not the
>> ceil
>> >>>> and floor. It would be nice to wrap them and fix bugs 2078 and 5692
>> by
>> >>>> using itk::Math::Floor. The patch for this is also rather
>> >>>> straightforward.
>> >>>>
>> >>>
>> >>>
>> >>> Yes, this is desirable functionality.
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>> Do you think that any of these points merit attention before the
>> >>>> release or should I wait until the reopening of the repository to
>> >>>> handle this?
>> >>>>
>> >>>
>> >>>
>> >>> This is indeed the big question.
>> >>>
>> >>>
>> >>> We are on day before the tagging date.
>> >>>
>> >>> We could, of course postpone the release and try to get
>> >>> these changes in, if more developers agree that this is
>> >>> worth the delay.
>> >>>
>> >>>
>> >>> Given that these changes affect very fundamental pieces of
>> >>> the toolkit, such as the itk::Index, the itk::Interpolators,
>> >>> and the conversions from Physical Point to Indexes, I'm
>> >>> skeptical that we could introduce more changes and still
>> >>> stabilize the Dashboard in just a couple of days.
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>> For your consideration, I have attached a patch for these three
>> >>>> points. I can of course split it if deemed necessary.
>> >>>>
>> >>>
>> >>>
>> >>> Have you submitted Experimental builds to the Dashboard with
>> >>> all the changes in the patch ?
>> >>>
>> >>> Have you tested them both with the PIXEL consistency
>> >>> flag ON and OFF ?
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>> Cheers,
>> >>>> Tom Vercauteren
>> >>>>
>> >>>>
>> >>>>
>> ------------------------------------------------------------------------
>> >>>>
>> >>>> _______________________________________________
>> >>>> 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
>> >>>
>> >>
>> > _______________________________________________
>> > 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
>> >
>> _______________________________________________
>> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090525/bfd7987e/attachment.htm>
More information about the Insight-developers
mailing list