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

BUG: Fix brittle pivot margins #4358

Closed
wants to merge 3 commits into from

Conversation

guyrt
Copy link
Contributor

@guyrt guyrt commented Jul 25, 2013

closes #3334 Brittle pivot margins

Issue was that pivot tables that use all columns of the original DataFrame in rows and cols failed on the margin computation. This is a special case for margins: one should use the index itself as the value.

>> df = DataFrame({'Response' : ['Y', 'N' ,'N', 'Y', 'Y', 'N'],
            'Type' : ['A', 'A', 'B', 'B', 'B', 'C']})

>> pivot_table(df, rows='Response',cols='Type',aggfunc=len,margins=True)
>> Type      A  B   C  All
>> Response               
>> N         1  1   1    3
>> Y         1  2 NaN    3
>> All       2  3   1    6

This is my first PR to Pandas, so apologies for anything out of the ordinary.

@jreback
Copy link
Contributor

jreback commented Jul 25, 2013

pls move the release notes to 0.13.thanks! (you prob need to rebase too)

@guyrt
Copy link
Contributor Author

guyrt commented Jul 25, 2013

I moved release note to v0.13.0.txt and rebased the branch.

@jreback
Copy link
Contributor

jreback commented Jul 25, 2013

can you squash into 1-2 commits?

git rebase -i <commit 1 prior to yours>...then s/f

@jreback
Copy link
Contributor

jreback commented Jul 25, 2013

sorry..should have mentioned that before

@guyrt
Copy link
Contributor Author

guyrt commented Jul 25, 2013

Fixed

@@ -29,6 +29,9 @@ Bug Fixes

- Fixed bug in ``PeriodIndex.map`` where using ``str`` would return the str
representation of the index (:issue:`4136`)

- Fixed some edge cases in pivot_table where margins did not compute if values is the index.
Copy link
Contributor

Choose a reason for hiding this comment

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

pls add the issue here (and need in release notes as well)..thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback do you mean add the issue number, or an example that would fail without the change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant (:issue:4358)....this creates a link in the document back to this actual issue

@jreback
Copy link
Contributor

jreback commented Aug 1, 2013

also...need to rebase on master (what happens is there are conflict on release notes....no biggie)....just rebase and i'll merge you right away

@guyrt
Copy link
Contributor Author

guyrt commented Aug 1, 2013

woops! Turns out my origin master was out of date. Should have used upstream. Hang on, will fix.

@guyrt guyrt closed this Aug 1, 2013
@guyrt guyrt reopened this Aug 1, 2013
@guyrt
Copy link
Contributor Author

guyrt commented Aug 1, 2013

@jreback I made a new branch off master and cherry-picked my commits into it. I picked up a change in a test file
https://github.com/pydata/pandas/blob/master/pandas/tools/tests/test_pivot.py#L11
that makes my tests fail. Any idea what could have happened there?

@jreback
Copy link
Contributor

jreback commented Aug 1, 2013

you need to rebase this

something like

updates master to current

git checkout master
git pull
git branch my-branch --set-upstream master
git checkout my-branch
git pull --rebase

will bring you up to current

…_table

Adds support for margin computation when all columns are used in rows and cols.
@guyrt
Copy link
Contributor Author

guyrt commented Aug 1, 2013

Fixed - sorry.

@hayd
Copy link
Contributor

hayd commented Aug 1, 2013

@jreback for some reason I made the decision to never use pull (I like to use fetch/merge fast-forward/rebase so I don't get in sticky situations) but now I look again it looks like you can do that with pull too, cooool

git pull --ff-only

not sure I'll change.

@guyrt
Copy link
Contributor Author

guyrt commented Aug 12, 2013

@jreback Travis is green. Any idea when this can get merged in?

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

@cpcloud can u take a look?

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

@guyrt thanks! merged in

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

via 69f5594

@jreback jreback closed this Aug 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pivot table margins brittleness
3 participants