Deprecate old fft functions #44876

peterbell10 · 2020-09-17T13:12:36Z

Stack from ghstack:

torch.fft: Two dimensional FFT functions #45164 torch.fft: Two dimensional FFT functions
codegen: Improve array default handing #45163 codegen: Improve array default handing
torch.fft helper functions #44877 torch.fft helper functions
Deprecate old fft functions #44876 Deprecate old fft functions
torch.fft: Multi-dimensional transforms #44550 torch.fft: Multi-dimensional transforms

Differential Revision: D23866715

[ghstack-poisoned]

dr-ci · 2020-09-17T14:45:40Z

💊 CI failures summary and remediations

As of commit cd37267 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 55 times.

[ghstack-poisoned]

ghstack-source-id: 21b330d Pull Request resolved: pytorch#44876

[ghstack-poisoned]

mruberry · 2020-09-22T04:09:44Z

aten/src/ATen/native/SpectralOps.cpp


 Tensor fft(const Tensor& self, const int64_t signal_ndim, const bool normalized) {
+  TORCH_WARN_ONCE(
+    "torch.fft is deprecated and will be removed in pytorch 1.8. "


Reviewing this now because I'd like to get this in for 1.7 (branch cut next week Tuesday) so torch.fft() is deprecated in 1.7 and can be removed in 1.8. Realistically this PR and the one it's stacked will need to be done and landable before Monday to make that happen. Let me know if there's something I can do to help, @peterbell10.

Overall this looks good but it needs two things:

the _tensor_docs.py for torch.fft also need a (deprecation) warning with language like this (except it can link to the torch.fft module documentation)

this warning should tell people to import torch.fft to get access to the functions that are replacing torch.fft()

Maybe something like:

"The function torch.fft is deprecated and will be removed in a future PyTorch release. Use the new torch.fft module functions, instead, by importing torch.fft and calling torch.fft.fft or torch.fft.fftn."

?

@mruberry I've updated the docs and warning message. Is there a policy on giving the version a deprecated function will be removed? I've put torch.fft() as expiring in 1.8 but left the others as "a furture PyTorch release" since that's not a priority.

No policy. "In a future PyTorch release" gives us some latitude.

mruberry · 2020-09-22T04:13:04Z

aten/src/ATen/native/SpectralOps.cpp


 Tensor rfft(const Tensor& self, const int64_t signal_ndim, const bool normalized,
            const bool onesided) {
+  TORCH_WARN_ONCE(


Deprecating torch.fft() is the priority (since its name conflicts with the module), but deprecating these is OK if the same pattern is followed. I.e.:

""The function torch.rfft is deprecated and will be removed in a future PyTorch release. Use the new torch.fft module functions, instead, by importing torch.fft and calling torch.fft.rfft or torch.fft.rfftn."

And also updating the torch.rfft and torch.irfft docs.

[ghstack-poisoned]

peterbell10 · 2020-09-22T14:22:21Z

torch/_torch_docs.py

+.. deprecated:: 1.7.0
+    The function :func:`torch.fft` is deprecated and will be removed in a
+    PyTorch 1.8. Use the new :mod:`torch.fft` module functions instead, by
+    importing :mod:`torch.fft` and calling :func:`torch.fft.fft` or


I'll note that torch.fft unfortunately links to the function instead of the module. I was hoping :mod: would disambiguate, but I guess it's unlikely to work properly until the function is removed.

Darn. I guess we can just write the url by hand for now? It's very unlikely to change.

Used a reference so I didn't have to hard-code the URL.

Smart solution.

[ghstack-poisoned]

aten/src/ATen/native/SpectralOps.cpp

mruberry

Nice work, @peterbell10. Just a few minor corrections to be applied. Ping me when this is ready.

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

peterbell10 · 2020-09-23T13:59:37Z

@mruberry I've corrected the warnings.

mruberry · 2020-09-23T17:11:43Z

torch/_torch_docs.py

 The inverse of this function is :func:`~torch.ifft`.

+.. deprecated:: 1.7.0
+    The function :func:`torch.fft` is deprecated and will be removed in a


"in a PyTorch 1.8" -> "in PyTorch 1.8"

@mruberry fixed.

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

mruberry

Awesome! Thanks @peterbell10!

codecov · 2020-09-23T23:21:59Z

Codecov Report

Merging #44876 into gh/peterbell10/11/base will increase coverage by 0.00%.
The diff coverage is n/a.

@@                   Coverage Diff                   @@
##           gh/peterbell10/11/base   #44876   +/-   ##
=======================================================
  Coverage                   68.08%   68.08%           
=======================================================
  Files                         393      393           
  Lines                       50765    50765           
=======================================================
+ Hits                        34564    34565    +1     
+ Misses                      16201    16200    -1

Impacted Files	Coverage Δ
torch/_torch_docs.py	`100.00% <ø> (ø)`
torch/testing/_internal/expecttest.py	`78.57% <0.00%> (+1.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bccee0...cd37267. Read the comment docs.

facebook-github-bot · 2020-09-24T10:03:56Z

@mruberry merged this pull request in dc67b47.

fritzo · 2020-10-28T13:47:20Z

Note the 1.7 release notes state

Please use torch.fft.foo as a drop-in replacement for torch.foo for the following functions: fft, ifft, rfft and irfft.

however the rfft and irfft signatures have changed. Would PyTorch devs have time to update release notes with old-versus-new translations for these changed functions?

mruberry · 2020-10-28T17:13:10Z

Note the 1.7 release notes state

Please use torch.fft.foo as a drop-in replacement for torch.foo for the following functions: fft, ifft, rfft and irfft.

however the rfft and irfft signatures have changed. Would PyTorch devs have time to update release notes with old-versus-new translations for these changed functions?

The release notes are, unfortunately, incorrect. All of these functions are different from each other, as described in the Wiki article here: https://github.com/pytorch/pytorch/wiki/The-torch.fft-module-in-PyTorch-1.7. You can translate rfft and irfft calls to rfftn and irfftn calls, however.

Deprecate old fft functions

363d6a7

[ghstack-poisoned]

pytorchbot added the open source label Sep 17, 2020

peterbell10 added 2 commits September 17, 2020 16:42

Update on "Deprecate old fft functions"

12ed8df

[ghstack-poisoned]

Update on "Deprecate old fft functions"

18f360d

[ghstack-poisoned]

peterbell10 requested a review from mruberry September 17, 2020 20:08

Update on "Deprecate old fft functions"

a17490d

[ghstack-poisoned]

Update on "Deprecate old fft functions"

6a57fea

[ghstack-poisoned]

peterbell10 added a commit to peterbell10/pytorch that referenced this pull request Sep 19, 2020

Deprecate old fft functions

3bc0907

ghstack-source-id: 21b330d Pull Request resolved: pytorch#44876

peterbell10 added 2 commits September 19, 2020 15:28

Update on "Deprecate old fft functions"

953f193

[ghstack-poisoned]

Update on "Deprecate old fft functions"

8b271dd

[ghstack-poisoned]

mruberry added this to the 1.7.0 milestone Sep 22, 2020

mruberry reviewed Sep 22, 2020

View reviewed changes

peterbell10 added 2 commits September 22, 2020 13:51

Update on "Deprecate old fft functions"

94bac81

[ghstack-poisoned]

Update on "Deprecate old fft functions"

db2c1d2

[ghstack-poisoned]

peterbell10 commented Sep 22, 2020

View reviewed changes

Update on "Deprecate old fft functions"

4d62eb3

[ghstack-poisoned]

ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 22, 2020

This was referenced Sep 22, 2020

codegen: Improve array default handing #45163

Closed

torch.fft: Two dimensional FFT functions #45164

Closed

mruberry reviewed Sep 23, 2020

View reviewed changes

aten/src/ATen/native/SpectralOps.cpp Outdated Show resolved Hide resolved

mruberry reviewed Sep 23, 2020

View reviewed changes

aten/src/ATen/native/SpectralOps.cpp Outdated Show resolved Hide resolved

mruberry approved these changes Sep 23, 2020

View reviewed changes

peterbell10 added 3 commits September 23, 2020 14:52

Update on "Deprecate old fft functions"

b1ab343

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

Update on "Deprecate old fft functions"

cc898c1

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

Update on "Deprecate old fft functions"

cde5fbc

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

mruberry reviewed Sep 23, 2020

View reviewed changes

peterbell10 added 2 commits September 23, 2020 19:04

Update on "Deprecate old fft functions"

edaf6d6

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

Update on "Deprecate old fft functions"

cd37267

Differential Revision: [D23866715](https://our.internmc.facebook.com/intern/diff/D23866715) [ghstack-poisoned]

mruberry approved these changes Sep 23, 2020

View reviewed changes

facebook-github-bot closed this in dc67b47 Sep 24, 2020

facebook-github-bot added the merged label Sep 24, 2020

facebook-github-bot deleted the gh/peterbell10/11/head branch September 27, 2020 14:15

mruberry added the Merged label Oct 28, 2020

fritzo mentioned this pull request Oct 28, 2020

Fixes for PyTorch 1.7 release pyro-ppl/pyro#2683

Merged

8 tasks

Deprecate old fft functions #44876

Deprecate old fft functions #44876

Uh oh!

Conversation

peterbell10 commented Sep 17, 2020 • edited by mruberry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

peterbell10 commented Sep 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot commented Sep 24, 2020

Uh oh!

fritzo commented Oct 28, 2020

Uh oh!

mruberry commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

peterbell10 commented Sep 17, 2020 •

edited by mruberry

Loading

dr-ci bot commented Sep 17, 2020 •

edited

Loading

codecov bot commented Sep 23, 2020 •

edited

Loading