ENH: add norm=forward,backward to numpy.fft functions
#16476
Conversation
# with '#' will be ignored, and an empty message aborts the commit. # # On branch fft-adding-norm-kwarg # Changes to be committed: # modified: _pocketfft.py # ENH: add new kwarg value `norm=inverse` to numpy.fft The kwarg option `norm=inverse` leads to scaling of the transforms inverse (opposite) to that of the default option `norm=None`; i.e. the forward transform is normalized with `1/n` whereas the backward one with `1`. The fft routines and their tests have been modified to reflect the changes; all tests have been passed successfully. Closes issue #16126
norm=inverse to numpy.fft (#16126)norm=inverse to numpy.fft
|
Thanks for quickly putting a PR together @cvav-git! I am not sure if I have enough time this week to help review, but it's on my plate 🙂 cc: @peterbell10 @grlee77 from SciPy/pyFFTW and @asi1024 from CuPy |
|
Absolutely @leofang, feel free to review it whenever it suits you best! I was just familiar with that part of the code and thought I'd put the PR together; it's one of my first contributions so I'm still learning a lot about the process, thank you for your help and support :) In the meantime I'll see if I can address some of the failing automated checks. EDIT: there are now 2 failing automated checks; one is on |
|
This PR led to some detailed discussions on normalization schemes in the NumPy community meeting; it would probably be good if this proposal hit the NumPy mailing list. |
|
Just wanna catch up: if there's already a discussion in the mailing list, can someone post a link to the thread head here please? |
|
You can follow the mailing list discussion here |
There was a problem hiding this comment.
Looks pretty good to me, the unitary function would be nice to refactor a bit to adapt to the new option I think. If you want, you can update this with forward with the slight risk of more discussion coming up with a better name.
Thanks, lets make sure to merge this pretty soon, please ping me if I forget!
|
I have not carefully checked the tests yet, but they seemed very straight forward and fine. |
There was a problem hiding this comment.
Thanks for working on this, @cvav-git! LGTM except for a few nitpicks.
I find NumPy's FFT tests have inconsistent styles (ex: some test if the original array can be obtained after a round trip, while others simply test the normalization factors). It could be that I'm finishing this on my phone so it's harder for me to see the full picture...Anyway I think this PR is fine.
Co-authored-by: Leo Fang <leofang@bnl.gov>
Co-authored-by: Leo Fang <leofang@bnl.gov>
|
following @peterbell10 comments on the NumPy mailing list, I've added |
|
following @seberg's earlier review suggestions, I've added a commit where I streamline the norm checks that were done by the private function Since @leofang has informed us previously that no |
|
following @peterbell10's review suggestions I've modified the hermitian FFTs to normalize the data only once; thanks to @peterbell10 for providing the new private function I've modified all docstrings to reflect the API changes and added a release document signaling the new feature. I've also modified the tests to explicitly test the In terms of the mailing list discussion, I think many ppl appreciate the naming consistence with respect to round trip equality; the ppl that don't find this naming scheme straightforward also seemed to understand its usefulness, and I believe there's been a couple of days with no new messages on this matter. |
numpy/fft/_pocketfft.py
Outdated
| return {"backward": "forward", None: "forward", | ||
| "ortho": "ortho", "forward": "backward"}[norm] |
There was a problem hiding this comment.
Maybe this is micro-optimization, but I'd prefer a plain if-else over dict for simple cases like this...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
What's wrong with this?
if ...:
# do this
elif ...:
# do that
else:
raise ValueError(...)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Co-authored-by: Leo Fang <leofang@bnl.gov>
|
I've added a commit with the review suggestions; the only thing I'd argue we keep as is is the |
|
@cvav-git, Can you please update the PR title to reflect the new option name (norm='forward')? (Or to something generic like "add new forward normalization option to numpy.fft") |
norm=inverse to numpy.fftnorm=forward,backward to numpy.fft functions
norm=forward,backward to numpy.fft functionsnorm=forward,backward to numpy.fft functions
There was a problem hiding this comment.
Thanks, LGTM, lets finish this off. The error message seems malformed, I would just avoid the \ line continuation entirely. It would be great to have some very short reminder in the norm kwarg docs, I am not quite sure what that should be, maybe anyone else here has an idea?
numpy/fft/_pocketfft.py
Outdated
| raise ValueError(f'Invalid norm value {norm}; should be "backward",\ | ||
| "ortho" or "forward".') |
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
(The line continuation change also above)
| .. versionadded:: 1.10.0 | ||
|
|
||
| Normalization mode (see `numpy.fft`). Default is None. | ||
| Normalization mode (see `numpy.fft`). Default is "backward". |
There was a problem hiding this comment.
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.
numpy/fft/_pocketfft.py
Outdated
| return True | ||
| raise ValueError("Invalid norm value %s, should be None or \"ortho\"." | ||
| % norm) | ||
| def _go_forw(n, norm): |
There was a problem hiding this comment.
Maybe rename to _get_forward_normalization? I guess these functions are so short that any docstring is not helpful :).
numpy/fft/__init__.py
Outdated
| 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}`. | ||
| The default normalization has the direct (forward) transforms unscaled and the |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
ok; added a commit with all review suggestions @seberg
There was a problem hiding this comment.
Thanks, will merge once the tests run through (maybe minus the azure tests if they are acting up again).
|
Ok, the s390x job is hanging and azure is acting up right now (due to a setuptools update). But everything was passing, and all other checks pass and it is a python only small update. So putting it in, thanks again @cvav-git. |
|
great, glad I could help! thanks for taking care of this PR :) |
|
Thank you, @cvav-git and all! |
In older versions of PyFFT, the `normalise_idft` argument was only relevant for the inverse direction, else ignored. ODLs `pyfftw_call` wrapper was modelled around this behaviour. Starting with PyFFT-0.13, the new NumPy normalisation arguments `norm='forward'` and `norm='backward'` (numpy/numpy#16476) were adopted for the high-level binders (pyFFTW/pyFFTW#308, merged in pyFFTW/pyFFTW#330). They implemented this by pre-matching the `norm` argument and setting `normalise_idft` accordingly. In case of the 'forward' normalisation this involves using `normalise_idft=False` to signal that the normalisation should instead happen in the forward direction, which was previously not even possible. In other words, in PyFFT<0.13 `normalise_idft=False` meant no normalisation at all was used (neither for fft nor ifft, with the consequence that they weren't inverses in this case), and that is the behaviour ODL expected. Explicitly setting `normalise_idft=True` in this specific case restores the desired behaviour, and is backwards-compatible because PyFFT<0.13 ignores the `normalise_idft` argument in the forward direction.
In older versions of PyFFT, the `normalise_idft` argument was only relevant for the inverse direction, else ignored. ODLs `pyfftw_call` wrapper was modelled around this behaviour. Starting with PyFFT-0.13, the new NumPy normalisation arguments `norm='forward'` and `norm='backward'` (numpy/numpy#16476) were adopted for the high-level binders (pyFFTW/pyFFTW#308, merged in pyFFTW/pyFFTW#330). They implemented this by pre-matching the `norm` argument and setting `normalise_idft` accordingly. In case of the 'forward' normalisation this involves using `normalise_idft=False` to signal that the normalisation should instead happen in the forward direction, which was previously not even possible. In other words, in PyFFT<0.13 `normalise_idft=False` meant no normalisation at all was used (neither for fft nor ifft, with the consequence that they weren't inverses in this case), and that is the behaviour ODL expected. Explicitly setting `normalise_idft=True` in this specific case restores the desired behaviour, and is backwards-compatible because PyFFT<0.13 ignores the `normalise_idft` argument in the forward direction.
ENH: add new kwarg value
norm=inverseto numpy.fftThe kwarg option
norm=inverseleads to scaling of the transformsinverse (opposite) to that of the default option
norm=None; i.e. theforward transform is normalized with
1/nwhereas the backward onewith
1. The fft routines and their tests have been modified toreflect the changes; all tests have been passed successfully.
Closes #16126
cc @leofang @anirudh2290 @charris @mattvend