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

[Bill Lorensen]

Luis,

I agree with you that it is too close to a release to make the changes
permanent. We will have the next few months to validate these changes.
Then we can remove the flag.

Bill

On Mon, May 25, 2009 at 7:55 AM, 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
>