-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG/ENH: group cummin/max handle skipna #41854
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
97d7e11
ee7a7ef
885f8ae
51f8f9c
d7cf5b5
21e5157
7858fc6
3c869ce
e8a08bf
c448837
9333d58
1aa3586
856a1b2
dfe8b73
22bef11
0d57fc0
5ccdf94
fb76640
a3db4de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2784,10 +2784,11 @@ def cummin(self, axis=0, **kwargs): | |
------- | ||
Series or DataFrame | ||
""" | ||
skipna = kwargs.get("skipna", True) | ||
if axis != 0: | ||
return self.apply(lambda x: np.minimum.accumulate(x, axis)) | ||
|
||
return self._cython_transform("cummin", numeric_only=False) | ||
return self._cython_transform("cummin", numeric_only=False, skipna=skipna) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally As a followup change, it would be nice to remove this inconsistency (especially since Making this change could enable actually exposing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so, yes.
Yah, looks like we currently silently ignore args/kwargs, which isnt great. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok sounds good, will plan to leave as followon since should be an independent improvement from these changes |
||
|
||
@final | ||
@Substitution(name="groupby") | ||
|
@@ -2800,10 +2801,11 @@ def cummax(self, axis=0, **kwargs): | |
------- | ||
Series or DataFrame | ||
""" | ||
skipna = kwargs.get("skipna", True) | ||
if axis != 0: | ||
return self.apply(lambda x: np.maximum.accumulate(x, axis)) | ||
|
||
return self._cython_transform("cummax", numeric_only=False) | ||
return self._cython_transform("cummax", numeric_only=False, skipna=skipna) | ||
|
||
@final | ||
def _get_cythonized_result( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this go in the signature explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some context in #41854 (comment).
Happy to make that change here, was planning on doing in followup to make
skipna
,numeric_only
consistent forcumsum
,cummin/max
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i agree we want this in the signature, can be a followup is ok.