Add a norm keyword arg to fft functions #3883
Conversation
| n = shape(a)[axis] | ||
| unitary = _unitary(norm) | ||
| return (_raw_fft(a, n, axis, fftpack.cffti, fftpack.cfftf, _fft_cache) / | ||
| (sqrt(n) if unitary else 1)) |
There was a problem hiding this comment.
What does inserting this divide by 1 do the run time in the norm=None mode?
There was a problem hiding this comment.
I don't know precisely, I just figured that it wasn't a problem since it is done anyway in the ifft case.
Another way to look at it is that divide by 1 is in O(n) while the fft is in O(n log(n)) so it is negligible.
I can set two separate calls to _raw_fft if necessary.
There was a problem hiding this comment.
I tested it with ipython:
In [1]: x = np.random.rand(4096, 4096) + 1j * np.random.rand(4096, 4096)
In [2]: %timeit np.fft.fft(x)
1 loops, best of 3: 649 ms per loop
In [3]: %timeit (np.fft.fft(x) / 1)
1 loops, best of 3: 894 ms per loop
That's a big increase, I'll do separate calls.
d39dfaf to
60e6659
Compare
| @@ -275,6 +297,8 @@ def rfft(a, n=None, axis=-1): | |||
| axis : int, optional | |||
| Axis over which to compute the FFT. If not given, the last axis is | |||
| used. | |||
| norm : {None, "ortho"}, optional | |||
There was a problem hiding this comment.
These should probably get the ..versionadded thingy (would have to look up the exact syntax myself)
There was a problem hiding this comment.
Which version then, 1.10 ?
|
Anyway, should be good, and since scipy already has it I don't see why we shouldn't add it. I am still a bit suprised we don't have single precision ffts, or is that typical? So mostly I would like a test for each of the fft functions. Other then that, looks good. |
|
Testing all functions is a very good idea, I broke rfft without noticing... |
|
I added tests for all fft transforms (turns out it was necessary, hum...). |
8fe77e7 to
440688b
Compare
|
Git rebase is easy to mess up. It should be ok now. |
| @@ -362,6 +392,9 @@ def irfft(a, n=None, axis=-1): | |||
| axis : int, optional | |||
| Axis over which to compute the inverse FFT. If not given, the last | |||
| axis is used. | |||
| norm : {None, "ortho"}, optional | |||
| Normalization mode (see `numpy.fft`). Default is None. | |||
| .. versionadded:: 1.10.0 | |||
There was a problem hiding this comment.
I think you need a blank line above for this to render correctly.
There was a problem hiding this comment.
Or move it to the beginning after the norm : ... line. Same thing on down. We don't have a standard for this, we should probably have one.
There was a problem hiding this comment.
How can I build the doc for the version in my local repo ? With PYTHONPATH I get:
ImportError: Error importing numpy: you should not try to import numpy from
its source directory; please exit the numpy source tree, and relaunch
your python interpreter from there.
There was a problem hiding this comment.
Don't know if it can be done. I usually install python setup.py install --user. You can just delete it after. Building the docs has some dependencies, which I've forgotten, and you also need to git submodule update for the sphinx extensions. If you haven't tried, then switch to doc and make html.
On where to put the .. versionadded:: for new parameters, I'm leaning towards on the first line of the description. That is more compact, and it is correctly rendered. Now I need to change all the ones I did differently...
|
I think this PR is ready to be merged, if there is no additionnal remarks ? |
|
Probably should be added to the release notes. |
|
I added a paragraph to the release notes. |
|
Rebase in #5992, so closing this. |
This fixes #2142.