Respect dtype when calling pivot_table with margins=True
State: closed

 - [x] closes #17013
 - [x] tests added / passed
 - [x] passes ``git diff master -u -- "*.py" | flake8 --diff``
 - [x] whatsnew entry

This fix actually exposed an occurrence of #17035 in an existing test (as well as in one I added).

Labels:
name: Bug
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
@toobaz : Rebase your PR onto `master` to fix the `pyarrow` failures.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=h1) Report
> Merging [#17062](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/395f712133a4f6003ec8029458ade7ab423096d0?src=pr&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17062/graphs/tree.svg?height=150&width=650&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17062      +/-   ##
==========================================
- Coverage   91.05%   91.01%   -0.04%     
==========================================
  Files         161      161              
  Lines       49363    49364       +1     
==========================================
- Hits        44948    44931      -17     
- Misses       4415     4433      +18
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.79% <100%> (-0.02%)` | :arrow_down: |
| #single | `40.25% <0%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `93.15% <100%> (-2.04%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `98.2% <0%> (-0.3%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `96.83% <0%> (-0.09%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `95.87% <0%> (-0.06%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=footer). Last update [395f712...0b191ab](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=h1) Report
> Merging [#17062](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f9a552dc12262d1d208f9cdc2c5ffd1731f9c361?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17062/graphs/tree.svg?height=150&token=eZ4WkYLtcO&width=650&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17062      +/-   ##
==========================================
- Coverage   91.02%   91.01%   -0.02%     
==========================================
  Files         161      161              
  Lines       49396    49397       +1     
==========================================
- Hits        44965    44957       -8     
- Misses       4431     4440       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.78% <100%> (ø)` | :arrow_up: |
| #single | `40.28% <0%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `95.2% <100%> (+0.01%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.66% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=footer). Last update [f9a552d...2737600](https://codecov.io/gh/pandas-dev/pandas/pull/17062?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> this will cause the entire test to xfail, separate the xfail portion to another test (and use the decorator). I find an embedded xfail problematic.

@jreback  I understand your concern. But if we refrain from embedded xfails, do you still think that marking the xfailing test as such is worth more than keeping the test structure consistent, and replacing the ``xfail`` with an ``AssertRaises``? We could have a standard way to make it clear in a comment that this is only temporary, to be removed when the bug is solved.
> But if we refrain from embedded xfails, do you still think that marking the xfailing test as such is worth more than keeping the test structure consistent

I'm not sure I follow here.  @jreback is just saying that you should separate out this portion of the code as a separate test (and decorate with `xfail`), even if it means duplicating some of the setup logic in the test to which it belongs currently.
> is just saying that you should separate out this portion of the code as a separate test, even if it means duplicating some of the setup logic in the test to which it belongs currently.

Yes, exactly. I personally would consider the duplication (and also the "entropy" caused by splitting tests which were designed together) a greater evil than replacing my ``pytest.xfail`` above with an ``AssertRaises`` (equipped with an appropriate comment)... after all, when the bug is fixed the problematic part will emerge anyway from the test results. Vice-versa, if we split tests, then we will just forget.

(but maybe I'm missing some clever use currently or potentially made of xfails)


> Yes, exactly. I personally would consider the duplication (and also the "entropy" caused by splitting tests which were designed together) a greater evil...Vice-versa, if we split tests, then we will just forget.

Then to ensure we don't forget.  In the `pytest.xfail` message, include a reminder to merge the test when this issue is fixed.  That will get the job done.
> Then to ensure we don't forget. In the pytest.xfail message, include a reminder to merge the test when this issue is fixed. That will get the job done.

I was just asking if, and possibly _why_, each (embedded) ``xfail`` is really worth this double refactoring effort. Just so that I understand and hence I stop asking such silly questions.

( @jreback  Alternative proposal: a decorator ``@ensure_xfail`` and a context manager ``with ensure_xfail():``. The context manager will emit a specific warning if the content does _not_ fail, and the decorator will capture precisely this warning, and report a "failed xfail" accordingly - otherwise act transparently. Should be easy to code, I can try if you like the idea.)
> ( @jreback Alternative proposal: a decorator @ensure_xfail and a context manager with ensure_xfail():. The context manager will emit a specific warning if the content does not fail, and the decorator will capture precisely this warning, and report a "failed xfail" accordingly - otherwise act transparently. Should be easy to code, I can try if you like the idea.)

no we have too many custom things already. 2nd this is already the style. xfails should fail an entire test.

this needs to be a separate test because the first part of the test works, then you are then NOT running it until it is fixed. 

pls make a separate test that is xfailed. you can certainly put a note to merge in with the previous test. 


> this needs to be a separate test because the first part of the test works, then you are then NOT running it until it is fixed.

Just to clarify: with my proposals, tests _would_ run even when decorated, and _would_ be reported as failed if raising outside the context manager.

Anyway, OK, I see there is no interest, I will split the test in three parts and add comments describing how to merge them back.
> Anyway, OK, I see there is no interest, I will split the test in three parts and add comments describing how to merge them back.

Its not even necessary to merge them pack. Having fine grained tests is ok, esp when they are covering different cases.
> Having fine grained tests is ok, esp when they are covering different cases.

In our codebase tests tend to be regrouped, so I assumed it was an attempt to save (setup) time. Anyway, done!
thanks!