[Insight-developers] Problem with ITK 3.20 itkQuadEdgeMeshDiscretePrincipalCurvaturesEstimator.h

[Arnaud GELAS]

  Kent,

I have just pushed your fix.

As you also suggested, I have added some concept checking on the 
OutputCurvatureType to be of floating precision.

See the following for details:
http://github.com/Kitware/ITK/commit/fd39d4138c579a3526511f21a12d3b03a611fa41

Thanks,
Arnaud

On 09/14/2010 11:07 AM, Vincent Magnotta wrote:
> Arnuad,
>
> I don't believe that this code was not actually using integer values. I
> believe that the problem may be that this warning is triggered when there
> mesh type is specified as float for the PixelType. I can verify this later
> today.
>
> Vince
>
>
>
> On 9/14/10 9:58 AM, "Arnaud GELAS"<arnaud_gelas at hms.harvard.edu>  wrote:
>
>>    Hi Kent,
>>
>> When this filter was coded, it was indeed only tested and validated for
>> float and double pixel type. I definitively agree with you for the
>> corrections!!!
>>
>> However, I foresee some problems when using integer based values for
>> curvature, you may end up with null curvature quite often; and I am not
>> sure that in practice you will be really happy to find null principal
>> curvatures (i.e. a plane) for cortical surface.
>>
>> Arnaud
>>
>> On 09/14/2010 10:26 AM, kent williams wrote:
>>> This has implications for ITK4, if the review QuadEdgeMesh stuff is being
>>> moved out of review. One of our students got the following compiler error:
>>>
>>> InsightToolkit/Review/itkQuadEd
>>> geMeshDiscretePrincipalCurvaturesEstimator.h:141:
>>> error: ISO C++ says that these are ambiguous, even though the worst
>>> conversion for the first is better than the worst conversion for the second:
>>>
>>> InsightToolkit/Utilities/vxl/core/vnl/vnl_math.h:536: note: candidate 1:
>>> double vnl_math_max(double, double)
>>> InsightToolkit/Utilities/vxl/core/vnl/vnl_math.h:535: note: candidate 2:
>>> float vnl_math_max(float, float)
>>>
>>> The problem is this method of
>>> itk::QuadEdgeMeshDiscretePrincipalCurvatureEstimator:
>>>
>>>     virtual OutputCurvatureType ComputeDelta( )
>>>       {
>>>       return  vnl_math_max( 0., m_Mean * m_Mean - m_Gaussian );
>>>       }
>>>
>>> The type of m_Mean and m_Gaussian is the pixel type of the output mesh
>>> PixelType.  I presume that whomever wrote this code only tried it with one
>>> pixel type (float or double) and if the other type is used, it's too
>>> ambiguous for the compiler.
>>>
>>> 3.20 is closed, and I don't know the status of the review QuadEdgeMesh code.
>>> But it's pretty clear to me that this function could be re-written to be
>>> un-ambiguous, eg:
>>>
>>> virtual OutputCurvatureType ComputeDelta()
>>> {
>>> return vnl_math_max(static_cast<OutputCurvatureType>(0.0),
>>>                                    m_Mean * m_Mean - m_Gaussian);
>>> }
>>>
>>> And I'd feel happier with
>>>
>>> (m_Mean * m_Mean) - m_Gaussian
>>>
>>> As well, because while the compiler always remembers operator precedence, I
>>> have to think 'My Dear Aunt Sally' to understand what that expression means.
>>>
>>> _______________________________________________
>>> Powered by www.kitware.com
>>>
>>> Visit other Kitware open-source projects at
>>> http://www.kitware.com/opensource/opensource.html
>>>
>>> Kitware offers ITK Training Courses, for more information visit:
>>> http://kitware.com/products/protraining.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
>>
>> Kitware offers ITK Training Courses, for more information visit:
>> http://kitware.com/products/protraining.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
> ----------------------
> Associate Professor
> Department of Radiology
> 0453-D JCP
> 200 Hawkins Drive
> Iowa City, IA 52242
> E-mail: vincent-magnotta at uiowa.edu
> Phone: 319-356-8255 Fax: 319-353-6275
> Website: http://www.radiology.uiowa.edu
>
>
>