BUG: pd.crosstab not working when margin and normalize are set together #27663
Conversation
pandas/core/reshape/pivot.py
Outdated
| index_margin = table.loc[margins_name, :].drop(margins_name) | ||
| # separate cases between multiindex and index | ||
| if isinstance(table_index, MultiIndex): | ||
| index_margin = table.loc[margins_name, :].drop(margins_name, axis=1) |
There was a problem hiding this comment.
why are we not always passing axis= here? I don't like the need for this if/then
There was a problem hiding this comment.
Yeah, i figured out a way to walk it around, and also avoid MI problem, seems even test speed is faster ^^ @jreback
pandas/core/reshape/pivot.py
Outdated
| index_margin = index_margin / index_margin.sum() | ||
| # index_margin is a dataframe, and use a hacky way: sum(axis=1)[0] | ||
| # to get the normalized result, and use sum() instead for series | ||
| if isinstance(index_margin, ABCDataFrame): |
There was a problem hiding this comment.
why do you need to distinguish MI here?
There was a problem hiding this comment.
yes, after i changed the code above, this issue can be solve as well. And i removed this if/then. thank you for pointing it out @jreback
|
ok this looks good, can you merge master and fix the conflic. ping on green. |
|
Thanks for your follow-up review @jreback !! I merged the master and resolved the conflict. |
pandas/core/reshape/pivot.py
Outdated
| # to keep index and columns names | ||
| table_index_names = table.index.names | ||
| table_columns_names = table.columns.names | ||
| # save the column and index margin |
There was a problem hiding this comment.
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.
yes, since this normalization will take one step further from the output of pivot_table function, and in this function, if margin is set to True, then there will be a new column/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! @jreback
There was a problem hiding this comment.
great, this just makes it clear to a future reader, otherwise lgtm. ping on green.
There was a problem hiding this comment.
I have added assertion, pls feel free to take a look @jreback
|
Yeah, this time it passes all checks. @jreback |
…alize are set together (#27783)
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff