Skip to content

Code Clean Up - groupby_helper.pxi.in #19724

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

Closed
WillAyd opened this issue Feb 16, 2018 · 1 comment · Fixed by #19734
Closed

Code Clean Up - groupby_helper.pxi.in #19724

WillAyd opened this issue Feb 16, 2018 · 1 comment · Fixed by #19734

Comments

@WillAyd
Copy link
Member

WillAyd commented Feb 16, 2018

A few of the methods in groupby_helper.pxi.in have a pattern that goes:

    N, K = (<object> values).shape

    with nogil:

        if K > 1:
            # loop over j in range(K) and do stuff
            ...
        else:
            # do stuff with j=0
            ...

Unless I'm missing something simple I don't think there's a reason to have the conditional and duplicate code branches. In the case where K=1 (which is all the time?) then the for j in range(K) construct inherently sets j=0, bypassing the need for the conditional

@chris-b1
Copy link
Contributor

I believe the K > 1 would get used sometime, from what I remember some of the aggregations are done by-block, which means a 2d array of multiple columns could get passed down.

def _cython_agg_general(self, how, alt=None, numeric_only=True,

It's probably a micro-micro optimization, but having it split like this may generate slightly better code for K=1 case. But welcome you to benchmark and if it doesn't matter, clean it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants