Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: ENH: Add BlockMatching registration Python wrappings + example #150

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tbirdso
Copy link
Contributor

@tbirdso tbirdso commented Jul 9, 2021

Adding wrappings and Python example to port block matching multi-resolution registration test from cxx. Wrappings will allow us to more rapidly iterate over registration parameter adjustments and take advantage of tools for examining intermediate results in Python.

Issues with runtime warnings to be discussed in comments below.

Comment on lines +27 to +34
# Wrap class hierarchy
itk_wrap_include("itkImageToImageFilter.h")
itk_wrap_class("itk::ImageToImageFilter" POINTER)
foreach(t ${WRAP_ITK_REAL})
foreach(d ${ITK_WRAP_IMAGE_DIMS})
itk_wrap_template("IIR${d}${d}I${ITKM_V${t}${d}}${d}"
"itk::Image<itk::ImageRegion<${d}>,${d}>, itk::Image<${ITKT_V${t}${d}},${d}>")
endforeach()
endforeach()
itk_end_wrap_class()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing the following warnings when instantiating any BlockMatching class at runtime:

Template itk::ImageToImageFilter<itk::Image<itk::Vector<double,2>,2>,itk::Image<itk::CovariantVector<double,2>,2>>
already defined as <class 'itk.itkImageToImageFilterAPython.itkImageToImageFilterIVD22ICVD22'>
is redefined as {cl}
Warning: template already defined 'itk::ImageToImageFilter<itk::Image<itk::Vector<double,2>,2>,itk::Image<itk::CovariantVector<double,2>,2>>'
Template itk::ImageToImageFilter<itk::Image<itk::Vector<double,3>,3>,itk::Image<itk::CovariantVector<double,3>,3>>
already defined as <class 'itk.itkImageToImageFilterAPython.itkImageToImageFilterIVD33ICVD33'>
is redefined as {cl}
Warning: template already defined 'itk::ImageToImageFilter<itk::Image<itk::Vector<double,3>,3>,itk::Image<itk::CovariantVector<double,3>,3>>'

The warnings are introduced with these changes, but we are not explicitly wrapping ImageToImageFilter for these templates. Any thoughts on where this could be coming from?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 interesting. I do not see anything via inspection, either. 👁️ I will start a local build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewtex Were you able to reproduce by any chance?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not reproduce these warnings locally.

I did get

itkBlockMatchingMultiResolutionImageRegistrationMethod: warning(4): ITK type not wrapped, or currently not known: itk::Image< itk::Point< double, 2 >, 2 >
itkBlockMatchingMultiResolutionImageRegistrationMethod: warning(4): ITK type not wrapped, or currently not known: itk::Image< itk::Point< double, 3 >, 3 >

at build time.

And,

>>> itk.Ultrasound.NormalizedCrossCorrelationMetricImageFilter.New()
Warning: Unknown parameter 'itk::SmartPointer<itk::Image<float,2>>' in template 'itk::Image'
Warning: Unknown parameter 'itk::SmartPointer<itk::Image<float,3>>' in template 'itk::Image'
Namespace already has a value for ImageRegistrationMethod, which is not an itkTemplate instance for class itk::BlockMatching::ImageRegistrationMethod. Overwriting old value.
Namespace already has a value for MultiResolutionImageRegistrationMethod, which is not an itkTemplate instance for class itk::BlockMatching::MultiResolutionImageRegistrationMethod. Overwriting old value.
<itk.ITKCommonBasePython.itkObject; proxy of <Swig Object of type 'itkObject *' at 0x7fa4d95d8660> >

at run time.

We could likely replace the SmartPointer's with raw pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thanks for taking a look. I'll try a clean build over the weekend and see if warnings still appear on my end.

My guess is that the point wrapping warnings are related to my having ITK_WRAP_DOUBLE enabled locally, I'll take a closer look.

We could likely replace the SmartPointer's with raw pointers.
Will do. This would occur within the cxx classes as well as the wrapping files, right?

Comment on lines 7 to 23
# Wrap base classes for itk::BlockMatching::MetricImageToDisplacementCalculator public members
itk_wrap_class("itk::Image" POINTER)
foreach(t ${WRAP_ITK_REAL})
foreach(d ${ITK_WRAP_IMAGE_DIMS})
itk_wrap_template("P${ITKM_${t}}${d}${d}" "itk::Point<${ITKT_${t}},${d}>, ${d}")
itk_wrap_template("SPI${ITKM_${t}}${d}${d}" "itk::SmartPointer<itk::Image<${ITKT_${t}},${d}>>, ${d}")
endforeach()
endforeach()
itk_end_wrap_class()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Image wrappings are required for itkBlockMatchingMetricImageToDisplacementCalculator public members of type 'MetricImageImageType. However, even after adding this wrapping we're seeing warnings about unknown parameters:

Warning: Unknown parameter 'itk::SmartPointer<itk::Image<float,2>>' in template 'itk::Image'
Warning: Unknown parameter 'itk::SmartPointer<itk::Image<float,3>>' in template 'itk::Image'
Warning: Unknown parameter 'itk::SmartPointer<itk::Image<double,2>>' in template 'itk::Image'
Warning: template already defined 'itk::Image<itk::SmartPointer<itk::Image<double,2>>,2>'
Warning: Unknown parameter 'itk::SmartPointer<itk::Image<double,3>>' in template 'itk::Image'
Warning: template already defined 'itk::Image<itk::SmartPointer<itk::Image<double,3>>,3>'

This may be an issue with SmartPointer not being wrapped, will revisit shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attempting to wrap SmartPointer yields compile time errors. Could this be an issue with circular dependencies?

16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(42678,30): error C3861: 'itkImageD2_cast': identifier not found
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(43971,16): error C3861: 'itkImageBase2_TransformPhysicalPointToIndex': identifier not found
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(43995,3): error C2065: 'itkContinuousIndexD2': undeclared identifier
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(43995,24): error C2146: syntax error: missing ';' before identifier 'result'
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(43995,24): error C2065: 'result': undeclared identifier
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(44037,7): error C2065: 'result': undeclared identifier
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(44037,16): error C3861: 'itkImageBase2_TransformPhysicalPointToContinuousIndex': identifier not found
16>C:\repos\itkus\Wrapping\Modules\Ultrasound\itkBlockMatchingMultiResolutionImageRegistrationMethodPython.cpp(44046,15): error C2061: syntax error: identifier 'itkContinuousIndexD2'
...

@tbirdso tbirdso requested review from dzenanz and thewtex July 9, 2021 13:44
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I don't have advice for hunting down the warnings :(

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbirdso beautiful!!

Comment on lines +27 to +34
# Wrap class hierarchy
itk_wrap_include("itkImageToImageFilter.h")
itk_wrap_class("itk::ImageToImageFilter" POINTER)
foreach(t ${WRAP_ITK_REAL})
foreach(d ${ITK_WRAP_IMAGE_DIMS})
itk_wrap_template("IIR${d}${d}I${ITKM_V${t}${d}}${d}"
"itk::Image<itk::ImageRegion<${d}>,${d}>, itk::Image<${ITKT_V${t}${d}},${d}>")
endforeach()
endforeach()
itk_end_wrap_class()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 interesting. I do not see anything via inspection, either. 👁️ I will start a local build.

@tbirdso
Copy link
Contributor Author

tbirdso commented Jul 16, 2021

Separated out some BlockMatching wrappings into individual files and added itkBlockMatchingBayesianRegularizationDisplacementCalculator.wrap.

@thewtex
Copy link
Member

thewtex commented Jul 20, 2021

Great progress here.

Next steps appear to be

  1. Rename classes to unique names when there is a conflict.
  2. Change SmartPointer to raw pointers in classes if possible.

@tbirdso
Copy link
Contributor Author

tbirdso commented Jul 30, 2021

Wrapping succeeds and MultiResolutionImageRegistrationMethod Python example passes, but seeing a new CTest error:

28/33 Test #28: itkBlockMatchingBayesianRegularizationDisplacementCalculatorTest ......................***Exception: SegFault 7.91 sec

Debugging locally, the error happens here in itkBlockMatchingBayesianRegularizationDisplacementCalculator.hxx:

MetricImageImageIteratorType imageImageIt(this->m_MetricImageImage, region);
MetricImagePointerType image;
// We add eps so we don't take the log of zero, and having a probability of
// 0.0 is ... pessimistic;-)
const PixelType lowerBound = this->m_MetricLowerBound + NumericTraits::epsilon();
for (imageImageIt.GoToBegin(); !imageImageIt.IsAtEnd(); ++imageImageIt)
{
image = imageImageIt.Get();
MetricImageIteratorType it(image, image->GetLargestPossibleRegion());

Where the segfault is thrown when the test attempts to access metadata for the MetricImageType object image, which is not a nullptr.

Current hypothesis is that changing MetricImageImageType from holding smart pointers to MetricImageType objects to holding raw pointers instead causes reference counts to not be updated when we try to initialize each MetricImageImageType pixel with a smart pointer to an allocated metric image, so the memory buffer for each metric image is deallocated before it can be used. We can't rely on smart pointer pixel types here due to wrapping constraints and I assume defining each pixel directly as a MetricImage could severely impact performance.

@thewtex do you have thoughts on this?

@thewtex
Copy link
Member

thewtex commented Dec 8, 2021

Current hypothesis is that changing MetricImageImageType from holding smart pointers to MetricImageType objects to holding raw pointers instead causes reference counts to not be updated

Yes, that sounds right -- we may have to revert that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants