-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
REF: use _validate_fill_value in Index.insert #38102
Changes from 8 commits
214820d
631cf65
eb8bc34
6d03c75
47fea62
d05a12c
fd5cb3f
c28fb2b
19086ea
6e2c0aa
c4aeb97
7382321
de857e5
f121087
d222c32
97ada24
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 |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
is_float, | ||
is_float_dtype, | ||
is_integer_dtype, | ||
is_number, | ||
is_numeric_dtype, | ||
is_scalar, | ||
is_signed_integer_dtype, | ||
|
@@ -120,15 +121,30 @@ def _validate_fill_value(self, value): | |
# force conversion to object | ||
# so we don't lose the bools | ||
raise TypeError | ||
elif isinstance(value, str) or lib.is_complex(value): | ||
raise TypeError | ||
elif is_scalar(value) and isna(value): | ||
if is_valid_nat_for_dtype(value, self.dtype): | ||
value = self._na_value | ||
if self.dtype.kind != "f": | ||
jreback marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# raise so that caller can cast | ||
raise TypeError | ||
else: | ||
# NaT, np.datetime64("NaT"), np.timedelta64("NaT") | ||
raise TypeError | ||
|
||
elif is_scalar(value): | ||
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. might make sense to override this in Float vs Int index to avoid some of this complication 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. can you do this as a followon. adding all of this logic here (in the index iteself) makes for duplication in other places more likely. this is almost like maybe_promote. |
||
if not is_number(value): | ||
# e.g. datetime64, timedelta64, datetime, ... | ||
raise TypeError | ||
|
||
elif lib.is_complex(value): | ||
# at least until we have a ComplexIndx | ||
raise TypeError | ||
|
||
elif is_float(value) and self.dtype.kind != "f": | ||
if not value.is_integer(): | ||
raise TypeError | ||
value = int(value) | ||
|
||
return value | ||
|
||
def _convert_tolerance(self, tolerance, target): | ||
|
@@ -164,15 +180,6 @@ def _is_all_dates(self) -> bool: | |
""" | ||
return False | ||
|
||
@doc(Index.insert) | ||
def insert(self, loc: int, item): | ||
try: | ||
item = self._validate_fill_value(item) | ||
except TypeError: | ||
return self.astype(object).insert(loc, item) | ||
|
||
return super().insert(loc, item) | ||
|
||
def _union(self, other, sort): | ||
# Right now, we treat union(int, float) a bit special. | ||
# See https://github.com/pandas-dev/pandas/issues/26778 for discussion | ||
|
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.
But pd.NA should not be an allowed fill_value for datetime dtypes?
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 thought (half of) the whole idea of pd.NA was that it would be a valid NA value for every dtype.
(the other half being kleene logic)
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 don't think we support
pd.NA
atm in datetimelikes (agree eventually we should but we should do this deliberately). However doesn'tis_valid_nat_for_dtype
distinguish this explicitly?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.
In the future yes, for sure. But at the moment, we don't use pd.NA for datetimelikes, and also don't properly support it in operations with datetimelikes.
So therefore I am wondering if we should allow it here.
(now, it seems that it already does work on master as well, though)
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.
is_valid_nat_for_dtype considers pd.NA valid for all dtypes, bc that was my understanding of the intent of pd.NA.