BUG #15150  normalization of crosstable with multiindex and margins
State: closed

 - [x] closes #15150
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master --name-only -- '*.py' | flake8 --diff``
 - [x] whatsnew entry

When debugging this issue I came across some unexpected results for margins 
when normalization 'index' or 'column' is performed. Here a cross table with 'column' normalization (example from line 1271 in test_pivot.py):

    b    3    4  All
    a               
    1  0.5  0.0  0.2
    2  0.5  1.0  0.8

I would expect, that margin values should always be the sums of rows/cols, regardless if values were normalized or not, so I would expect the following:

    b    3    4  All
    a               
    1  0.5  0.0  0.5
    2  0.5  1.0  1.5

This is not the case for 'index' and 'column' normalization. In fact, margin values are calculated as sums of raw values and then normalized. This is fine for normalization 'all'. But for normalization 'columns' and 'index', this leads to -- at least for me -- unexpected results.

I left the calculation as it is, because this kind of behavior is validated in test_crosstab_normalize
(test_pivot.py), so maybe the calculation is wanted like that. Is it?
 

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

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

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

```diff
@@            Coverage Diff             @@
##           master   #16599      +/-   ##
==========================================
- Coverage   90.93%   90.93%   -0.01%     
==========================================
  Files         161      161              
  Lines       49267    49253      -14     
==========================================
- Hits        44800    44787      -13     
+ Misses       4467     4466       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.69% <100%> (-0.01%)` | :arrow_down: |
| #single | `40.23% <0%> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `95.2% <100%> (+0.11%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16599?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/16599?src=pr&el=footer). Last update [bf99975...268ce49](https://codecov.io/gh/pandas-dev/pandas/pull/16599?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/16599?src=pr&el=h1) Report
> Merging [#16599](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/793020293ee1e5fa023f45c12943a4ac51cc23d0?src=pr&el=desc) will **increase** coverage by `1.57%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #16599      +/-   ##
==========================================
+ Coverage   91.02%    92.6%   +1.57%     
==========================================
  Files         161      161              
  Lines       49393    63210   +13817     
==========================================
+ Hits        44962    58536   +13574     
- Misses       4431     4674     +243
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.42% <100%> (+1.64%)` | :arrow_up: |
| #single | `44.48% <4.65%> (+4.14%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/pivot.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9waXZvdC5weQ==) | `97.51% <100%> (+2.32%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/util/\_print\_versions.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvX3ByaW50X3ZlcnNpb25zLnB5) | `14.28% <0%> (-1.43%)` | :arrow_down: |
| [pandas/core/config\_init.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29uZmlnX2luaXQucHk=) | `95.33% <0%> (-0.57%)` | :arrow_down: |
| [pandas/core/computation/eval.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vZXZhbC5weQ==) | `96.47% <0%> (-0.46%)` | :arrow_down: |
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `91.26% <0%> (-0.18%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `98.53% <0%> (+0.02%)` | :arrow_up: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `97.03% <0%> (+0.11%)` | :arrow_up: |
| [pandas/tseries/frequencies.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvZnJlcXVlbmNpZXMucHk=) | `96.87% <0%> (+0.2%)` | :arrow_up: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `95.82% <0%> (+0.36%)` | :arrow_up: |
| ... and [40 more](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16599?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/16599?src=pr&el=footer). Last update [7930202...93cb736](https://codecov.io/gh/pandas-dev/pandas/pull/16599?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Ok. Thank you for checking. I'll modify the error messages and add the whatsnew.

Could someone comment on my point above: I think that margins are not calculated correctly when normalize is set to 'columns' or 'index'. See my example above. Should I post a bug report for that? Or is the calculation correct?  
I tried to implement the changes as requested. I also modified some values in validation data of test_margin_dropna, because the expected margin values were not correct.
@jreback 
I reformatted the test to be more readable, as you requested. is it ok now? 
@gfyoung can you have a look
@toobaz can you have a look
@cmohl2013 Sorry for coming in the discussion now only.

This said: from #15150 :

> Possible solution: calling pivot_table in crosstab always with margins=False, then
> do normalization and finally call _add_margins, if margins=True.

I wonder whether we couldn't directly fix ``pivot_table``... as an ugly hack, the following seems to work: https://github.com/pandas-dev/pandas/compare/master...toobaz:crosstab_hack?expand=1

(it's not just a hack - one of the two changes just works around #17024 I think - it will also fail on >2 levels... but it seems pretty simple to fix)
@toobaz Yes, fixing pivot_table was my original plan as commented in #15150, but I did not manage to come up with a good solution, so I implemented adding margins in crosstable directly. 
Your solution looks good to me (and can be adjusted as soon as #17024 is fixed).

@toobaz 
I tried your solution, but then ran into problems with concat and MultiIndex. So I ended up with my previous solution and added the function _add_margins_to_multiindex as workaround for #17024. I left a comment in the code that the workaround should be removed when #17024 is solved. 
> I tried your solution, but then ran into problems with concat and MultiIndex

Couldn't we try to solve these problems? I'd be happy to help.

On the other hand, the current PR introduces a lot of code duplication which I personally would prefer to avoid. Moreover, my understanding (admittedly after just a quick glance at the changes) is that you are fixing something in ``crosstable`` but the same exact problem will remain in ``pivot_table``, while if you fix it in ``pivot_table`` then ``crosstable`` will automatically benefit.

(Disclaimer: those are just suggestions, I'm not a maintainer)
@toobaz 

>On the other hand, the current PR introduces a lot of code duplication which I personally would >prefer to avoid.

In fact, it reduces the code. We get rid of the whole margins dropping and fixing that goes on in lines  568 to 606 (in the master branch). Here we have also a separate calculation of margins, independent from the calculation in pivot_table. So the PR did not introduce this duplication of margins calculation, it was there before. 

However, I see your point that it is not good to use different code to either calculate margins in pivot_table or crosstable. I find the calculation of margins in pivot_table not very readable and cryptic. Would it be an option to transfer the code for calculating margins how I did it in crosstable to pivot_table and rewrite _add_margins? Could well be that I miss something and that it would be not as easy. What do you think?
not sure what to do with this PR. @toobaz want to take a look and see how we can reconcile this (and other's indicated above)
@jreback @cmohl2013 sorry for disappearing

> In fact, it reduces the code

Cool, but this is not a guarantee that there is no (also future) duplication involved... and the fact that you call ``pivot_table`` with ``margins=False`` looks to me conceptually wrong (the ``if margins:`` part really mostly replicates stuff from ``pivot_table``).

> Would it be an option to transfer the code for calculating margins how I did it in crosstable to pivot_table and rewrite _add_margins?

Probably, yes. I admit I'm a bit confused (and rebasing could help): the bug you are fixing is raised in the call to ``_normalize`` inside ``crosstab``. So either 1) ``_normalize`` must be fixed, or 2) the input that ``crosstab`` passes to ``_normalize`` is in some way wrong (or both). Now [my approach](https://github.com/pandas-dev/pandas/compare/master...toobaz:crosstab_hack) just fixed ``_normalize``, and apparently worked. So I tend to exclude that 2) is a problem. However, most of your changes take effect _before_ the call to ``_normalize``. Maybe some of them are good, but I fail to understand their purpose. Are they unrelated to the bug?

Oh, by the way:

> When debugging this issue I came across some unexpected results for margins

Admittedly the result looks strange and the design choice can be discussed, but it is, strictly speaking, correct. You are asking to normalize columns, which means that each column should add up to 1... including the ``All`` column. But if you think this is worth reconsidering (i.e. "_first_ normalize, _then_ calculate margins"), I suggest to do so in a separate issue.
closing as stale. if you'd like to continue working, pls ping.