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

[Vincent Magnotta]

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