-
Notifications
You must be signed in to change notification settings - Fork 743
Griffin-Lim Transformation Implementation #365
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
|
Thanks for submitting the PR!
|
ec5a8bc to
3efca15
Compare
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.
To make sure the code works as expected, the following test will need to be added.
- compare against librosa's output (e.g. here), or a fixed example (say by exporting the output from librosa and loading in test).
- jitability, e.g. here
See other examples in test/test_functional.py and test/test_transforms.py.
|
@vincentqb Since EDIT: made |
|
@vincentqb As a part of implementing jitability for GriffinLim, I ended up writing jitability for istft as well. It passes the tests on my machine, but please double check? |
torchaudio/functional.py
Outdated
|
|
||
| if window is None: | ||
| window = torch.ones(win_length, requires_grad=False, device=device, dtype=dtype) | ||
| window = torch.ones(win_length) |
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.
Is there a reason to create the tensor and then move to a different device?
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.
It has to do with the JITability of the function: the signature of torch.ones seem not to allow for device and dtype setting when the out parameter is not specified. A workaround was to create the tensor then to move it.
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.
The ones and eye tensor can be created on device directly. No need for two separate steps.
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.
The last time I checked, the function signatures required an out parameter when specifying the device. Since out needed to be of type Tensor and window is of Optional[Tensor], it complained. I might be misinterpreting the errors, though.
Thanks! jitability test looks good to me! and thanks for taking care of |
44011d1 to
cff6cfc
Compare
vincentqb
left a comment
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. Waiting on test to run, and will merge.
- I removed the extra spectrogram step in test, and used smaller error bound.
- I matched the signature of the transform to the docstring.
Implementation of Griffin-Lim Transformation for feature request #351
@vincentqb I am not quite sure how I might go about writing the test code for this.
Meanwhile, I have a simple comparison of the waveforms of the
steam-train-whistle-daniel_simon.mp3file, which by the looks and sounds seem to be working properly.