[Insight-developers] vnl updates

[Luis Ibanez]

Hi Tom,

Thanks for looking into this.

Choosing the rounding standard based on the performance of the platform
doesn't seem to be a good solution for ITK.

We would want a round() function that produces the *same* output on
*every* platform.


If we pick a rounding policy, it should be the same for all
platforms, and it should also include a specification on how
it will apply to negative numbers.

------

Now that,
Talking about performance, without citing numbers is not very productive.

How much speed up are we talking about ?

     * 1% ?
     * 10% ?

Was that profiled in Release mode ?
In what platforms ?
Does it depend on the image dimension ? 2D/3D ?

------

That is,
my point is that any claim about "performance" that is not followed
by a table with numbers in milliseconds, and a description of a
reproducible way of measuring those timings.... is in practice... useless.

We should probably start with a test that we can put in ITK,
that includes TimeProbes, and when we can see the output of
this test in all platforms, then we will be in a better position
to judge what should be done with the software.

See for example:

    Insight/Testing/Code/Common/
        itkOrientedImageProfileTest1.cxx
        itkOrientedImageProfileTest2.cxx
        itkOrientedImageProfileTest3.cxx



     Luis



-----------------------
Tom Vercauteren wrote:
> Hey Luis,
> 
> These issues are most certainly related to how half integers are
> rounded by vnl_math_round. As specified by the until test of vnl, the
> result of vnl_math_round(12.5) could either be 12 or 13. Within ITK, I
> think that it is assumed that half integers are rounded to away from
> zero, e.g. vnl_math_round(12.5)==13. With the version of vnl_math.h
> that ships with ITK, this should be true on every platform except 32
> bits windows using MSVC (half integers rounded to nearest even
> integer). This behavior has been modified in vnl cvs version. Most
> platform will now round half integers to the nearest even integer, ie
> vnl_math_round(12.5)==12 and vnl_math_round(11.5)==12.
> 
> This may explain the test failures. One thing that I didn't check is
> whether the test that are failing in your experimental build are also
> failing with the current version of ITK on 32 bits windows using MSVC.
> 
> Since, in the version that ships with ITK, the semantic of
> vnl_math_round for half integer was not strictly enforced and not
> consistent across the platforms, I took the liberty of choosing the
> most efficient one depending on the platform (round half integers to
> the nearest even integer is now the most common). Does this look like
> a problem to you? We could maybe simply make the unit tests in ITK a
> little more permissive to make them pass no matter how half integers
> are rounded by vnl_math_round.
> 
> Hope this helps.
> 
> Regards,
> Tom
> 
> 
> On Mon, Dec 29, 2008 at 11:45 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> 
>>Hi Tom,
>>
>>The Experimental build (From Ubuntu Linux with gcc 4.2):
>>
>>http://www.cdash.org/CDash/viewSite.php?siteid=1086&project=2&currenttime=1230512400
>>
>>came up with three failing tests.
>>
>>http://www.cdash.org/CDash/viewTest.php?onlyfailed&buildid=239928
>>
>>They are:
>>
>>    760 - itkInterpolateTest (Failed)
>>    1310 - ResampleImageFilter3Test1 (Failed)
>>    1314 - ResampleImageFilter9Test (Failed)
>>
>>
>>We will have to address these failure before we
>>update the vnl_math.h file.
>>
>>
>>Would you have a chance to look at one of them ?
>>
>>The first one and third one seem to be the ones providing a better hint
>>to the source of the problem.
>>
>>In particular, the diff image at
>>http://www.cdash.org/CDash/testDetails.php?test=14663712&build=239928
>>may help.
>>
>>
>>   Please let us know,
>>
>>----------------------------------------------------------------------------------------------
>>On Mon, Dec 29, 2008 at 12:27 PM, Luis Ibanez <luis.ibanez at kitware.com>
>>wrote:
>>
>>>Hi Tom,
>>>
>>>Thanks for looking at this.
>>>
>>>I'm running an experimental build with this change (new vnl_math.h).
>>>
>>>If it comes out green I'll commit the new vnl_math.h file,
>>>and we will see the effect on tomorrow's Nightly builds...
>>>
>>>
>>>      Luis
>>>
>>>
>>>
>>>-------------------------------------------------------------------------------------------------------
>>>On Mon, Dec 29, 2008 at 12:24 PM, Tom Vercauteren
>>><tom.vercauteren at m4x.org> wrote:
>>>
>>>>Small mistake... I meant VNL_CONFIG_ENABLE_SSE2_ROUNDING will not be
>>>>defined by cmake.
>>>>Tom
>>>>
>>>>On Mon, Dec 29, 2008 at 6:04 PM, Tom Vercauteren
>>>><tom.vercauteren at m4x.org> wrote:
>>>>
>>>>>>Could you point me to the set of lines that we want to import
>>>>>>from the new version of vnl_math.h  ?
>>>>>
>>>>>The difference between ITK's vnl_math.h and vxl's vnl_math.h is
>>>>>exactly what is needed:
>>>>>
>>>>>http://vxl.cvs.sourceforge.net/viewvc/vxl/vxl/core/vnl/vnl_math.h?r1=1.50&r2=1.42
>>>>>
>>>>>So the only thing that needs to be done is replace the current
>>>>>vnl_math.h file by the most up-to-date one.
>>>>>
>>>>>As I mentioned, the sse2 optimization will not be compiled since
>>>>>VNL_CHECK_FPU_ROUNDING_MODE will not be defined by cmake.
>>>>>
>>>>>
>>>>>
>>>>>>We could insert them today or tomorrow...
>>>>>
>>>>>Great!
>>>>>
>>>>>Thanks,
>>>>>Tom
>>>>>
>>>>>P.S.: As far as vnl_math.h is concerned, there should be no backward
>>>>>compatibility issue.
>>>>>
>>>
>>
>