ENH: Add cumulative methods to ea #48111
Conversation
updates pandas
Updates fork
updates from upstream
merges upstream
| data = self._data.copy() | ||
|
|
||
| if name in {"cummin", "cummax"}: | ||
| func = np.minimum.accumulate if name == "cummin" else np.maximum.accumulate |
There was a problem hiding this comment.
do the numpy functions not work directly?
There was a problem hiding this comment.
This has a different behavior than going through nanops. The initial pr included this but I ripped it out to keep it a bit more focused. Plan to tackle this afterwards, but we have to decide what we actually want here first. Hence only doing masked ops here
There was a problem hiding this comment.
This has a different behavior than going through nanops
can you expand on this?
There was a problem hiding this comment.
skipna is the problem, difference in behavior with regards to floats, remembered this incorrect.
See #28509 (comment)
Wanted to investigate this as a follow up when we can check this more isolated
…e-methods-to-EA # Conflicts: # pandas/core/arrays/datetimelike.py
|
Should have addressed everything |
|
|
||
| if name in {"cummin", "cummax"}: | ||
| func = np.minimum.accumulate if name == "cummin" else np.maximum.accumulate | ||
| result = cast(np.ndarray, nanops.na_accum_func(data, func, skipna=skipna)) |
There was a problem hiding this comment.
this looks like it might choke on PeriodDtype?
There was a problem hiding this comment.
Seems to work, could you elaborate what you suspect?
ser = Series([pd.Period('2012-1-1', freq='D'), pd.Period('2013-1-1', freq='D')])
ser.cummin()
There was a problem hiding this comment.
i think this goes wrong when there's a NaT present
There was a problem hiding this comment.
This would restore the previous behavior (but the previous behavior was wrong as well...). Are you ok with addressing this in a follow up?
There was a problem hiding this comment.
This would restore the previous behavior
previous as of when? IIRC this was last changed multiple years ago.
but the previous behavior was wrong as well
so does this PR get the behavior right for dt64/td64? If so, the solution for PeriodDtype is similar to median/min/max to do a view to dt64, do the op, then view back.
There was a problem hiding this comment.
Sorry, my explanation was a bit confusing.
The initial pr which this work is based on tried the following:
- add accumulate to the ea interface
- implement masked-based accumulators
- implement new accumulation logic for date time and related arrays
This got confusing, since it was pretty big and the new date time logic changed the behavior. So I made the decision to reduce scope and only tackle the first 2 points and defer the third to a follow up.
To achieve this, I just send the date time logic through nanops again (this is what I meant with previous behavior, previous as in before _accumulate was added to the ea interface). This avoid any change in behavior for the date time was and should keep review more focused.
There was a problem hiding this comment.
Let's try this one more time, thanks for your patience in explaining this to me.
IIUC this PR should not change any behavior for dt64/dt64tz/td64 dtypes, correct?
There was a problem hiding this comment.
Yes, correct. Want to do those changes as follow ups
There was a problem hiding this comment.
Cool, ill trust you to handle these.
There was a problem hiding this comment.
Thx, yep want to get this into 2.0 as well
| self.assert_series_equal(result, expected, check_dtype=False) | ||
|
|
||
|
|
||
| class BaseNoAccumulateTests(BaseAccumulateTests): |
There was a problem hiding this comment.
i find this pattern really weird (xref #44742), is there a way to do this with just one class?
There was a problem hiding this comment.
Combined the classes. Have to overwrite the tests that should not get executed now.
|
Thanks @phofl |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.This tries to finish #28509. I've reduced the scope to masked arrays and would prefer to do datetimelike as a follow up, if we want to change the behavior there. This is currently a big buggy as mentioned in #28509 (comment)
Currently, we are dispatching back to the current implementation.
I was wondering about the exact interface. Currently, you'd have to call
_accumulatewith the name of the function, sincecumsumetc. are not really added to the interface, meaningraises, because the method is not registered. I this intended?