-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
DEPS: Test NEP 50 #55739
DEPS: Test NEP 50 #55739
Changes from 31 commits
d93ffd0
85fca37
4ec19f5
0dd702c
df26df7
b8e3eaa
1ce0252
a039851
45d3d51
7569ea0
1f94724
6fb901c
707543c
017d49d
560f42d
d5daa39
38eace5
9df7f0f
5153123
97ebfd6
ee5b6b0
db470bc
ac104df
9a0dec0
a12cad9
2eac0e3
1cd5acd
42c8fff
b3ac004
f79bc66
d7ac452
0d80c1c
399bdf0
b50c6de
79f4dde
9ce828b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ | |
is_supported_dtype, | ||
) | ||
from pandas._libs.tslibs.timedeltas import array_to_timedelta64 | ||
from pandas.compat.numpy import np_version_gt2 | ||
from pandas.errors import ( | ||
IntCastingNaNError, | ||
LossySetitemError, | ||
|
@@ -1314,6 +1315,37 @@ def find_result_type(left_dtype: DtypeObj, right: Any) -> DtypeObj: | |
# which will make us upcast too far. | ||
if lib.is_float(right) and right.is_integer() and left_dtype.kind != "f": | ||
right = int(right) | ||
# After NEP 50, numpy won't inspect Python scalars | ||
# TODO: do we need to recreate numpy's inspection logic for floats too | ||
# (this breaks some tests) | ||
if isinstance(right, int) and not isinstance(right, np.integer): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm the most worried about here. A lot of things seem to hit around here, but the test coverage isn't great. We need the modifications here, since result_type won't inspect Python scalars anymore, so we have to figure out the "smallest" dtype for right ourselves. 2 questions here are:
|
||
# This gives an unsigned type by default | ||
# (if our number is positive) | ||
|
||
# If our left dtype is signed, we might not want this since | ||
# this might give us 1 dtype too big, though. | ||
# We should check if the corresponding int dtype (e.g. int64 for uint64) | ||
# can hold the number by a little hack where we ask numpy for the min | ||
# type of the negative of the number (which can't return an unsigned) | ||
# If the dtype is the same size, then we will use that | ||
right_dtype = np.min_scalar_type(right) | ||
if right == 0: | ||
# Special case 0, our trick will not work for it since | ||
# np.min_scalar_type(-0) will still give something unsigned | ||
right = left_dtype | ||
elif right > 0 and not np.issubdtype(left_dtype, np.unsignedinteger): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would be simpler if you just did: elif right > 0 and right <= 2 ** (8 * left_dtype.itemsize - 1) - 1:
# use signed dtype would cut down on some of the branching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, do you know a way to programatically convert from the unsigned dtype to the signed one? I think that was why I went with my hack in the first place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could create the new type by doing something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Updated the code. |
||
maybe_right_dtype = np.min_scalar_type(-right) | ||
WillAyd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ( | ||
maybe_right_dtype != np.dtype("O") | ||
and right_dtype.itemsize == maybe_right_dtype.itemsize | ||
): | ||
# It can fit in the corresponding int version | ||
right = maybe_right_dtype | ||
else: | ||
right = right_dtype | ||
else: | ||
right = right_dtype | ||
|
||
new_dtype = np.result_type(left_dtype, right) | ||
|
||
elif is_valid_na_for_dtype(right, left_dtype): | ||
|
@@ -1619,11 +1651,13 @@ def maybe_cast_to_integer_array(arr: list | np.ndarray, dtype: np.dtype) -> np.n | |
with warnings.catch_warnings(): | ||
# We already disallow dtype=uint w/ negative numbers | ||
# (test_constructor_coercion_signed_to_unsigned) so safe to ignore. | ||
warnings.filterwarnings( | ||
"ignore", | ||
"NumPy will stop allowing conversion of out-of-bound Python int", | ||
DeprecationWarning, | ||
) | ||
if not np_version_gt2: | ||
warnings.filterwarnings( | ||
"ignore", | ||
"NumPy will stop allowing conversion of " | ||
"out-of-bound Python int", | ||
DeprecationWarning, | ||
) | ||
casted = np.array(arr, dtype=dtype, copy=False) | ||
else: | ||
with warnings.catch_warnings(): | ||
|
@@ -1660,6 +1694,7 @@ def maybe_cast_to_integer_array(arr: list | np.ndarray, dtype: np.dtype) -> np.n | |
raise ValueError(f"string values cannot be losslessly cast to {dtype}") | ||
|
||
if dtype.kind == "u" and (arr < 0).any(): | ||
# TODO: can this be hit anymore after numpy 2.0? | ||
raise OverflowError("Trying to coerce negative values to unsigned integers") | ||
|
||
if arr.dtype.kind == "f": | ||
|
@@ -1672,6 +1707,7 @@ def maybe_cast_to_integer_array(arr: list | np.ndarray, dtype: np.dtype) -> np.n | |
raise ValueError("Trying to coerce float values to integers") | ||
|
||
if casted.dtype < arr.dtype: | ||
# TODO: Can this path be hit anymore with numpy > 2 | ||
# GH#41734 e.g. [1, 200, 923442] and dtype="int8" -> overflows | ||
raise ValueError( | ||
f"Values are too large to be losslessly converted to {dtype}. " | ||
|
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.
I think we need to still check for a DeprecationWarning here