-
Notifications
You must be signed in to change notification settings - Fork 89
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: correct dtypes for numpy v2 #3159
Conversation
62792aa
to
c4c1be0
Compare
eee80e3
to
fa313c4
Compare
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.
@jpivarski - this PR fixes the issue with Numpy v2: typically, np.intp
is used to represent the platform integer type and the check for fallback on it is valid only for Numpy
version 2 and higher.
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.
Great! Thanks for finding and fixing this. However, it's not fully testing the platform-version combinations users might have. The
awkward/.github/workflows/test.yml
Lines 52 to 54 in 02858f8
- python-version: '3.9' | |
python-architecture: x86 | |
runs-on: windows-latest |
test used to check 32-bit Windows with NumPy < 2, now it tests 32-bit Windows with NumPy ≥ 2. However, nothing tests 32-bit Windows with NumPy < 2 anymore, and we need to be sure that the _use32
evaluates to the right boolean value for all of those cases. This test should be split into two tests; one for NumPy < 2, one for NumPy ≥ 2, and both for 32-bit Windows.
While adding that 1 new test, these 3 old tests can be removed:
awkward/.github/workflows/test.yml
Lines 64 to 75 in 02858f8
- python-version: '3.11' | |
python-architecture: x64 | |
runs-on: ubuntu-latest | |
dependencies-kind: numpy2 | |
- python-version: '3.11' | |
python-architecture: x64 | |
runs-on: macos-11 | |
dependencies-kind: numpy2 | |
- python-version: '3.11' | |
python-architecture: x64 | |
runs-on: windows-latest | |
dependencies-kind: numpy2 |
which implies that we can also delete this file:
https://github.com/scikit-hep/awkward/blob/main/requirements-test-numpy2.txt
When those tests are changed, I'll have to update the set of required tests, but just request a review again. (These are all adjusting for the fact that NumPy has been officially released.)
@jpivarski - please, update required tests. Thanks! |
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.
At first, I thought that "Tests / Run Tests (pypy3.9, x64, ubuntu-latest, pypy)" was another new test, since it's not required yet, either. But now I remember that it's there so we can keep an eye on it, not so that we can promise that Awkward works on PyPy. (We don't provide a wheel for that.)
So, this gets us entirely up-to-date with NumPy 2.0 in its fully released state. This is ready to merge into main
, and it can fix #3136 when that is subsequently synced with main
.
fixes #3160