-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH/BUG: add count to grouper / ensure that grouper keys are not included in the returned #7000
Conversation
I reverted the ohlc stuff, as we don't have a test for this, what was the issue again? note that the fixes here are only for the non-cython methods (incl mean/sum that work on mixed type), and head/tail/nth are taken care of by cumcount (not 100% sure) |
the #6594 causing issues....maybe do later |
the grouping axis | ||
""" | ||
self._set_selection_from_grouper() | ||
return self._python_agg_general(lambda x: notnull(x).sum(axis=axis)).astype('int64') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a much simpler way to solve the upcasting!
This looks like some nice cleaning. Good thing to move the ohlc thing, basically it was just to remove it / make it raise. a sep issue. |
This is from 0.13.1 (but currently works the same in master) I think this should return a MI with first level of [0,1](and 2nd level as given) for
like this?
|
CLN: remove __inv__, __neg__ from series and use generic version CLN: remove __wrap_array__ from generic (replace with __array_wrap__)
I had to 'fix' the last one (because otherwise this is from
|
sorry...error in the above (the apply should not be returing the this is a rabbit hole! |
I was about to say "gaaargh", was staring at that example... no idea what should be correct behaviour! ...I'm sticking with "gaaargh" |
@jreback head is not an aggregation, it always ignores "gaaargh" is to the describe behaviour. |
So as a corallary of having apply NOT returning the grouped column (if its named) the following is also true (in 0.13.1, this returned the A column as well)
|
So this is another 'edgeish' case, and fixing is related to the above.
|
All that said (in my last 2 comments).... I don't think going to do this last still some cases on returing the group by column e.g. if I have to complicated |
ok...going to merge.... @hayd you are on! |
ENH/BUG: add count to grouper / ensure that grouper keys are not included in the returned
the filter thing was a bug in 0.13.1 which we fixed IIRC the rest looks ok? |
is |
yes it should return it, as filter is not an aggregation (like we updated head to be). IIRC we tested that it respected "sub-group" (??) |
ok...it passes all that well, check out v0.14.0 once this builds everything makes sense, however sometimes an |
i realize i'm a little late to the game, but is there any reason |
hah! I think that is some old code, not being used anywhere so do you want to do: delete |
sure. just ran into a perf issue trying to count 700k groups :( |
hah! only tricky thing is this needs a different temple (use the take template) as it can accept ALL dtypes and possibly some logic to get around the so prob need a test to go thru a bunch of dtypes and count stuff |
Looking at this discussion with a lot of Aaarghs (and also seeing some of the other issues with groupby), I was thinking: should we try to write down some 'design document' where we try to describe the 'rules'. Eg the "we regard This could maybe clarify some things for ourselves, and can be used as a reference for future PRs. (Or could it also turn out as a rabbit hole ..) -> moved to #5755 |
closes #5610