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

Wrap fft for dask #139

Merged
merged 22 commits into from
Oct 16, 2024
Merged

Wrap fft for dask #139

merged 22 commits into from
Oct 16, 2024

Conversation

lithomas1
Copy link
Contributor

@lithomas1 lithomas1 commented May 19, 2024

There's some failures relating to the expected dtype on older numpy versions.

@asmeurer
Copy link
Member

The wrappers in common should already be handling the dtype difference. It looks like the norm keyword isn't working, though.

@lithomas1
Copy link
Contributor Author

Going to pick this up again this week.

Thanks for not closing.

@@ -40,7 +40,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.9', '3.10', '3.11', '3.12']
python-version: ['3.10', '3.11', '3.12']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe array-api-tests is using a typing feature (|) from Python 3.10, which is causing us to fail here.

Not sure if it's worth patching there, as NEP29 says 3.9 should be dropped anyways.

Copy link
Member

Choose a reason for hiding this comment

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

That should be fixed upstream now. Let's keep 3.9 support for now. We should discuss dropping Python support in another issue. I'd prefer to be as conservative as possible with supported Python versions (i.e., not drop support unless we really need to), to maximize compatibility for people.

@lithomas1 lithomas1 marked this pull request as ready for review August 24, 2024 18:43
@lithomas1
Copy link
Contributor Author

This should be ready for a look now.

Tests are passing with dask main.
(this should be mergeable whenever dask cuts its next release).

@asmeurer
Copy link
Member

asmeurer commented Sep 3, 2024

Does dask still require the wrappers here? The only thing they do for NumPy is fix upcasting for 32-bit types, which has been fixed in NumPy 2.0. If the next version of dask has the NumPy 2.0 behavior then no wrapping is necessary at all.

Copy link
Contributor

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

There are errors in SciPy for the device arg of {r}fftfreq. I don't know if array-api-tests has coverage of that, but it should be wrapped here.

EDIT: yes, it looks like CI has caught that here.

@lithomas1
Copy link
Contributor Author

Ah thanks, I'll go and wrap that.

Curious why it's only on fftfreq and not on fft

@lucascolley
Copy link
Contributor

fftfreq creates an array from scalar inputs, so there is no device to infer.

fft takes arrays as input so can infer the device.

@lithomas1
Copy link
Contributor Author

I think I was actually wrapping stuff incorrectly all this time
(I forgot to import the wrappers in init)

I'm going to give only wrapping fftfreq/rfftfreq a shot.

@lithomas1
Copy link
Contributor Author

@asmeurer
This should be ready for a look now.

Sorry for the long delay, had to wrangle with Github Actions/the __all__ tests for a while.

Copy link
Contributor

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

nice stuff!

array_api_compat/dask/array/fft.py Outdated Show resolved Hide resolved
@@ -40,7 +40,8 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ['3.9', '3.10', '3.11', '3.12']
# min version of dask we needs drops support for python 3.9
python-version: ${{ inputs.package-name == 'dask' && fromJson('[''3.10'', ''3.11'', ''3.12'']') || fromJson('[''3.9'', ''3.10'', ''3.11'', ''3.12'']') }}
Copy link
Member

Choose a reason for hiding this comment

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

There's other similar skips in some of the lines below. We should probably consolidate. I don't know if there's a cleaner way to do it but if you are aware of one let me know!

Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would put these sorts of skips in the individual action files for each package, but I don't know if it's possible to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for now, it might be easier to keep it in here (even though it's less clean), since I don't think any of the other libraries need this Python restriction.

Assuming Python 3.9 gets dropped sometime soonish, it'd probably be easier to remove in this state.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably come up with a policy, but I've tried to be very conservative in supporting old versions to make it so this package is as easy for people to adopt as possible. So I don't have immediate plans to drop Python 3.9, although we will obviously want to do so at some point especially as all the wrapped packages stop supporting it.

@asmeurer
Copy link
Member

This looks good, and I think the existing tests should be checking all the code that's been added here, so if tests are passing I'm reasonably confident this is all correct.

@asmeurer asmeurer merged commit 5affae5 into data-apis:main Oct 16, 2024
42 checks passed
@lithomas1 lithomas1 deleted the dask-fft branch October 16, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants