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

rollback torch.norm() in spectrogram() #747

Merged
merged 4 commits into from Jun 26, 2020
Merged

Conversation

lbjcom
Copy link
Contributor

@lbjcom lbjcom commented Jun 24, 2020

rollback torch.norm() in spectrogram() to v0.4.0 because torch.norm() is very slow in CPU mode as mentioned in following issues:

rollback torch.norm() in spectrogram() to v0.4.0.
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Hi @lbjcom

Thanks for submitting the PR. Looks like you forgot to remove the slow implementation.

torchaudio/functional.py Outdated Show resolved Hide resolved
comment out `spec_f = complex_norm(spec_f, power=power)`.
@vincentqb
Copy link
Contributor

Thanks for submitting this! It is indeed enough to address the original concern. However, as mentioned here, this affects any function invoking complex_norm (since the latter uses torch.norm). I would prefer fixing complex_norm here instead of spectrogram. Is there a reason why you modified spectrogram instead?

@lbjcom
Copy link
Contributor Author

lbjcom commented Jun 25, 2020

Thanks for submitting this! It is indeed enough to address the original concern. However, as mentioned here, this affects any function invoking complex_norm (since the latter uses torch.norm). I would prefer fixing complex_norm here instead of spectrogram. Is there a reason why you modified spectrogram instead?

That's a good point! The reason I changed the spectrogram() is that the difference between v0.4.0 and v0.5.0 is from that function:
image

But there's no change in complex_norm() between v0.4.0 and v0.5.0. So I changed spectrogram() since I don't know all history of torchaudio. If there's no problem, as you mentioned, fixing complex_norm() is the right solution 😄

@vincentqb
Copy link
Contributor

vincentqb commented Jun 25, 2020

Thanks for being careful! In this case though, changing complex_norm is the better solution: not only will it fix your problem, but it will also make complex_norm faster :)

@lbjcom
Copy link
Contributor Author

lbjcom commented Jun 26, 2020

Thanks for being careful! In this case though, changing complex_norm is the better solution: not only will it fix your problem, but it will also make complex_norm faster :)

I updated the code as you mentioned and found out that there's no problem in my training script. Thanks!

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting on tests to finish before merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants