-
Notifications
You must be signed in to change notification settings - Fork 661
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
Make F.phase_vocoder
and T.TimeStretch
handle complex dtype
#1410
Conversation
92f7755
to
88fba5c
Compare
88fba5c
to
d27a820
Compare
F.phase_vocoder
and T.TimeStretch
handle complex dtype
torchaudio/transforms.py
Outdated
assert complex_specgrams.size(-1) == 2, "complex_specgrams should be a complex tensor, shape (..., complex=2)" | ||
if not complex_specgrams.is_complex() and complex_specgrams.size(-1) != 2: | ||
raise ValueError( | ||
"complex_specgrams must be either complex dtype or " |
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.
nit - must be either native complex tensors or real valued tensors with shape (..., 2)
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.
nit: in particular "e.g." should be removed 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.
Left a minor comment regarding docs but looks good overall. cc. @vincentqb if you had any comments/concerns/ wanted to take a look.
Actually just realized, we don't have serialization tests for |
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.
thx @anjali411 for the ping -- LGTM too! (with minor phrasing as you pointed out)
About the serialization test:
- @anjali411: are there tests on the pytorch-side around serialization? can/should we make the assumption the serialization is covered on that front? say by supporting the operations in the doc?
- Since adding a test for serialization could eventually catch errors especially when changing complex dtype parameters (say across versions), I can see this could help catch errors. However, since the parameters in
TimeStretch
are not changing for this pull request, I do not expect thenn.Module
state to break BC compatibility with this pull request (say a user had saved the state prior to this pull request and now re-load it after this pull request).
torchaudio/transforms.py
Outdated
assert complex_specgrams.size(-1) == 2, "complex_specgrams should be a complex tensor, shape (..., complex=2)" | ||
if not complex_specgrams.is_complex() and complex_specgrams.size(-1) != 2: | ||
raise ValueError( | ||
"complex_specgrams must be either complex dtype or " |
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.
nit: in particular "e.g." should be removed here
else: | ||
rate = overriding_rate | ||
|
||
if rate == 1.0: |
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.
could you clarify this change?
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.
TimeStretch
is a thin wrapper around phase_vocoder
. and not altering the input Tensor when rate == 1.0
is a valid branching for phase_vocoder
itself, not just TimeStretch
. so it makes more sense if it's in phase_vocoder
.
No, because the JIT support for complex numbers is very new and some may say, it's still "under construction".
That makes total sense and I agree. But I think it would be nice to have serialization tests, just in general, and especially for a sanity check now that we are migrating to complex types. |
@anjali411 I am merging this so that I can resolve the conflict in other PRs. Let me know if you have further update, we can follow up on it in a separate PR. |
…pe (pytorch#1410)" This reverts commit 0433b7a.
spec_stretch = F.phase_vocoder(spec, rate=rate, phase_advance=phase_advance) | ||
|
||
assert spec.dim() == spec_stretch.dim() | ||
expected_shape = torch.Size([batch_size, num_freq, int(np.ceil(num_frames / rate))]) |
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.
FWIW torch also supports ceil
This PR supersedes #758 and includes further updates.
See #1337 for the overview.
F.phase_vocoder
accepts Tensor with complex dtype.torch.polar
for simpler Tensor generation from magnitude and angle.{CPU | CUDA} x {complex64 | complex128}
F.phase_vocoder
andT.TimeStretch
.T.TimeStretch
.Benchmark of
phase_vocoder
env
CPU
code
unit:
msec
CUDA
code
unit:
msec