CLN: avoid values_from_object in Series#32426
Conversation
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
looks good, some comments
| ): | ||
|
|
||
| dtype_is_dt = self.dtypes.apply(lambda x: x.kind == "M") | ||
| dtype_is_dt = self.dtypes.apply(lambda x: x.kind == "M" or is_period_dtype(x)) |
There was a problem hiding this comment.
can you apply needs_i8_conversion instead here? (like you do below), this is a non-standard usage
There was a problem hiding this comment.
trouble is we don't want to include td64
There was a problem hiding this comment.
ok how about using is_datetime64_any_dtype then; this is a non-obvious pattern
…ries-values_from_object
| ): | ||
|
|
||
| dtype_is_dt = self.dtypes.apply(lambda x: x.kind == "M") | ||
| dtype_is_dt = self.dtypes.apply(lambda x: x.kind == "M" or is_period_dtype(x)) |
There was a problem hiding this comment.
ok how about using is_datetime64_any_dtype then; this is a non-obvious pattern
| # finding `mask` above | ||
| values = getattr(values, "asi8", values) | ||
| values = values.view(np.int64) | ||
| if isinstance(values, np.ndarray): |
There was a problem hiding this comment.
why is this case still here? when is this actually an ndarray? (or conversely, when is this a DTI/TDI). it is non-obvious how we get to this point.
…ries-values_from_object
…ries-values_from_object
| expected = pd.Series({"A": 1.0, "C": df.loc[1, "C"]}) | ||
| tm.assert_series_equal(result, expected) | ||
|
|
||
| @pytest.mark.xfail( |
There was a problem hiding this comment.
I guess you should technically have a whatsnew note as this 'bug' is fixed (do in followon)
|
thanks |
| """ | ||
| skipna = nv.validate_argmax_with_skipna(skipna, args, kwargs) | ||
| i = nanops.nanargmax(com.values_from_object(self), skipna=skipna) | ||
| i = nanops.nanargmax(self._values, skipna=skipna) |
There was a problem hiding this comment.
nanargmax/nanargmin expect to get an ndarray. Due to this change, it is no longer guaranteed to be an ndarray. Reported this as #32749
So those lines should either be reverted, or another "convert to ndarray" function should be used (or nanargmax/nanargmin could be rewritten to support EAs, but personally I think it is much cleaner to keep those algos based on numpy arrays)
There was a problem hiding this comment.
(or nanargmax/nanargmin could be rewritten to support EAs, but personally I think it is much cleaner to keep those algos based on numpy arrays)
I'd be fine with either of these options. Probably prefer both actually: a EA-supporting public method and an ndarray-only private method for each of the relevant nanops funcs.
|
@jbrockmendel another problem with this PR is that you enabled "mean" for Period dtype (but only for DataFrames), while we had long discussions before (when initially adding mean support for datetimelikes) that ended in not supporting mean for period (-> #24757) |
|
Opened a PR for that in the mean time: #33758 |
3 participants
xref #32422, #32419