Skip to content

Implement first/last reductions#886

Merged
mrocklin merged 14 commits into
blaze:masterfrom
cpcloud:first-last-reduction
Dec 26, 2014
Merged

Implement first/last reductions#886
mrocklin merged 14 commits into
blaze:masterfrom
cpcloud:first-last-reduction

Conversation

@cpcloud

@cpcloud cpcloud commented Dec 19, 2014

Copy link
Copy Markdown
Member

No description provided.

@jreback

jreback commented Dec 19, 2014

Copy link
Copy Markdown
Contributor

prob should also do tail for these back ends as well (I think that is an issue already)

@cpcloud

cpcloud commented Dec 19, 2014

Copy link
Copy Markdown
Member Author

that would go in a separate pr and would probably just be what @mrocklin said:

def tail(expr, n=10):
    return expr[-n:]

@jreback first blaze PR ? :)

@cpcloud

cpcloud commented Dec 19, 2014

Copy link
Copy Markdown
Member Author

another approach to first/last would be to can this PR and make arbitrary expressions work with groupbys

@cpcloud

cpcloud commented Dec 19, 2014

Copy link
Copy Markdown
Member Author

then we wouldn't really need a first/last expression, we could use expr[0] and expr[-1]

@mrocklin

Copy link
Copy Markdown
Member

What is the benefit to this over Slice?

@cpcloud

cpcloud commented Dec 20, 2014

Copy link
Copy Markdown
Member Author

Slice doesn't work in reductions

@cpcloud

cpcloud commented Dec 20, 2014

Copy link
Copy Markdown
Member Author

Sorry in summarys

@cpcloud

cpcloud commented Dec 20, 2014

Copy link
Copy Markdown
Member Author

I'm going to play with this today and see if I can get slices to work in groupbys. If it's simple then I'll close this PR. I DO think that first and last are more readable names for these computations, but that's subjective and I don't feel too strongly about it.

@mrocklin

Copy link
Copy Markdown
Member

They can be names / user-functions without being expressions. Probably
various checks need to be changed from isinstance(expr, Reduction) to
not iscollection(expr.dshape).

On Sat, Dec 20, 2014 at 8:07 AM, Phillip Cloud notifications@github.com
wrote:

I'm going to play with this today and see if I can get slices to work in
groupbys. If it's simple then I'll close this PR. I DO think that first and
last are more readable names for these computations, but that's subjective
and I don't feel too strongly about it.


Reply to this email directly or view it on GitHub
#886 (comment).

@cpcloud

cpcloud commented Dec 21, 2014

Copy link
Copy Markdown
Member Author

@mrocklin whenever you get a chance.

@mrocklin can i ask that you rebase off of master after this PR instead of merging? i think if you merge master into your branch that many of the dead imports that i removed here will come back.

@cpcloud

cpcloud commented Dec 21, 2014

Copy link
Copy Markdown
Member Author

The only thing I haven't done here is allow first and last to work with reductions in pandas because we can only do agg at the moment and first and last are Slices. Even though pandas does have a first/last reduction on GroupBys we don't actually know that slice[0] is called first and slice[-1] is called last.

@cpcloud cpcloud added this to the Release 0.8 milestone Dec 21, 2014
@cpcloud

cpcloud commented Dec 21, 2014

Copy link
Copy Markdown
Member Author

we should merge #832 (negative slicing with toolz.tail) before this PR

Comment thread blaze/expr/reductions.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make sure that this is the same as DataShape(10, 10, bool_) rather than DataShape(10, 10, DataShape(bool_))

@jreback

jreback commented Dec 21, 2014

Copy link
Copy Markdown
Contributor

q -> English like bah!

@cpcloud

cpcloud commented Dec 21, 2014

Copy link
Copy Markdown
Member Author

happy to remove first and last here, i simply wanted expr[0] and expr[-1] to work in the context of a groupby, the change from _dtype to schema fixes that

@mrocklin

Copy link
Copy Markdown
Member

Ah, so we don't need first and last methods?

@cpcloud

cpcloud commented Dec 22, 2014

Copy link
Copy Markdown
Member Author

no those aren't strictly necessary

@cpcloud

cpcloud commented Dec 26, 2014

Copy link
Copy Markdown
Member Author

@mrocklin can this be merged?

@mrocklin

Copy link
Copy Markdown
Member

Looks great. Merging.

mrocklin added a commit that referenced this pull request Dec 26, 2014
@mrocklin mrocklin merged commit 2661fa3 into blaze:master Dec 26, 2014
@cpcloud cpcloud deleted the first-last-reduction branch December 26, 2014 18:06
@cpcloud cpcloud modified the milestones: Release 0.8, Release 0.7.3 Mar 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants