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

Add FFT factory for CurvilinearArraySpecialCoordinatesImage image type #172

Closed
tbirdso opened this issue Jan 18, 2022 · 6 comments · Fixed by #182
Closed

Add FFT factory for CurvilinearArraySpecialCoordinatesImage image type #172

tbirdso opened this issue Jan 18, 2022 · 6 comments · Fixed by #182
Assignees

Comments

@tbirdso
Copy link
Contributor

tbirdso commented Jan 18, 2022

As of 5.3rc3 FFT filter overrides through the object factory override Image directly, so attempts to instantiate FFT filters with other image types fails unless a factory is explicitly registered. ITKUltrasound can be made to register necessary factory overrides by default.

Discussed in #161

@tbirdso
Copy link
Contributor Author

tbirdso commented Jan 19, 2022

@dzenanz @thewtex I'm getting a bit confused about itk::CurvilinearArraySpecialCoordinatesImage's class hierarchy and how it pertains to FFTs and other image filters.

itk::CurvilinearArraySpecialCoordinatesImage inherits from itk::ImageBase rather than itk::Image:

itk::CurvilinearArraySpecialCoordinatesImage -> itk::SpecialCoordinatesImage -> itk::ImageBase

However we have templated FFT filters over itk::Image:

itk::Image -> itk::ImageBase

I've tried revising editing FFT factories to override with itk::ImageBase specializations rather than itk::Image specializations, but this failed. ImageSource and its subclasses (including FFT filters) require template parameters to define PixelType, which it seems itk::SpecialCoordinatesImage and itk::Image do independent of each other.

Could one of you perhaps provide insight into why itk::SpecialCoordinatesImage is not a subclass of itk::Image?

I propose these potential paths forward:

  • Refactor itk::SpecialCoordinatesImage to inherit from itk::Image. Probably not the best solution as the existing structure is a the root of much of our hierarchy and refactoring could break things unexpectedly. Pending above discussion, I'm also guessing that there is a good reason for the current structure to be the way it is.
  • Add factories in ITK to override FFT filters for itk::SpecialCoordinatesImage inputs alongside existing itk::Image inputs. This would be the most effective at heading off similar failures in other external modules, though it would take the most time to review and release.
  • Add factories here in ITKUltrasound to override FFT filters for itk::CurvilinearArraySpecialCoordinatesImage inputs. Fastest solution, but with the least total impact.

@dzenanz
Copy link
Member

dzenanz commented Jan 19, 2022

Could one of you perhaps provide insight into why itk::SpecialCoordinatesImage is not a subclass of itk::Image?

itk::Image claims to be a regular grid (though it usually behaves in a slightly more general affine way). Therefore itk::SpecialCoordinatesImage does not inherit from it. I am not sure whether it could have been done differently, but it was done this way.

Add factories here in ITKUltrasound to override FFT filters for itk::CurvilinearArraySpecialCoordinatesImage inputs. Fastest solution, but with the least total impact.

I think this is the best solution.

@thewtex
Copy link
Member

thewtex commented Feb 7, 2022

InsightSoftwareConsortium/ITK#3156 will help address this issue, but it needs re"factor"ing first :-)

@tbirdso
Copy link
Contributor Author

tbirdso commented Feb 7, 2022

Note that an initial implementation was added in #161 but should be refined. itk::CurvilinearArraySpecialCoordinatesForwardFFTImageFilterFactory and itk::CurvilinearSpecialCoordinatesInverseFFTImageFilterFactory are both defined as inheriting from itk::ObjectFactoryBase with one representing a transform from physical curvilinear images -> itk::Image frequency images and the other overriding itk::Image frequency -> curvilinear image transforms. There is a good bit of redundancy inherent in both. These could potentially be reduced into one class using FFTImageFilterTraits to select the type of input/output image class, and potentially inheriting from FFTImageFilterFactory in ITK proper.

Also, factory overrides must currently be manually carried out for each FFT filter type being applied to an itk.CurvilinearArraySpecialCoordinatesImage and should be extended for default registration when the module is loaded. It will be best to wait until InsightSoftwareConsortium/ITK#3156 is finalized and take advantage of improved FFT factory registration mechanisms defined there.

@tbirdso
Copy link
Contributor Author

tbirdso commented Feb 9, 2022

Note that the 0.5.2 ITKUltrasound PyPi release is loadable in ITK, but this issue still blocks instantiation of classes that rely on an FFT filter with itk.CurvilinearArraySpecialCoordinatesImage input or output, such as itk::BModeImageFilter:

>>> itk.BModeImageFilter.values()[-1].New()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\tom.birdsong\Anaconda3\envs\venv-itk\lib\site-packages\itk\itkBModeImageFilterPython.py", line 154, in New
    obj = itkBModeImageFilterCASCID2CASCID2CASCcomplexID2.__New_orig__()
RuntimeError: C:\P\IPP\ITK-source\ITK\Modules\Filtering\FFT\include\itkForward1DFFTImageFilter.h:69:
ITK ERROR: Object factory failed to instantiate class itk::Forward1DFFTImageFilter<class itk::CurvilinearArraySpecialCoordinatesImage<double,2>,class itk::CurvilinearArraySpecialCoordinatesImage<class std::complex<double>,2> >

I'm uncertain of our ability to define default factory initializations in external modules via ITK macros as itkFFTImageFilterFactoryRegisterManager.h and other factory registration files are configured in ITK proper at the time wrapping is built and there is not an easy way to add additional default registrations. My impression is that ITK#3156 should make this easier, but will not be available until the next release candidate. Attempting to approach the problem now before ITK#3156 is completed could result in redundant code that will need to be refactored shortly thereafter.

@dzenanz @thewtex In your opinion is this functionality immediately critical, or should we wait until ITK Python factory registrations are stabilized in ITK?

@dzenanz
Copy link
Member

dzenanz commented Feb 9, 2022

We can wait with this for now, I think.

tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue Mar 7, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects.

Closes issue KitwareMedical#172.
tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue Mar 8, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects. Includes
additional wrappings for image inputs.

Closes issue KitwareMedical#172.
tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue Mar 8, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects. Includes
additional wrappings for image inputs.

Closes issue KitwareMedical#172.
tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue Apr 14, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects. Includes
additional wrappings for image inputs.

Closes issue KitwareMedical#172.
tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue Apr 14, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects. Includes
additional wrappings for image inputs.

Closes issue KitwareMedical#172.
tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue Apr 14, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects. Includes
additional wrappings for image inputs.

Closes issue KitwareMedical#172.
@tbirdso tbirdso linked a pull request Apr 14, 2022 that will close this issue
tbirdso added a commit to tbirdso/ITKUltrasound that referenced this issue May 9, 2022
Leverages ITK Python factory updates introduced in 10320a6 to initialize
default backends to run FFT on
`itk::CurvilinearArraySpecialCoordinatesImage` objects. Includes
additional wrappings for image inputs.

Closes issue KitwareMedical#172.
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 a pull request may close this issue.

3 participants