Use istft from torch #523
Use istft from torch #523
Conversation
|
Did you double check that |
torchaudio/functional.py
Outdated
|
|
||
| return y | ||
| warnings.warn( | ||
| 'istft has been moved to PyTorch and will be deprecated, please use torch.istft instead.') |
There was a problem hiding this comment.
this should produce a UserWarning, which is our convention.
I don't know what it means that something "will be deprecated" -- something being deprecated means it will be changed eventually, so there's not really a delta between "will be deprecated" and "is deprecated".
There was a problem hiding this comment.
Oops I meant, 'will be removed'...
torchaudio/functional.py
Outdated
| warnings.warn( | ||
| 'istft has been moved to PyTorch and will be removed, please use torch.istft instead.') |
There was a problem hiding this comment.
nit: "torchaudio.istft has been moved to PyTorch and will be removed from torchaudio, please use torch.istft instead.'
torchaudio/functional.py
Outdated
| warnings.warn( | ||
| '`pad_mode` for istft function is deprecated and will be removed. ' | ||
| 'It was introduced accidentally and never used. ' | ||
| 'You can safely remove it from your function call') |
There was a problem hiding this comment.
In the sense that pad_mode was added to mirror the signature of stft, pad_mode in istft is indeed not needed. Also, pad_mode="reflect" in istft still provides a reasonable inverse to stft with pad_mode="constant", as shown in torchaudio tests.
If we are to show this warning, let's only do so if the user attempts to change the default value. An alternative is to set the default value to None, and show the warning if the user changes pad_mode to something else than that.
nit: 'The parameter pad_mode was ignored in isftft, and is thus being deprecated. Please set pad_mode to None to suppress this warning.')
|
Marked as "do not merge before istft" |
vincentqb
changed the title
vincentqb
changed the title
|
Now that pytorch/pytorch#35569 has been merged, removed label marking as do not merge. |
There was a problem hiding this comment.
Tests are failling.
=========================== short test summary info ============================
FAILED test/test_functional.py::TestIstft::test_istft_requires_nola - Runtime...
FAILED test/test_functional.py::TestIstft::test_istft_requires_non_empty - Ru...
FAILED test/test_functional.py::TestIstft::test_istft_requires_overlap_windows
FAILED test/test_torchscript_consistency.py::TestFunctionalCPU::test_griffinlim
FAILED test/test_torchscript_consistency.py::TestTransformsCPU::test_GriffinLim
====== 5 failed, 207 passed, 57 skipped, 34 warnings in 315.87s (0:05:15) ======
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
==========================================
- Coverage 89.03% 88.81% -0.22%
==========================================
Files 21 21
Lines 2261 2218 -43
==========================================
- Hits 2013 1970 -43
Misses 248 248
Continue to review full report at Codecov.
|
| # pack batch | ||
| shape = stft_matrix.size() | ||
| stft_matrix = stft_matrix.reshape(-1, shape[-3], shape[-2], shape[-1]) |
There was a problem hiding this comment.
This is what enabled the implementation to support batching.
Corresponding PR pytorch/pytorch#35569
This PR will use
istftfrom torch repo, which is ported to C++.This PR will not succeed until the upstream PR is merged and
torch.istftbecomes available.