-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
BUG: Fix pocketfft umath strides for powerpc compatibility #29768
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks. ptrdiff_t vs. npy_intp usage differs a bit between functions now, but it doesn't matter.
EDIT: To be clear, the test failures are unrelated, there are some general problems that will be fixed very soon hopefully (maybe it already is).
Unfortunately, tests are failing. It would be nice if you can move the npts_in just to keep things slightly simpler.
numpy/fft/_pocketfft_umath.cpp
Outdated
| #ifndef POCKETFFT_NO_VECTORS | ||
| size_t npts_in = nout / 2 + 1; | ||
|
|
||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nicer to just move it into the existing #ifndef below, but a small nit, but thanks for fixing the warning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, while you are at it, please do move it inside (it should have been there in the first place, as it is not used outside).
|
@seberg Do we need to change something here at the moment re test failures? |
|
It would be nice to move that one thing, but otherwise should be all good. We can re-trigger CI manually (or close/reopen PR) when things are resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this -- as @seberg noted in the issue, the reason I used ptrdiff_t here was that that was what pocketfft expected. I vaguely recall getting compiler warnings without it, but if this works now without explicit casts, then all fine by me.
numpy/fft/_pocketfft_umath.cpp
Outdated
| #ifndef POCKETFFT_NO_VECTORS | ||
| size_t npts_in = nout / 2 + 1; | ||
|
|
||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, while you are at it, please do move it inside (it should have been there in the first place, as it is not used outside).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending CI success
|
@seberg Thank you for helping here, and sorry for a delay, I did not have much time this weekend. |
* _pocketfft_umath.cpp: fix types Fixes: numpy#29758 Credits to @seberg * _pocketfft_umath.cpp: fix unused variable warning * Move npts_in calculation into #ifndef --------- Co-authored-by: Sebastian Berg <sebastianb@nvidia.com>
BUG: Fix pocketfft umath strides for AIX compatibility (#29768)
See: #29758