BUG: pivot_table not returning correct type when margin=True and aggfunc='mean'

[mabelvj]

• closes pivot_table margins=True default aggfunc='mean' does integer division #24893
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

What's new

Use of maybe_downcast_to_dtype in _add_margins so it can resolve the dtype conversion, avoiding floats being converted to integers when the result of the aggfunc is a float.

For the new case, if after applying the aggfunc, if the margin result is not an integer, the whole column is converted to float:

Example:

_df
   A  B  C  D
0  2  1  1  X
1  4  4  3  X
2  6  5  4  Y
3  8  8  6  Y

Currently pandas does this:

print(pd.pivot_table(_df, index="D", margins=True))                                                              
     A    B  C
D             
X    3  2.5  2
Y    7  6.5  5
All  5  4.5  3

with the fix the result is:

print(pd.pivot_table(_df, index="D", margins=True))                                                              
     A    B  C
D             
X    3  2.5  2.0
Y    7  6.5  5.0
All  5  4.5  3.5

Issues

There are test referencing that np.means of ints are casted back into ints. However, giving that for the aggregations in the rows, floats are kept when the np.mean of integers is a float, it does not make sense that this behavior does not hold for the margins.

[pep8speaks]

Hello @mabelvj! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-23 20:43:22 UTC

[simonjayhawkins]

@mabelvj Thanks for the PR.

to fix the ci failure you'll need to run black pandas https://dev.pandas.io/development/contributing.html#python-pep8-black

also add test to confirm the fix works.

[mabelvj]

I see that there are tests referencing that np.means of ints are casted back into ints marked with pytest.mark.xfail that are related to this issue, should I update them?:

pandas/pandas/tests/reshape/test_pivot.py

Line 1604 in 03b3c8f

@pytest.mark.xfail(reason="GH#17035 (np.mean of ints is casted back to ints)")

pandas/pandas/tests/reshape/test_pivot.py

Line 1618 in 03b3c8f

@pytest.mark.xfail(reason="GH#17035 (np.mean of ints is casted back to ints)")

[TomAugspurger]

should I update them?

Has the output changed? I would have expected them to fail CI if they were changed to no longer cast from float to int.

[jreback]

lgtm. can you add a release note in reshaping bug fix section for 1.0

[mabelvj]

@TomAugspurger the results do not change for those, the tests are skipped if failing since they are marked to do so. Still, checking them (since they are related to this issue, because they check the float casting on the margins), the expected values do not match the output result (old and new pivot_table give the same output here).

[jreback]

this lgtm. @mabelvj can you merge master; ping on green.

[WillAyd]

ping @mabelvj - if you can fix up merge conflicts can get this one in

[mabelvj]

@jreback @WillAyd Fixed the conflicts, but Flake8 keeps giving a false positive F841 error on L1670-1671 because of unused variables, but they are used at the last line of the function.

It's strange because other functions have the same structure and do not raise any error.

[WillAyd]

I think you need to add tm.assert_result_equal(result, expected) to the end of the test

[mabelvj]

@WillAyd Fixed now. It seems the line disappeared when fixing previous conflicts with master.

[WillAyd]

Great - thanks @mabelvj for seeing this one through!