Skip to content
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

[BC-Breaking] Drop pseudo complex support from phase_vocoder / TimeStretch #1957

Merged

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Nov 2, 2021

Following the plan #1337, this PR drops the support for pseudo complex type from F.phase_vocoder and T.TimeStretch.

@mthrok mthrok changed the title [BC-Breaking] Remove pseudo complex type support from phase_vocoder / TimeStretch [BC-Breaking] Drop pseudo complex support from phase_vocoder / TimeStretch Nov 2, 2021
@mthrok mthrok force-pushed the remove-pseudo-complex-support-phase-vocoder branch from 85c56eb to 6093387 Compare November 3, 2021 00:35
Copy link
Member

@nateanl nateanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


if not is_complex:
complex_specgrams = torch.view_as_complex(complex_specgrams)

# pack batch
shape = complex_specgrams.size()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complex_specgrams can be renamed as specgram since the dtype of it is always complex, but that is BC-Breaking, maybe it's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unfortunately, it is BC-breaking and we do not have a strong reason to push the BC-breaking here.

n_freq = 400
hop_length = 512
fixed_rate = 1.3
tensor = torch.view_as_complex(torch.rand((10, 2, n_freq, 10, 2)))
tensor = torch.rand((10, 2, n_freq, 10), dtype=torch.cfloat)
Copy link
Member

@nateanl nateanl Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change it by using the get_spectrogram method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Using get_specgrogram first time in a while and now I feel it's not user-friendly...

@@ -124,17 +124,13 @@ def test_batch_lfcc(self):

self.assert_batch_consistency(transform, waveform, atol=1e-4, rtol=1e-5)

@parameterized.expand([(True, ), (False, )])
def test_batch_TimeStretch(self, test_pseudo_complex):
def test_batch_TimeStretch(self):
rate = 2
num_freq = 1025
num_frames = 400
batch = 3

spec = torch.randn(batch, num_freq, num_frames, dtype=torch.complex64)
Copy link
Member

@nateanl nateanl Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

self._assert_consistency_complex(
T.TimeStretch(n_freq=n_freq, hop_length=hop_length, fixed_rate=fixed_rate),
tensor,
test_pseudo_complex
False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is False by default, so we could also just remove the parameter here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the option from the helper function as it is no longer necessary.

@mthrok mthrok marked this pull request as ready for review November 3, 2021 15:53
@mthrok mthrok force-pushed the remove-pseudo-complex-support-phase-vocoder branch from 6093387 to 8d8feab Compare November 3, 2021 19:42
@mthrok mthrok force-pushed the remove-pseudo-complex-support-phase-vocoder branch from 8d8feab to 8cf40ef Compare November 3, 2021 20:18
@mthrok mthrok merged commit d3e146f into pytorch:main Nov 3, 2021
@mthrok mthrok deleted the remove-pseudo-complex-support-phase-vocoder branch November 3, 2021 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants