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

Question on hilbert transform #13

Open
gzhu06 opened this issue Apr 15, 2021 · 3 comments
Open

Question on hilbert transform #13

gzhu06 opened this issue Apr 15, 2021 · 3 comments

Comments

@gzhu06
Copy link

gzhu06 commented Apr 15, 2021

Thanks for this project, very impressive contribution! I have a question on the hibert transform from the code asteroid-filterbanks/asteroid_filterbanks/analytic_free_fb.py :

    ft_f = rfft(self._filters, 1, normalized=True)
    hft_f = conj(ft_f)
    hft_f = irfft(hft_f, 1, normalized=True, signal_sizes=(self.kernel_size,))
    return torch.cat([self._filters, hft_f], dim=0)`

As far as I know, the hilbert transform is performed like this:

  1. Take the real part of the signal;
  2. Rotating the phase of the signal by 90°
  3. Analytical signal = real + i*(rotated signal).

From the code, it looks like using conj to perform rotation operation, is this correct?

@mpariente
Copy link
Contributor

mpariente commented Apr 15, 2021

You're absolutely right, thanks a lot!
It's a clear oversight I made when adding support to torch 1.8!

For torch under 1.8, it will work as expected though as what I called it conj (in a very stupid way), does the rotation

    def conj(filt):
        return torch.stack([filt[:, :, :, 1], -filt[:, :, :, 0]], dim=-1)

Do you want to submit a fix for torch version > 1.8 with the correct rotation? That would be awesome!

Thanks again for spoting it!

@gzhu06
Copy link
Author

gzhu06 commented Apr 15, 2021

Yes, I'd like to. I'll submit a fix some time this or next week.

Just want to make sure that to fix this: in 1.7, pytorch uses additional dimension to store real and image parts, while in 1.8 it's already a tensor in complex datatype.

So it's simply:

hft_f = -1j * ft_f

Right? Also I think conj could be misleading.

@mpariente
Copy link
Contributor

Yes, I'd like to. I'll submit a fix some time this or next week.

Thanks ! 🎉

Yes this is correct : hft_f = -1j * ft_f (-90° phase shift).

And the conj name should also be changed, you're right

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

2 participants