-
Notifications
You must be signed in to change notification settings - Fork 664
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
Fix nan gradient by using native complex abs op #1013
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
torchaudio/functional/functional.py
Outdated
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