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

Fix documented dtype inputs for rfft-like functions #696

Closed
wants to merge 2 commits into from

Conversation

honno
Copy link
Member

@honno honno commented Oct 9, 2023

For the real fft family of functions:

  • Replace instances of "complex-valued" in documented input with "real-valued"
  • Use "must" language instead of "should"—this is to follow how rfft is right now, although I could see maybe that it should all be "should"?

- Replace complex with real
- Use must language instead of should
@kgryte
Copy link
Contributor

kgryte commented Oct 9, 2023

These changes should likely be backported, correct?

Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

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

This is incorrect. Rfft is for r2c transforms, and irfft for c2r.

@honno
Copy link
Member Author

honno commented Oct 9, 2023

These changes should likely be backported, correct?

I should've remembered to do that 😅 Backported to 2022.12.

This is incorrect. Rfft is for r2c transforms, and irfft for c2r.

Oh okay, did only skim over #189 so I might of missed relevant discussion? I was looking at np.rfft, which documents that the input is real-valued, e.g. np.rfft's docstring starts with "Compute the one-dimensional discrete Fourier Transform for real input." But I 'spose that means it can take complex input and just ignores the imaginary components?

@leofang
Copy link
Contributor

leofang commented Oct 10, 2023

These changes should likely be backported, correct?

Sorry for brevity @honno, I left the comment on cell in a rush...

This is incorrect. Rfft is for r2c transforms, and irfft for c2r.

Oh okay, did only skim over #189 so I might of missed relevant discussion? I was looking at np.rfft, which documents that the input is real-valued, e.g. np.rfft's docstring starts with "Compute the one-dimensional discrete Fourier Transform for real input."

No, that's a misnomer. rfft means to do Fourier transform on a real input, and irfft means to do the inverse operation of it such that irfft(rfft(arr)) == arr (subject to caveats noted everywhere, array API or NumPy). Note that after irfft you need to get back a real input.

Now, repeat after me 🙂

  • rfft refers to the real-to-complex (r2c) transform
  • irfft refers to the complex-to-real (c2r) transform

This is the only way that irfft(rfft(arr)) == arr could make sense. As long as you memorize this, I can assure you success for writing docs, sample codes and tests. This would also help when you cross-reference low-level FFT library docs (ex: FFTW or cuFFT).

@leofang
Copy link
Contributor

leofang commented Oct 10, 2023

But I 'spose that means it can take complex input and just ignores the imaginary components?

Missed this one. No, this is a behavior that we purposely forbid. Ignoring the imaginary part (either silently or raising a warning) was a poor behavior in NumPy, and I pushed during the discussions to ensure array API does not pick it up.

@kgryte
Copy link
Contributor

kgryte commented Oct 18, 2023

@honno Have you had the chance to follow up here?

@honno
Copy link
Member Author

honno commented Oct 19, 2023

Here's a summary of input-output behaviour

Function Array API NumPy docs NumPy actual
rfft Must have real input Real input Also accepts complex input, always returns complex output
rfftn Must have real input Real input Also accepts complex input, always returns complex output
irfft Should have complex input Real input Also accepts complex input, always returns real output
irfftn Should have complex input Real input Also accepts complex input, always returns real output

I think I see what @leofang means now, so disregard this PR! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: FFT Fast Fourier transforms.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants