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

Begin deprecation of implicit input conversion in FFT module #20818

Closed
wants to merge 1 commit into from

Conversation

Micky774
Copy link
Collaborator

@Micky774 Micky774 commented Apr 18, 2024

Towards #20200

Array API 2023 changelog.

Updates the jax.numpy.fft namespace to begin a deprecation of implicitly converted inputs -- explicitly sets fft function domains to either real floating or complex, warning on implicit conversion (i.e. fft(x: float32), rfft(x: int32), etc.).

Updates the jax.experimental.array_api.fft namespace with corresponding changes which directly raise ValueError, marked with deprecation once jax.numpy.fft is array API compliant.

Adds corresponding tests for both modules.

Updates dtype coverage in tests/fft_test.py to reflect new restricted domains.

@jakevdp
Copy link
Collaborator

jakevdp commented Apr 19, 2024

I think this is a case where we should follow numpy rather than following the Array API to the letter. There are many places where the array API is more restrictive than NumPy, which we've been taking our cue from up until this point. Deprecating previously supported behavior when NumPy still supports it will lead to undue downstream churn, so I think we should avoid it. What do you think?

@Micky774
Copy link
Collaborator Author

I think this is a case where we should follow numpy rather than following the Array API to the letter. There are many places where the array API is more restrictive than NumPy, which we've been taking our cue from up until this point. Deprecating previously supported behavior when NumPy still supports it will lead to undue downstream churn, so I think we should avoid it. What do you think?

I would generally agree, however there are a few benefits for this that make me prefer the domain restrictions.

The domain restriction itself forces users to make an explicit decision regarding the transform they choose to use, pushing for using the most specific transform possible as opposed e.g. fft everything. Looking towards the scientific computing community, I think this by itself is a valuable aid, especially for folks that aren't familiar with the computational implications of fft vs say rfft or hfft.

Also, pulling from @kgryte who provided a helpful summary of some of the discussion that went into the decision from the consortium, both in meetings and in PRs (e.g. this comment thread):

in the spec, we've sided against implicit upcasting from real-to-complex, as users should be clear about what/how they want to cast. While a real-valued array can be interpreted as a complex array having omitted imaginary components which are all zero, this doesn't have to be the case. The array could be imaginary components or interleaved real and imaginary, etc. Particularly for the FFT APIs, certain transforms are very explicitly C2C or C2R transforms. In which case, providing a real-valued array is not the intent.

And ultimately afaik NumPy 2.0 will be implementing the domain restrictions, as numpy.array_api.fft already does and they intend for the general numpy namespace to be properly array API compatible, so we're just getting ahead of them on this one. We don't need to complete the deprecation until they've fully adopted it anyways, so we won't be fully diverging.

cc: @pearu in case you have some opinions on this too

@jakevdp
Copy link
Collaborator

jakevdp commented Apr 22, 2024

And ultimately afaik NumPy 2.0 will be implementing the domain restrictions,

Do you have a link to a NEP or other forum that discusses this?

@Micky774
Copy link
Collaborator Author

Okay it looks like I had misinterpreted the word "should" in the array API spec to imply room for backwards compatibility with the intent of deprecation, given the changelog wording

fft.fft: require the input array to have a complex-valued floating-point data type

however, that is not the case. As such, we are already compliant and this PR is not needed. Thanks for helping me catch that @jakevdp

@Micky774 Micky774 closed this Apr 22, 2024
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.

None yet

2 participants