Skip to content

ENH: Add fft optional extension submodule to numpy.array_api #25317

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

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Dec 6, 2023

Note: we need to address a few issues upstream in the standard and the array-api-tests before this should be merged.

@asmeurer
Copy link
Member Author

asmeurer commented Jan 8, 2024

This should be ready for review. It's possible some of the dtype restrictions will change in the spec (see the discussion at data-apis/array-api#720 (comment)), but if that happens we can update this with another PR.

asmeurer added a commit to asmeurer/array-api-compat that referenced this pull request Jan 9, 2024
@charris charris changed the title Add fft optional extension submodule to numpy.array_api ENH: Add fft optional extension submodule to numpy.array_api Jan 10, 2024
@charris
Copy link
Member

charris commented Jan 10, 2024

Will need a release note.

@jakevdp
Copy link
Contributor

jakevdp commented Jan 10, 2024

I think the tests are still skipped:

array_api_tests/test_fft.py

When trying this in JAX, I found that array_api_tests/test_fft.py has a number of dtype expectations that don't align with the dtypes of jax.numpy.fft, which was designed to follow numpy.fft. So I'm interested to see if you run into the same isssue.

@asmeurer
Copy link
Member Author

I didn't realize that the array api tests are run on the numpy CI now. I've been running them manually. The dtype stuff is addressed here. The fft functions in numpy all upcast 32-bit inputs, which the spec says they shouldn't. This is addressed here by re-downcasting, but I also plan to fix this in numpy.fft itself (my understanding is that this should be straightforward once #25536 is merged). There are also some issues in the test suite itself so we may need to get those addressed before we remove the skips.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks Aaron, and thanks Jake for the review. I think this is close - I think I found one mistake in dtype handling still, and the device handling will need an update either here or in gh-25370 depending on which PR is merged first.


See its docstring for more information.
"""
if device not in ["cpu", None]:
Copy link
Member

Choose a reason for hiding this comment

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

Note that this will have a cross-merge conflict with gh-25370, where the 'cpu' string is replaced with a custom object. The same applies to rfftfreq below.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This looks good to go now, thanks Aaron. In it goes.

@rgommers rgommers merged commit 8e8da65 into numpy:main Jan 16, 2024
@lucascolley
Copy link
Contributor

I'll test this in SciPy with a NumPy nightly soon

@lucascolley
Copy link
Contributor

Note: we need to address a few issues upstream in the standard and the array-api-tests before this should be merged

Does this mean that array-api-tests is up to scratch for fft now and we can remove the skip in CI here?

@asmeurer
Copy link
Member Author

Does this mean that array-api-tests is up to scratch for fft now and we can remove the skip in CI here?

No, the test suite still needs to be updated for data-apis/array-api#720, and also some of the shape tests are wrong. (cc @honno)

At any rate, as soon as #25370 is merged the plan is to move numpy.array_api to another repo, and (presumably) delete it from this one, so I wouldn't worry too much right now about the array api CI skips for numpy.array_api. As for the main namespace fft, it is still incorrect in upcasting 32-bit inputs. I plan to fix this, but I need #25536 first, as I understand it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants