Skip to content

Add warning about complex tensor to spectrogram #1431

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add warning about complex tensor to spectrogram #1431

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb21724
Add warning about complex tensor to spectrogram
mthrok Apr 5, 2021
9c1da49
Update message
mthrok Apr 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions torchaudio/functional/functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link

@anjali411 anjali411 Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

@mthrok mthrok Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. Add support for native complex type (upcoming release)
  2. Switch to native complex type by default
  3. Remove pseudo complex type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

yup agreed! let's make a note of that in the main tracker issue so that we don't forget

"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, "
Copy link

Choose a reason for hiding this comment

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

Suggested change
"Currently, the function returns pseudo complex type (..., 2) by default, "
"Currently, this 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.")
Copy link

Choose a reason for hiding this comment

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

Suggested change
"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.")


if power is not None and return_complex:
raise ValueError(
'When `power` is provided, the return value is real-valued. '
Expand Down