10000 Add `device` keyword to fftfreq and rfftfreq by steff456 · Pull Request #506 · data-apis/array-api · GitHub
[go: up one dir, main page]
Skip to content

Add device keyword to fftfreq and rfftfreq #506

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
Merged
merged 4 commits into from
Nov 28, 2022
Merged

Add device keyword to fftfreq and rfftfreq #506

merged 4 commits into from
Nov 28, 2022

Conversation

steff456
Copy link
Member
@steff456 steff456 commented Nov 11, 2022 •
edited
Loading

This PR,

  • Adds device as arguments to fftfreq
  • Adds device as arguments to rfftfreq

This PR fixes #500

@steff456 steff456 added the API change Changes to existing functions or objects in the API. label Nov 11, 2022
@steff456 steff456 added this to the v2022 milestone Nov 11, 2022
@steff456 steff456 requested review from kgryte and leofang November 11, 2022 19:20
@steff456 steff456 self-assigned this Nov 11, 2022
@oleksandr-pavlyk
Copy link
Contributor

Should we mention that only inexact data types, i.e. "real floating" and "complex floating" are expected to be supported?

kgryte
kgryte reviewed Nov 14, 2022
View reviewed changes
kgryte
kgryte reviewed Nov 14, 2022
View reviewed changes
@kgryte
Copy link
Contributor
kgryte commented Nov 14, 2022

@oleksandr-pavlyk Updated the PR to require that dtype must be a floating-point data type. This work for you?

@rgommers
Copy link
Member

This change matches the discussion. device seems clear. Just a question about dtype before we actually merge this though: is that actually useful in any real-world circumstance? I assume it would return the same values as for real dtypes, just with the .imag part all zeros?

@oleksandr-pavlyk
Copy link
Contributor

@rgommers I was thinking the same thing. The dtype is always going to be dictated by data type of d, since d is allowed to be 0d array. If d is a Python scalar, then data type is either default real-valued or complex-valued floating-point type.

@steff456
Copy link
Member Author

As discussed in our previous meeting, we are not going to add dtype as an argument.

@kgryte
Copy link
Contributor
kgryte commented Nov 27, 2022

Found a previous issue on the NumPy issue tracker (numpy/numpy#9126) raised by @jakirkham concerning supporting a dtype keyword for fftfreq and rfftfreq. The PR implementing the feature (numpy/numpy#16496) was closed due to apparent implementation concerns, but not on merits.

@rgommers rgommers changed the title Add dtype and device to fftfreq and rfftfreq Add ~dtype and~ device to fftfreq and rfftfreq Nov 28, 2022
@rgommers rgommers changed the title Add ~dtype and~ device to fftfreq and rfftfreq Add device keyword to fftfreq and rfftfreq Nov 28, 2022
rgommers
rgommers approved these changes Nov 28, 2022
View reviewed changes
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.

LGTM, this addresses gh-500 by adding the needed device keyword.

The dtype discussion seems to be of theoretical interest, but - given that no library actually implement it - of very limited practical value. Let's punt on that; if anyone actually needs it, let them implement it at least in numpy first.

@rgommers rgommers merged commit dc98c25 into data-apis:main Nov 28, 2022
@steff456 steff456 deleted the device-fft branch November 28, 2022 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgryte kgryte kgryte left review comments

@rgommers rgommers rgommers approved these changes

@leofang leofang leofang approved these changes

Assignees

@steff456 steff456

Labels
API change Changes to existing functions or objects in the API.
Projects
None yet
Milestone
v2022
Development

Successfully merging this pull request may close these issues.

fftfreq, rfftfreq are missing device keyword
5 participants
@steff456 @oleksandr-pavlyk @kgryte @rgommers @leofang
0