-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
ENH: Add new scipy.fft module using pocketfft #10238
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
Conversation
|
Thanks @peterbell10, great start! I created a new For all the things that need significant discussion and deciding, I suggest to create separate issues. And keep the list in this PR as the "master issue". Otherwise things will get unwieldy. Suggestion of separate issues:
|
|
Thanks @rgommers, that's a good idea. |
36484d1 to
66d1081
Compare
|
Regarding the failing test, single precision fftpack tests were marked as xfail so fftpack failed for the same test case. Investigating it shows that pocketfft is in fact more accurate than fftpack. I've given single precision variants slightly more tolerance so the test should be passing now. |
3bffa16 to
550f389
Compare
|
I've had a look through the pybind11 repo and the only python code is either tests or the |
|
Also, I'm planning to base a lot of the documentation off the numpy fft docs as they seemed to be slightly more consistent. I hope that's not a problem. |
Definitely not, please borrow whatever is helpful! |
|
I'm struggling to get the documentation to build correctly. The which comes from this section heading in the docstring, which is exactly the same as for fftpack If I remove the section headings entirely, the module summary builds. However, there are no Any help would be appreciated. |
Hmm, that error is normally for function docstrings. Perhaps |
|
CircleCI should be doing a clean build and it's happening on there as well. Edit: I just tried removing all reference to numpy.fft.fft from the scipy |
|
It looks like the root cause is that |
|
I've added a workaround for sphinx documenting I've also been trying to update the |
|
Okay the FFT people appear to be happy and no other complaints, in it goes. Thanks @peterbell10 ! |
This should be documented in INSTALL.rst.txt. |
The addition of the scipy.fft module [1] adds pybind11 as an additional build time dependency. This has already been documented in INSTALL.rst.txt [2] but not in HACKING.rst.txt. This commit adds pybind11 to the required dependencies. [1] scipy#10238 [2] scipy#10348
|
|
||
| def rfft2(x, shape=None, axes=(-2,-1), norm=None, overwrite_x=False): | ||
| """ | ||
| 2-D dicsrete Fourier transform of a real sequence |
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.
"discrete" typo
|
|
||
| def irfft2(x, shape=None, axes=(-2,-1), norm=None, overwrite_x=False): | ||
| """ | ||
| 2-D dicsrete inverse Fourier transform of a real sequence |
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.
"discrete" typo
| return pfft.ifftn(tmp, axes, norm, overwrite_x, _default_workers) | ||
|
|
||
| def rfftn(x, shape=None, axes=None, norm=None, overwrite_x=False): | ||
| """Return multi-dimentional discrete Fourier transform of real input""" |
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.
"multi-dimentional" typo
|
|
||
| def rfft(x, n=None, axis=-1, norm=None, overwrite_x=False): | ||
| """ | ||
| Discrete Fourier transform of a real sequence. |
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 guess these docstrings will be expanded to the same level of detail as the old ones in a different PR?
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.
The user-facing API is in scipy/fft/_basic.py which has the full docstrings.
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.
See the rendered docs
|
I've added a release note to the wiki page: |
|
That looks great, thanks Peter |
|
I was about to update some old code to use |
|
I vaguely remember that we discussed that at the time and think we agreed that no replacement was needed, since |
|
The docstring that I linked to is not complete, but I wouldn't say the function was not documented. It is definitely part of the public API. It might not be widely used, but:
Having said all that, apparently this is the first time someone has asked about the "missing" functions, so I guess the demand isn't really that high. Certainly the method is widely used, but I suspect the existence of the function is not well known. If you understand the method, it is not hard to implement, but I wouldn't call our current implementation trivial: scipy/scipy/fftpack/_pseudo_diffs.py Lines 49 to 74 in 39fa485
|
Closes #10175
Enhancements
Multi-threading using OpenMP(disabled for now)rffts including n-dimensional transformsrfftnnorm = 'ortho')Issues
extra_compile_argsbased on the compiler which would be needed to compile with OpenMP in a portable way.fftwhich is missing from pocketfft currently.numpy.ffttests and most of thescipy.fftpacktests after some modifications. I have 1 remaining failure locally which is a precision difference for float32 transforms. Needs investigation.Open questions
raisewhen the user specifies a number of threads that isn't available. (Response when number of workers cannot be satisfied #10242)hfft,ihfft) are missing. This is trivial to add since it is only a slight variant onrfftandirfft. We could also extend beyond numpy and give n-dimensional versionshfftnandihfftn. (scipy.fft: What functions should be included #10240)overwrite_xto be confusing (Theoverwrite_xargument of FFT functions is deceptive in certain conditions cupy/cupy#1988). It has been suggested to remove it entirely but could also be renamed. FFTW calls thisFFTW_DESTROY_INPUTwhich makes it clearer that this is not a guarunteed inplace transform/out-parameter. (fft(pack): The overwrite_x parameter can be confusing #10241)