8000 ENH: add `norm=forward,backward` to numpy.fft functions by cval26 · Pull Request #16476 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: add norm=forward,backward to numpy.fft functions #16476

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Jul 12, 2020
Merged

ENH: add norm=forward,backward to numpy.fft functions #16476

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
087f7d3
modified _unitary, fft & ifft and their tests; tests passed
Jun 1, 2020
8fc777a
modified rfft & irfft and their tests; tests passed
Jun 1, 2020
789646d
modified hfft & ihfft and all tests; all modifications are now done
Jun 1, 2020
93d834b
correction in ihfft; all 79 fft tests passed successfully
Jun 1, 2020
5e5e9c5
# Please enter the commit message for your changes. Lines starting
Jun 1, 2020
da249db
undo unnecessary docstrings changes made in my previous commits
Jun 2, 2020
7a15c22
use norm=forward kwarg value name
Jun 27, 2020
ca0dd88
Update numpy/fft/__init__.py
Jun 28, 2020
48a3ef0
add code review suggestions
Jun 28, 2020
2110457
add default norm=None alias norm=backward
Jun 29, 2020
4427019
streamline private normalization functions
Jun 30, 2020
aaa438e
modify hermitian FFTs
Jul 2, 2020
8a86168
add review suggestions
Jul 9, 2020
693c942
add review suggestions v2 (dict as module const)
Jul 9, 2020
21964b9
make review suggestions v3
Jul 12, 2020
29149da
Apply suggestions from code review
seberg Jul 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
modify hermitian FFTs
  • Loading branch information
chris vavaliaris committed Jul 2, 2020
commit aaa438e2f0d534bb34e01af5d99893a2ae371934
14 changes: 9 additions & 5 deletions doc/release/upcoming_changes/16476.new_feature.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
``norm=forward`` keyword option available for ``numpy.fft`` functions
---------------------------------------------------------------------
Using the keyword argument option ``norm=forward`` has the direct transforms
scaled by ``1/n`` and the inverse transforms unscaled (i.e. directly opposite
to the default option ``norm=None``).
``norm=backward``, ``forward`` keyword options for ``numpy.fft`` functions
--------------------------------------------------------------------------
The keyword argument option ``norm=backward`` is added as an alias for ``None``
and acts as the default option; using it has the direct transforms unscaled
and the inverse transforms scaled by ``1/n``.

Using the new keyword argument option ``norm=forward`` has the direct
transforms scaled by ``1/n`` and the inverse transforms unscaled (i.e. directly
opposite to the default option ``norm=backward``).
12 changes: 6 additions & 6 deletions numpy/fft/__init__.py
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,13 @@
Normalization
-------------

The default normalization has the direct transforms unscaled and the inverse
transforms are scaled by :math:`1/n`. It is possible to obtain unitary
transforms by setting the keyword argument ``norm`` to ``"ortho"`` (default is
`None`) so that both direct and inverse transforms will be scaled by
:math:`1/\\sqrt{n}`. Finally, setting the keyword argument "norm" to
The default normalization has the direct (forward) transforms unscaled and the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add "backward" here. Maybe also an initial sentence just about the fact that the argument refers to the pair/which direction is transformed. It reads a bit more difficult than feels necessary to me, but I think its fine to leave for later improvements. Its not easy to write easy to read docs...

Copy link
Contributor Author
@cval26 cval26 Jul 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok; added a commit with all review suggestions @seberg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will merge once the tests run through (maybe minus the azure tests if they are acting up again).

inverse (backward) transforms scaled by :math:`1/n`. It is possible to obtain
unitary transforms by setting the keyword argument ``norm`` to ``"ortho"``
(default is ``"backward"``) so that both direct and inverse transforms are
scaled by :math:`1/\\sqrt{n}`. Finally, setting the keyword argument "norm" to
``"forward"`` has the direct transforms scaled by :math:`1/n` and the inverse
transforms unscaled (i.e. directly oppposite to the default option ``None``).
transforms unscaled (i.e. directly oppposite to the default ``"backward"``).

Real and Hermitian transforms
-----------------------------
Expand Down
136 changes: 72 additions & 64 deletions numpy/fft/_pocketfft.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@

Routines in this module:

fft(a, n=None, axis=-1, norm=None)
ifft(a, n=None, axis=-1, norm=None)
rfft(a, n=None, axis=-1, norm=None)
irfft(a, n=None, axis=-1, norm=None)
hfft(a, n=None, axis=-1, norm=None)
ihfft(a, n=None, axis=-1, norm=None)
fftn(a, s=None, axes=None, norm=None)
ifftn(a, s=None, axes=None, norm=None)
rfftn(a, s=None, axes=None, norm=None)
irfftn(a, s=None, axes=None, norm=None)
fft2(a, s=None, axes=(-2,-1), norm=None)
ifft2(a, s=None, axes=(-2, -1), norm=None)
rfft2(a, s=None, axes=(-2,-1), norm=None)
irfft2(a, s=None, axes=(-2, -1), norm=None)
fft(a, n=None, axis=-1, norm="backward")
ifft(a, n=None, axis=-1, norm="backward")
rfft(a, n=None, axis=-1, norm="backward")
irfft(a, n=None, axis=-1, norm="backward")
hfft(a, n=None, axis=-1, norm="backward")
ihfft(a, n=None, axis=-1, norm="backward")
fftn(a, s=None, axes=None, norm="backward")
ifftn(a, s=None, axes=None, norm="backward")
rfftn(a, s=None, axes=None, norm="backward")
irfftn(a, s=None, axes=None, norm="backward")
fft2(a, s=None, axes=(-2,-1), norm="backward")
ifft2(a, s=None, axes=(-2, -1), norm="backward")
rfft2(a, s=None, axes=(-2,-1), norm="backward")
irfft2(a, s=None, axes=(-2, -1), norm="backward")

i = inverse transform
r = transform of purely real data
Expand Down Expand Up @@ -85,8 +85,8 @@ def _go_forw(n, norm):
return sqrt(n)
elif norm == "forward":
return n
raise ValueError(f"Invalid norm value {norm}; should be \"backward\"\
(None), \"ortho\" or \"forward\".")
raise ValueError(f'Invalid norm value {norm}; should be "backward",\
"ortho" or "forward".')


def _go_back(n, norm):
Expand All @@ -99,8 +99,17 @@ def _go_back(n, norm):
return sqrt(n)
elif norm == "forward":
return 1
raise ValueError(f"Invalid norm value {norm}; should be \"backward\"\
(None), \"ortho\" or \"forward\".")
raise ValueError(f'Invalid norm value {norm}; should be "backward",\
"ortho" or "forward".')


def _swap_direction(norm):
try:
return {"backward": "forward", None: "forward",
"ortho": "ortho", "forward": "backward"}[norm]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is micro-optimization, but I'd prefer a plain if-else over dict for simple cases like this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from how compact it is, I think this is superior to a plain if-else clause bcs it handles the KeyError exception very well; if it's alright with you I'd suggest we go ahead with the try clause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with this?

if ...:
    # do this
elif ...:
    # do that
else:
    raise ValueError(...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move the dictionary into a module level constant, dict lookup can actually be faster on average than a series of if-else chains:

_SWAP_DIRECTION_MAP = {...}

def _swap_direction(norm):
  try:
    return _SWAP_DIRECTION_MAP[norm]
  except KeyError:
    raise ValueError("")

There's not much in it though, a few 10s of nanoseconds at most I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Peter! Is it because this way we save time for the dict construction?

Like I said this is micro optimization, that I was just curious why dict was preferred is all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my reply I didn't mean that using an if clause cannot handle the exception, I just meant that using a try clause makes it (IMO) cleaner and easier to read; other than that I'm happy with an if clause too;

initially I was thinking of writing it with an if clause, but @peterbell10 suggested the dict solution which I found much more streamlined compared to what I would have come up with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've added a commit moving the dictionary into a module level constant to accelerate the lookup; it looks like our two options are: (1) use a try clause with the dict lookup with the module level constant, (2) revert to an if clause for the direction swap instead;

let me know what you think; from the comments I understand that these are comparable in terms of performance @peterbell10 @leofang

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it because this way we save time for the dict construction?

Yes, if it's inside the function then the dict is reconstructed for every function call whereas the module level variable is constructed only once at import time.

As for dict vs if-else in a more general sense. I have no strong feeling either way, both are not that much code and both can be quite readable.

except KeyError:
raise ValueError(f'Invalid norm value {norm}; should be "backward",\
"ortho" or "forward".')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise ValueError(f'Invalid norm value {norm}; should be "backward",\
"ortho" or "forward".')
raise ValueError(f'Invalid norm value {norm}; should be "backward", '
'"ortho" or "forward".') from None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The line continuation change also above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok



def _fft_dispatcher(a, n=None, axis=None, norm=None):
Expand Down Expand Up @@ -131,12 +140,11 @@ def fft(a, n=None, axis=-1, norm=None):
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`).
Default is "backward" (alias of None).
Normalization mode (see `numpy.fft`). Default is "backward".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have 1-2 sentences here pointing out that the normalization signals where the normalization occurs in the fft-ifft pair. And/or just making this slightly longer to provide some of the info from the general docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


.. versionadded:: 1.20.0

The "backward", "forward" options were added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -239,14 +247,14 @@ def ifft(a, n=None, axis=-1, norm=None):
axis : int, optional
Axis over which to compute the inverse DFT. If not given, the last
axis is used.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -321,14 +329,14 @@ def rfft(a, n=None, axis=-1, norm=None):
axis : int, optional
Axis over which to compute the FFT. If not given, the last axis is
used.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -421,14 +429,14 @@ def irfft(a, n=None, axis=-1, norm=None):
axis : int, optional
Axis over which to compute the inverse FFT. If not given, the last
axis is used.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -513,14 +521,14 @@ def hfft(a, n=None, axis=-1, norm=None):
axis : int, optional
Axis over which to compute the FFT. If not given, the last
axis is used.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -584,8 +592,8 @@ def hfft(a, n=None, axis=-1, norm=None):
a = asarray(a)
if n is None:
n = (a.shape[axis] - 1) * 2
output = irfft(conjugate(a), n, axis)
output *= _go_back(n, norm)
new_norm = _swap_direction(norm)
output = irfft(conjugate(a), n, axis, norm=new_norm)
return output


Expand All @@ -607,14 +615,14 @@ def ihfft(a, n=None, axis=-1, norm=None):
axis : int, optional
Axis over which to compute the inverse FFT. If not given, the last
axis is used.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -649,8 +657,8 @@ def ihfft(a, n=None, axis=-1, norm=None):
a = asarray(a)
if n is None:
n = a.shape[axis]
output = conjugate(rfft(a, n, axis))
output *= 1./_go_back(n, norm)
new_norm = _swap_direction(norm)
output = conjugate(rfft(a, n, axis, norm=new_norm))
return output


Expand Down Expand Up @@ -713,14 +721,14 @@ def fftn(a, s=None, axes=None, norm=None):
axes are used, or all axes if `s` is also not specified.
Repeated indices in `axes` means that the transform over that axis is
performed multiple times.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -823,14 +831,14 @@ def ifftn(a, s=None, axes=None, norm=None):
axes are used, or all axes if `s` is also not specified.
Repeated indices in `axes` means that the inverse transform over that
axis is performed multiple times.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -916,14 +924,14 @@ def fft2(a, s=None, axes=(-2, -1), norm=None):
axes are used. A repeated index in `axes` means the transform over
that axis is performed multiple times. A one-element sequence means
that a one-dimensional FFT is performed.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -1017,14 +1025,14 @@ def ifft2(a, s=None, axes=(-2, -1), norm=None):
axes are used. A repeated index in `axes` means the transform over
that axis is performed multiple times. A one-element sequence means
that a one-dimensional FFT is performed.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -1101,14 +1109,14 @@ def rfftn(a, s=None, axes=None, norm=None):
axes : sequence of ints, optional
Axes over which to compute the FFT. If not given, the last ``len(s)``
axes are used, or all axes if `s` is also not specified.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -1183,14 +1191,14 @@ def rfft2(a, s=None, axes=(-2, -1), norm=None):
Shape of the FFT.
axes : sequence of ints, optional
Axes over which to compute the FFT.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -1246,14 +1254,14 @@ def irfftn(a, s=None, axes=None, norm=None):
`len(s)` axes are used, or all axes if `s` is also not specified.
Repeated indices in `axes` means that the inverse transform over that
axis is performed multiple times.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down Expand Up @@ -1333,14 +1341,14 @@ def irfft2(a, s=None, axes=(-2, -1), norm=None):
axes : sequence of ints, optional
The axes over which to compute the inverse fft.
Default is the last two axes.
norm : {None, "ortho", "forward"}, optional
norm : {"backward", "ortho", "forward"}, optional
.. versionadded:: 1.10.0

Normalization mode (see `numpy.fft`). Default is None.
Normalization mode (see `numpy.fft`). Default is "backward".

.. versionadded:: 1.20.0

The "forward" option was added.
The "backward", "forward" values were added.

Returns
-------
Expand Down
Loading
0