Add return_complex to F.spectrogram and T.Spectrogram
#1366
Conversation
mthrok
force-pushed
the
migrate-spectrogram
branch
2 times, most recently
from
2212bfa to
f23738b
Compare
mthrok
force-pushed
the
migrate-spectrogram
branch
from
f23738b to
7ff9741
Compare
mthrok
changed the title
return_complex to Spectrogram and MelSpectrogramreturn_complex to Spectrogram
mthrok
force-pushed
the
migrate-spectrogram
branch
from
7ff9741 to
f1a5459
Compare
mthrok
changed the title
return_complex to Spectrogramreturn_complex to F.spectrogram and T.Spectrogram
mthrok
force-pushed
the
migrate-spectrogram
branch
from
f1a5459 to
529de05
Compare
| 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We try to always mention the new path. To me, the definition of deprecation is two parts:
- this isn't recommended anymore
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I will make a follow up PR.
Co-authored-by: Brian Johnson <brianjo@fb.com>
4 participants
This PR adds
return_complexargument toF.spectrogramT.SpectrogramT.MelSpectrogramNote: Since
MelSpectrogramperforms scale conversion on real valued power spectrogram (R->R function),return_complexis not added to it.TODO:
return_complex=Truereturn_complex=Truereturn_complex=TruePart of #1337
Previous discussion #1009