KitwareMedical / ITKUltrasound

ITK module with classes particularly useful for ultrasound.
http://www.insight-journal.org/browse/publication/722
Apache License 2.0
51 stars 22 forks source link

Add Test Dependency on ITKVkFFTBackend #159

Open tbirdso opened 3 years ago

tbirdso commented 3 years ago

ITKUltrasound will depend on ITKVkFFTBackend for accelerated FFT computation.

ITKVkFFTBackend defines factory overrides for accelerated implementations of ITK FFT interface classes and should not be considered a direct dependency for ITKUltrasound. However, it would be valuable to test the user scenario where speedup on large ultrasound datasets can be accomplished with GPU-accelerated FFT.

Similar to GPU testing in ITKVkFFTBackend, a GPU-based test should be added to demonstrate speedup when accelerated FFTs are registered.

From ITKVkFFTBackend Issue 14.

tbirdso commented 3 years ago

@thewtex @dzenanz are there particular guidelines / design constraints that need to be considered for ITK remote modules depending on other ITK remote modules?

In this case of ITK -> ITKVkFFTBackend -> ITKUltrasound we can build ITKVkFFTBackend and its dependencies in CI, but I'm not clear on how this will affect or impede our ability to build ITKUltrasound as a remote module with ITK.

dzenanz commented 3 years ago

When building ITK directly, enabling one remote module will automatically enable all other remote modules it depends on.

When building externally, all the required dependencies must be enabled in the original ITK build.

tbirdso commented 3 years ago

Thanks @dzenanz . It sounds like it would cause issues to have ITKUltrasound depend on ITKVkFFTBackend without making ITKVkFFTBackend available as an ITK remote module. But, ITKVkFFTBackend is still under development.

Is it reasonable to add ITKVkFFTBackend as a remote module now? Alternatively this dependency change can be pushed back until 1D FFT base classes have been updated and moved to ITK.

dzenanz commented 3 years ago

this dependency change can be pushed back until 1D FFT base classes have been updated and moved to ITK

That seems reasonable. We could add VkFFTBackend as a remote module after 5.3 final is tagged.

dzenanz commented 3 years ago

1D FFT base classes have been updated and moved to ITK

This can probably be done before 5.3 final. It might even be desirable for using 5.3 final in the ultrasound-related remote modules.

tbirdso commented 2 years ago

Having a better understanding now of how FFT factory registration is intended to work, I'm not sure that we actually need ITKUltrasound to depend on ITKVkFFTBackend. The goal of how we're using the object factory is to override ITK interfaces with accelerated implementations, so as long as ITKUltrasound classes are written with respect to ITK FFT base interface classes then a user can independently substitute accelerated classes at will.

@thewtex @dzenanz would it be reasonable for me to either remove this issue or amend so that ITKUltrasound may depend on ITKVkFFTBackend as a test dependency only? (for verifying speedup in an ultrasound context)

dzenanz commented 2 years ago

Yes!

thewtex commented 2 years ago

We could avoid the module / C++ test dependency, and test in CI by installing the itk-vtkfftbackend package and running Python script tests.

tbirdso commented 2 years ago

That would help reduce dependencies. One caveat on this issue is that we can't provide GPU-accelerated FFTs for curvilinear ultrasound images without one module having knowledge of the other. We may find ourselves needing to revisit this issue at some point in the future.