ENH: Raise error in certain unhandled _reduce cases. #8592

staple · 2014-10-21T03:14:00Z

No description provided.

jreback · 2014-10-21T10:59:16Z

pandas/core/series.py

@@ -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]:


you can just do: axis = self._get_axis_number(axis) (which errors if the axis is not valid / exceeds dims)

staple · 2014-10-21T19:26:19Z

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.

jreback · 2014-10-21T23:16:05Z

pls rebase and add a doc-note in v0.15.1 enhancements (reffing this pr as it doesn't have a separate issue)

staple · 2014-10-22T21:51:29Z

Ok, rebased and added to whatsnew.

jorisvandenbossche · 2014-10-23T06:59:45Z

@staple sorry, you will have to rebase again, I moved the whatsnew file to a subfolder

jreback · 2014-10-23T11:53:43Z

@staple pls squash as well, otherwise looks ok

staple · 2014-10-23T18:58:31Z

Ok, rebased and squashed.

jorisvandenbossche · 2014-10-23T19:04:20Z

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!)

staple · 2014-10-23T19:16:52Z

@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?

jorisvandenbossche · 2014-10-23T19:18:35Z

Would it be possible to detect in which kind of aggregation function this is called?

staple · 2014-10-23T19:23:55Z

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?

jorisvandenbossche · 2014-10-23T19:24:28Z

I just wanted to suggest the same!

staple · 2014-10-23T19:24:59Z

Ok, cool.

jorisvandenbossche · 2014-10-23T19:26:27Z

It is indeed an optional argument, bug eg in https://github.com/staple/pandas/blob/reduce/pandas/core/generic.py#L3924 where it is used in _make_stat_function, name is not an optional argument, so it would always be present (but you should look if it is used in other places)

staple · 2014-10-23T19:49:04Z

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.

jorisvandenbossche · 2014-10-23T19:54:27Z

doc/source/whatsnew/v0.15.1.txt

@@ -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`)


can you also make this a bit more clear? (no mention of _reduce) give eg the concrete case of numeric_only as an example

the second line should be aligned with Raise ...

staple · 2014-10-23T19:59:38Z

Ok changed the whats new text.

jorisvandenbossche · 2014-10-23T20:02:18Z

Yep, better!
@jreback OK with you that name is now a required positional arg of _reduce?

jreback · 2014-10-23T20:19:54Z

let me have a look

jorisvandenbossche · 2014-10-24T09:48:44Z

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?

jreback · 2014-10-24T23:53:51Z

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.

staple · 2014-10-26T23:44:17Z

Ok, I rebased and squashed.

jreback · 2014-10-26T23:51:00Z

pandas/core/frame.py

@@ -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,


where these the only 2 that didn't pass 'name' ?

There's also this line where name wasn't being passed:
https://github.com/pydata/pandas/pull/8592/files#diff-03b380f521c43cf003207b0711bac67fR4007

I think the plan is to remove the above two (any/all) in the separate PR for any/all implementation.

right, I was meaning any definitions (e.g. make_stat_function etc). no answer looks like no, ok, good.

jreback · 2014-10-26T23:51:47Z

I think this is fine. @jorisvandenbossche ?

ENH: Raise error in certain unhandled _reduce cases.

jreback · 2014-10-28T00:03:50Z

thanks!

staple mentioned this pull request Oct 21, 2014

ENH: Move any/all to NDFrame, support additional arguments for Series. GH8302 #8550

Merged

staple changed the title ~~Raise error in certain unimplemented _reduce cases.~~ BUG: Raise error in certain unimplemented _reduce cases. Oct 21, 2014

jreback reviewed Oct 21, 2014
View reviewed changes

jreback added the Error Reporting Incorrect or improved errors from pandas label Oct 21, 2014

jreback added this to the 0.15.1 milestone Oct 21, 2014

staple changed the title ~~BUG: Raise error in certain unimplemented _reduce cases.~~ ENH: Raise error in certain unimplemented _reduce cases. Oct 22, 2014

staple changed the title ~~ENH: Raise error in certain unimplemented _reduce cases.~~ ENH: Raise error in certain unhandled _reduce cases. Oct 22, 2014

staple force-pushed the reduce branch from c164132 to 410ff6f Compare October 22, 2014 21:50

staple force-pushed the reduce branch from 410ff6f to fb96fe8 Compare October 23, 2014 18:57

jorisvandenbossche reviewed Oct 23, 2014
View reviewed changes

Raise error in certain unimplemented aggregation cases.

1d99270

staple force-pushed the reduce branch from 3619a38 to 1d99270 Compare October 26, 2014 23:43

jreback reviewed Oct 26, 2014
View reviewed changes

jreback added a commit that referenced this pull request Oct 28, 2014

Merge pull request #8592 from staple/reduce

211c80c

ENH: Raise error in certain unhandled _reduce cases.

jreback merged commit 211c80c into pandas-dev:master Oct 28, 2014

jreback modified the milestones: 0.15.2, 0.15.1 Oct 30, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Raise error in certain unhandled _reduce cases. #8592

ENH: Raise error in certain unhandled _reduce cases. #8592

staple commented Oct 21, 2014

jreback Oct 21, 2014

staple commented Oct 21, 2014

jreback commented Oct 21, 2014

staple commented Oct 22, 2014

jorisvandenbossche commented Oct 23, 2014

jreback commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

jreback commented Oct 23, 2014

jorisvandenbossche commented Oct 24, 2014

jreback commented Oct 24, 2014

staple commented Oct 26, 2014

jreback Oct 26, 2014

staple Oct 27, 2014

jreback Oct 27, 2014

jreback commented Oct 26, 2014

jreback commented Oct 28, 2014

ENH: Raise error in certain unhandled _reduce cases. #8592

ENH: Raise error in certain unhandled _reduce cases. #8592

Conversation

staple commented Oct 21, 2014

jreback Oct 21, 2014

Choose a reason for hiding this comment

staple commented Oct 21, 2014

jreback commented Oct 21, 2014

staple commented Oct 22, 2014

jorisvandenbossche commented Oct 23, 2014

jreback commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

staple commented Oct 23, 2014

jorisvandenbossche Oct 23, 2014

Choose a reason for hiding this comment

staple commented Oct 23, 2014

jorisvandenbossche commented Oct 23, 2014

jreback commented Oct 23, 2014

jorisvandenbossche commented Oct 24, 2014

jreback commented Oct 24, 2014

staple commented Oct 26, 2014

jreback Oct 26, 2014

Choose a reason for hiding this comment

staple Oct 27, 2014

Choose a reason for hiding this comment

jreback Oct 27, 2014

Choose a reason for hiding this comment

jreback commented Oct 26, 2014

jreback commented Oct 28, 2014