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

BUG: Fix rfft for even input length. #26354

Merged
merged 3 commits into from Apr 27, 2024
Merged

Conversation

WarrenWeckesser
Copy link
Member

Closes gh-26349

@WarrenWeckesser WarrenWeckesser added component: numpy.fft C++ Related to introduction and use of C++ in the NumPy code base labels Apr 26, 2024
@ngoldbaum
Copy link
Member

mac failure is an unrelated heisenbug

@ngoldbaum ngoldbaum requested a review from mhvk April 26, 2024 22:59
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Apr 27, 2024
@charris charris added this to the 2.0.0 release milestone Apr 27, 2024
def test_rfft_even(self):
x = np.arange(8)
y = np.fft.rfft(x, 4)
assert_allclose(y, [6.0, -2.0+2.0j, -2.0], rtol=1e-14)
Copy link
Member

Choose a reason for hiding this comment

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

Could use the same sort of test as for the odd case, I like comparing to the full complex fft case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@charris charris merged commit 4340733 into numpy:main Apr 27, 2024
65 checks passed
@charris
Copy link
Member

charris commented Apr 27, 2024

Thanks Warren.

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Apr 27, 2024
@@ -183,12 +183,15 @@ rfft_impl(char **args, npy_intp const *dimensions, npy_intp const *steps,
* Pocketfft uses FFTpack order, R0,R1,I1,...Rn-1,In-1,Rn[,In] (last
* for npts odd only). To make unpacking easy, we place the real data
* offset by one in the buffer, so that we just have to move R0 and
* create I0=0. Note that copy_data will zero the In component for
* create I0=0. Note that copy_input will zero the In component for
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird - so, the statement I made in the comment is not true? I'll try to have a quick look, but in any case the fix clearly works.

@mhvk
Copy link
Contributor

mhvk commented Apr 27, 2024

#26359 does a follow-up that ensures the code actually does what the comment states. Would be nice to have both this one and #26359 be in 2.0.0....

Must admit that overall rewriting the fft routines in terms of ufuncs has been more of an experience in how hard it is to write bug-free code than I would have liked... Sorry all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug C++ Related to introduction and use of C++ in the NumPy code base component: numpy.fft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: behavioral change of numpy.fft.rfft with numpy 2.x
4 participants