-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG+DEPR: undeprecate item, fix dt64/td64 output type #30175
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
Changes from all commits
0d2867d
ef1c9ed
f87f03a
c88bc14
a98fda2
39c13df
aabcf92
6c18f38
f6cb1b7
de4baff
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 |
---|---|---|
|
@@ -5,7 +5,6 @@ | |
from collections import OrderedDict | ||
import textwrap | ||
from typing import Dict, FrozenSet, List, Optional | ||
import warnings | ||
|
||
import numpy as np | ||
|
||
|
@@ -26,6 +25,7 @@ | |
is_object_dtype, | ||
is_scalar, | ||
is_timedelta64_ns_dtype, | ||
needs_i8_conversion, | ||
) | ||
from pandas.core.dtypes.generic import ABCDataFrame, ABCIndexClass, ABCSeries | ||
from pandas.core.dtypes.missing import isna | ||
|
@@ -659,19 +659,27 @@ def item(self): | |
""" | ||
Return the first element of the underlying data as a python scalar. | ||
|
||
.. deprecated:: 0.25.0 | ||
|
||
Returns | ||
------- | ||
scalar | ||
The first element of %(klass)s. | ||
|
||
Raises | ||
------ | ||
ValueError | ||
If the data is not length-1. | ||
""" | ||
warnings.warn( | ||
"`item` has been deprecated and will be removed in a future version", | ||
FutureWarning, | ||
stacklevel=2, | ||
) | ||
return self.values.item() | ||
if not ( | ||
is_extension_array_dtype(self.dtype) or needs_i8_conversion(self.dtype) | ||
): | ||
# numpy returns ints instead of datetime64/timedelta64 objects, | ||
# which we need to wrap in Timestamp/Timedelta/Period regardless. | ||
return self.values.item() | ||
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. This won't work for ExtensionArrays. We can discuss adding 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 would be +1 on adding to EA arrays, why have inconsistency in code paths. 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.
This uses 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 have no objection to adding item to EAs separately 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.
And to fix that, the only thing that is needed is adding a 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. im happy to do that here, will need tests in a follow-up |
||
|
||
if len(self) == 1: | ||
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. Was adding this condition discussed somewhere? I would have thought just keep existing behaviour 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. This is the bugfix part. dt64, dt64tz, and td64 we're currently incorrectly returning int 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. Hmm doesn't this break non-DTA though? >>> type(pd.Series(range(1)).item())
<class 'int'>
>>> type(pd.Series(range(1))[0])
<class 'numpy.int64'> I thought one of the points of item was to return a Python object (at least in the Numpy world) 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. ^ current behavior 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. Yes, I think we should keep the behaviour of 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. ok, will update. 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. Has this been resolved? 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 the concerns raised by @WillAyd and @jorisvandenbossche have been addressed |
||
return next(iter(self)) | ||
else: | ||
raise ValueError("can only convert an array of size 1 to a Python scalar") | ||
|
||
@property | ||
def nbytes(self): | ||
|
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.
isn't this redundant? as all needs_i8_conversion are already EA
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.
no, dt64 and td64 are need i8 conversion but are not EA