Skip to content

PEP: pandas/core round 3 (generic, groupby, index) #12062

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
wants to merge 1 commit into from

Conversation

rockg
Copy link
Contributor

@rockg rockg commented Jan 16, 2016

yapf with manual modifications for error strings and bad yapf formatting.

@jreback jreback added the Code Style Code style, linting, code_checks label Jan 16, 2016
@jreback jreback added this to the 0.18.0 milestone Jan 16, 2016
@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

No github diffs again!

There are some pieces of code that I think can be removed but want to confirm:
index.py i/j are not used anywhere.
groupby.py how can this work? agg_func is not defined.

@jreback
Copy link
Contributor

jreback commented Jan 16, 2016

index i/j ok to remove

@jreback
Copy link
Contributor

jreback commented Jan 16, 2016

yeh that looks not tested, it for a transform on a Panel or greater. I think just raise NotImplemented (if the ndim >= 3) & remove the 2nd clause (where you noted). If nothing breaks in the test suite then ok!

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

I assume it should be transform_func(result[:, :, i], chunk, comp_ids, accum)

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

k, I can do that.

@jorisvandenbossche
Copy link
Member

@rockg I would keep style changes and code clean-up (as in removal of unused code or a change of actual code) strictly separated (at least in a separate commit). Just to be sure, in case we overlooked something, then it is not burried in a gigantic commit for which you can't see the diff.

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

The main goal of this exercise is to make flake8 pass and cleanup some style were necessary. What I cleaned out were unused variables reported by flake8 and, as a result, I don't see a problem including that here.

@wesm
Copy link
Member

wesm commented Jan 16, 2016

@rockg I've been putting # noqa on these lines and adding a TODO comment to come back and investigate

@wesm
Copy link
Member

wesm commented Jan 16, 2016

This is especially important in unit tests where I suspect that there are some test cases that may have skipped some intended checks

@rockg
Copy link
Contributor Author

rockg commented Jan 16, 2016

Okay, that seems reasonable.

@jreback
Copy link
Contributor

jreback commented Jan 19, 2016

ok, rebasing core/groupby.py is going to be painful for #11841 but go ahead (my touches are mostly reorg anyhow)

@wesm
Copy link
Member

wesm commented Jan 19, 2016

Is this good to merge?

@wesm
Copy link
Member

wesm commented Jan 19, 2016

I'll leave this to @jreback with the pending rebase

@jreback
Copy link
Contributor

jreback commented Jan 19, 2016

if u want to remove groupby.py from this would be great (I'll catch that in resample fixes)

otherwise lgtm

@wesm
Copy link
Member

wesm commented Jan 20, 2016

thanks! we're almost there =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants