stft: Require return_complex to be passed explicitly for real input #86724

peterbell10 · 2022-10-11T20:56:19Z

Stack from ghstack (oldest at bottom):

This behavior has been deprecated since PyTorch 1.8 but this step of
the deprecation cycle was put on hold in #50102 waiting for JIT
upgraders functionality which doesn't seem to have panned out. I'd say
there has been more than enough of a deprecation period, so we should
just continue.

BC-breaking message:

torch.stft takes an optional return_complex parameter that
indicates whether the output should be a floating point tensor or a
complex tensor. return_complex previously defaulted to False for
real input tensors. This PR removes the default and makes
return_complex a required argument for real inputs. However, complex
inputs will continue to default to return_complex=True.

1.13

>>> a = torch.rand(1024)
>>> _ = torch.stft(a, n_fft=128)

2.0

>>> t = torch.rand(1024)
>>> _ = torch.stft(t, n_fft=128, return_complex=False)

This behavior has been deprecated since PyTorch 1.8 but this step of the deprecation cycle was put on hold in #50102 waiting for JIT upgraders functionality which doesn't seem to have panned out. I'd say there has been more than enough of a deprecation period, so we should just continue. BC-breaking message: `torch.stft` takes an optional `return_complex` parameter that indicates whether the output should be a floating point tensor or a complex tensor. `return_complex` previously defaulted to `False` for real input tensors. This PR removes the default and makes `return_complex` a required argument for real inputs. However, complex inputs will continue to default to `return_complex=True`. [ghstack-poisoned]

pytorch-bot · 2022-10-11T20:56:21Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86724

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 792aa85:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

mruberry · 2022-10-14T18:21:32Z

test/test_spectral_ops.py

-        y = x.stft(10, pad_mode='constant')
-        # with self.assertRaisesRegex(RuntimeError, 'stft requires the return_complex parameter'):
-        #     y = x.stft(10, pad_mode='constant')
+        with self.assertRaisesRegex(RuntimeError, 'stft requires the return_complex parameter'):


Nit: this could be an error input

mruberry · 2022-10-14T18:21:46Z

torch/functional.py

@@ -612,6 +612,15 @@ def stft(input: Tensor, n_fft: int, hop_length: Optional[int] = None,
            a real tensor with an extra last dimension for the real and
            imaginary components.

+            .. versionchanged:: 1.14.0


Nice directives

mruberry · 2022-10-14T18:23:35Z

torch/functional.py

@@ -612,6 +612,15 @@ def stft(input: Tensor, n_fft: int, hop_length: Optional[int] = None,
            a real tensor with an extra last dimension for the real and
            imaginary components.

+            .. versionchanged:: 1.14.0
+               ``return_complex`` is now a required argument for real inputs,
+               as the default is being trainsitioned to ``True``.


"trainsitioned" -> "transitioned"

mruberry

This seems reasonable to me -- @ngimel @albanD is there someone at Meta who should sanity-check the BC implications?

@peterbell10 take a look at my questions about the docs, however -- I think they can be streamlined

albanD

I think we should just land this!
Checking is so painful that it is easier to try and land it :)

If it comes back to us we can go down the rabbit hole.

This behavior has been deprecated since PyTorch 1.8 but this step of the deprecation cycle was put on hold in pytorch#50102 waiting for JIT upgraders functionality which doesn't seem to have panned out. I'd say there has been more than enough of a deprecation period, so we should just continue. BC-breaking message: `torch.stft` takes an optional `return_complex` parameter that indicates whether the output should be a floating point tensor or a complex tensor. `return_complex` previously defaulted to `False` for real input tensors. This PR removes the default and makes `return_complex` a required argument for real inputs. However, complex inputs will continue to default to `return_complex=True`. ghstack-source-id: 32da297a1ffb8c438910cfd237ad20857da5c15b Pull Request resolved: pytorch#86724

…eal input" This behavior has been deprecated since PyTorch 1.8 but this step of the deprecation cycle was put on hold in #50102 waiting for JIT upgraders functionality which doesn't seem to have panned out. I'd say there has been more than enough of a deprecation period, so we should just continue. BC-breaking message: `torch.stft` takes an optional `return_complex` parameter that indicates whether the output should be a floating point tensor or a complex tensor. `return_complex` previously defaulted to `False` for real input tensors. This PR removes the default and makes `return_complex` a required argument for real inputs. However, complex inputs will continue to default to `return_complex=True`. [ghstack-poisoned]

This behavior has been deprecated since PyTorch 1.8 but this step of the deprecation cycle was put on hold in pytorch#50102 waiting for JIT upgraders functionality which doesn't seem to have panned out. I'd say there has been more than enough of a deprecation period, so we should just continue. BC-breaking message: `torch.stft` takes an optional `return_complex` parameter that indicates whether the output should be a floating point tensor or a complex tensor. `return_complex` previously defaulted to `False` for real input tensors. This PR removes the default and makes `return_complex` a required argument for real inputs. However, complex inputs will continue to default to `return_complex=True`. ghstack-source-id: 32da297a1ffb8c438910cfd237ad20857da5c15b Pull Request resolved: pytorch#86724

…eal input" This behavior has been deprecated since PyTorch 1.8 but this step of the deprecation cycle was put on hold in #50102 waiting for JIT upgraders functionality which doesn't seem to have panned out. I'd say there has been more than enough of a deprecation period, so we should just continue. BC-breaking message: `torch.stft` takes an optional `return_complex` parameter that indicates whether the output should be a floating point tensor or a complex tensor. `return_complex` previously defaulted to `False` for real input tensors. This PR removes the default and makes `return_complex` a required argument for real inputs. However, complex inputs will continue to default to `return_complex=True`. [ghstack-poisoned]

peterbell10 · 2022-10-16T20:59:14Z

@pytorchbot merge

pytorchmergebot · 2022-10-16T21:00:44Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

This behavior has been deprecated since PyTorch 1.8 but this step of the deprecation cycle was put on hold in pytorch#50102 waiting for JIT upgraders functionality which doesn't seem to have panned out. I'd say there has been more than enough of a deprecation period, so we should just continue. BC-breaking message: `torch.stft` takes an optional `return_complex` parameter that indicates whether the output should be a floating point tensor or a complex tensor. `return_complex` previously defaulted to `False` for real input tensors. This PR removes the default and makes `return_complex` a required argument for real inputs. However, complex inputs will continue to default to `return_complex=True`. ghstack-source-id: fba3d911016009d7e68e053eba01f4d3bf053f1f Pull Request resolved: pytorch#86724

peterbell10 mentioned this pull request Oct 11, 2022

istft: require complex input #86628

Closed

pytorchbot added the open source label Oct 11, 2022

peterbell10 added module: bc-breaking Related to a BC-breaking change module: fft release notes: python_frontend release notes category topic: bc breaking topic category labels Oct 13, 2022

peterbell10 marked this pull request as ready for review October 13, 2022 15:00

peterbell10 requested review from mruberry and ngimel as code owners October 13, 2022 15:00

mruberry reviewed Oct 14, 2022

View reviewed changes

mruberry approved these changes Oct 14, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 14, 2022

albanD approved these changes Oct 14, 2022

View reviewed changes

albanD approved these changes Oct 16, 2022

View reviewed changes

pytorchmergebot added the Merged label Oct 16, 2022

pytorchmergebot closed this in 3007efd Oct 16, 2022

peterbell10 mentioned this pull request Oct 27, 2022

stft: move towards always returning complex #72882

Closed

facebook-github-bot deleted the gh/peterbell10/444/head branch June 8, 2023 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stft: Require return_complex to be passed explicitly for real input #86724

stft: Require return_complex to be passed explicitly for real input #86724

peterbell10 commented Oct 11, 2022 •

edited

pytorch-bot bot commented Oct 11, 2022 •

edited

mruberry Oct 14, 2022

mruberry Oct 14, 2022

mruberry Oct 14, 2022

mruberry left a comment

albanD left a comment

peterbell10 commented Oct 16, 2022

pytorchmergebot commented Oct 16, 2022

stft: Require return_complex to be passed explicitly for real input #86724

stft: Require return_complex to be passed explicitly for real input #86724

Conversation

peterbell10 commented Oct 11, 2022 • edited

pytorch-bot bot commented Oct 11, 2022 • edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86724

✅ No Failures

mruberry Oct 14, 2022

Choose a reason for hiding this comment

mruberry Oct 14, 2022

Choose a reason for hiding this comment

mruberry Oct 14, 2022

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

albanD left a comment

Choose a reason for hiding this comment

peterbell10 commented Oct 16, 2022

pytorchmergebot commented Oct 16, 2022

Merge started

peterbell10 commented Oct 11, 2022 •

edited

pytorch-bot bot commented Oct 11, 2022 •

edited