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: correct dtypes for numpy v2 #3159

Merged
merged 8 commits into from
Jun 21, 2024
Merged

fix: correct dtypes for numpy v2 #3159

merged 8 commits into from
Jun 21, 2024

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Jun 19, 2024

fixes #3160

@ianna ianna force-pushed the ianna/import_numpy_for_tests branch from eee80e3 to fa313c4 Compare June 20, 2024 18:56
@ianna ianna changed the title fix: import numpy for tests fix: correct dtypes for numpy v2 Jun 20, 2024
Copy link
Collaborator Author

@ianna ianna left a 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.

Copy link
Member

@jpivarski jpivarski left a 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

- 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:

- 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.)

@ianna ianna requested a review from jpivarski June 21, 2024 01:54
@ianna
Copy link
Collaborator Author

ianna commented Jun 21, 2024

@jpivarski - please, update required tests. Thanks!

Copy link
Member

@jpivarski jpivarski left a 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.

@ianna ianna merged commit 6f688e9 into main Jun 21, 2024
39 checks passed
@ianna ianna deleted the ianna/import_numpy_for_tests branch June 21, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

np.sum of an array of boolean dtype in tests/test_0115_generic_reducer_operation.py
2 participants