RecursiveSeparableImageFilter the normal execution flow is broken.

1

Dear Dženan Zukić,

Recently you have changed the RecursiveSeparableImageFilter for new threading support.

Change-Id: I5ce9d8b965035c9716ee5b25694d39bc15ba666e
Date: Wed Aug 1 12:45:38 2018 -0400
ENH: refactoring RecursiveSeparableImageFilter to use new threading

And that has broken the normal flow and executing of this filter for us.
We have implemented the GPU(OpenCL) version of this filter in elastix see (itkGPURecursiveGaussianImageFilter).

http://elastix.isi.uu.nl/ or more extensive testing available in http://www.insight-journal.org/browse/publication/884

You have removed the BeforeThreadedGenerateData(), which was essential call to perform the setting up of this filter call (this->SetUp).
And move it into the GenerateData(). This is not how that suppose to work. In out implementation the GPUImageToImageFilter
follows the standard execution path of the ImageSource::GenerateData(), that is

GPUImageToImageFilter::GenerateData()
{
  if( !m_GPUEnabled )  // call CPU update function
  {
    Superclass::GenerateData();
  }
  else // call GPU update function
  {
    this->AllocateOutputs();

    this->BeforeThreadedGenerateData();

    this->GPUGenerateData();

    // Update CPU buffer for all outputs

    this->AfterThreadedGenerateData();
  }
}

What you have done for us is merging the BeforeThreadedGenerateData into GenerateData().
Therefore the GPU filter is not set correctly before the GPU performs the execution with GPUGenerateData().

I’ve locally modified the RecursiveSeparableImageFilter back to its original design.
After that all our GPU test are fine again. Please merge it back into master before the ITK 5.0

template< typename TInputImage, typename TOutputImage >
void
RecursiveSeparableImageFilter< TInputImage, TOutputImage >
::BeforeThreadedGenerateData()
{
  using RegionType = ImageRegion< TInputImage::ImageDimension >;

  typename TInputImage::ConstPointer inputImage( this->GetInputImage () );
  typename TOutputImage::Pointer     outputImage( this->GetOutput() );

  const unsigned int imageDimension = inputImage->GetImageDimension();

  if ( this->m_Direction >= imageDimension )
    {
    itkExceptionMacro("Direction selected for filtering is greater than ImageDimension");
    }

  const typename InputImageType::SpacingType & pixelSize =
    inputImage->GetSpacing();

  this->SetUp(pixelSize[m_Direction]);

  const RegionType region = outputImage->GetRequestedRegion();

  const unsigned int ln = region.GetSize()[this->m_Direction];

  if ( ln < 4 )
    {
    itkExceptionMacro("The number of pixels along direction " << this->m_Direction <<
      " is less than 4. This filter requires a minimum of four pixels along the dimension to be processed.");
    }
}

template< typename TInputImage, typename TOutputImage >
void
RecursiveSeparableImageFilter< TInputImage, TOutputImage >
::GenerateData()
{
  // Call a method that can be overridden by a subclass to allocate
  // memory for the filter's outputs
  this->AllocateOutputs();
  
  // Call a method that can be overridden by a subclass to perform
  // some calculations prior to splitting the main computations into
  // separate threads
  this->BeforeThreadedGenerateData();

  using RegionType = ImageRegion< TInputImage::ImageDimension >;
  typename TOutputImage::Pointer outputImage(this->GetOutput());
  const RegionType region = outputImage->GetRequestedRegion();

  this->GetMultiThreader()->SetNumberOfWorkUnits(this->GetNumberOfWorkUnits());
  this->GetMultiThreader()->template ParallelizeImageRegionRestrictDirection<TOutputImage::ImageDimension>(
    this->m_Direction, region, [this](const RegionType & lambdaRegion) { this->DynamicThreadedGenerateData(lambdaRegion); }, this);
}

Thanks, Denis P. Shamonin

2

Sure, I could do that. But I see that you already have a profile on Gerrit. If you submit a patch, you will retain full ownership and attribution. Tell me if you don’t want to do that.

I made that change in order to simplify the source code, unaware of your code depending on overriding BeforeThreadedGenerateData() method.

2 Likes
3

Hi Dženan,
Yes I have account there. But I need time to set up everything correctly. So, for this time I would prefer to skip it. Could you commit it with reference to this report? For next time I will make sure to submit it via Gerrit with patch.
Thanks, Denis

2 Likes
4

Gerrit patch submitted.