[Insight-developers] BillsBasement Nightly Failing Test
Luis Ibanez
luis.ibanez at kitware.com
Tue Feb 24 17:36:15 EST 2009
Hi Brad,
I made some clean up in the itkImageIORegion.
Moved the implementation of methods to the .cxx
file (most of it was in the header file).
Added bound checks to the
SetIndex(i)
GetIndex(i)
SetSize(i)
GetSize(i)
methods, and throw exceptions when they
are out of bounds.
Unfortunately it is still possible to be out
of bounds by calling
IndexType index = ioRegion.GetIndex()
and then accessing the index directly:
index[7];
---
Feel free to go ahead an review the code for
min dimension. It will be great is you write
it using the methods above, that now have
bounds checking.
And we have to add two explicit test
that do:
Read a 2D image into a 3D image
Read a 3D image into a 2D image
In that way we can verify that the code is
doing the right thing in both cases.
BTW: At the end, you are using this code to
figure out if the filter is currently streaming,
isn't it ?
It would probably be more appropriate to put
this into a bool method with a description
name (related to streaming).
BTW: Do you think that the registration failing tests in
mini3.nlm may be related in any way to these IO changes ?
Thanks
Luis
----------------------
Bradley Lowekamp wrote:
> Hello,
>
> Should I finish up this or are you taking care of it all?
>
> This is what I came up with for this section of code, incorporating your
> RegionIO safety:
>
> // this is a check to see if we are actually streaming
> ImageIORegion largestRegion(nDims);
> for(unsigned int i=0; i<nDims; i++)
> {
> largestRegion.SetIndex(i, 0);
> largestRegion.SetSize(i, this->GetDimensions(i));
> }
>
> // since largestRegion and m_IORegion may not be the same dimensions
> // we must be very careful in comparision and asignment to
> // indexMin/indexMax
> if(largestRegion.IsEqualWithRegionDimension(m_IORegion))
> {
> int* indexMin = new int[nDims];
> int* indexMax = new int[nDims];
> for(unsigned int i=0;i<nDims;i++)
> {
> if ( i < m_IORegion.GetImageDimension() )
> {
> indexMin[i] = static_cast<int>(m_IORegion.GetIndex(i));
> indexMax[i] = static_cast<int>(indexMin[i] +
> m_IORegion.GetSize(i) - 1);
> }
> else
> {
> indexMin[i] = 0;
> // this is zero since this is a (size - 1)
> indexMax[i] = 0;
> }
> }
>
> I also did a similar things in the write method too.
>
> On Feb 24, 2009, at 9:55 AM, Lowekamp, Bradley (NIH/NLM/LHC) [C] wrote:
>
>> Luis,
>>
>> I agree we should add more tests. I thought I added enough IO tests :)
>> Should this just be for meta or test it with a bunch of types? I can't
>> think of a way to validate this except valgrind and std lib debug.
>>
>>
>> Based on this methods:
>>
>> /** Dimension of the region to be written. This differs from the
>> * the image dimension and is calculated at run-time by examining
>> * the size of the image in each coordinate direction. */
>> unsigned int ImageIORegion::GetRegionDimension() const
>>
>> I think we should add:
>>
>> /* Performs element by element comparison up to the GetRegionDimension()
>> * This is equivalent to removing any trailing ones, then doing a
>> operator==
>> */
>> bool ImageIORegion::IsEqualWithRegionDimension(const Self &) const
>>
>>
>> There are clear use cases for this for reading. But there is
>> also similar code for writing, is this need? In itkImageFileWriter
>> there is a check to make sure the paste region is inside the largest.
>> This is enforce the IORegions are the same image dimension. So it may
>> not be, but if we have IsEqualWithRegionDimension is east todo.
>>
>> minIndex and maxIndex need to be padded with 1 up to nDims. This is
>> because metaIO has been initialized with nDims and is expecting things
>> to be of that size!
>>
>> Brad
>>
>>
>> On Feb 24, 2009, at 8:16 AM, Luis Ibanez wrote:
>>
>>>
>>> Brad,
>>>
>>> mm,...
>>> I don't see how...,
>>>
>>> minIndex and maxIndex are populated with values taken from
>>>
>>> m_IORegion.GetIndex()[i];
>>>
>>> so, there is no way to use more elements that the ones
>>> contained in m_IORegion...
>>>
>>>
>>> I think we should add a formal test to IO for
>>>
>>> * Reading a 2D image file into a 3D image type
>>> * Reading a 3D image file into a 2D image type
>>>
>>> This will illustrate both cases, and then we can verify if
>>> the behavior is correct.
>>>
>>> Luis
>>>
>>>
>>> ----------------------------------------------------------------------------------
>>> On Mon, Feb 23, 2009 at 11:19 PM, Bradley Lowekamp
>>> <blowekamp at mail.nih.gov <mailto:blowekamp at mail.nih.gov>> wrote:
>>>
>>> I think that indexMin and indexMax still need to be nDims size.?
>>>
>>> I agree that it make since to make this type of comparison part
>>> of the ImageIORegion API. As that is the only reason for this
>>> issue it to remove padded 1's at the end.
>>>
>>>
>>> On Feb 23, 2009, at 11:10 PM, Luis Ibanez wrote:
>>>
>>>> Hi Brad,
>>>>
>>>> The changes have been committed.
>>>>
>>>> Here is the log & diff
>>>>
>>>> http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/IO/itkMetaImageIO.cxx?root=Insight&r1=1.93&r2=1.94&sortby=date
>>>> <http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/IO/itkMetaImageIO.cxx?root=Insight&r1=1.93&r2=1.94&sortby=date>
>>>>
>>>> --------------------------------------------------------------------------------------------
>>>> BUG: When comparing the largestRegion against the
>>>> m_ImageIORegion, the minimum
>>>> of the dimensions of the two should be used. The reason is
>>>> that it should
>>>> possible to read a 2D image into a 3D image, (as a 1-slice
>>>> 3D image) and
>>>> it should be possible to read a 3D image into a 2D image by
>>>> taking only
>>>> the first slice. If we don't use the min dimension between
>>>> the largestRegion
>>>> and the m_ImageIORegion, one of the two is going to try to
>>>> access elements
>>>> out of bound during their comparison of index components
>>>> and sizes.
>>>> --------------------------------------------------------------------------------------------
>>>>
>>>> It almost look like this comparison & assignment
>>>> should be part of the API of the ImageIORegion...
>>>>
>>>>
>>>> Luis
>>>>
>>>>
>>>> -------------------------------------------------------------------------------------------------------
>>>> On Mon, Feb 23, 2009 at 10:53 PM, Luis Ibanez
>>>> <luis.ibanez at kitware.com <mailto:luis.ibanez at kitware.com>> wrote:
>>>>
>>>> Hi Brad,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> I think the problem is that this expression should be using
>>>> the minimum of both dimensions
>>>> between the largestRegion and the m_ImageIORegion.
>>>>
>>>>
>>>> I have the following local diff, that seems to fix the problem.
>>>> I'm running some more test before committing it.
>>>>
>>>>
>>>>
>>>> Index: itkMetaImageIO.cxx
>>>> ===================================================================
>>>> RCS file: /cvsroot/Insight/Insight/Code/IO/itkMetaImageIO.cxx,v
>>>> retrieving revision 1.93
>>>> diff -r1.93 itkMetaImageIO.cxx
>>>> 832,833c832,836
>>>> < // Pass the IO region to the MetaImage library
>>>> < unsigned int nDims = this->GetNumberOfDimensions();
>>>> ---
>>>> > const unsigned int nDims = this->GetNumberOfDimensions();
>>>> > const unsigned int ioDims =
>>>> this->m_IORegion.GetImageDimension();
>>>> >
>>>> > const unsigned int minDimension = ( nDims > ioDims ) ?
>>>> ioDims : nDims;
>>>> >
>>>> 837,838c840,841
>>>> < ImageIORegion largestRegion(m_IORegion);
>>>> < for(unsigned int i=0; i<nDims; i++)
>>>> ---
>>>> > ImageIORegion largestRegion(minDimension);
>>>> > for(unsigned int i=0; i<minDimension; i++)
>>>> 846,848c849,851
>>>> < int* indexMin = new int[nDims];
>>>> < int* indexMax = new int[nDims];
>>>> < for(unsigned int i=0;i<nDims;i++)
>>>> ---
>>>> > int* indexMin = new int[minDimension];
>>>> > int* indexMax = new int[minDimension];
>>>> > for(unsigned int i=0;i<minDimension;i++)
>>>> -------------------------------------------------------------
>>>>
>>>> Similar minDimension code has been used in
>>>> itkImageIORegion.h in lines 299-308
>>>>
>>>>
>>>>
>>>> Luis
>>>>
>>>>
>>>> --------------------------------------------------------
>>>>
>>>> On Mon, Feb 23, 2009 at 10:42 PM, Bradley Lowekamp
>>>> <blowekamp at mail.nih.gov <mailto:blowekamp at mail.nih.gov>> wrote:
>>>>
>>>> Hello,
>>>>
>>>> I am glad this was tracked down! Sorry it was my fault :(
>>>>
>>>> Giving it a quick look I see a small problem with this
>>>> change:
>>>>
>>>> http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/IO/itkMetaImageIO.cxx?root=Insight&r1=1.92&r2=1.93
>>>> <http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/IO/itkMetaImageIO.cxx?root=Insight&r1=1.92&r2=1.93>
>>>>
>>>> nDim and the dimensionality of m_RegionIO may not match.
>>>> Not sure why it'd be failing since the 30th if this was
>>>> the case. I'll have to give this some more thought
>>>> tomorrow. When those dimensions don't match is a tricky
>>>> case....
>>>>
>>>> Brad
>>>>
>>>> On Feb 23, 2009, at 10:04 PM, Luis Ibanez wrote:
>>>>
>>>>> Hi Brad,
>>>>>
>>>>> This error seems to be related to the changes in MetaImage.
>>>>>
>>>>> Here is the backtrace that I get from GDB:
>>>>>
>>>>>
>>>>> -----------------------------------------------------------------------
>>>>>
>>>>> (gdb) run itkImageToCooccurrenceListAdaptorTest
>>>>> /home/ibanez/src/Insight/Testing/Data/Input/HeadMRVolume.mhd
>>>>> Starting program:
>>>>> /home/ibanez/bin/InsightGcc4.3/Debug/bin/itkStatisticsTests
>>>>> itkImageToCooccurrenceListAdaptorTest
>>>>> /home/ibanez/src/Insight/Testing/Data/Input/HeadMRVolume.mhd
>>>>> [Thread debugging using libthread_db enabled]
>>>>> /usr/include/c++/4.3/debug/vector:237:error: attempt to
>>>>> subscript container
>>>>> with out-of-bounds index 2, but container only
>>>>> holds 2 elements.
>>>>>
>>>>> Objects involved in the operation:
>>>>> sequence "this" @ 0x0xbf9e58fc {
>>>>> type = NSt7__debug6vectorIlSaIlEEE;
>>>>> }
>>>>> [New Thread 0xb7d0e6c0 (LWP 8686)]
>>>>>
>>>>> Program received signal SIGABRT, Aborted.
>>>>> [Switching to Thread 0xb7d0e6c0 (LWP 8686)]
>>>>> 0xb7fe4430 in __kernel_vsyscall ()
>>>>> (gdb) bt
>>>>> #0 0xb7fe4430 in __kernel_vsyscall ()
>>>>> #1 0xb7d3b8a0 in raise () from
>>>>> /lib/tls/i686/cmov/libc.so.6
>>>>> #2 0xb7d3d268 in abort () from
>>>>> /lib/tls/i686/cmov/libc.so.6
>>>>> #3 0xb7ecd849 in
>>>>> __gnu_debug::_Error_formatter::_M_error () from
>>>>> /usr/lib/libstdc++.so.6
>>>>> #4 0x083c9d14 in std::__debug::vector<long,
>>>>> std::allocator<long> >::operator[] (this=0xbf9e58fc,
>>>>> __n=2) at /usr/include/c++/4.3/debug/vector:237
>>>>> #5 0x083c9d53 in itk::ImageIORegion::SetIndex
>>>>> (this=0xbf9e58f4, i=2, idx=0) at
>>>>> /home/ibanez/src/Insight/Code/IO/itkImageIORegion.h:183
>>>>> #6 0x085a60b8 in itk::MetaImageIO::Read
>>>>> (this=0x9335f40, buffer=0x93501a8) at
>>>>> /home/ibanez/src/Insight/Code/IO/itkMetaImageIO.cxx:840
>>>>> #7 0x084a2efb in
>>>>> itk::ImageFileReader<itk::Image<float, 2u>,
>>>>> itk::DefaultConvertPixelTraits<float> >::GenerateData
>>>>> (this=0x9331cf0) at
>>>>> /home/ibanez/src/Insight/Code/IO/itkImageFileReader.txx:406
>>>>> #8 0x0884cc49 in itk::ProcessObject::UpdateOutputData
>>>>> (this=0x9331cf0) at
>>>>> /home/ibanez/src/Insight/Code/Common/itkProcessObject.cxx:991
>>>>> #9 0x0883412b in itk::DataObject::UpdateOutputData
>>>>> (this=0x9334410) at
>>>>> /home/ibanez/src/Insight/Code/Common/itkDataObject.cxx:420
>>>>> #10 0x0883406a in itk::DataObject::Update
>>>>> (this=0x9334410) at
>>>>> /home/ibanez/src/Insight/Code/Common/itkDataObject.cxx:344
>>>>> #11 0x0884c00d in itk::ProcessObject::Update
>>>>> (this=0x9331cf0) at
>>>>> /home/ibanez/src/Insight/Code/Common/itkProcessObject.cxx:619
>>>>> #12 0x084855f3 in itkImageToCooccurrenceListAdaptorTest
>>>>> (argc=2, argv=0xbf9e6008) at
>>>>> /home/ibanez/src/Insight/Testing/Code/Numerics/Statistics/itkImageToCooccurrenceListAdaptorTest.cxx:53
>>>>> #13 0x0838fe3d in main (ac=3, av=0xbf9e6004) at
>>>>> /home/ibanez/src/Insight/Code/Common/itkTestMain.h:162
>>>>> -----------------------------------------------------------------------
>>>>>
>>>>> For some reason the itkImageIORegion thinks it is a 2D one
>>>>> while the MetaImage is asking for the component [2].
>>>>>
>>>>>
>>>>> Bill:
>>>>> I'm looking at the CVS diff of the day that you pointed
>>>>> out.
>>>>>
>>>>>
>>>>>
>>>>> Luis
>>>>>
>>>>>
>>>>> -----------------------------------------------------------------------
>>>>>
>>>>> On Mon, Feb 23, 2009 at 7:34 PM, Luis Ibanez
>>>>> <luis.ibanez at kitware.com
>>>>> <mailto:luis.ibanez at kitware.com>> wrote:
>>>>>
>>>>> Hi Bill,
>>>>>
>>>>> I'm looking at it...
>>>>>
>>>>> building with gcc 4.3 and STL debug on...
>>>>>
>>>>> Luis
>>>>>
>>>>>
>>>>> ---------------------------
>>>>>
>>>>> Bill Lorensen wrote:
>>>>>
>>>>> Luis,
>>>>>
>>>>> This build has stl debugging turned on. The test:
>>>>> http://www.cdash.org/CDash/viewTest.php?onlyfailed&buildid=277757
>>>>> <http://www.cdash.org/CDash/viewTest.php?onlyfailed&buildid=277757>
>>>>> shows an issue. Typically the culprit can be
>>>>> found by running the test
>>>>> from this build with gdb. However, I'm out of
>>>>> town and don't have
>>>>> access to my basement machine.
>>>>>
>>>>> I notice that other platforms are crashing
>>>>> also. The failures started
>>>>> on Jan 30. Since this test uses a meta file, I
>>>>> would look at the
>>>>> changes to MetaImageIO that occurred that day:
>>>>> http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/IO/itkMetaImageIO.cxx?root=Insight&r1=1.89&r2=1.90
>>>>> <http://public.kitware.com/cgi-bin/viewcvs.cgi/Code/IO/itkMetaImageIO.cxx?root=Insight&r1=1.89&r2=1.90>
>>>>>
>>>>>
>>>>> Bill
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>> ========================================================
>> Bradley Lowekamp
>> Lockheed Martin Contractor for
>> Office of High Performance Computing and Communications
>> National Library of Medicine
>> blowekamp at mail.nih.gov <mailto:blowekamp at mail.nih.gov>
>>
>>
>> <ATT00001.txt>
>
>
> ========================================================
>
> Bradley Lowekamp
>
> Lockheed Martin Contractor for
>
> Office of High Performance Computing and Communications
>
> National Library of Medicine
>
> blowekamp at mail.nih.gov <mailto:blowekamp at mail.nih.gov>
>
>
>
More information about the Insight-developers
mailing list