Use new FFT operators in stft#47601
Use new FFT operators in stft#47601peterbell10 wants to merge 12 commits intogh/peterbell10/24/basefrom
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit f4cc17c (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. This comment has been revised 41 times. |
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Summary: Ref #42175 This removes the 4 deprecated spectral functions: `torch.{fft,rfft,ifft,irfft}`. `torch.fft` is also now imported by by default. The actual `at::native` functions are still used in `torch.stft` so can't be full removed yet. But will once #47601 has been merged. Pull Request resolved: #48594 Reviewed By: heitorschueroff Differential Revision: D25298929 Pulled By: mruberry fbshipit-source-id: e36737fe8192fcd16f7e6310f8b49de478e63bf0
Fixes #42175 (comment) [ghstack-poisoned]
| } | ||
| } | ||
|
|
||
| // Like view_as_complex but may clone if the input can't be directly viewed as complex |
There was a problem hiding this comment.
Can you elaborate on how you expect this function to be used?
It's a little odd to me that it's so aggressive at coercing tensors to complex, but I guess that makes sense for the current state deprecated state of stft and istft. If that's the case, however, the comment should probably clarify.
Is this the right PR to update stft to the next stage in the deprecation process where it requires return_complex? Also, should we deprecate istft accept a float tensor mimicking a half tensor now? Then after 1.8 we can have stft always return a complex tensor and istft always accept a complex tensor.
There was a problem hiding this comment.
It's a little odd to me that it's so aggressive at coercing tensors to complex
I don't see the issue. It converts any tensor from the old complex format, to a complex dtype tensor. view_as_complex is limited by the fact that it has to create a view of the original tensor but there's no reason to force that limitation onto the user now. Would it be clearer if the comment didn't mention view_as_complex?
Is this the right PR to update stft to the next stage in the deprecation process where it requires
return_complex?
That doesn't simplify anything in this PR and I guess could hold up merging, so I would say no. I can open a separate PR for it though.
Also, should we deprecate istft accept a float tensor mimicking a half tensor now? Then after 1.8 we can have stft always return a complex tensor and istft always accept a complex tensor.
Sure, I can do that.
There was a problem hiding this comment.
A separate PR for those latter issues sounds good.
As for this particular function, mentioned view_as_complex in the comment seems fine. Maybe just making the comment more specific by saying that this function is only intended to be used for istft and picking a name for it that matches? That is, I would think that once istft is updated to ONLY accept complex inputs this function will be killed, right?
There was a problem hiding this comment.
@mruberry I've reworded the comment, PTAL.
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
Fixes #42175 (comment) [ghstack-poisoned]
mruberry
left a comment
There was a problem hiding this comment.
Cool! Thanks @peterbell10!
ghstack-source-id: 4391fdf Pull Request resolved: pytorch#47601
Summary: Ref pytorch#42175 This removes the 4 deprecated spectral functions: `torch.{fft,rfft,ifft,irfft}`. `torch.fft` is also now imported by by default. The actual `at::native` functions are still used in `torch.stft` so can't be full removed yet. But will once pytorch#47601 has been merged. Pull Request resolved: pytorch#48594 Reviewed By: heitorschueroff Differential Revision: D25298929 Pulled By: mruberry fbshipit-source-id: e36737fe8192fcd16f7e6310f8b49de478e63bf0
Summary: Pull Request resolved: pytorch#47601 Fixes pytorch#42175 (comment) Test Plan: Imported from OSS Reviewed By: ngimel Differential Revision: D25457217 Pulled By: mruberry fbshipit-source-id: 455d216edd0b962eb7967ecb47cccc8d6865975b
Fixes #42175 (comment)
Stack from ghstack:
Differential Revision: D25457217