BUG: fixed buggy behavior in pivot_table with margins=True and numeric columns #46778
Conversation
Removed whitespace
| # GH26568 reorder_levels improperly uses a key that is also a number | ||
| # If a number in new_order is also a key, | ||
| # convert new_order to their respective keys in row_margin.index.names | ||
| for key in row_margin.index.names: |
There was a problem hiding this comment.
Can't this just be a take(new_order) in the index names?
There was a problem hiding this comment.
@mroeschke I tried using take() to do what we're doing here but it doesn't seem like its doing the same thing. Could you elaborate on what take(new_order) should be doing here?
There was a problem hiding this comment.
If I am interpreting your for loop correctly, you're converting new_order which originally represented positions to labels? This should be equivalent to a take operation if so: https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.Index.take.html
There was a problem hiding this comment.
I'm converting positions in the MultiIndex.names attribute into its corresponding labels. However, using take on a MultiIndex (rather than Index) doesn't replicate the same functionality:
(Pdb) p row_margin.index
MultiIndex([('x', 2),
('y', 2),
('z', 2)],
names=[1, None])
(Pdb) p row_margin.index.take([1,0])
MultiIndex([('y', 2),
('x', 2)],
names=[1, None])
(Pdb) p row_margin.index.take([1,0], axis=1)
MultiIndex([('y', 2),
('x', 2)],
names=[1, None])
What I want is something like this:
MultiIndex([(2, 'x'),
(2, 'y'),
(2, 'z')],
names=[None, 1])
Are we thinking about it the same way?
There was a problem hiding this comment.
Based on your for loop though, it appears you want to take on the names not the actual values?
| @@ -607,6 +607,7 @@ Reshaping | |||
| - Bug in :func:`get_dummies` that selected object and categorical dtypes but not string (:issue:`44965`) | |||
| - Bug in :meth:`DataFrame.align` when aligning a :class:`MultiIndex` to a :class:`Series` with another :class:`MultiIndex` (:issue:`46001`) | |||
| - Bug in concanenation with ``IntegerDtype``, or ``FloatingDtype`` arrays where the resulting dtype did not mirror the behavior of the non-nullable dtypes (:issue:`46379`) | |||
| - Bug in :func:`_generate_marginal_results` when passing in integer key values to :func:`reorder_levels` referenced level by both number and key instead of only one (:issue:`26568`) | |||
There was a problem hiding this comment.
This should describe the user facing impact of the change not the internal change i.e. the bug fix of pivot_table
| ) | ||
|
|
||
| expected = DataFrame([[1, 2, 0, 3], [0, 3, 4, 7], [1, 5, 4, 10]]) | ||
| expected.columns = ["x", "y", "z", "All"] |
There was a problem hiding this comment.
These attributes can be included in the DataFrame constructor e.g. DataFrame(..., columns=Index(..., name=1))
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
Thanks for the pull request, but it appears to have gone stale. If interested in contributing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen. |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.Found a error in the way reorder_levels was being called in _generate_marginal_values in pivot.py. Added a change that would modify the representation of the new order to use keys instead of position in cases where valid key was also a valid position. Included a test from original bug report that exposed the buggy behavior.