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

[Cherry-picked 0.9] Ignore return_complex when returning real-valued tensor in spectrogram. #1551

Merged
merged 2 commits into from Jun 3, 2021

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jun 3, 2021

In 0.9.0, we are adding return_complex=False to spectrogram operation, so that users can choose whether they receive the resulting complex-valued tensor in pseudo complex tensor (real dtype with an extra dimension for real/imaginary parts) or native complex tensor (torch.cfloat / torch.cdouble).

Spectrogram operation returns raw, complex-valued tensor only when power=None. If power has some number, it returns the corresponding norm of the raw complex element. (such as power spectrogram) In this case, the value is always represented with real type, and there is no need to represent them in complex number.

The current version (0.9-RC1) checks if return_complex=True when power is not non. However, this does not align well with the use case. Users expecting power-spectrogram should not worry about this return_complex argument, which has its purpose in a different operation mode of spectrogram op. Also in #1549, I am switching the default value of return_complex to True and this means that by default, users who want power spectrogram has to provide return_complex=False additionally. This is totally unnecessary and avoidable. Therefore we should only care about the value of return_complex, when power=None in spectrogram op.

@mthrok
Copy link
Collaborator Author

mthrok commented Jun 3, 2021

@carolineechen Can you review this? The return_complex argument here is very similar to beta parameter of F.resample, which is only effective when dealing with Kaiser window.

Copy link
Contributor

@carolineechen carolineechen left a comment

Choose a reason for hiding this comment

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

looks good

torchaudio/functional/functional.py Outdated Show resolved Hide resolved
torchaudio/functional/functional.py Outdated Show resolved Hide resolved
@mthrok mthrok merged commit 6882342 into pytorch:master Jun 3, 2021
@mthrok mthrok deleted the fix-spectrogram branch June 3, 2021 18:08
@mthrok mthrok changed the title Ignore return_complex when returning real-valued tensor in spectrogram. [Cherry-picked 0.9] Ignore return_complex when returning real-valued tensor in spectrogram. Jun 3, 2021
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

3 participants