[Insight-developers] Warnings : Slicer & ITK
Luis Ibanez
luis.ibanez at kitware.com
Tue Apr 28 12:38:15 EDT 2009
Hi Steve,
1) My objections to using floats and doubles
for representing labels are:
a) Comparison for EQUAL between floats and doubles should be done
using a tolerance. (not done in this way in the filter code).
b) Poor use of the bit space. Floats and Double types are an
inefficient representation for integer types. (e.g the
number of distinct integers that can be represented per byte)
I see your point that the filter should be usable with floats and
doubles. In fact, it will be (with no warnings) with the current
CVS version. Still, I would discourage anybody from using an image
of floats or doubles to represent an image of labels.
2) About what the warning doesn't show up in the build of the test..
yeap, I have not explanation either.
I should build Slicer in order to track this down.
will report back later today...
Thanks
Luis
--------------------
Steve Pieper wrote:
> Hi Luis -
>
> As you suggest, I'm changing slicer to not instantiate the type
> combinations that trigger the compile warnings (unless compiled against
> ITK >= 3.14). This was pretty easy and accomplishes what I needed.
>
>
> A couple of non-urgent details, because, of course, we all love the
> details ;)
>
>
> 1) Can you be more specific about why a float or double label map is
> problematic conceptually? It may be impractical or not appropriate for
> certain uses, but as a general rule I can't see why this filter
> shouldn't be usable for floating point types.
>
> 2) It's not clear why you don't get the warnings in your build. The
> underlying issue seems pretty clear. If you look at this change, for
> example:
>
> >>>>> < mapIt = sizeMap.find( inputValue );
> >>>>> ---
> >>>>> > mapIt = sizeMap.find( static_cast<unsigned
> long>(inputValue) );
>
> the sizeMap uses unsigned long, while inputValue is of InputPixelType.
> Without the cast, when InputPixelType is short, int, etc you should get
> a warning about signed/unsigned mismatch like the one I emailed about
> yesterday.
>
> Thanks,
> Steve
>
>
>
> Luis Ibanez wrote:
>
>> Hi Steve,
>>
>> I'm not being able to reproduce the warning message that you reported.
>>
>> I'm compiling the code from:
>>
>> http://viewvc.slicer.org/viewcvs.cgi/trunk/Applications/CLI/OtsuThresholdSegmentation.cxx?rev=9136&view=markup
>>
>>
>> against a version of ITK that doesn't have last night commits.
>>
>> I'm using the CXX_FLAGS: -Wall -Wshadow -Wcast-qual
>>
>> Looking at the warnings in today's Slicer3 Dashboard,
>>
>> http://www.cdash.org/CDash/viewBuildError.php?type=1&buildid=322001
>>
>> the warnings that are related to the itkRelabelConnectedComponents
>> filter seem to be:
>>
>>
>> itkRelabelComponentImageFilter.txx:109: warning: converting to 'long
>> unsigned int' from 'float'
>>
>> and are the result of attempting to instantiate this filter over an
>> input and output image of pixel type "float":
>>
>> itk::RelabelComponentImageFilter<TInputImage,
>> TOutputImage>::GenerateData()
>> [with TInputImage = itk::Image<float, 3u>, TOutputImage =
>> itk::Image<float, 3u>]':
>>
>>
>> and an input and output images of pixel type "double"
>>
>> In member function 'void itk::RelabelComponentImageFilter<TInputImage,
>> TOutputImage>::GenerateData() [with TInputImage = itk::Image<double,
>> 3u>, TOutputImage = itk::Image<double, 3u>]':
>>
>> both of which can easily be considered inappropriate type for
>> representing
>> the labels resulting from a segmentation.
>>
>> The problem seem to originate from:
>>
>> Slicer3/Libs/vtkITK/vtkITKIslandMath.cxx
>>
>> which is instantiating images of type double and float in lines 170-171.
>>
>> I would argue that the warnings are real and
>> that they are indicating a conceptual error in
>> the instantiation of the filter.
>>
>> My suggestion will be to remove lines 170 and 171
>> from vtkITKIslandMath.cxx
>>
>>
>> Luis
>>
>>
>>
>>
>> -------------------------------------------------------------------------------
>>
>> On Mon, Apr 27, 2009 at 10:14 PM, 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
>>>>>>
>
More information about the Insight-developers
mailing list