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

[Feature] Support Short-Time Fourier Transforms #1004

Open
Rifur13 opened this issue Apr 18, 2024 · 8 comments
Open

[Feature] Support Short-Time Fourier Transforms #1004

Rifur13 opened this issue Apr 18, 2024 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@Rifur13
Copy link
Contributor

Rifur13 commented Apr 18, 2024

Short-Time Fourier Transforms (STFT) and its inverse are used extensively in signal processing.

Function definition will match scipy:
https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.stft.html
https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.istft.html

@Rifur13
Copy link
Contributor Author

Rifur13 commented Apr 18, 2024

You can assign this to me.

@awni
Copy link
Member

awni commented Apr 18, 2024

Cool.. if we include this, I wonder where it would go? I'm not opposed to including it in core or a sub-package but I think there is a decision to be made there.

I see jax implemented it in a scipy.signal subpackage https://jax.readthedocs.io/en/latest/_autosummary/jax.scipy.signal.stft.html, and PyTorch implemented it at the top level https://pytorch.org/docs/stable/generated/torch.stft.html

Also our Whisper implementation already has an STFT in MLX you might find useful: https://github.com/ml-explore/mlx-examples/blob/main/whisper/whisper/audio.py#L104-L127

@awni awni added the enhancement New feature or request label Apr 18, 2024
@Rifur13
Copy link
Contributor Author

Rifur13 commented Apr 18, 2024

I think adding them in core is best - they’re fundamental algorithms and used frequently. A separate signal sub-package makes sense if you anticipate adding more functions from scipy.signal in he near future. (https://jax.readthedocs.io/en/latest/jax.scipy.html#module-jax.scipy.signal)

Thanks for pointing me to the Whisper implementation, I haven't seen in yet

@tqtifnypmb
Copy link

tqtifnypmb commented Apr 30, 2024

One tiny issue about stft in mlx-example/whisper, the noverlap parameter does not actually represent the overlap of frames; instead, it is semantically equivalent to hop_length in torch.stft.

I found it a little bit misleading while implementing istft.

@awni
Copy link
Member

awni commented Apr 30, 2024

Is it not the same as noverlap in the Scipy implementation? https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.stft.html

I think that's a good reference to follow.

@Rifur13
Copy link
Contributor Author

Rifur13 commented Apr 30, 2024

Still working on this CL, shouldn't be much longer.

t = (x.size - nperseg + noverlap) // noverlap does look wrong though, maybe that's where the confusion is. The denominator should be the number of steps.

@tqtifnypmb
Copy link

Is it not the same as noverlap in the Scipy implementation? https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.stft.html

I think that's a good reference to follow.

No.

According to this hop_length = nperseg - noverlap

In stft in mlx-example/whisper noverlap is being used as hop_length. It appears to be a naming mistake, the logic looks good to me.

@tqtifnypmb
Copy link

Still working on this CL, shouldn't be much longer.

Cool.

I was implementing a istft specific to my problem, not a general one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants