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

Luis Ibanez luis.ibanez at kitware.com
Mon May 25 09:08:14 EDT 2009 


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

More information about the Insight-developers mailing list