-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: pd.crosstab not working when margin and normalize are set together #27663
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
Merged
Merged
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
7e461a1
remove \n from docstring
charlesdong1991 1314059
fix conflicts
charlesdong1991 8bcb313
Merge remote-tracking branch 'upstream/master'
charlesdong1991 466d192
Recommit the PR
charlesdong1991 95869b8
correct typo
charlesdong1991 1b130a6
Correct codes
charlesdong1991 25302d4
Code change based on review
charlesdong1991 bf40467
Add more test to test robustness
charlesdong1991 77aafcd
Optimize the code
charlesdong1991 b0b90e6
Mark issue number in test file
charlesdong1991 5c13549
Resubmit
charlesdong1991 469f22e
Use black to reformat the file
charlesdong1991 b431ea1
fix conflict
charlesdong1991 93a9805
Merge master
charlesdong1991 64c0b13
Add assertion for margin name
charlesdong1991 1e7dc11
Rephrase comments and resubmit PR
charlesdong1991 2112387
Merge remote-tracking branch 'upstream/master' into fix_issue_27500
charlesdong1991 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if margins is True, then we are guaranteed to have the margins_name be the last row / column? can you add an assert to this, that the last row.name / col.name == margins_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, since this normalization will take one step further from the output of
pivot_table
function, and in this function, if margin is set toTrue
, then there will be a newcolumn/index
added to the end ('All' or 'New_Margin_Name'
). But you are right, it's better to add an assertion to this, will do later today! @jrebackThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, this just makes it clear to a future reader, otherwise lgtm. ping on green.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added assertion, pls feel free to take a look @jreback