Skip to content
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

Groupby tuples #18249

Closed
wants to merge 4 commits into from
Closed

Groupby tuples #18249

wants to merge 4 commits into from

Conversation

toobaz
Copy link
Member

@toobaz toobaz commented Nov 12, 2017

  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

Sorry for having missed #17996, where this could have been discussed, but this said I don't think we should treat specially the case of a MultiIndex (see the test I added with tuples as labels): we should just interpret what is not a list as a label, as the docs already state (although with some incoherence, which I tried to address).

While I was at it, I fixed a typo which made a test inserted in #17996 trivial.

# when MultiIndex, allow tuple to be a key
if not isinstance(key, (tuple, list)) or \
(isinstance(key, tuple) and is_axis_multiindex):
if not isinstance(key, list):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a nice comment here, explaining

values are used as-is determine the groups. A str or list of strs
may be passed to group by the columns in ``self``
values are used as-is determine the groups. A label or list of
labels may be passed to group by the columns in ``self``.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe callout what a tuple does.

@jreback
Copy link
Contributor

jreback commented Nov 13, 2017

linting issue

@codecov
Copy link

codecov bot commented Nov 13, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@7b5ca80). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #18249   +/-   ##
=========================================
  Coverage          ?   91.41%           
=========================================
  Files             ?      164           
  Lines             ?    50090           
  Branches          ?        0           
=========================================
  Hits              ?    45790           
  Misses            ?     4300           
  Partials          ?        0
Flag Coverage Δ
#multiple 89.22% <100%> (?)
#single 40.37% <0%> (?)
Impacted Files Coverage Δ
pandas/core/generic.py 95.72% <ø> (ø)
pandas/core/groupby.py 92.02% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b5ca80...dafc838. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 13, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@7b5ca80). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #18249   +/-   ##
=========================================
  Coverage          ?   91.39%           
=========================================
  Files             ?      164           
  Lines             ?    50095           
  Branches          ?        0           
=========================================
  Hits              ?    45783           
  Misses            ?     4312           
  Partials          ?        0
Flag Coverage Δ
#multiple 89.2% <100%> (?)
#single 40.37% <0%> (?)
Impacted Files Coverage Δ
pandas/core/generic.py 95.72% <ø> (ø)
pandas/core/groupby.py 92.03% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b5ca80...dafc838. Read the comment docs.

@toobaz
Copy link
Member Author

toobaz commented Nov 13, 2017

linting issue

Fixed! ping @jreback

@jreback jreback added this to the 0.22.0 milestone Nov 13, 2017
@jreback jreback closed this in 4c63875 Nov 13, 2017
@jreback
Copy link
Contributor

jreback commented Nov 13, 2017

thanks!

@toobaz toobaz deleted the groupby_tuples branch November 13, 2017 12:06
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
xref pandas-dev#17996

Author: Pietro Battiston <[email protected]>

Closes pandas-dev#18249 from toobaz/groupby_tuples and squashes the following commits:

dafc838 [Pietro Battiston] DOC: Clarification of groupby(by=) argument
e0bdfa7 [Pietro Battiston] TST: Test for tuples in columns, fixes to previous tests
74f91e0 [Pietro Battiston] TST: Fix tests which used tuples to pass multiple keys
201a4fe [Pietro Battiston] BUG: Never interpret a tuple as a list of keys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants