[Insight-developers] Bug in Code/Review/itkOptImageToImageMetric.txx?
Hans Johnson
hans-johnson at uiowa.edu
Thu Jun 11 22:42:42 EDT 2009
Luis,
No problem. The variable m_NumberOfFixedImageSamples is no longer mutable,
and detangling so that const correctness is maintained without the need for
this to be mutable identified many of the dependancies.
There are still a few other suspect mutable variables in this class, but I
did not attempt to address if they were made mutable for performance
reasons, or coding ease.
I'll attack those another day when they start to cause me problems ;)
Hans
On 6/11/09 8:48 PM, "Luis Ibanez" <luis.ibanez at kitware.com> wrote:
>
> Hans,
>
>
> Thanks a lot for tracking and fixing this bug.
>
>
> Luis
>
>
>
> -----------------------
> Hans Johnson wrote:
>> Committed fix to previously reported bug.
>>
>> /cvsroot/Insight/Insight/Code/Review/itkOptImageToImageMetric.h,v <--
>> itkOptImageToImageMetric.h
>> new revision: 1.23; previous revision: 1.22
>> /cvsroot/Insight/Insight/Code/Review/itkOptImageToImageMetric.txx,v <--
>> itkOptImageToImageMetric.txx
>> new revision: 1.30; previous revision: 1.29
>>
>> --
>> Hans J. Johnson, Ph.D.
>> Associate Faculty
>> hans-johnson at uiowa.edu
>> 319 353 8587
>>
>>
>>
>> ------------------------------------------------------------------------
>> *From: *Hans Johnson <hans-johnson at uiowa.edu>
>> *Date: *Thu, 11 Jun 2009 14:15:14 -0500
>> *To: *Hans Johnson <hans-johnson at uiowa.edu>, Luis Ibanez
>> <luis.ibanez at kitware.com>, Kent Williams <norman-k-williams at uiowa.edu>
>> *Cc: *ITK <insight-developers at itk.org>
>> *Subject: *Re: [Insight-developers] Bug in
>> Code/Review/itkOptImageToImageMetric.txx?
>>
>> Luis,
>>
>> I have committed a test that now exercises the situation described
>> below. It should start failing on the dashboard for those builds that
>> use ITK_USE_OPTIMIZED_REGISTRATION_METHODS:BOOL=ON.
>>
>> cvs commit -m"BUG:0009139: Added a test that fails because the number of
>> samples is internally reduced when the fixed image mask is smaller than
>> the number of samples requested. This leads to an inconsistent
>> initialization of several variables. The solution listed in bug 0009139
>> ensures that all internal arrays are re-initialized to the correct
>> length if this internal reduction in samples occurs."
>> Testing/Code/Algorithms/itkMattesMutualInformationImageToImageMetricTest.cxx
>> Committer: Hans Johnson <hjohnson at mail.psychiatry.uiowa.edu>
>> /cvsroot/Insight/Insight/Testing/Code/Algorithms/itkMattesMutualInformationIm
>> ageToImageMetricTest.cxx,v
>> <--
>> Testing/Code/Algorithms/itkMattesMutualInformationImageToImageMetricTest.cxx
>> new revision: 1.21; previous revision: 1.20
>>
>>
>> ====================
>> I¹ve not yet committed the fix to the code that is attached to the bug
>> report. I¹ll do that in about 1 hour.
>>
>> Hans
>> --
>> Hans J. Johnson, Ph.D.
>> Associate Faculty
>> hans-johnson at uiowa.edu
>> 319 353 8587
>>
>>
>>
>> ------------------------------------------------------------------------
>> *From: *Hans Johnson <hans-johnson at uiowa.edu>
>> *Date: *Thu, 11 Jun 2009 11:15:26 -0500
>> *To: *Luis Ibanez <luis.ibanez at kitware.com>, Kent Williams
>> <norman-k-williams at uiowa.edu>
>> *Cc: *ITK <insight-developers at itk.org>
>> *Subject: *Re: [Insight-developers] Bug in
>> Code/Review/itkOptImageToImageMetric.txx?
>>
>> Luis,
>>
>> I think I¹ve finally tracked down the exact problem.
>>
>> Much of the code in ::MultiThreadingInitialize has a dependency on the
>> variable m_NumberOfFixedImageSamples for determining the length of the
>> weights arrays. This is OK if that variable were only set from the
>> class API before computing the registration. Unfortunately, after
>> initialization it is possible that the FixedImageRegion, or the number
>> of voxels available in the FixedImage mask can change this value after
>> initialization to a smaller number. If that is the case, then the size
>> of the weights arrays also need to be re-adjusted in size. Ultimately
>> the segmentation fault arises in a for loop over several arrays that
>> should all be the same length (i.e. The new smaller value of
>> m_NumberOfFixedImageSamples), but the loop index is specified as one of
>> the arrays that has not been adjusted to the smaller size.
>>
>> Currently the code changes I have pass all the old regression test, and
>> produce a good result in the ³real world² code I have (where it was
>> previously segmentation faulting). I do not yet have a regression test
>> that fails with the old code and succeeds with the new code.
>>
>> I¹ve spent more time trying to make a small test case than it took to
>> actually fix the bug that was discovered by running a real test case.
>> Currently when attempting to make a test case that only causes this
>> problem, other error checking is stepping in front of this and not even
>> getting to cause this problem.
>>
>> I¹ve added bug number ³0009139² with a proposed patch that still needs
>> testing.
>>
>> Hans
>>
>> --
>> Hans J. Johnson, Ph.D.
>> Associate Faculty
>> hans-johnson at uiowa.edu
>> 319 353 8587
>>
>>
>>
>> ------------------------------------------------------------------------
>> *From: *Luis Ibanez <luis.ibanez at kitware.com>
>> *Date: *Wed, 10 Jun 2009 12:57:56 -0400
>> *To: *Kent Williams <norman-k-williams at uiowa.edu>
>> *Cc: *ITK <insight-developers at itk.org>
>> *Subject: *Re: [Insight-developers] Bug in
>> Code/Review/itkOptImageToImageMetric.txx?
>>
>>
>> Hi Kent,
>>
>> The number of fixed samples is updated according the number
>> of pixels inside the (potential) fixed image mask in lines: 454-513,
>>
>> more specifically in 467-472
>>
>> if ( count > maxcount || randIter.IsAtEnd() )
>> {
>> m_NumberOfFixedImageSamples = samples_found;
>> samples.resize(samples_found);
>> break;
>> }
>>
>>
>> but... of course... it is always possible that there is a bug
>> and we missed some combination of circumstances.
>>
>>
>> Any chance that we could reduce this to a minimal case ?
>>
>>
>> E.g. I would think in taking one of the examples in
>> Insight/Examples/Registration and adding a Mask to it,
>> so that we can reproduce this in a small test case.
>>
>> I'll be happy to help setting this up.
>>
>>
>>
>> BTW: In your case are us using AllPixels or are you
>> letting the Metric subsample the image ?
>>
>> Depending on that, the code will take two different paths.
>>
>>
>> Thanks
>>
>>
>> Luis
>>
>>
>> -----------------------------------------------------------------------------
>> ---------------------
>> On Wed, Jun 10, 2009 at 12:27 PM, kent williams
>> <norman-k-williams at uiowa.edu> wrote:
>>
>> We ran into a problem that Hans could probably explain better than
>> I: We are
>> getting core dumps in itkOptImageToImageMetric.txx.
>>
>> The problem is that there is a member array
>> m_BSplineTransformWeightsArray,
>> that gets sized based on m_NumberOfFixedImageSamples and
>> m_NumBSplineWeights. It's getting allocated (in our case to 10,000
>> samples), but when it gets filled in (in
>> PreComputeTransformValues()), every
>> element is not set -- some are left with a value of zero or some random
>> value.
>>
>> I think the problem is that the iteration over weights doesn't take into
>> account the reduced actual sample count due to using masks.
>>
>>
>> if(sampleOk)
>> {
>> // If the transform is BSplineDeformable, we can use the
>> precomputed
>> // weights and indices to obtained the mapped position
>> const WeightsValueType * weights = if(sampleOk)
>> {
>> // If the transform is BSplineDeformable, we can use the
>> precomputed
>> // weights and indices to obtained the mapped position
>> const WeightsValueType * weights =
>>
>> m_BSplineTransformWeightsArray[sampleNumber];
>> const IndexValueType * indices =
>>
>> m_BSplineTransformIndicesArray[sampleNumber];
>>
>> for( unsigned int j = 0; j < FixedImageDimension; j++ )
>> {
>> mappedPoint[j] =
>> m_BSplinePreTransformPointsArray[sampleNumber][j];
>> }
>>
>> for ( unsigned int k = 0; k < m_NumBSplineWeights; k++ )
>> {
>> for ( unsigned int j = 0; j < FixedImageDimension; j++ )
>> {
>> //CRASHES HERE line 908
>> mappedPoint[j] += weights[k] * m_Parameters[ indices[k]
>> + m_BSplineParametersOffset[j] ];
>> }
>> }
>> }
>> }
>>
>>
>> m_BSplineTransformWeightsArray[sampleNumber];
>> const IndexValueType * indices =
>>
>> m_BSplineTransformIndicesArray[sampleNumber];
>>
>> for( unsigned int j = 0; j < FixedImageDimension; j++ )
>> {
>> mappedPoint[j] =
>> m_BSplinePreTransformPointsArray[sampleNumber][j];
>> }
>>
>> for ( unsigned int k = 0; k < m_NumBSplineWeights; k++ )
>> {
>> for ( unsigned int j = 0; j < FixedImageDimension; j++ )
>> {
>> mappedPoint[j] += weights[k] * m_Parameters[ indices[k]
>> +
>> m_BSplineParametersOffset[j] ];
>> }
>> }
>> }
>> }
>>
>>
>>
>>
>>
>>
>> ------------------------------------------------------------------------
>> _______________________________________________
>> 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
>>
>> ------------------------------------------------------------------------
>> _______________________________________________
>> 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