-
Notifications
You must be signed in to change notification settings - Fork 908
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
MAINT: Adapt to NumPy 2 promotion changes #16141
MAINT: Adapt to NumPy 2 promotion changes #16141
Conversation
try: | ||
is_safe = source_dtype.type(other) == other | ||
except OverflowError: | ||
is_safe = False |
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.
This just adds the OverflowError
check, because NumPy is now strict, otherwise the error type would change here.
# Go via NumPy to get the value | ||
other = np.array(other) | ||
if other.dtype.kind in "ifc": | ||
other = other.item() |
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.
The biggest change, on NumPy 2, we need to pass Python scalars to align with Pandas (not NumPy, which pandas does not align with here).
On NumPy 1, this doesn't really make a difference.
To get those Python scalars, .item()
is one reasonable way.
@@ -341,7 +341,6 @@ def test_dtype(in_dtype, expect): | |||
np.complex128, | |||
complex, | |||
"S", | |||
"a", |
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.
Just avoids an error "a"
has been removed, and seems not particularly important here.
/okay to test |
1 similar comment
/okay to test |
def prinoptions(cls): | ||
# TODO: NumPy now prints scalars as `np.int8(1)`, etc. this should | ||
# be adapted evantually. | ||
if np.lib.NumpyVersion(np.__version__) >= "2.0.0rc1": |
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.
Another suggestion to use packaging.version.parse
if you recommend against np.lib
usage
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.
Since it is used in many tests, adopted now (and rebased, but all except last commits are unchanged).
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.
An optional fix https://github.com/rapidsai/cudf/pull/16141/files#r1671422229 but overall looks good
Note that pandas `where` seems to promote the Series based on the value even with NumPy 2. This was never copied by cudf (i.e. an outstanding issue)
Pandas keeps using weak promotion even for strongly typed "scalars" (i.e. 0-d objects). This tries to (mostly) match that, but there may be better ways to do it. I am having difficulty to think of the best way though.
66efccf
to
927904c
Compare
927904c
to
6af0dec
Compare
/okay to test |
Friendly ping: I think this should be pretty safe to push through. There is the follow-up needed to ensure integer comparisons don't misbehave (the test suite currently doesn't catch this problem). |
/merge |
Splitting out the non API changes from gh-15897, the Scalar API change is required for the tests to pass with NumPy 2, but almost all changes should be relatively straight forward here on their own.
(I will add inline comments.)
This PR does not fix integer comparisons, there are currently no tests that run into these.
xref: rapidsai/build-planning#38