add optional default value to GroupBy.get_group
State: closed

I've used a try except block as opposed to and if else (below) to keep the non-default code path fast.  What I didn't do:

```
if default is None :
    inds = self.indices[name]
else :
    inds = self.indices.get(name, default)
```

Here is an example use case:

```
def foo(df, keys) :
    g = df.groupby('key')
    for key in keys :
        for x in g.get_group(key, default=[]).x :
            yield key, x
```

vs

```
def foo(df, keys) :
    g = df.groupby('key')
    for key in keys :
        if key in g.indices :
            for x in g.get_group(key).x :
                yield key, x
```

Many of the simple cases are actually handled by other higher level functions 


Labels:
name: Groupby
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: API Design

Comments:
- pls setup travis (see contributing.md)
- need tests (I don't think our implementation will work if default was say a string), you can't take with it

Can you add some examples of why this would be useful to your PR? Are you
thinking this would be like dict . get?

yeah, just added an example to the top, but yeah, it would act similar to .get in that it would allow you to avoid an if key in g.indices or an extra try/catch block

oops

needs to be rebased to master, see here: https://github.com/pydata/pandas/wiki/Using-Git

alright, squashed commits and rebased to master.

still, failing build tests though ... will figure out whats going on there

if you put in  my fix I'm guessing (more of) the builds will pass. inds needs to be an array of integer-like, not a string key.

exactly.  A string key should raise an exception and that was what I was testing for, I was just expecting the wrong exception type.  I'm not sure why the other test was failing since it worked on my machine a while back.  Anyway, I've changed that test as well, so now they all pass on my machine, will wait and see how it looks to Travis

btw - if only "slow" passes, that really means all your tests are failing
:) that build just skips the majority of the test suite in favor of running
tests that take a while.

gotcha, I was running python setup.py test, not sure what exactly that gets mapped to.  I guess ./test.sh would probably be better to run.  Anyway, it looks like Travis passed.  Does it do the full large suite of tests?

Yes it does. Easiest is to run this:

nosetests -A 'not network and not slow'

(that runs non slow tests which is usually want you want)

Can you add a docstring to this that describes what kind of default should be passed to get_group? 

Also need to add release notes at some point.

cool, thanks.  Is there anything else I should do to help get this PR accepted?

The part that concerns me is that if you call it with a bad default it only raises if you give a group that's not in the grouper. So you could have a silent bug if you were using this. Maybe test whether it's actually a perf hit if you validate every time?

(I might be reading it wrong - no access to a computer right now) 

added the docstring.

I could do a type check on all calls, the problem is that it appears that there is no explicit expected type.  In pandas.core.indexing._maybe_convert_indices, it converts isinstance(indices, list) to np.array, and leaves everything else alone.  Its possible that if a user has their own np.array like object which implements all of the necessary operations take will work fine.  Is there a way to test that default has a type which provides the desired interface?

Maybe common.is_list_like?

So,

if default is not None and not com.is_list_like(default):
   raise TypeError

awesome :) I can try that.  How do you recommend I test the performance impact of that?

I could write my own tests, but I know there are already some performance tests in pandas, maybe I can use existing tests?

Look at test_perf.sh. I'm assuming there's a groupby test (just run the whole thing and see what falls out) . I wouldn't expect it to be that much of an issue though. 

alright, I'll check that out.  Yeah I agree that it will in most cases be far outweighed by the time required to obj.take

Alright, I just added the test for is_list_like and it seems to have not impacted performance.

@jtratner ok?

@dwiel actually..can you add release notes? does this have a reference issue?

there isn't a reference issue that I know of.  Should I create one?  Just added release notes

no need to add it if its not their now (generally we have one as its first reported as such), in the release notes reference THIS PR then (so when you click on it you get here)

alright, added a link, though I didn't see any way to do it in an analogous way to :issue:`###`, so I just put a link in.  Could also add a magic to allow a :pullrequest:`###` syntax, but that should probably be a different pull request.  Or maybe there is already a way to do this and I just didn't see it.

just add it like an issue ; the number is the pr number

alright, just used (:issue:`5452`)