[Insight-developers] Portable round - Is it still time to commit?
Tom Vercauteren
tom.vercauteren at m4x.org
Mon May 25 09:34:08 EDT 2009
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
>
More information about the Insight-developers
mailing list