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
Add centered FFT example to fftshift docs #51223
Conversation
💊 CI failures summary and remediationsAs of commit a35097b (more details on the Dr. CI page):
🚧 1 fixed upstream failure:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
>>> torch.fftshift(f) | ||
tensor([-0.5000, -0.2500, 0.0000, 0.2500]) | ||
>>> torch.fft.fftshift(f) | ||
tensor([-0.5000, -0.2500, 0.0000, 0.2500]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found these discrepancies from running the doctests manually. So, it seems fft doctests aren't being run in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix.
torch/fft/__init__.py
Outdated
@@ -865,6 +865,10 @@ | |||
Reorders n-dimensional FFT data, as provided by :func:`~torch.fft.fftn`, to have | |||
negative frequency terms first. | |||
|
|||
This performs a periodic shift of n-dimensional data such that the origin | |||
``(0, ..., 0)`` is moved to the center of the tensor. Specifically, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
"Specifically, ..." -> "Specifically, to ..." to make the second sentence parallel to the first.
torch/fft/__init__.py
Outdated
>>> fft_centered = torch.fft.fftshift(fft_uncentered) | ||
|
||
The inverse transform, from centered Fourier space back to centered spatial | ||
data, is simply applying the inverse shifts in reverse order: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: "is simply applying" -> "can be performed by applying"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a couple suggestions for your review @peterbell10. If they're helpful, great, and if not no worries. Just ping me when you'd like this merged.
527472c
to
a35097b
Compare
@mruberry I've added your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Closes #51022