Griffin-Lim Transformation Implementation#365
Conversation
|
Thanks for submitting the PR!
|
jaeyeun97
force-pushed
the
feature_griffinlim
branch
from
ec5a8bc to
3efca15
Compare
There was a problem hiding this comment.
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 |
jaeyeun97
changed the title
|
@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.
Is there a reason to create the tensor and then move to a different device?
There was a problem hiding this comment.
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.
The ones and eye tensor can be created on device directly. No need for two separate steps.
There was a problem hiding this comment.
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 |
jaeyeun97
force-pushed
the
feature_griffinlim
branch
from
44011d1 to
cff6cfc
Compare
vincentqb
left a comment
vincentqb
left a comment
There was a problem hiding this comment.
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.
vincentqb
changed the title
2 participants
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.