ENH: Raise error in certain unhandled _reduce cases. by staple · Pull Request #8592 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation28 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Raise error in certain unimplemented _reduce cases. BUG: Raise error in certain unimplemented _reduce cases.
| @@ -2067,9 +2067,16 @@ def _reduce(self, op, axis=0, skipna=True, numeric_only=None, |
|---|
| """ |
| delegate = self.values |
| if isinstance(delegate, np.ndarray): |
| if axis != 0 and axis != self._AXIS_IALIASES[0]: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just do: axis = self._get_axis_number(axis) (which errors if the axis is not valid / exceeds dims)
Oh, I’d thought there was possibly interest in supporting axis 1 (single element reduction along a perpendicular axis) because the preexisting group by tests I modified had been attempting to sum a series on axis 1.
I changed to make this a value error per your comment.
pls rebase and add a doc-note in v0.15.1 enhancements (reffing this pr as it doesn't have a separate issue)
BUG: Raise error in certain unimplemented _reduce cases. ENH: Raise error in certain unimplemented _reduce cases.
ENH: Raise error in certain unimplemented _reduce cases. ENH: Raise error in certain unhandled _reduce cases.
Ok, rebased and added to whatsnew.
@staple sorry, you will have to rebase again, I moved the whatsnew file to a subfolder
@staple pls squash as well, otherwise looks ok
Ok, rebased and squashed.
minor comment: is this error message only supposed to be raised internally? Or will users also see this in some cases?
If that is the case (users will see it), I don't think it is clear as users don't know what _reduce is. If it is not the case, the enhancement entry seems not very interesting for users (not to say that the PR would not be interesting of course!)
@jorisvandenbossche Users can see this error message. What would make more sense for a user message? Would something like 'Series aggregation does not implement numeric_only' make sense?
Would it be possible to detect in which kind of aggregation function this is called?
It looks like 'name' (the name of the aggregation function) is generally passed to _reduce. But it's an optional argument. I could try to make it a non optional argument, and use the value there in the error message. Would that make sense?
I just wanted to suggest the same!
Ok, I made the requested changes. Left them in separate commits for now, for ease of review. But if you like I can squash them when you're ready. Let me know if the change to make 'name' a required argument to _reduce should be a separate commit.
| @@ -30,6 +30,9 @@ Enhancements |
|---|
| - Qualify memory usage in ``DataFrame.info()`` by adding ``+`` if it is a lower bound (:issue:`8578`) |
| - Raise errors in certain _reduce cases where the arguments are not handled. |
| (:issue:`8592`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- can you also make this a bit more clear? (no mention of
_reduce) give eg the concrete case ofnumeric_onlyas an example - the second line should be aligned with
Raise ...
Ok changed the whats new text.
Yep, better!
@jreback OK with you that name is now a required positional arg of _reduce?
Another remark: shouldn't this rather be a UserWarning instead of an error? (I don't know how similar things are handled in other places in pandas)
Also, if we raise an error and not a warning, we should maybe keep this for 0.16 instead of 0.15.1?
I think this is ok (only thing, maybe need a couple of specific cases that actually trigger this error, rather than a constructed case). can't think of any off-hand though. pls rebase / squash.
Ok, I rebased and squashed.
| @@ -4129,7 +4129,7 @@ def any(self, axis=None, bool_only=None, skipna=True, level=None, |
|---|
| if level is not None: |
| return self._agg_by_level('any', axis=axis, level=level, |
| skipna=skipna) |
| return self._reduce(nanops.nanany, axis=axis, skipna=skipna, |
| return self._reduce(nanops.nanany, 'any', axis=axis, skipna=skipna, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where these the only 2 that didn't pass 'name' ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, I was meaning any definitions (e.g. make_stat_function etc). no answer looks like no, ok, good.
jreback added a commit that referenced this pull request
ENH: Raise error in certain unhandled _reduce cases.