-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Complex support for stft and istft #43886
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
Conversation
💊 CI failures summary and remediationsAs of commit d92008e (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 60 times. |
50c49c2 to
bb234e9
Compare
Codecov Report
@@ Coverage Diff @@
## master #43886 +/- ##
==========================================
- Coverage 67.95% 67.94% -0.01%
==========================================
Files 384 384
Lines 49743 49744 +1
==========================================
- Hits 33801 33800 -1
- Misses 15942 15944 +2
Continue to review full report at Codecov.
|
aten/src/ATen/native/SpectralOps.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment here, like in the above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this function I'm also curious about preserving the current behavior and supporting new complex behavior. How does SciPy's istft handle the case where onesided is false for float and complex inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For SciPy, if input_onesided is False it does a full ifft and always returns a complex array.
torch/functional.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if return_complex is true, ...",
but when return complex is none the behavior is dependent on the input and window, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but when return complex is none the behavior is dependent on the input and window, right?
Yes, but I describe the defaulting behaviour above this paragraph:
If :attr:`return_complex` is ``True`` (default if input is complex)
mruberry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @peterbell10, sorry it took me so long to take a look at this. The notification escaped me. Please ping me if I'm ever going more than a few days without getting back to you.
As expected, this is a thoughtful and well-written PR. One of the challenges with this work is that stft and istft functions are so old, implemented over two years ago. So I have questions about how we might improve on their bindings and documentation.
I like your approach to make these ops support complex, too, but have a couple questions about alternatives. I'm curious to hear your thinking on the best course of action.
|
Hey @peterbell10, is this ready for another look? |
531b0c0 to
85da7fe
Compare
|
Yes, this is ready for another look. I've also rebased on |
| in :math:`\left[0, 1, 2, \dots, \left\lfloor \frac{\text{n\_fft}}{2} \right\rfloor + 1\right]` | ||
| are returned because the real-to-complex Fourier transform satisfies the | ||
| conjugate symmetry, i.e., :math:`X[m, \omega] = X[m, \text{n\_fft} - \omega]^*`. | ||
| * If :attr:`onesided` is ``True`` (default for real input), only values for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a warning that this behavior will change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onesided default isn't deprecated but I added a warning to return_complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, this comment is at the wrong place. I also meant a warning in the documentation about the return_complex change.
torch/tensor.py
Outdated
| ) | ||
| return torch.istft(self, n_fft, hop_length, win_length, window, center, | ||
| normalized, onesided, length) | ||
| normalized, onesided, return_complex=return_complex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to passing length here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It seems the method variant isn't tested very well. I'll add it to one of the tests in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just ping me when it's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also notice the torch_function calls were missing arguments as well so I've updated those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mruberry this should be ready now, assuming CI passes.
mruberry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @peterbell10! This was especially tricky since we're not exactly following librosa or SciPy, and we want to update existing behavior.
I made a couple doc suggestions (like including a warning for stft) and have a question about preserving the length parameter on torch.Tensor.istft. Let me know when you're satisfied and we'll get this merged!
|
Looks like tests need a fix. |
|
Sorry about that @mruberry. CI is all passing now. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Ref #42175, fixes #34797
This adds complex support to
torch.stftandtorch.istft. Note that there are really two issues with complex here: complex signals, and returning complex tensors.Complex signals and windows
stftcurrently assumes all signals are real and usesrfftwithonesided=Trueby default. Similarly,istftalways takes a complex fourier series and usesirfftto return real signals.For
stft, I now allow complex inputs and windows by calling the fullfftif either are complex. If the user givesonesided=Trueand the signal is complex, then this doesn't work and raises an error instead. Foristft, there's no way to automatically know what to do whenonesided=Falsebecause that could either be a redundant representation of a real signal or a complex signal. So there, the user needs to pass the argumentreturn_complex=Truein order to useifftand get a complex result back.stft returning complex tensors
The other issue is that
stftreturns a complex result, represented as a(... X 2)real tensor. I think ideally we want this to return proper complex tensors but to preserver BC I've had to add areturn_complexargument to manage this transition.return_complexdefaults to false for real inputs to preserve BC but defaults to True for complex inputs where there is no BC to consider.In order to
return_complexby default everywhere without a sudden BC-breaking change, a simple transition plan could be:return_complex, defaulted to false when BC is an issue but giving a warning. (this PR)return_complexdefaults to false, making it a required argument.return_complexdefault to true in all cases.