[Insight-developers] Portable round - Is it still time to commit?
Michel Audette
michel.audette at kitware.com
Mon May 25 12:13:56 EDT 2009
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090525/a71e1bc6/attachment.htm>
More information about the Insight-developers
mailing list