KEMBAR78
Add autograd test for T.GriffinLim by yoyolicoris · Pull Request #1421 · pytorch/audio · GitHub
Skip to content

Add autograd test for T.GriffinLim #1421

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

Jump to bottom
Merged
merged 9 commits into from
Apr 6, 2021
Merged

Add autograd test for T.GriffinLim #1421

merged 9 commits into from
Apr 6, 2021

Conversation

yoyolicoris
Copy link
Contributor

@yoyolicoris yoyolicoris commented Apr 3, 2021 •
edited
Loading

ref #1414
cc @mthrok

Have incorrect jacobian when doing gradcheck.

@yoyolicoris yoyolicoris marked this pull request as draft April 3, 2021 09:10
@mthrok mthrok mentioned this pull request Apr 5, 2021
Closed
21 tasks
@anjali411 anjali411 requested a review from mthrok April 5, 2021 17:51
@mthrok mthrok added this to the Complex Tensor Migration milestone Apr 5, 2021
@mthrok mthrok mentioned this pull request Apr 5, 2021
Merged
3 tasks
@mthrok
Copy link
Contributor

mthrok commented Apr 5, 2021 •
edited
Loading

Have incorrect jacobian when doing gradcheck.

Hi @yoyololicon

Thanks for the PR. Maybe the reason why it is failing could be the use of float() in the middle of the for loop.

audio/torchaudio/functional/functional.py

Lines 196 to 207 in 90311ed

for _ in range(n_iter):
# Store the previous iterate
tprev = rebuilt
# Invert with our current estimate of the phases
inverse = torch.istft(specgram * angles,
n_fft=n_fft,
hop_length=hop_length,
win_length=win_length,
window=window,
length=length).float()

Could you modify the code (just removing .float() is a good start and could be sufficient) so that all the intermediate variables have the same dtype as the input Tensor?

mthrok
mthrok reviewed Apr 5, 2021
View reviewed changes
@mthrok mthrok marked this pull request as ready for review April 5, 2021 22:19
@mthrok mthrok marked this pull request as draft April 5, 2021 22:20
@yoyolicoris yoyolicoris marked this pull request as ready for review April 6, 2021 05:26
@yoyolicoris yoyolicoris requested review from anjali411 and mthrok April 6, 2021 05:26
mthrok
mthrok approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

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

Looks good. Thank you!

@mthrok mthrok merged commit 6929331 into pytorch:master Apr 6, 2021
@mthrok mthrok mentioned this pull request Apr 6, 2021
Closed
15 tasks
@yoyolicoris yoyolicoris deleted the griffinlim-autograd-test branch April 6, 2021 15:49
carolineechen pushed a commit to carolineechen/audio that referenced this pull request Apr 30, 2021
@yoyolicoris
Add autograd test for T.GriffinLim (pytorch#1421)
d6a5d29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anjali411 anjali411 Awaiting requested review from anjali411

1 more reviewer

@mthrok mthrok mthrok approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed complex dtype migration

Projects

None yet

Milestone

Complex Tensor Migration

Development

Successfully merging this pull request may close these issues.

4 participants

@yoyolicoris @mthrok @anjali411 @facebook-github-bot