Clean multiindex keys
State: closed

 - [x] closes #15452
 - [x] tests added / passed
 - [x] passes ``git diff master | flake8 --diff``
 - [x] whatsnew entry

Most of the change to code is mere refactoring (the small change in ``pandas/core/frame.py`` can be undone once we decide about #15438), but I did suppress three obsolete (the same job [is already done](https://github.com/toobaz/pandas/blob/clean_multiindex_keys/pandas/core/indexing.py#L1076) in the right way in ``_getitem_iterable``) and buggy [lines](https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexing.py#L1544) which actually not only caused #15452, but were the real explanation for #15424.

As a side consequence, generators can now be used to index a ``MultiIndex``. This is not documented, as far as I know, but it was already possible with a flat ``Index``, and it was [actually also tested](https://github.com/pandas-dev/pandas/blob/master/pandas/tests/series/test_indexing.py#L270), so I added tests for ``MultiIndex`` too and to setitem (except that not all of them currently work). Once this PR goes through, I plan to open three separate issues for
- the failing tests with setitem and generators (of boolean values)
- ``df.loc[['missing', 'keys'], :]``, which still erroneously returns an empty ``DataFrame`` rather than raising (as ``df.loc[['missing', 'keys']]`` does)
- [my proposal](https://github.com/pandas-dev/pandas/issues/15452#issuecomment-281542685) about a change in behaviour for missing keys

Feel free to tell me if any of this is not worth discussing.

Labels:
name: Bug
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: MultiIndex

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

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

```diff
@@            Coverage Diff             @@
##           master   #15615      +/-   ##
==========================================
+ Coverage   90.99%   91.01%   +0.01%     
==========================================
  Files         161      143      -18     
  Lines       49303    49387      +84     
==========================================
+ Hits        44863    44949      +86     
+ Misses       4440     4438       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `?` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.87% <100%> (+0.06%)` | :arrow_up: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `94.1% <100%> (+0.17%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `80.92% <0%> (-19.08%)` | :arrow_down: |
| [pandas/tools/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL3Rvb2xzL3Bsb3R0aW5nLnB5) | `71.79% <0%> (-10.03%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `90.27% <0%> (-9.73%)` | :arrow_down: |
| [pandas/util/\_tester.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvX3Rlc3Rlci5weQ==) | `35.29% <0%> (-3.6%)` | :arrow_down: |
| [pandas/conftest.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL2NvbmZ0ZXN0LnB5) | `94.11% <0%> (-2.32%)` | :arrow_down: |
| [pandas/types/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL3R5cGVzL2NvbmNhdC5weQ==) | `98.06% <0%> (-1.94%)` | :arrow_down: |
| [pandas/io/pickle.py](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree#diff-cGFuZGFzL2lvL3BpY2tsZS5weQ==) | `79.54% <0%> (-1.71%)` | :arrow_down: |
| ... and [194 more](https://codecov.io/gh/pandas-dev/pandas/pull/15615?src=pr&el=tree-more) | |

------

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

(green)
not sure why this didn't fail. This is a change in behavior (which we should prob do, but needs discussion).

```
In [1]:         idx = pd.MultiIndex.from_product([['A', 'B', 'C'],
   ...:                                           ['foo', 'bar', 'baz']],
   ...:                                          names=['one', 'two'])
   ...:         s = pd.Series(np.arange(9, dtype='int64'), index=idx).sort_index()
   ...:     
   ...: 

In [2]: s
Out[2]: 
one  two
A    bar    1
     baz    2
     foo    0
B    bar    4
     baz    5
     foo    3
C    bar    7
     baz    8
     foo    6
dtype: int64
In [3]: s.loc[['A', 'D']]
KeyError: "['D'] not in index"
```
> not sure why this didn't fail.

Which commit are you on?!

```python
In [3]: idx = pd.MultiIndex.from_product([['A', 'B', 'C'], ['foo', 'bar', 'baz']],names=['one', 'two'])

In [4]: s = pd.Series(np.arange(9, dtype='int64'), index=idx).sort_index()

In [5]: s.loc[['A', 'D']]
Out[5]: 
one  two
A    bar    1
     baz    2
     foo    0
dtype: int64

In [6]: pd.util.print_versions.get_sys_info()[0]
Out[6]: ('commit', 'b4a1c5b168961324d2260657424df621bbd0a645')
```
I rebased the PR. so rebase and push again.
This is related to 05d70f4e617a274813bdb02db69143b5554aa106. Before:
``` python
In [2]: s = pd.Series(range(9), index=pd.MultiIndex.from_product([['A', 'B', 'C'], ['foo', 'bar', 'baz']],names=['one', 'two']))

In [3]: s.loc._getitem_iterable(['A', 'E'])
Out[3]: 
one  two
A    foo    0
     bar    1
     baz    2
dtype: int64

In [4]: pd.util.print_versions.get_sys_info()[0]
Out[4]: ('commit', '998c801f76256990b98d3f0d2ad885ae27c955a1')
```

After:
``` python
In [2]: s = pd.Series(range(9), index=pd.MultiIndex.from_product([['A', 'B', 'C'], ['foo', 'bar', 'baz']],names=['one', 'two']))

In [3]: s.loc._getitem_iterable(['A', 'E'])
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-3-62e87e14f902> in <module>()
----> 1 s.loc._getitem_iterable(['A', 'E'])

/home/nobackup/repo/pandas/pandas/core/indexing.py in _getitem_iterable(self, key, axis)
   1090             # if it cannot handle
   1091             indexer, keyarr = labels._convert_listlike_indexer(
-> 1092                 key, kind=self.name)
   1093             if indexer is not None:
   1094                 return self.obj.take(indexer, axis=axis)

/home/nobackup/repo/pandas/pandas/indexes/multi.py in _convert_listlike_indexer(self, keyarr, kind)
   1598             mask = check == -1
   1599             if mask.any():
-> 1600                 raise KeyError('%s not in index' % keyarr[mask])
   1601 
   1602         return indexer, keyarr

KeyError: "['E'] not in index"

In [4]: pd.util.print_versions.get_sys_info()[0]
Out[4]: ('commit', '05d70f4e617a274813bdb02db69143b5554aa106')

```

My patch is good as before, but your new code is not coherent with our "desired" behavior for indexers.

Now, I assumed that proceeding step by step was the best way to move forward, but my assumption was clearly wrong if while I patiently wait for my trivial patch to get merged, you do find the time to [completely rewrite related indexing methods](https://github.com/pandas-dev/pandas/commit/05d70f4e617a274813bdb02db69143b5554aa106) (deleting a line I had explicitly mentioned as important when submitting my PR - all this in a commit which appears as "_DOC: ..._" in the logs, just to make it easier to spot it).

So, considering that the changes you made ironically go in a direction I like, we'll probably rather have the "famous" discussion first and then come back on this. I'm opening an issue for this in a minute.
> My patch is good as before, but your new code is not coherent with our "desired" behavior for 

your patch fails on master. 
> Now, I assumed that proceeding step by step was the best way to move forward, but my assumption was clearly wrong if while I patiently wait for my trivial patch to get merged, you do find the time to 

and how is this not step-by-step?

any indexing patch is hardly trivial.
> any indexing patch is hardly trivial.

... if you don't understand what you are doing
> ... if you don't understand what you are doing

@toobaz these comments are hardly necessary.

You can put up patches all you like. But they need review and I will get to them when I have time.
> You can put up patches all you like. But they need review and I will get to them when I have time.

Patches are a total waste of time if you change the underlying code, and even more if you introduce new bugs there. You have time for your huge patch but not for my three-liner?! (because the other 10 lines were trivial refactoring)
> Patches are a total waste of time if you change the underlying code, and even more if you introduce new bugs there. You have time for your huge patch but not for my three-liner?! (because the other 10 lines were trivial refactoring)


how so, we have 75 open PR's of which I have 10, some of which have sat here for weeks waiting for review / comment by others. This is how things work. I looked at your patch and it didn't work. 

So I asked you to rebase? I am not sure of the problem. 
> even more if you introduce new bugs there

How is this even true? It completely passed the test suite. Yours does not.
> I am not sure of the problem.

I am, but my time here is currently wasted. See my code above, it's not complicated, you just changed the behavior of a (non-public) method. No idea how to reproduce with user code, currently.

> It completely passed the test suite. Yours does not.

It did before you introduced yours, so this is _obviously_ a non-argument.
@toobaz I have asked you to rebase. If you can great.

PR's are merged when they fully pass the test suite.

patches are certainly require rebase a lot sometimes. To be honest I am working on someother PR's which DID require some cleanup in the indexing code.

You should be more appreciative of others's efforts. 
> You should be more appreciative of others's efforts.

Ah ah, you telling me this. When you'll want to be appreciative of my efforts, feel free to reopen [this](https://github.com/pandas-dev/pandas/compare/master...toobaz:cat_align_vec) (which I will be happy to rebase).

> @toobaz I have asked you to rebase. If you can great.

Doesn't make sense as of now (you tried yourself, as you said)
> Ah ah, you telling me this. When you'll want to be appreciative of my efforts, feel free to reopen [this](https://github.com/pandas-dev/pandas/compare/master...toobaz:cat_align_vec) (which I will be happy to rebase).

again not sure of what you mean.

> Doesn't make sense as of now (you tried yourself, as you said)

also not sure what you mean.

Sure I appreciate your efforts @toobaz but at the same time I ask you to appreciate that others are contributing. It doesn't matter that a patch was put in before yours, this PR still needs rebasing.
and to clarify I just ran this PR and saw the failing tests, I didn't attempt to fix it.
needs a rebase and moved whatsnew. if its easier to close and re-submit that is fine too.
Rebasing is fine, but to apply this, ``s.loc._getitem_iterable(.)`` must be brought back to its pre- 05d70f4 behavior (on my TODO list but not terribly high), unless we decide for a change of API in #15747.
@toobaz : I know it's been some time, but according to GitHub, the only thing you absolutely need to update is the `whatsnew`.  Do you still want to get this merged?
> @toobaz : I know it's been some time, but according to GitHub, the only thing you absolutely need to update is the whatsnew.

Yes in order to solve conflicts, but then unfortunately tests will fail (see [my previous comment](https://github.com/pandas-dev/pandas/pull/15615#issuecomment-301277779)). So I see it as pointless, but if instead there's some utility, I can do it easily.

> Do you still want to get this merged?

Well, in principle yes, but no problem for me if you want to close, I can open another PR in the future. ``s.loc._getitem_iterable(.)`` must be fixed first, and I fear it will take more time than it took me to write this patch.
@toobaz : How about this: if you could patch the merge conflict (it's pretty simple to do as you said), and we can let Travis run the tests again just to see where things stand now.

If they fail, I can close this for you.  However, if that happens, if you could add a comment regarding `s.loc._getitem_iterable(...)` in the issue that you posted (i.e. #15452) that this needs to be patched first, that would be useful for future reference.  Or, if you think it should standalone as a separate issue, feel free to file a separate that references your initial issue.

How does that sound?
> : How about this: if you could patch the merge conflict (it's pretty simple to do as you said), and we can let Travis run the tests again just to see where things stand now.

Sure, no problem! Done.


> if you could add a comment regarding s.loc._getitem_iterable(...) in the issue that you posted (i.e. #15452) that this needs to be patched first, that would be useful for future reference. Or, if you think it should standalone as a separate issue, feel free to file a separate that references your initial issue.

Done: #16920
Alright: Two test failures due to the fact that you're using outdated testing functions (`pytest.raises` instead of `tm.assertRaises`).  However, you do have one test failure as you predicted because of that issue re: the `_get_item` issue.

I will close this PR given your previous comments.  As you said previously, #16920 will need to be patched before fixing #15452.  Feel free to take a stab at the former if you have the time.