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: Add FFTHilbertImageFilter #371

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

Conversation

acoussat
Copy link
Collaborator

First attempt at creating a filter to compute the Hilbert transform of an image.

A few questions:

  • there are no tests of this new filter; should I write some?
  • the kernel is a band-limited Hilbert kernel, but there are no ways in the current implementation to choose that band-limit, should that be an option?

Thanks!

Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

In addition to the 2 comments, I think we should remove the application and create a wrap file. I think @acoussat does not have time to finalize this in the coming days, do you need it urgently @mathu59?

{
ix = it.GetIndex();

double x = ix[0] * spacing;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a m_PixelShift option. This will require redefining the GenerateOutputInformation() member.

#include "rtkFFTProjectionsConvolutionImageFilter.h"
#include "rtkMacro.h"

// The Set macro is redefined to clear the current FFT kernel when a parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is useless in thecurrent code since itkSetMacro is not used. I am suggesting to add a new member, m_PixelShift, so it would become useful, but it's actually simpler to directly use the new definition instead of this complicated trick.

@acoussat
Copy link
Collaborator Author

I have implemented all your comments, thanks again!

I have also created a test for this new filter, but it might still require some work. Specifically:

  • I think I correctly disabled zero padding to write the test, but I don't exactly get the result I am supposed to (the one obtained analytically). Am I supposed to manually mirror the signal out of its current bounds? Does RTK already has an option somewhere to do this?
  • I don't really know how to pick good parameters for CheckImageQuality, as of now they are nothing but random choices. Any tip about that?

Thanks!

@acoussat
Copy link
Collaborator Author

Here is a comparison representing the current state of the test. The signal t -> sin(20*pi*t) has the analytic Hilbert transform t -> -cos(20*pi*t). The current implementation kind of matches the analytic inverse, but still seems incorrect.
hilbert_compare
I'm unsure about how to improve that... There is probably something to do with the way zero padding is performed, but I'm not sure I understand. Any pointers?

@thewtex
Copy link
Contributor

thewtex commented Dec 18, 2020

Hi,

Hopefully a helpful pointer (and a request :-)) for a monogenic signal filter:

https://arxiv.org/abs/1703.09199

which is the multi-dimensional analog to the analytic signal from the Hilbert transform. We have an AnalyticSignalImageFilter here:

https://github.com/KitwareMedical/ITKUltrasound/blob/master/include/itkAnalyticSignalImageFilter.h

@SimonRit
Copy link
Collaborator

Thanks. itkAnalyticSignalImageFilter.hseems to be doing the same thing as the current rtkHilbertImageFilter.h (which was badly named because we followed Matlab). Is this going to be moved to an ITK module at some point?

@thewtex
Copy link
Contributor

thewtex commented Dec 21, 2020

@SimonRit Yes, we will make ITKUltrasound available as an ITK remote module following @dzenanz 's work to support remote module dependencies in ITK 5.2:

InsightSoftwareConsortium/ITK@fa1082b#diff-e7efda826b6d097e47d7cb2cd6b1cf126084197d7834e7b022048183509c774b

It would be nice to also have ultrasound filters depend on the CUDA filter support in RTK. I am wondering if now is the time to create a CUDA base module, and a module for FFT extensions, e.g. analytic signal, etc. that the ultrasound and RTK modules could share as dependencies. What do you think?

@SimonRit
Copy link
Collaborator

Sounds good. For CUDA, it might be easy but we should fix #296. For FFT extensions, RTK would certainly benefit from the analytic signal. We also have filters which work line-by-line or slice-by-slice, the mother class is rtk::FFTProjectionsConvolutionImageFilter. They are pretty specific to tomographic reconstruction but they might be useful in other contexts. I'm sure you'll have some changes to suggest on the code to make it better!

@acoussat
Copy link
Collaborator Author

I'm not sure I understand the conclusion of this discussion. Do we keep the current implementation of the Hilbert transform? Should I improve it somehow, for instance by including the analytic signal? Should it be moved somewhere, like to a module that contains FFT extensions?

@dzenanz
Copy link
Contributor

dzenanz commented Jan 25, 2021

I understood that I should finish KitwareMedical/ITKUltrasound#123 before proceeding. Then we should take the parts of ITKUltrasound which overlap ones in RTK and put them in a new common module on which both ITKUltrasound and RTK would depend.

@SimonRit SimonRit mentioned this pull request May 25, 2021
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.

4 participants