[Insight-developers] Warnings : Morphological filters: improper concept check & lack of NumericTraits use.
Luis Ibanez
luis.ibanez at kitware.com
Tue Apr 28 10:39:15 EDT 2009
Hi Tom,
The changes have been committed.
Please let me know if you see any other problems.
Thanks
Luis
-------------------------------------------------------
On Tue, Apr 28, 2009 at 10:18 AM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
> Hi Tom,
>
> Thanks for pointing out these warnings.
>
>
> Historical Note:
> -------------------
>
> They are the result of one sloppy practice followed by a superficial review.
>
> The filter in question is comparing the pixel of a Kernel against "0",
> and using the symbol "0" instead of the proper:
>
> itk::NumericTraits< KernelPixelType >::Zero
>
> which would have generated a Zero value stored in the correct type,
> and which is the correct practice of Generic Programming.
>
> because this was a comparison against "0", when the concept checks
> where added. It looked like we needed a concept check for
>
> KernelPixelType comparable to "int"
>
> while in practice we only needed a concept check for:
>
> KernelPixelType comparable to KernelPixelType
>
>
> ---
>
>
> I'm now fixing the concept check and the comparison expressions
> in this family of morphological filters.
>
> Should be able to commit the fixes before the end of the morning.
>
>
> Thanks
>
>
> Luis
>
>
> ----------------------------------------------------------------------
> On Tue, Apr 28, 2009 at 4:45 AM, Tom Vercauteren
> <tom.vercauteren at gmail.com> wrote:
>> Hi Luis,
>>
>> While we are at it, I would also like to report the following compiler
>> warning that I keep having from ITK when I instantiated a
>> GrayscaleDilateImageFilter with unsigned int images:
>>
>> itk::GrayscaleDilateImageFilter<
>> itk::Image<unsigned int, 2u>,
>> itk::Image<unsigned int, 2u>,
>> itk::BinaryBallStructuringElement<
>> unsigned int, 2u, itk::NeighborhoodAllocator<unsigned int>
>> >
>> >
>>
>>
>> The warning I get is:
>>
>> itkConceptChecking.h:238: warning: comparison between signed and
>> unsigned integer expressions
>>
>>
>> This is triggered by the following concept check in GrayscaleDilateImageFilter:
>>
>> itkConceptMacro(KernelGreaterThanIntCheck,
>> (Concept::GreaterThanComparable<typename TKernel::PixelType, int>));
>>
>>
>> I am not sure how to fix this warning and if it makes sense to fix.
>> That is, is the concept check really telling me that I am doing
>> something stupid when I instantiated a GrayscaleDilateImageFilter with
>> unsigned int images?
>>
>> Regards,
>> Tom
>>
>> On Tue, Apr 28, 2009 at 04:14, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>>> Hi Steve,
>>>
>>>
>>> Thanks for the compilation line. It seems that the
>>> relevant flag is -Wall, and we have several gcc builds
>>> with this flag on.
>>>
>>> As you pointed out, the most likely cause is that this is
>>> a case that is not being instantiated in the ITK testing suite.
>>>
>>> I'll try to reproduce the warning in an ITK example.
>>>
>>> It may actually be that two incompatible concepts
>>> are being confounded in these filters...
>>>
>>>
>>> Thanks,
>>>
>>>
>>> Luis
>>>
>>>
>>> ------------------
>>> On Mon, Apr 27, 2009 at 8:56 PM, Steve Pieper <pieper at bwh.harvard.edu> wrote:
>>>> Hi Luis -
>>>>
>>>> Thanks for your help with this.
>>>>
>>>> Here's the code in question:
>>>>
>>>> http://viewvc.slicer.org/viewcvs.cgi/trunk/Applications/CLI/OtsuThresholdSegmentation.cxx?rev=9136&view=markup
>>>>
>>>> I believe it is actually an ITK example program that Bill modified a bit.
>>>> It looks to me like the types are actually correct - InternalImageType is
>>>> unsigned long, which is the output of the ConnectedComponents filter. So
>>>> this doesn't fall into your 'risky' case.
>>>>
>>>> I guess this was a case that wasn't covered with the existing ITK tests.
>>>>
>>>> The compiler version, compile line and sample error messages are below.
>>>>
>>>> Best,
>>>> Steve
>>>>
>>>>
>>>> % g++ --version
>>>> g++ (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)
>>>>
>>>> [ 93%] Building CXX object
>>>> Applications/CLI/CMakeFiles/OtsuThresholdSegmentation.dir/OtsuThresholdSegmentation.cxx.o
>>>> cd /workspace/pieper/slicer3/Slicer-3-4/Slicer3-build/Applications/CLI &&
>>>> /usr/bin/g++ -ftemplate-depth-50 -Wall -Wno-deprecated
>>>> -ftemplate-depth-50 -ftemplate-depth-50 -Wall -ftemplate-depth-50 -Wall -g
>>>
>>> .....
>>>>
>>>> Luis Ibanez wrote:
>>>>>
>>>>> Hi Steve,
>>>>>
>>>>> Thanks for pointing this out.
>>>>>
>>>>> --
>>>>>
>>>>> Fixes for the warnings that you identified in ITK
>>>>> have now been committed:
>>>>>
>>>>>
>>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkConnectedComponentImageFilter.h?root=Insight&r1=1.22&r2=1.23&sortby=date
>>>>>
>>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkConnectedComponentImageFilter.txx?root=Insight&r1=1.31&r2=1.32&sortby=date
>>>>>
>>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkRelabelComponentImageFilter.txx?root=Insight&r1=1.17&r2=1.18&sortby=date
>>>>>
>>>>> http://www.itk.org/cgi-bin/viewcvs.cgi/Code/BasicFilters/itkRelabelComponentImageFilter.h?root=Insight&r1=1.16&r2=1.17&sortby=date
>>>>>
>>>>>
>>>>> The changes are a bit different of what is proposed in your patch.
>>>>>
>>>>> Instead of just adding the static_casts, we have introduced additional
>>>>> typedefs to clarify the use of the two distinct concepts:
>>>>>
>>>>> a) LabelType of a connected component.
>>>>> b) ObjectSizeType of a connected component.
>>>>>
>>>>>
>>>>> I'm curious about why these warnings didn't show up in the
>>>>> ITK Dashboard...
>>>>>
>>>>> One possibility may be that Slicer (or Slicer Modules) may be
>>>>> instantiating connected component filters by using images of
>>>>> pixel type different than "unsigned long". Although it is
>>>>> admissible to do so, it is also risky, specially if the pixel
>>>>> type in question doesn't really has the capacity for counting
>>>>> all the potential connected components that are present in
>>>>> the image or that can be generated during the intermediate
>>>>> procedure of labeling.
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Could you share with use the Warning flags that you are using ?
>>>>> (and remind us of what platform pointed out these warnings)
>>>>>
>>>>> We will set them in one of the similar builds in the ITK Dashboards.
>>>>>
>>>>>
>>>>> ---
>>>>>
>>>>>
>>>>> About porting these changes back to ITK 3.10, we rather avoid this.
>>>>>
>>>>>
>>>>> We only port back fixes for bugs, and only do it for the most recent
>>>>> release branch. In this case ITK 3.12.
>>>>>
>>>>>
>>>>> On the Bright side, ITK 3.14 will be out by the end of May.
>>>>>
>>>>>
>>>>> Please let us know if you find any other problems,
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>
>>>>> Luis
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> ---------------------
>>>>> Steve Pieper wrote:
>>>>>>
>>>>>> Hi -
>>>>>>
>>>>>> Remember Bill's graffiti wisdom?
>>>>>>
>>>>>>
>>>>>> http://www.na-mic.org/Wiki/index.php/2009_Winter_Project_Week_Compiler_Warnings:Slicer3_Graffiti
>>>>>>
>>>>>> Well, I took it to heart and cleaned up the remaining compile warnings in
>>>>>> slicer3 (trunk and 3.4 release branch). *Except* for a few lingering ones
>>>>>> that trace back to ITK.
>>>>>>
>>>>>> The following changes are against ITK 3.12. It would also be great if
>>>>>> the same fixes (and the other recent fixes to this class) could go into ITK
>>>>>> 3.10 since that's what we're using for slicer3.4.
>>>>>>
>>>>>> If we do this, then people who download and build the release branch
>>>>>> would get 0 warnings and 0 errors, and we'd all like that, wouldn't we?
>>>>>>
>>>>>> Thanks,
>>>>>> Steve
>>>>>>
>>>>>> p.s. please also doublecheck that the changes make sense - they work for
>>>>>> my test cases and I believe the are correct.
>>>>>>
>>>>>>
>>>>>> % cvs -q diff
>>>>>> Index: Code/BasicFilters/itkConnectedComponentImageFilter.txx
>>>>>> ===================================================================
>>>>>> RCS file:
>>>>>> /cvsroot/Insight/Insight/Code/BasicFilters/itkConnectedComponentImageFilter.txx,v
>>>>>> retrieving revision 1.31
>>>>>> diff -r1.31 itkConnectedComponentImageFilter.txx
>>>>>> 600c600
>>>>>> < m_Consecutive[m_BackgroundValue] = m_BackgroundValue;
>>>>>> ---
>>>>>> > m_Consecutive[static_cast<unsigned long>(m_BackgroundValue)] =
>>>>>> static_cast<unsigned long>(m_BackgroundValue);
>>>>>> Index: Code/BasicFilters/itkRelabelComponentImageFilter.txx
>>>>>> ===================================================================
>>>>>> RCS file:
>>>>>> /cvsroot/Insight/Insight/Code/BasicFilters/itkRelabelComponentImageFilter.txx,v
>>>>>> retrieving revision 1.17
>>>>>> diff -r1.17 itkRelabelComponentImageFilter.txx
>>>>>> 105c105
>>>>>> < mapIt = sizeMap.find( inputValue );
>>>>>> ---
>>>>>> > mapIt = sizeMap.find( static_cast<unsigned long>(inputValue) );
>>>>>> 109,110c109,110
>>>>>> < initialSize.m_ObjectNumber = inputValue;
>>>>>> < sizeMap.insert( MapValueType( inputValue, initialSize ) );
>>>>>> ---
>>>>>> > initialSize.m_ObjectNumber = static_cast<unsigned
>>>>>> long>(inputValue);
>>>>>> > sizeMap.insert( MapValueType( static_cast<unsigned
>>>>>> long>(inputValue), initialSize ) );
>>>>>> 213c213
>>>>>> < outputValue =
>>>>>> static_cast<OutputPixelType>(relabelMap[inputValue]);
>>>>>> ---
>>>>>> > outputValue =
>>>>>> static_cast<OutputPixelType>(relabelMap[static_cast<unsigned
>>>>>> long>(inputValue)]);
>>>>>> _______________________________________________
>>>>>> NAMIC-Eng mailing list
>>>>>> NAMIC-Eng at na-mic.org
>>>>>> http://public.kitware.com/cgi-bin/mailman/listinfo/namic-eng
>>>>>>
>>>>
>>> _______________________________________________
>>> 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
>>>
>>
>
More information about the Insight-developers
mailing list