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 Test Dependency on ITKVkFFTBackend #159

Open
tbirdso opened this issue Sep 30, 2021 · 9 comments
Open

Add Test Dependency on ITKVkFFTBackend #159

tbirdso opened this issue Sep 30, 2021 · 9 comments
Assignees

Comments

@tbirdso
Copy link
Contributor

tbirdso commented Sep 30, 2021

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 tbirdso self-assigned this Sep 30, 2021
@tbirdso
Copy link
Contributor Author

tbirdso commented Sep 30, 2021

@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
Copy link
Member

dzenanz commented Sep 30, 2021

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
Copy link
Contributor Author

tbirdso commented Sep 30, 2021

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
Copy link
Member

dzenanz commented Sep 30, 2021

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
Copy link
Member

dzenanz commented Sep 30, 2021

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
Copy link
Contributor Author

tbirdso commented Feb 7, 2022

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
Copy link
Member

dzenanz commented Feb 7, 2022

Yes!

@tbirdso tbirdso changed the title Add Dependency on ITKVkFFTBackend Add Test Dependency on ITKVkFFTBackend Feb 7, 2022
@thewtex
Copy link
Member

thewtex commented Mar 10, 2022

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

@tbirdso
Copy link
Contributor Author

tbirdso commented Mar 10, 2022

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.

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

No branches or pull requests

3 participants