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

Add return_complex to F.spectrogram and T.Spectrogram #1366

Merged
merged 5 commits into from Apr 2, 2021

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Mar 6, 2021

This PR adds return_complex argument to

  • F.spectrogram
  • T.Spectrogram
  • T.MelSpectrogram
    Note: Since MelSpectrogram performs scale conversion on real valued power spectrogram (R->R function), return_complex is not added to it.

TODO:

  • Add autograd test for return_complex=True
  • Add TorchScript test for return_complex=True
  • Add librosa test for return_complex=True

Part of #1337
Previous discussion #1009

@mthrok mthrok modified the milestones: v0.8, v0.9 Mar 6, 2021
@mthrok mthrok requested a review from anjali411 March 6, 2021 04:27
@mthrok mthrok force-pushed the migrate-spectrogram branch 2 times, most recently from 2212bfa to f23738b Compare March 16, 2021 21:28
@mthrok mthrok changed the title [D] Add return_complex to Spectrogram and MelSpectrogram [D] Add return_complex to Spectrogram Apr 1, 2021
@mthrok mthrok changed the title [D] Add return_complex to Spectrogram Add return_complex to F.spectrogram and T.Spectrogram Apr 1, 2021
complex dtype, otherwise it returns the resulting Tensor in real dtype with extra
dimension for real and imaginary parts. (see ``torch.view_as_real``).
When ``power`` is provided, the value must be False, as the resulting
Tensor represents real-valued power.
Copy link

@anjali411 anjali411 Apr 2, 2021

Choose a reason for hiding this comment

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

I'd suggest adding a warning that Spectrogram now supports native complex tensors. Currently we return pseudo complex tensors (..., 2) by default, but that will change in the future and return_complex would be set to True by default ... something along those lines

Copy link
Collaborator Author

@mthrok mthrok Apr 2, 2021

Choose a reason for hiding this comment

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

I like the idea but in domain libraries, it is agreed to not mention the migration plan or future plan in docstring. (even though there are exceptions that I made, when this rule was made)

@cpuhrsch brought this up and this is meant to follow the same approach as PyTorch. But if PyTorch also mentions future plan in docstring, please point us to it.

The general idea behind of it is, this kind of direction should better live in release note or issue.

Copy link

Choose a reason for hiding this comment

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

We try to always mention the new path. To me, the definition of deprecation is two parts:

  1. this isn't recommended anymore
  2. this is what you should do instead

you could, I guess, but those in the release notes by why make it more difficult for your users?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh sorry I misread the comment by @anjali411 as adding the suggested sentence to docstring. Yes, there is nothing goes against on adding it to warning.

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 will make a follow up PR.

Copy link

@anjali411 anjali411 left a comment

Choose a reason for hiding this comment

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

Thanks Moto! This looks a great overall. Consider adding a warning to alert the users about this migration, and also a serialization test.

@mthrok mthrok merged commit 6a677ac into pytorch:master Apr 2, 2021
@mthrok mthrok deleted the migrate-spectrogram branch April 2, 2021 19:32
@mthrok mthrok modified the milestones: v0.9, Complex Tensor Migration Apr 5, 2021
mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
Co-authored-by: Brian Johnson <brianjo@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants