Skip to content
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.

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

MAINT: Update fft.helper import #19426

Merged
merged 3 commits into from Oct 23, 2023

Conversation

mtsokol
Copy link
Contributor

@mtsokol mtsokol commented Oct 23, 2023

Hi!
numpy.fft.helper submodule was deprecated and accessing it raises a warning (it was renamed to a private name numpy.fft._helper).

Here I update the import statement in fftpack - it uses fftshift, ifftshift and fftfreq functions defined in scipy.fft._helper submodule. Internally they import the correct (non-deprecated) function from NumPy.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

So the change is IMO more substantial than the PR title implies, but from what I can tell, it LGTM.

Approval conditional on passing CI

Copy link
Member

Choose a reason for hiding this comment

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

I checked the submodule update, and the one relevant change w.r.t. this PR is:

-from numpy.core.numeric import normalize_axis_tuple
+import numpy as np
+if np.__version__[0] == "2":
+    from numpy.lib.array_utils import normalize_axis_tuple
+else:
+    from numpy.core.numeric import normalize_axis_tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was introduced for SciPy specifically: data-apis/array-api-compat#63.

Copy link
Member

Choose a reason for hiding this comment

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

See gh-19406. This change needs to make its way into SciPy at some point, but I don't know whether it should come in a separate PR.

Comment on lines 2 to 4
from numpy.fft.helper import fftshift, ifftshift, fftfreq

import scipy.fft._pocketfft.helper as _helper
from scipy.fft._helper import fftshift, ifftshift, fftfreq
Copy link
Member

Choose a reason for hiding this comment

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

This changes the import from numpy to scipy, and post-#19005, that means going through the array API (see here).

Co-authored-by: peterbell10 <peterbell10@live.co.uk>
@tylerjereddy tylerjereddy added the maintenance Items related to regular maintenance tasks label Oct 23, 2023
@lucascolley
Copy link
Member

Agreed with Peter that we should avoid going through scipy.fft (hitting array API stuff) until it is decided whether fftpack will become compatible or not (currently TBD on gh-18867).

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

CI is happy and the imports only change to avoid the deprecated NumPy namespace now, after the PR revisions.

@tylerjereddy tylerjereddy merged commit 930fac3 into scipy:main Oct 23, 2023
21 of 22 checks passed
@tylerjereddy tylerjereddy added this to the 1.12.0 milestone Oct 23, 2023
@mtsokol mtsokol deleted the update-fft-helper-import branch October 23, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants