Skip to content

Conversation

@peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented Aug 31, 2020

Ref #42175, fixes #34797

This adds complex support to torch.stft and torch.istft. Note that there are really two issues with complex here: complex signals, and returning complex tensors.

Complex signals and windows

stft currently assumes all signals are real and uses rfft with onesided=True by default. Similarly, istft always takes a complex fourier series and uses irfft to return real signals.

For stft, I now allow complex inputs and windows by calling the full fft if either are complex. If the user gives onesided=True and the signal is complex, then this doesn't work and raises an error instead. For istft, there's no way to automatically know what to do when onesided=False because that could either be a redundant representation of a real signal or a complex signal. So there, the user needs to pass the argument return_complex=True in order to use ifft and get a complex result back.

stft returning complex tensors

The other issue is that stft returns 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 a return_complex argument to manage this transition. return_complex defaults 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_complex by default everywhere without a sudden BC-breaking change, a simple transition plan could be:

  1. introduce return_complex, defaulted to false when BC is an issue but giving a warning. (this PR)
  2. raise an error in cases where return_complex defaults to false, making it a required argument.
  3. change return_complex default to true in all cases.

@dr-ci
Copy link

dr-ci bot commented Aug 31, 2020

💊 CI failures summary and remediations

As 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.

See how this bot performed.

This comment has been revised 60 times.

@peterbell10 peterbell10 requested a review from mruberry August 31, 2020 17:38
@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 31, 2020
@peterbell10 peterbell10 force-pushed the complex-stft branch 5 times, most recently from 50c49c2 to bb234e9 Compare September 1, 2020 22:41
@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #43886 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
torch/overrides.py 98.05% <ø> (ø)
torch/functional.py 88.28% <100.00%> (ø)
torch/tensor.py 88.67% <100.00%> (+0.24%) ⬆️
torch/utils/_benchmark/utils/common.py 77.68% <0.00%> (-2.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e107ef5...d92008e. Read the comment docs.

@mruberry mruberry added the fft label Sep 9, 2020
Copy link
Collaborator

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

Copy link
Collaborator

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?

Copy link
Collaborator Author

@peterbell10 peterbell10 Sep 10, 2020

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.

Copy link
Collaborator

@mruberry mruberry Sep 10, 2020

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?

Copy link
Collaborator Author

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)

Copy link
Collaborator

@mruberry mruberry left a 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.

@mruberry
Copy link
Collaborator

Hey @peterbell10, is this ready for another look?

@peterbell10
Copy link
Collaborator Author

Yes, this is ready for another look. I've also rebased on viable/strict so CI can run properly.

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

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

@mruberry mruberry left a 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!

@mruberry
Copy link
Collaborator

Looks like tests need a fix.

@peterbell10
Copy link
Collaborator Author

Sorry about that @mruberry. CI is all passing now.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in caea1ad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

torch.stft does not accept complex input

4 participants