10000 Support `norm="unnormalized"` for FFT functions? · Issue #16126 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Support norm="unnormalized" for FFT functions? #16126

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

Jump to bottom
Closed
leofang opened this issue May 1, 2020 · 5 comments · Fixed by #16476
Closed

Support norm="unnormalized" for FFT functions? #16126

leofang opened this issue May 1, 2020 · 5 comments · Fixed by #16476
Labels
01 - Enhancement component: numpy.fft

Comments

@leofang
Copy link
Contributor
leofang commented May 1, 2020

I tried searching in the issue tracker here but didn't find any, so let me know if this is a duplicate.

This issue was first brought up in CuPy (cupy/cupy#944) and has regained some interest, and I believe it's better to be implemented/considered first in upstream, which downstream libraries like SciPy and CuPy can follow.

The desired effect is to do the normalization opposite to norm=None; that is, the backward/inverse transform is unscaled, and the forward transform is scaled by 1/n. For a perfectionist's point of view adding this option makes a lot of sense, as it would cover every possible normalization scheme.

In particular, for GPU libraries like CuPy this could be performance critical for certain applications, as kernel launching is more costly in the GPU world. People usually don't do extra work whenever possible, especially if the normalization is of no interest.

The name "unnormalized" can be changed to a more informative one. I don't have any preference (in fact I'm just a messenger bringing this to NumPy...)
@leofang leofang mentioned this issue May 1, 2020
Closed
@anirudh2290
Copy link
Member

Related PR and Issue: #3883 and #2142 , adding a new kwarg value for norm seems to be doable if there is a usecase for it.
cc @charris

@leofang leofang mentioned this issue May 18, 2020
Closed
@cval26
Copy link
Contributor
cval26 commented Jun 1, 2020

Checked the issue and I'm about to start working on it; is somebody else working on that at the moment? I understand that we work without assigning issues, so I thought I'd check with you first.

I've been using the fft and ifft functions and am also interested in the normalization you're discussing; to do so up to now, I was using norm=None and then dividing/multiplying the returned vector manually by n.

I'm going to introduce an additional norm option for the new scaling; I was thinking of calling it norm=inverse, since it's the inverse scaling compared to norm=None (ie the forward transform is scaled with 1/n and the backward with 1). But that's just a recommendation, let me know if you think something else would be better suited.

@leofang
Copy link
Contributor Author
leofang commented Jun 1, 2020

Hi @cvav-git I am not aware of anyone working on this. norm=inverse sounds fine to me (previously I was thinking norm=full, but yours seems more informative), but perhaps you should discuss the work plan with @anirudh2290 @charris?

cc: @mattvend who was working on the CuPy counterpart.

@cval26
Copy link
Contributor
cval26 commented Jun 1, 2020

Thanks for your quick reply @leofang!

Hi @anirudh2290 @charris my current plan is to modify the _unitary(norm) flag to be True if norm=ortho and False if norm=None or norm=inverse (name still under discussion); this seems to make the smallest modifications to the current code, then I'll have to add an additional if clause in the various transform functions to distinguish between norm=None and inverse so that the scaling is done accordingly.

Let me know if you have any thoughts or comments so that we try to find the best solution possible; and of course the new norm name is still open to discussion, I'm just using the tentative name inverse to proceed with the modifications.

cc @mattvend

@cval26 cval26 mentioned this issue Jun 1, 2020
Merged
@cval26
Copy link
Contributor
cval26 commented Jun 1, 2020

I've pushed my code changes and opened a pull request regarding this issue; I've cc'd you on the request description; let me know how to proceed, thanks!

@leofang @anirudh2290 @charris @mattvend

@grlee77 grlee77 mentioned this issue Jul 9, 2020
Closed
seberg pushed a commit that referenced this issue Jul 12, 2020
@leofang
ENH: add norm=forward,backward to numpy.fft functions (#16476)
0862be9 
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 #16126

* modified _unitary, fft & ifft and their tests; tests passed

* modified rfft & irfft and their tests; tests passed

* modified hfft & ihfft and all tests; all modifications are now done

* correction in ihfft; all 79 fft tests passed successfully

* undo unnecessary docstrings changes made in my previous commits

* use norm=forward kwarg value name

* Update numpy/fft/__init__.py

Co-authored-by: Leo Fang <leofang@bnl.gov>

* add code review suggestions

Co-authored-by: Leo Fang <leofang@bnl.gov>

* add default norm=None alias norm=backward

* streamline private normalization functions

* modify hermitian FFTs

* add review suggestions

Co-authored-by: Leo Fang <leofang@bnl.gov>

* add review suggestions v2 (dict as module const)

* make review suggestions v3

* Apply suggestions from code review

Co-authored-by: Leo Fang <leofang@bnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
01 - Enhancement component: numpy.fft
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ENH: add norm=forward,backward to numpy.fft functions
3 participants
@anirudh2290 @leofang @cval26
0