Add warning about complex tensor to spectrogram #1431
Conversation
torchaudio/functional/functional.py
Outdated
| warnings.warn( | ||
| "spectrogram now supports returning native complex tensor " | ||
| "when `power=None` by setting `return_complex=True`. " | ||
| "Currently, the function returns pseudo complex type (..., 2) by default, " |
There was a problem hiding this comment.
| "Currently, the function returns pseudo complex type (..., 2) by default, " | |
| "Currently, this function returns pseudo complex type (..., 2) by default, " |
torchaudio/functional/functional.py
Outdated
| "when `power=None` by setting `return_complex=True`. " | ||
| "Currently, the function returns pseudo complex type (..., 2) by default, " | ||
| "but this will change in the future and `return_complex` would be set to True by default. " | ||
| "Please refer to https://github.com/pytorch/audio/issues/1337 for the detail.") |
There was a problem hiding this comment.
| "Please refer to https://github.com/pytorch/audio/issues/1337 for the detail.") | |
| "Please refer to https://github.com/pytorch/audio/issues/1337 for more details about the migration plan.") |
| @@ -84,6 +84,14 @@ def spectrogram( | |||
| ``n_fft // 2 + 1`` and ``n_fft`` is the number of | |||
| Fourier bins, and time is the number of window hops (n_frame). | |||
| """ | |||
| if power is None and not return_complex: | |||
| warnings.warn( | |||
There was a problem hiding this comment.
I think we should explicitly mention it's a deprecation warning:
"Deprecation warning: In a future TorchAudio release, `spectrogram` ",
"will no longer return tensors of pseudo complex type (..., 2) by default. ",
"Instead, `return_complex` will be set to `True` by default and spectrogram",
"would return native complex tensors by default."
There was a problem hiding this comment.
actually even better, we could use https://docs.python.org/3/library/exceptions.html#DeprecationWarning
warnings.warn(msg, category=DeprecationWarning) where msg equals
"In a future TorchAudio release, `spectrogram` ",
"will no longer return tensors of pseudo complex type (..., 2) by default. ",
"Instead, `return_complex` will be set to `True` by default and spectrogram",
"would return native complex tensors by default."
There was a problem hiding this comment.
@anjali411
I do not have a preference here but, according to this comment by @gchanan, PyTorch uses UserWarning for deprecation, so in torchaudio we used UserWarning.
However, I see mixed usage in pytorch.
https://github.com/pytorch/pytorch/search?q=DeprecationWarning
https://github.com/pytorch/pytorch/blob/c371542efc31b1abfe6f388042aa3ab0cef935f2/torch/hub.py#L512
There was a problem hiding this comment.
@anjali411 Do we want to deprecate the pseudo complex in the upcoming release?
The planned migrations phases are as following and I was thinking to deprecate the native complex type in phase 2.
- Add support for native complex type (upcoming release)
- Switch to native complex type by default
- Remove pseudo complex type
There was a problem hiding this comment.
I gave some thoughts on this and now I think it makes more sense to give heads up on deprecation prior to defaulting native complex type.
There was a problem hiding this comment.
yup agreed! let's make a note of that in the main tracker issue so that we don't forget
|
See #1445 |
Follow-up of #1366 .
cc. @anjali411
part of #1337
Replaced by #1445