-
Notifications
You must be signed in to change notification settings - Fork 732
Fix nan gradient by using native complex abs op #1013
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
Conversation
a305fb8 to
05bcf4d
Compare
| n_fft=2048, | ||
| hop_length=None, | ||
| win_length=None, | ||
| power=1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anjali411 Should I also ensure this when power is other than 1?
54df934 to
defc8b7
Compare
| if power == 1.0: | ||
| return spec_f.abs() | ||
| if power == 2.0 and spec_f.is_cuda: | ||
| return torch.view_as_real(spec_f).pow(power).sum(-1) | ||
| return spec_f.abs().pow(power) | ||
| return torch.view_as_real(spec_f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic could be moved to complex_norm directly, no? This would benefit other locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree we should move this logic to complex_norm
torchaudio/functional/functional.py
Outdated
| if power == 2.0 and spec_f.is_cuda: | ||
| return torch.view_as_real(spec_f).pow(power).sum(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why power == 2.0 and spec_f.is_cuda must be treated differently from the generic case on line 116 below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this is the reason. It's not clear the gain is significant. If this is indeed to go in, I would add a comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I would move the logic to compute the complex norm out of spectrogram, and move to the complex_norm functional. This would potentially benefit other locations, and keep the semantic cleaner.
defc8b7 to
ed798ad
Compare
Resolves #993