InsightSoftwareConsortium / ITKIsotropicWavelets

External Module for ITK, implementing Isotropic Wavelets and Riesz Filter for multiscale phase analysis.
Apache License 2.0
13 stars 11 forks source link

Python wrapping for all the classes. #56

Closed phcerdan closed 3 years ago

phcerdan commented 7 years ago

A list to keep track of classes without wrapping: https://github.com/phcerdan/ITKIsotropicWavelets/tree/master/wrapping:

Core

Decimate/Expand (in frequency or spatial domain).

Utilities


Wrap enhancements:

thewtex commented 7 years ago

:+1:

jhlegarreta commented 7 years ago

Which fits best/applies to the Decimate Expand filters: itk_wrap_image_filter (filters that need one or more image parameters of the same type) ; itk_wrap_image_filter_combinations (If it is desirable or required to instantiate an image filter with different image types) or itk_wrap_image_filter_types?

thewtex commented 7 years ago

@jhlegarreta It depends how the classes will be commonly used. There is a trade-off between build time / package size, and available options.

phcerdan commented 7 years ago

@jhlegarreta They are used in the Decimated pyramid on images with complex pixel type. I think the minimum requirements are: Dimensions: 2 and 3, PixelType: float/double, and complex<float/double>. The OutputImageType is ok to be the same than InputImageType.

phcerdan commented 7 years ago

Now I am thinking that the wrapping is to expose it to the user, the internal use of the Shrinker/Expander doesn't matter.

The Decimate filter basically removes every Nth pixel without interpolation (Nth pixel based on the m_ShrinkFactor), and the Expander adds zeros on those spots. I think they are good to have for the development of filters (especially in the signal analysis area) but not super useful for Pythonists I guess. Are there people developing filters directly in Python?

thewtex commented 7 years ago

Since we can't create new templated classes directly in Python, they can be hidden when wrapping.

jhlegarreta commented 7 years ago

When trying to a add the wrapping for the itk::StructureTensor, I've realized that it seems to be a filter or other construct, rather than a tensor, as defined in the module. The name seems misleading to me. Is the name still correct or should we propose a new one and do a separate PR?

AFAIK, ITK iterators are not wrapped. Does this mean that we cannot wrap subclasses? Shall we add a .notwrapped file for them?

About the utilities classes, I'd like to have some pointers at how to perform their wrapping, since they contain a variety of free functions, some requiring template arguments, and others not.

Thanks.

thewtex commented 7 years ago

AFAIK, ITK iterators are not wrapped. Does this mean that we cannot wrap subclasses? Shall we add a .notwrapped file for them?

Right, there is no use to wrapping the iterators. A .notwrapped file with a comment that explains this may help others in the future.

phcerdan commented 7 years ago

@jhlegarreta about itk::StructureTensor. You are right, it is indeed an ImageToImageFilter with the OutputPixel being a matrix. Thanks for pointing it!

The output matrix per pixel is composed by the eigenvectors and eigenvalues computed from a StructureTensor, we can factor out this StructureTensor into a new class. Should we append ImageFilter to the name to make it clear. Or even better but verbose: itkStructureTensorEigenSystemImageFilter?

Opening an issue to rename it and also to refactor a StructureTensor into a new class: #62

phcerdan commented 7 years ago

The refactor wont't affect the wrapping though, we would need to wrap an output of type:

 typename TOutputImage =
 itk::Image<itk::VariableSizeMatrix<typename TInputImage::PixelType>,TInputImage::ImageDimension> >

where TInputImage::PixelType would be limited to ${WRAP_ITK_REAL}

jhlegarreta commented 7 years ago

Renaming it to itkStructureTensorEigenSystemImageFilter seems OK to me. May be @thewtex can comment better on whether it conforms to what is used for potentially similar situations across the toolkit.

As for the wrapping, I'd wait for the refactoring to avoid requiring to rename the .wrap file for the sake of consistency/avoid forgetting it. If wrapping an image whose pixels are matrices at the same time is at least possible, as you said.

dzenanz commented 3 years ago

Can we close this now?

jhlegarreta commented 3 years ago

@dzenanz not sure if all classes are wrapped :worried:. Maybe @phcerdan knows better.

phcerdan commented 3 years ago

I don't think anything has changed to close it. Are open issues a burden? We can add a enhancement label to it, to differentiate them from bugs.