REF: use _validate_fill_value in Index.insert #38102
Conversation
…f-numeric-validate_fill_value
…f-numeric-validate_fill_value
|
I assume that you are going to move some of this logic to the array/_mixins (or that is the goal) |
For all our existing ExtensionIndex subclasses the logic is already there. I haven't 100% decided where I expect this logic to end up, am still tracking down all the places where we do something similar and figuring out if we can align all the behavior. |
| elif is_valid_nat_for_dtype(fill_value, dtype): | ||
| # e.g. pd.NA, which is not accepted by Timestamp constructor | ||
| fill_value = np.datetime64("NaT", "ns") |
There was a problem hiding this comment.
But pd.NA should not be an allowed fill_value for datetime dtypes?
There was a problem hiding this comment.
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.
I don't think we support pd.NA atm in datetimelikes (agree eventually we should but we should do this deliberately). However doesn't is_valid_nat_for_dtype distinguish this explicitly?
There was a problem hiding this comment.
i thought (half of) the whole idea of pd.NA was that it would be a valid NA value for every type.
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.
However doesn't is_valid_nat_for_dtype distinguish this explicitly?
is_valid_nat_for_dtype considers pd.NA valid for all dtypes, bc that was my understanding of the intent of pd.NA.
| else: | ||
| # NaT, np.datetime64("NaT"), np.timedelta64("NaT") | ||
| raise TypeError | ||
|
|
||
| elif is_scalar(value): |
There was a problem hiding this comment.
might make sense to override this in Float vs Int index to avoid some of this complication
There was a problem hiding this comment.
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.
|
PR itself looks ok to me. @jorisvandenbossche @TomAugspurger on the pd.NA comments above (e.g. checking for NA actually will work for datetimelikes). |
|
any more thoughts here? trying to unify all our casting code |
| else: | ||
| # NaT, np.datetime64("NaT"), np.timedelta64("NaT") | ||
| raise TypeError | ||
|
|
||
| elif is_scalar(value): |
There was a problem hiding this comment.
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.
very much so |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff