Use ea interface to calculate accumulator functions for datetimelike #50297
Conversation
| """ | ||
| try: | ||
| fill_value = { | ||
| np.cumprod: 1, |
There was a problem hiding this comment.
Hm I guess we can remove it
| return result | ||
|
|
||
|
|
||
| def cumsum(values: np.ndarray, *, skipna: bool = True): |
There was a problem hiding this comment.
Yep good point, this one works. The others aren't that easy...
pandas/core/arrays/datetimelike.py
Outdated
|
|
||
| raise TypeError(f"Accumulation {name} not supported for {type(self)}") | ||
| return type(self)._simple_new( | ||
| result, freq=self.freq, dtype=self.dtype # type: ignore[call-arg] |
There was a problem hiding this comment.
i dont think retaining self.freq is going to be correct in general
There was a problem hiding this comment.
Can you elaborate when we should set freq to None?
There was a problem hiding this comment.
Pretty much anything that isn't a no-op should be not-freq-preserving. e.g. pd.date_range("2016-01-01", periods=3)._data.cummin()
There was a problem hiding this comment.
Thx, makes sense. Should we retain the freq in cases where we only have one element or not retain in general?
Not easy to hit btw :) But added a test now
There was a problem hiding this comment.
id just not-retain in general
There was a problem hiding this comment.
Sounds good, that's how I implemented it right now
| ("cummax", pd.Period("2012-1-2", freq="D")), | ||
| ], | ||
| ) | ||
| def test_cummin_cummax_period(self, func, exp): |
|
|
||
| Parameters | ||
| ---------- | ||
| func : np.cumsum, np.cumprod, np.maximum.accumulate, np.minimum.accumulate |
There was a problem hiding this comment.
| func : np.cumsum, np.cumprod, np.maximum.accumulate, np.minimum.accumulate | |
| func : np.cumsum, np.maximum.accumulate, np.minimum.accumulate |
| ) | ||
|
|
||
| elif skipna and not issubclass(values.dtype.type, (np.integer, np.bool_)): | ||
| if skipna and not issubclass(values.dtype.type, (np.integer, np.bool_)): |
There was a problem hiding this comment.
maybe a comment/check that "mM" cases should not get here?
There was a problem hiding this comment.
(OK for follow-up, i can add this into my next "assorted" branch)
| @@ -0,0 +1,46 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
can you call this test_cumulative.py to match tests/series/ and tests/frame/
|
Thanks @phofl |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.cc @jbrockmendel Follow up from the other pr
As mentioned on the other pr, this changes the behavior (highlighted by the test).