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

staple · 2014-10-13T17:39:48Z

Move any/all implementations from DataFrame to NDFrame, adding support for Series
Special case single dimension case in NDFrame, to handle numpy.any/all specific arguments
Add assorted NotImplementedErrors in cases where arguments are ignored (including in preexisting _reduce functions)
Move the other any/all implementations from IndexOpsMixin to Index, outside of Series’ inheritance tree

jreback · 2014-10-20T01:03:10Z

pandas/core/generic.py

+level : int or level name, default None
+        If the axis is a MultiIndex (hierarchical), count along a
+        particular level, collapsing into a """ + name + """
+bool_only : boolean, default None


why is this necessary?

These logical functions have slightly different documentation: a 'bool_only' field instead of a 'numeric_only' field, and potentially a message about supporting additional numpy arguments via the bool_extended_args variable.

I don't think this is necessary (nor is any other 'bool_extended_args' variable), its just not needed. That said If you have a use case pls show it.

I'd like to better understand your comment. I mentioned two special cases in the documentation here, the bool_only argument name and the documentation message about supporting additional numpy arguments, which are currently supported in master's Series.any/all, which forwards to numpy's any/all.

bool_only is the preexisting argument name for DataFrame's any and all, in master. Are you suggesting I should change it to numeric_only?

You had suggested earlier that I should continue to support the ndarray.any/all arguments that are supported by Series.any/all in master (these are the 'out' and 'keepdims' parameters). Are you saying that I should not support these, or that I should not document that they are supported?

maybe I wasn't clear. The signature should be something like: any(laxis, skipna, level, **kwargs); we accept but don't deal explicity with the numpy args (e.g. out) and such. Its not useful, nor is it consistent with how pandas works.

I realized I was confusing about bool_only, yes that is a good idea (just not bool_extended_args

Thanks, got it.

shouldn't bool_only=True be the default?

I made bool_only default to None (False) here because:

The default is None in DataFrame’s current implementation of any/all in master

The corresponding numeric_only argument to sum, mean, etc defaults to None

bool_only is not currently implemented in Series._reduce or Panel._reduce. Once we add not implemented errors, they would be thrown in the case of all default arguments, were bool_only to be True by default.

ok, though I find its a little odd (e.g. I can't think of a reason to use any/all on non-boolean data), but I am sure people will try it!.

I guess, also, if they're calling any/all on Series in master, they are using the numpy version which does not offer a bool_only option.

Would it make sense for me to create a new issue for implementing bool_only (numeric_only) in Series an Panel, and look into whether it should be enabled by default for any/all?

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

Hi, here is the separate PR for the _reduce changes: #8592

staple · 2014-10-21T03:41:23Z

Ok, I think I've addressed the pending items.

Some tests are currently disabled pending merge of the _reduce changes I separated into the other PR.

jreback · 2014-10-27T00:03:24Z

pls add a release note v0.15.1 (put in API section for now, though that's getting reorganized). This is not really an API change, more of an internal change.

pls rebase/squash and I'll have another look.

staple · 2014-10-27T00:08:25Z

Sure, there are some minor dependencies on the _reduce PR we've also been discussing (some tests are disabled). Since it sounds like we're closed to finishing up that PR, I'll plan to rebase/squash this one once the other is in master.

jreback · 2014-10-27T00:12:42Z

@staple ideally I'd like to see a SINGLE any/all that goes in NDFrame AND Index (so we have argument consistency). This is tricky of course. because you have multi-dim objects. I think this CAN be done though.

staple · 2014-10-29T00:47:26Z

Ok, updated. Go ahead and take a look.

jreback · 2014-10-30T01:36:15Z

pandas/core/index.py

+
+        Parameters
+        ----------
+        axis : None or int or tuple of ints, optional


Index are only 1-dim by definition, so can remove this parm.

staple · 2014-11-02T22:01:38Z

Ok, I made the requested changes.

jreback · 2014-11-02T22:45:14Z

doc/source/whatsnew/v0.15.1.txt

+
+  .. ipython:: python
+
+     p = pd.Panel(np.random.rand(2, 5, 4) > 0.1)


why is this example here? (the 2nd one)

Panel now supports any and all. Panel didn't implement these functions before, so I added an example of how to call them. Do you want me to remove the example?

ok, say that then. (I didn't know that!)

It's already in the description:
Panel now supports the all and any aggregation functions.

ahh ok, (I think that line is too long. Better to move that part to right before the example. Follows better on the eye.

Sure, will do.

jreback · 2014-11-02T22:54:26Z

pandas/tests/test_series.py

+        assert_series_equal(s.any(level=0), Series([False, True, True]))
+
+        # bool_only is not implemented with level option.
+        self.assertRaises(NotImplementedError, s.any, bool_only=True, level=0)


hmm, I don't buy either one of these. Too special case. They should just work (if its a bool dtype then it will work, otherwise it should fail)

Could you be more specific about which test cases your are referring to? Are you asking me to remove the tests for the Series behavior where there nan values?

no, the level and bool_only raises NotImplementedError. was this from somewhere else? I think that should work, no?

Oh, do you mean the bool_only cases? Do you mean I should remove the bool_only argument?

Well, bool_only uses _reduce's numeric_only which was never implemented for Series or Panel. (Recall recent PR where I added exceptions in those cases instead of ignoring the argument.)

I think _agg_by_level is one issue (currently numeric_only isn't passed). But also Series._reduce just doesn't implement numeric_only in what I think is the standard case:

if numeric_only: raise NotImplementedError( 'Series.{0} does not implement numeric_only.'.format(name))

Is your suggestion that we should add support for these? That could be a separate PR (which would probably be merged w/ master before this one).

So, for example (note use of sum not any):

>>> pd.Series([1]).sum(numeric_only=True) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "pandas/core/generic.py", line 3938, in stat_func skipna=skipna, numeric_only=numeric_only) File "pandas/core/series.py", line 2074, in _reduce 'Series.{0} does not implement numeric_only.'.format(name)) NotImplementedError: Series.sum does not implement numeric_only.

Are you suggesting to fix this?

Or, are you saying make sure bool_only is passed through but don't implement numeric_only in Series._reduce (so that in theory the combination of bool_only and level is no longer a special case)?

hmm, ok, let's leave as its, but create a new issue to discuss / fix this. In that issue show the examples where it is not-implemented ATM (so the above, and the 2 in your tests).
We'll see what we can do.

though I think you should pass thru the bool_only arg now (and thus eliminate 1 case).

I like to eliminate all special cases if they don't actually make sense (we can look at this in detail later in the new issue).

jreback · 2014-11-02T23:09:54Z

@staple FYI this PR is really good. I am just being nit-picky! on some details.

staple · 2014-11-02T23:14:09Z

@jreback, sure no worries!

jreback · 2014-11-05T01:45:49Z

can you fixup and rebase as discussed. e.g. remove args,*kwargs to Index funcs (and remove keep_dims tests). change test_logical_compat to test the functions are actually disabled on the various Indexes. thxs

staple · 2014-11-07T05:59:42Z

Ok, see new commit.

jreback · 2014-11-09T21:31:32Z

doc/source/whatsnew/v0.15.1.txt

@@ -211,6 +211,19 @@ API changes
  in plotted integer values. To keep the previous behaviour, you can do
  ``del matplotlib.units.registry[np.datetime64]`` (:issue:`8614`).

+- ``Series.all`` and ``Series.any`` now support the ``level`` and ``skipna`` parameters. ``Series.all``, ``Series.any``, ``Index.all``, and ``Index.any`` no longer support the ``out`` and ``keepdims`` parameters, which existed for compatibility with ndarray. Various index types no longer support the ``all`` and ``any`` aggregation functions. (:issue:`8302`):


pls move to v0.15.2.txt

…. GH8302

staple · 2014-11-10T16:57:10Z

Ok

jreback · 2014-11-11T14:33:25Z

@staple thanks! This was a bigger rabbit hole, but you got it done!

staple mentioned this pull request Oct 13, 2014

API: add level kwarg for Series.any/.all #8302

Closed

staple force-pushed the GH8302 branch 2 times, most recently from 5bfd86c to bf46795 Compare October 15, 2014 18:42

jreback added API Design Enhancement Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Oct 20, 2014

jreback added this to the 0.15.1 milestone Oct 20, 2014

jreback removed the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Oct 20, 2014

jreback reviewed Oct 20, 2014
View reviewed changes

staple force-pushed the GH8302 branch from 101bd20 to 219b007 Compare October 29, 2014 00:46

jreback reviewed Oct 30, 2014
View reviewed changes

staple force-pushed the GH8302 branch from 219b007 to 7965a3a Compare November 2, 2014 22:01

jreback reviewed Nov 2, 2014
View reviewed changes

jreback modified the milestones: 0.15.1, 0.15.2 Nov 2, 2014

jreback modified the milestones: 0.15.1, 0.15.2 Nov 6, 2014

staple force-pushed the GH8302 branch 2 times, most recently from 65011e4 to f37e2d8 Compare November 7, 2014 05:52

jreback reviewed Nov 9, 2014
View reviewed changes

staple force-pushed the GH8302 branch 3 times, most recently from 992a882 to 518a62c Compare November 10, 2014 16:46

ENH: Move any/all to NDFrame, support additional arguments for Series…

518a62c

…. GH8302

jreback merged commit 518a62c into pandas-dev:master Nov 11, 2014

qwhelan mentioned this pull request Feb 1, 2019

PERF: O(n) speedup in any/all by re-enabling short-circuiting for bool case #25070

Merged

4 tasks


		.. ipython:: python

		p = pd.Panel(np.random.rand(2, 5, 4) > 0.1)

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

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

Conversation

staple commented Oct 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staple commented Oct 21, 2014

staple commented Oct 21, 2014

jreback commented Oct 27, 2014

staple commented Oct 27, 2014

jreback commented Oct 27, 2014

staple commented Oct 29, 2014

Choose a reason for hiding this comment

staple commented Nov 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 2, 2014

staple commented Nov 2, 2014

jreback commented Nov 5, 2014

staple commented Nov 7, 2014

Choose a reason for hiding this comment

staple commented Nov 10, 2014

jreback commented Nov 11, 2014