CLN: avoid values_from_object in Series #32426
Conversation
pandas/core/frame.py
Outdated
| @@ -7803,11 +7804,11 @@ def _reduce( | |||
| self, op, name, axis=0, skipna=True, numeric_only=None, filter_type=None, **kwds | |||
| ): | |||
|
|
|||
| 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
…ries-values_from_object
pandas/core/frame.py
Outdated
| @@ -7803,11 +7804,11 @@ def _reduce( | |||
| self, op, name, axis=0, skipna=True, numeric_only=None, filter_type=None, **kwds | |||
| ): | |||
|
|
|||
| 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
pandas/core/nanops.py
Outdated
| # changing timedelta64/datetime64 to int64 needs to happen after | ||
| # 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
| @@ -875,11 +875,6 @@ def test_mean_datetimelike(self): | |||
| 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 |
| @@ -2055,7 +2055,7 @@ def idxmax(self, axis=0, skipna=True, *args, **kwargs): | |||
| nan | |||
| """ | |||
| 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.
@jbrockmendel can you respond to this?
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 |
xref #32422, #32419