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

Migrate TorchAudio to use complex tensors in place of real tensors of shape (..., 2) #860

Closed
4 of 15 tasks
anjali411 opened this issue Aug 6, 2020 · 5 comments
Closed
4 of 15 tasks

Comments

@anjali411
Copy link

anjali411 commented Aug 6, 2020

Complex Tensors were released in 1.6 release of torch. The progress of complex numbers support in torch can be tracked here. This issue is meant to outline the tasks that need to be completed to migrate torchaudio API to use these newly added complex tensors.

The general plan for this migration, would be to add a local USE_COMPLEX flag to avoid bc breaking changes. This flag detect if the input is complex or not. If it's not complex, the old code (that supports the real tensors with shape (..., 2)) will be used and the returned tensor will be a real tensor. If it's complex, the returned tensor will be a complex tensor.
Here's a demo PR for this migration: #758

Here's the tracker for the issue for deprecation of torch.fft functions. The new torch.fft.fft functions will take as input and return complex tensors instead of the real representation of complex tensors.

torchaudio.functional

torchaudio.transforms

torchaudio.compliance.kaldi

cc. @vincentqb @mthrok
cc. @mruberry, who is working on the old API of torch.ftt deprecation, for visibility.

@mthrok
Copy link
Collaborator

mthrok commented Aug 6, 2020

Thanks @anjali411.

Do we have an estimate on completion of this migration?
(By the completion of the migration, I mean we will only support complex dtype.)
I assume after all the implementation is completed, we want to have certain release cycles where users can opt-in for complex dtype, and make sure that some major downstream users migrate too.

@vincentqb
Copy link
Contributor

vincentqb commented Oct 13, 2020

The general plan for this migration, would be to add a local USE_COMPLEX flag to avoid bc breaking changes. This flag detect if the input is complex or not. If it's not complex, the old code (that supports the real tensors with shape (..., 2)) will be used and the returned tensor will be a real tensor. If it's complex, the returned tensor will be a complex tensor.

To elaborate, the plan we had discussed was to:

  • We need to decide whether torchaudio needs to test that autograd runs without errors with complex numbers.
  • For each transform having a complex equivalent, detect when a complex dtype tensor is offered (edited following comment below) add use_complex flag, and use the complex implementation in this case.
  • Once all operations with complex support are implemented, we add a deprecation warning to all transform for at least one release that triggers when the non-complex path is used.
  • After at least one release, we then remove the code path for non-complex tensor.

Can you remind me why the flag USE_COMPLEX is needed here @anjali411 ?

@anjali411
Copy link
Author

anjali411 commented Oct 13, 2020

Can you remind me why the flag USE_COMPLEX is needed here @anjali411 ?

to detect if the input is complex and if we should use the code for complex case (instead of the (..., 2) style float tensors. It also ensures we don't break autograd support in torchaudio (since it relies on torch for autograd correctness).

@vincentqb
Copy link
Contributor

Can you remind me why the flag USE_COMPLEX is needed here @anjali411 ?
to detect if the input is complex and if we should use the code for complex case (instead of the (..., 2) style float tensors. It also ensures we don't break autograd support in torchaudio (since it relies on torch for autograd correctness).

Right, so the concern is that we could be passing a real tensor, which we want to treat as complex with zero imaginary for instance. In that case, we no longer need to detect whether the tensor is real or complex, right? We can just go through the complex path. Is that right?

@mthrok
Copy link
Collaborator

mthrok commented Apr 1, 2021

This issue is replaced by #1337

@mthrok mthrok closed this as completed Apr 1, 2021
@mthrok mthrok modified the milestones: Complex Tensor Migration, v0.9 Apr 5, 2021
mpc001 pushed a commit to mpc001/audio that referenced this issue Aug 4, 2023
…-docstrings

[*.py] Rename "Arguments:" to "Args:"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants