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

[Luis Ibanez]

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