ENH: update interfaces to support new numpy.fft normalization options #308
Conversation
| # if I change it to map None:"forward" some `scipy_interface` tests fail. | ||
| # see also the if-clauses in the sine/cosine transforms in `scipy_fft.py`; | ||
| # there's some mishandling of norm=None somewhere. | ||
| _swap_norm_dictionary = {"backward": "forward", None: None, |
There was a problem hiding this comment.
that mapping of None:None was like that in the code, and I'm not sure why it shouldn't map None:"forward"; I've left it as is for now because some of the tests were failing otherwise (see code comments)
| if norm is None: | ||
| norm = 'backward' | ||
| norm = "backward" |
There was a problem hiding this comment.
also not sure why that change is necessary in the scipy_fft sine/cosine transforms
There was a problem hiding this comment.
This seems suspicious to me...
There was a problem hiding this comment.
both this if-clause and the mapping None:None referenced in my comment above seem to be related to the work done on #256.
I don't want to start changing too many things unrelated to the current PR, so I left them unchanged to make sure no tests are failing. Could be looked into with a new PR that focuses on changes leftover from #256 and similar work.
There was a problem hiding this comment.
Ok, but changing single quotes to double quotes shouldn't change anything.
There was a problem hiding this comment.
no the single-to-double quotes change was just done while I was doing aesthetic changes in bulk (unrelated); the change I'm referring to is why should one need to change norm=None to norm="backward" for sine/cosine transforms, even though they're defined as equivalent parameter values.
Such a change is not necessary for any of the other FFTs.
There was a problem hiding this comment.
Yeah, not sure about that.
As an aside, it would be appreciated if you didn't make bulk minor stylistic changes - it makes parsing the PR much harder and is arguably undesirable. Changes to be PEP8 compliant are valued, but just changing single quotes to double quotes seems like unnecessary noise to me.
There was a problem hiding this comment.
you're right I'm sorry about that; I made them in bulk through my editor so I underestimated how noisy it makes the PR changes look
There was a problem hiding this comment.
There's too much noise that is impossible for non-maintainers like me to help review (not saying I have time right now, but still)...(btw I made the counterpart change in CuPy: cupy/cupy#4797, cupy/cupy#4816) It'd be really nice if this PR can be split into two.
|
none of the CI build checks seem to run the tests with EDIT: NumPy is pinned to |
|
Great. Is this ready for review now? |
|
Yes I believe so, the main work is done. |
|
It would be good to get this in. Let me help rebase this after the tests folder was renamed. |
|
Thanks @chrisvalx, I have updated this in #330 so we can close this one and proceed from there (you are still credited as a commit author). |
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: update interfaces to support new numpy.fft normalization options
Closes #295.
This PR extends the normalization options of the
pyfftwbuilders andnumpy_interface, so that they're up to date with the currentnumpy.fftoptions (see numpy/numpy#16476). In particular, thenormkeyword argument now accepts the following values:norm=backward(alias ofnorm=None, default case) which scales the backward FFT with 1/N and leaves the forward unscaled;norm=orthowhich scales both directions with 1/sqrt(N);norm=forwardwhich scales the forward FFT with 1/N and leaves the backward unscaled.pyFFTW uses two boolean variables
orthoandnormalise_idftto determine the correct normalization, instead of one string variablenormlike in NumPy. The main change is in the builders utilities function_norm_args(norm)which receivesnormand outputs a dictionary with the correct scaling flags. Both the builders and the interfaces use this function._Xfftnhas also been modified to apply the correct normalization after the FFT has been executed. Thedctanddstbuilders don't accept any normalization options other than the default, so no changes made there.The main change in the NumPy interface is in the hermitian routines
hfftandihfft; due to symmetry,hfftis done viairfftandihfftviarfft, which means that before calling the real FFTs we need to swap direction of the normalization flags. For that I've added the_swap_direction_dictmodule variable and the_swap_direction(norm)function, which make sure the "real" norm value is established correctly; that's now handled in the exact same way as in NumPy.I've added tests in the builders testsuite that check the new norm options are called correctly, and that roundtrip equality holds for all norm values (i.e.
ifft(fft(a)) == aup to numerical accuracy for allnormvalues). I've added those in the FFTW wrapper tests, since they require both forward and backward FFTs. For the NumPy interface I've added test cases for the newnormoptions, but couldn't figure out how to check for roundtrip consistency. The interfaces use the same underlying routines already checked by the builders tests, but it'd be nice to e 8000 xplicitly check the interface calls as well.All tests pass at the time of writing, but I'm not sure my approach is correct so if someone more experienced could take a look at the tests that'd be appreciated. Just so that we're sure all cases are covered properly.