Allow aggregate funcs to return arrays in groupby
State: closed

fixes #3788

Please check out whether you like the error message for `Performance Warning`.

Also, I'm not sure whether this means that groupby fails under certain conditions and not others (like when trying Cython, etc.).


Labels:

Comments:
Thanks for putting that together. But, do you think we could:

```
warnings.warn("Function does not produce aggregated values. Will not be able to optimize.", PerformanceWarning)
```

rather than raise?

Hmmm, about the message, we could cover ourselves a bit more and say "Function does not produce aggregated values. Will not be able to optimize and may produce unexpected results."

What's the difference between `raise` and `warnings.warn`?

~~Oh, I thought that~~ warnings.warn would give you a warning message but still let you do it. Raise kills it, raises an Exception, throws toys out of pram and gives no result. (I could be wrong... but I was hoping for the former :) )

@hayd maybe? I don't know. How do you test that the warning occurred?  Also, how about we do even more couched:

`Function does not produce aggregated values. Will not be able to optimize and may produce unexpected results. This is experimental and results are not guaranteed to stay the same between versions.`

I think like [this](http://docs.python.org/2/library/warnings.html#testing-warnings) (or do you have it working?).

There are some PerformanceWarnings in test_pytables, but @jreback seems to ignore (?) them with:

```
warnings.filterwarnings('ignore', category=PerformanceWarning)
```

Also, I think we may as well include tests for the current results of those calculations. :)

I did add some tests like that with `warnings.catch_warning`. I'll try and
come back to this tomorrow.

(haha! I think the experimental bit... is a little OTT!)

The usual assertEqual(result, expected) tests :)

Given there are multiple interpretations of what it means to produce an
aggregate that's a sequence, I didn't want us to be stuck supporting this
edge case in the future. Your call.

On Fri, Jun 7, 2013 at 9:25 PM, Andy Hayden notifications@github.comwrote:

> (haha! I think the experimental bit... is a little OTT!)
> 
> The usual assertEqual(result, expected) tests :)
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3805#issuecomment-19140771
> .

@hayd I ignore them so they won't print on the test display; if you don't silence they will...

Or if you use `warnings.catch_warnings`, you can change the warnings just
for the duration of the test.

Ah of course! ~~Do you test the warnings appear?~~

Personally I think that saying "may produce unexpected results" is enough to say we're not really supporting it, and having those assertEquals tests would just highlight when there's a problem and we can then choose to break. :)

@jtratner that is the right way......I was ignoring them before I knew how to do that!

Yeah, I just need to fix the tests slightly.

Actually, I'd say that in general we should probably set the warning filter to `error` so that all errors are explicitly caught (right now Travis has two `DeprecationWarning` errors).

The issue is that some of these warnings we are explicity testing something in order to generate the warning (like here, the Performance issue).....so you would have to have a try except block around them which would mask an actual problem....

Well, if you're testing something to generate the warning, you can do a few things:
1. Set to `'always'` and then assert that the length of the `catch_warnings` object is greater than 1
2. Use `'error'` and assert that it causes the warning.

I guess 1 is actually better in general. 

Also, @hayd I went with your warning message :)

@hayd @jreback does this look okay to you? Do you want me to make any other changes or updates?

The only thing I would mention is that we might as well add tests for the actual results of those two calls.

oh sorry! totally forgot. I'll add that.

On Sat, Jun 8, 2013 at 2:25 PM, Andy Hayden notifications@github.comwrote:

> The only thing I would mention is that we might as well add tests for the
> actual results of those two calls.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3805#issuecomment-19153076
> .

Hmm an additional test to add is the example from the [original issue](https://github.com/pydata/pandas/issues/3788), as I think that is a non ambiguous thing (and after all, it works fine with lists...).

Just going through them, none of the test returned `ValueError('Function does not reduce')`, can we think of a good/bad example of this, either that's bad or good? (Did you work out why that one was there?)

```
grouped.agg(lambda x: x.describe())
Exception: Must produce aggregated value
Intermediary output:
count    3.000000
mean     0.971312
std      1.130488
min     -0.330182
25%      0.602385
50%      1.534953
75%      1.622060
max      1.709167
dtype: float64
Now:
A
bar    [3.0, -0.704089998761, 0.619650989953, -1.0771...
foo    [5.0, 0.696226374171, 0.679933321298, 0.288482...
Name: C, dtype: object
```

```
grouped.agg(lambda x: x.index[:2])
Exception: Must produce aggregated value
Intermediary output:
Int64Index([1, 3], dtype=int64)
Now:
A
bar    [1, 3]
foo    [0, 2]
Name: C, dtype: object
```

```
grouped.aggregate, lambda x: x * 2
Exception: Must produce aggregated value
Intermediary output:
1    3.069905
3   -0.660364
5    3.418333
Name: bar, dtype: float64
Now:
A
bar    [-2.15438895675, -2.09254823116, 0.0223971953452]
foo    [3.78051447736, 0.576965254291, 0.687811502056...
Name: C, dtype: object
```

In the first example they "obviously"  _really_ want the result (and this would also work for the second in this instance):

```
pd.DataFrame([s for s in res], index=res.index)
```

I really have no idea what's going on in the third one... unexpected is correct lol.

_All cases there is a better way I think._

I see now what you are saying about ambiguity. So maybe tests for these weird example aren't so important... :S

Yeah, and it makes the test cases really annoying, because they have to deal with truth values in numpy arrays...ugh.

I don't want to unintentionally force us to support weird edge case behavior with groupby. On the upside, this got me to write a nice little test utility to neatly check that something raises a warning.

Okay, so this now has test cases & such. I'd vote to remove https://github.com/jtratner/pandas/commit/d1ce7227ec78640f7702e2a3c34ab87b22792b57 (which specifically tests the results), but I wanted to include it if you want it. 

I also added a new test utility to check for warnings (https://github.com/jtratner/pandas/commit/86d56acbe6085a49bb02ac549432ec1f293671d5 ). Given that it is a wrapper around a context manager and that it explicitly changes `warnings.simplefilter`, I felt it made more sense to use as a context manager, especially because it means that you can add additional tests around the warning if you want.

Doctests on the test utility all pass as well. (not sure if that' s included with the Travis tests).

Yeah, you're right about removing https://github.com/jtratner/pandas/commit/d1ce7227ec78640f7702e2a3c34ab87b22792b57 (sorry!).

_assert_produces_warning looks whizzy!_

Hahaha, I just finished fixing the test case from the SO question. Removing that forever and then pushing.

Okay, this should all pass + I added notes to `RELEASE.rst` for this.

Never mind. I'm wrong, needed to reset the warning filter each time in `assert_produces_warnings`.

Did you ever find any way of getting the previous `ValueError('Function does not reduce')` ?

@hayd Nope. Not at all - very weird.