Clean up some wording in the fft extension #720

asmeurer · 2023-12-12T00:31:10Z

Fixes #717.

I also plan to fix #718 here.

Note: this should also be backported, but I want to make sure the wording here looks good first before I do that.

Also, I haven't done it here, but we should consider whether we want to add a dtype argument to fftfreq and rfftfreq for consistency with the other creation functions. Right now, they return the default real floating-point dtype, but there's no way to change that. However, note that np.fft does not implement a dtype argument for these functions.

Ref: #189

Fixes data-apis#717

asmeurer · 2023-12-12T00:42:40Z

"trimmed to length n" also isn't very clear. Does this mean it should take the first n elements?

src/array_api_stubs/_draft/fft.py

Also cleanup some wording for consistency. This fixes the incorrect output shape for ihfft. Fixes data-apis#718.

asmeurer · 2023-12-12T01:09:11Z

I've also pushed an update to specify output lengths. Please check carefully.

I've also tried to clean up the wording a bit. However, I'm not sure if my wording here is always consistent with wording elsewhere in the spec, so let me know if something should be changed.

I noticed that we aren't consistent throughout the spec with "length" vs. "size" to refer to the size of an axis.

src/array_api_stubs/_draft/fft.py

PR-URL: data-apis#653

PR-URL: data-apis#701

Closes: data-apis#667 PR-URL: data-apis#713

PR-URL: data-apis#723 Reviewed-by: Athan Reines <kgryte@gmail.com>

rgommers · 2024-01-09T21:38:03Z

Okay, this took a bit of digging, but I believe that @leofang is largely correct. The current state of the fft docs is inconsistent, and has been in that state since the initial merge of gh-189. The problem is that the Parameters section says that the first input x must have a "floating-point dtype" (so may be real), while the prominent note right above it says:

"Applying the one-dimensional inverse discrete Fourier transform to the output of this function must return the original (i.e., non-transformed) input array within numerical accuracy (i.e., ifft(fft(x)) == x), provided that the transform and inverse transform are performed with the same arguments (length, axis, and normalization mode)."

Clearly that is not the case if x would be real, because the roundtripping changes the dtype from real to complex (e.g., float64 to complex128). Hence real input should be rejected.

So far the spec - let's update the wording to reflect that. For implementations:

The reference implementation should raise on non-complex input dtypes to fft/fftn.
For NumPy 2.0, that'd be one bridge too far I think, so let's open an issue first - there are more issues with numpy.fft that need addressing. Accepting non-complex input and treating it like complex with a zero imaginary component will simply be one oddity that perhaps can be addressed over time.

asmeurer · 2024-01-10T00:34:44Z

Would we want to backport this to 2022.12, or just add it to the draft?

See the discussion at data-apis#720 This still requires changelog entries.

asmeurer · 2024-01-10T00:40:16Z

I've pushed a change that updates these functions to only require complex-valued inputs. I think this will also require a changelog entry, but I didn't add it yet because I don't know if it will be part of 2023.12 only or backported.

This PR also requires some review for other the wording changes that have been made, as discussed above.

kgryte · 2024-01-10T01:29:09Z

IMO, I think it is a bug in the 2022.12 revision and should be backported. For the changelog entry, we can add that in a subsequent PR along with the other specification changes which should be documented similarly.

The problem was that the sidebar doesn't show the named pages in the toctree if that toctree is under a subheading in index.rst Closes data-apisgh-716

rgommers · 2024-01-11T22:48:20Z

These changes all LGTM. I think if they are backported and added to the Changelog, this is good to merge.

leofang · 2024-01-12T00:58:03Z

Sorry I dropped the ball here. I think the PR looks good now from a quick glance. I can do a more thorough look tomorrow, but don't let me stop the progress 🙂 Thanks Aaron and all for the improvement!

Also extend the description of errors when DLPack cannot be supported

asmeurer · 2024-01-12T21:45:11Z

I've backported everything to 2022.12. I've left out the changelog based on #720 (comment).

kgryte · 2024-02-08T07:16:03Z

@leofang Looking at the CI logs, looks like you need to locally install dev deps in order to run the pre-commit hook which will apply black formatting before pushing.

…o fft-fixes

kgryte · 2024-02-08T09:50:00Z

src/array_api_stubs/_2022_12/fft.py

-        - ``n`` is less than the length of the input array, the input array is trimmed to length ``n``.
-        - ``n`` is not provided, the length of the transformed axis of the output must equal the length of the input along the axis specified by ``axis``.
+        - ``n`` is greater than the length of the input array along ``axis``, the input array along ``axis`` must be zero-padded to length ``n``.
+        - ``n`` is less than the length of the input array along ``axis``, the input array along ``axis`` must be trimmed to length ``n``.


@leofang Sorry for my ignorance, but "trimmed" in this case means the last N-n elements are removed? Or equivalently, only use the first n elements when computing the transform?

Let's address this in a small follow-on PR.

kgryte · 2024-02-08T13:09:50Z

I think we are good to go ahead and merge this in. Any additional tweaks can be addressed in follow-on PRs.

Thanks, @asmeurer and @leofang!

leofang · 2024-02-08T13:21:31Z

@kgryte thanks, I'll check again later today. One thing I noticed: the diff contains a lot of non-FFT-related changes, is this expected?

kgryte · 2024-02-08T13:22:47Z

@leofang GitHub seems confused. The actual diff only includes FFT changes. See 937d3b8.

leofang · 2024-02-08T14:03:57Z

Interesting, I thought only Gitlab would be confused not GitHub 😀 We squash-merged, right?

kgryte · 2024-02-08T17:56:51Z

We squash-merged, right?

Yes, we did. 😀

leofang · 2024-02-08T21:16:10Z

Thanks for all the editing, Aaron/Athan. Looks nice! I sent a tiny follow-up: #743

PR-URL: #743 Reviewed-by: Athan Reines <kgryte@gmail.com> Ref: #720

Use better wording for the return dtypes for fft functions

c51ce80

Fixes data-apis#717

leofang reviewed Dec 12, 2023

View reviewed changes

src/array_api_stubs/_draft/fft.py Outdated Show resolved Hide resolved

leofang reviewed Dec 12, 2023

View reviewed changes

src/array_api_stubs/_draft/fft.py Outdated Show resolved Hide resolved

asmeurer added 2 commits December 11, 2023 18:02

Specify the output shapes for fft functions

1dd9757

Also cleanup some wording for consistency. This fixes the incorrect output shape for ihfft. Fixes data-apis#718.

Fix typo for rfft return dtype

bde524e

leofang requested changes Dec 12, 2023

View reviewed changes

src/array_api_stubs/_draft/fft.py Outdated Show resolved Hide resolved

kgryte and others added 4 commits December 13, 2023 21:01

Add specification for computing the cumulative sum

1fd3429

PR-URL: data-apis#653

Add note clarifying behavior for out-of-bounds indices in take

bda9c48

PR-URL: data-apis#701

Add specifications for maximum and minimum

50ca679

Closes: data-apis#667 PR-URL: data-apis#713

Fix definition of complex inner product in vecdot (data-apis#723)

5c2423a

PR-URL: data-apis#723 Reviewed-by: Athan Reines <kgryte@gmail.com>

asmeurer mentioned this pull request Jan 8, 2024

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

Merged

Update fft, ifft, fftn, ifftn, and hfft to only accept complex inputs

3295776

See the discussion at data-apis#720 This still requires changelog entries.

Add searchsorted to the specification (data-apis#699)

72f86f3

rgommers added Maintenance Bug fix, typo fix, or general maintenance. topic: FFT Fast Fourier transforms. labels Jan 11, 2024

Fix sidebar contents under Extensions (data-apis#729)

8fe8b08

The problem was that the sidebar doesn't show the named pages in the toctree if that toctree is under a subheading in index.rst Closes data-apisgh-716

leofang self-requested a review January 12, 2024 00:50

leofang added this to the v2023 milestone Jan 12, 2024

rgommers and others added 2 commits January 12, 2024 10:41

Update DLPack content to address unsupported use cases (data-apis#709)

7a56675

Also extend the description of errors when DLPack cannot be supported

Backport fft language improvements to 2022.12

7c3f99a

Fix typo for hfft return dtype

62f95fd

Merge branch 'fft-fixes' of https://github.com/asmeurer/array-api int…

17124ba

…o fft-fixes

kgryte reviewed Feb 8, 2024

View reviewed changes

kgryte added 17 commits February 8, 2024 02:16

Update copy for fft

6d9fac4

Update copy

66e9e29

Update copy for ifft

99cb776

style: satisfy linting

04a4e66

Update copy for fftn

9f208f6

Update copy for ifftn

9a6fcc7

Update copy

454ded7

Update copy for rfft

21e2840

Update copy for irfft

22d354a

Update copy for rfftn

4d0160c

Update copy for irfftn

9d90326

Update copy for hfft

8f2b6df

Update copy for ihfft

43e96fa

Update copy

c180034

Copy changes over to draft specification

9f1c20e

Update copy

b97bb34

Fix indentation

0a7df55

kgryte approved these changes Feb 8, 2024

View reviewed changes

kgryte merged commit 937d3b8 into data-apis:main Feb 8, 2024
3 checks passed

kgryte pushed a commit that referenced this pull request Feb 8, 2024

Fix typo in FFT guidance

425e9eb

PR-URL: #743 Reviewed-by: Athan Reines <kgryte@gmail.com> Ref: #720

Micky774 mentioned this pull request Apr 22, 2024

Begin deprecation of implicit input conversion in FFT module google/jax#20818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up some wording in the fft extension #720

Clean up some wording in the fft extension #720

asmeurer commented Dec 12, 2023 •

edited by kgryte

asmeurer commented Dec 12, 2023

asmeurer commented Dec 12, 2023

rgommers commented Jan 9, 2024

asmeurer commented Jan 10, 2024

asmeurer commented Jan 10, 2024

kgryte commented Jan 10, 2024

rgommers commented Jan 11, 2024

leofang commented Jan 12, 2024

asmeurer commented Jan 12, 2024

kgryte commented Feb 8, 2024

kgryte Feb 8, 2024 •

edited

kgryte Feb 8, 2024

kgryte commented Feb 8, 2024

leofang commented Feb 8, 2024

kgryte commented Feb 8, 2024

leofang commented Feb 8, 2024

kgryte commented Feb 8, 2024

leofang commented Feb 8, 2024

Clean up some wording in the fft extension #720

Clean up some wording in the fft extension #720

Conversation

asmeurer commented Dec 12, 2023 • edited by kgryte

asmeurer commented Dec 12, 2023

asmeurer commented Dec 12, 2023

rgommers commented Jan 9, 2024

asmeurer commented Jan 10, 2024

asmeurer commented Jan 10, 2024

kgryte commented Jan 10, 2024

rgommers commented Jan 11, 2024

leofang commented Jan 12, 2024

asmeurer commented Jan 12, 2024

kgryte commented Feb 8, 2024

kgryte Feb 8, 2024 • edited

Choose a reason for hiding this comment

kgryte Feb 8, 2024

Choose a reason for hiding this comment

kgryte commented Feb 8, 2024

leofang commented Feb 8, 2024

kgryte commented Feb 8, 2024

leofang commented Feb 8, 2024

kgryte commented Feb 8, 2024

leofang commented Feb 8, 2024

asmeurer commented Dec 12, 2023 •

edited by kgryte

kgryte Feb 8, 2024 •

edited