Emit warning for missing labels in Multiindex.loc[[...]] (and more)
State: closed

- [x] closes #17758
- [x] closes #20748
- [x] closes #20753
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Main changes:
- the presence of at least one key (and of all of them, for the temporary warning) is now checked _after_ building the indexer, using the indexer itself, in a unique place
- for this to be possible, ``obj.reindex`` was replaced with ``obj._reindex_with_indexers``
- ``_multi_take`` is uglier than before... but it's ugly anyway, and must be removed in a future refactoring (in which first indexers are built in all possible code paths, _then_ values are extracted)
- ``_has_valid_type`` was mostly used in "assertion mode" (without looking at its return value), so I changed it to ``_validate_key`` and now it is _only_ used in "assertion mode"

Asv (``--bench indexing``) give
```
       before           after         ratio
     [3a2e9e6c]       [25711d3d]
+       162±0.2ms          273±5ms     1.68  indexing.NumericSeriesIndexing.time_getitem_array(<class 'pandas.core.indexes.numeric.Float64Index'>)
+         162±1ms        269±0.7ms     1.66  indexing.NumericSeriesIndexing.time_loc_array(<class 'pandas.core.indexes.numeric.Float64Index'>)
+       162±0.6ms          267±1ms     1.65  indexing.NumericSeriesIndexing.time_ix_list_like(<class 'pandas.core.indexes.numeric.Float64Index'>)
+         164±2ms          270±1ms     1.64  indexing.NumericSeriesIndexing.time_ix_array(<class 'pandas.core.indexes.numeric.Float64Index'>)
+       166±0.6ms        273±0.6ms     1.64  indexing.NumericSeriesIndexing.time_getitem_lists(<class 'pandas.core.indexes.numeric.Float64Index'>)
+       163±0.5ms        267±0.8ms     1.64  indexing.NumericSeriesIndexing.time_getitem_list_like(<class 'pandas.core.indexes.numeric.Float64Index'>)
+         163±3ms        267±0.4ms     1.64  indexing.NumericSeriesIndexing.time_loc_list_like(<class 'pandas.core.indexes.numeric.Float64Index'>)
+         412±3μs          664±4μs     1.61  indexing.NumericSeriesIndexing.time_ix_list_like(<class 'pandas.core.indexes.numeric.Int64Index'>)
+        1.08±0ms      1.42±0.05ms     1.32  indexing.NumericSeriesIndexing.time_ix_array(<class 'pandas.core.indexes.numeric.Int64Index'>)
+     1.14±0.01ms      1.31±0.02ms     1.15  indexing.PanelIndexing.time_subset
-       133±0.2ms        121±0.4ms     0.91  frame_methods.Iteration.time_iteritems_indexing
-         442±2ns          399±2ns     0.90  indexing.MethodLookup.time_lookup_iloc
-       145±0.8μs        130±0.8μs     0.90  indexing.IntervalIndexing.time_getitem_list
-         151±2μs          134±1μs     0.89  indexing.AssignTimeseriesIndex.time_frame_assign_timeseries_index
-      48.5±0.8μs       41.2±0.1μs     0.85  indexing.IntervalIndexing.time_getitem_scalar
-        44.3±1μs       35.6±0.1μs     0.80  indexing.NumericSeriesIndexing.time_iloc_list_like(<class 'pandas.core.indexes.numeric.Float64Index'>)
-     3.81±0.02ms      2.06±0.02ms     0.54  indexing.DataFrameNumericIndexing.time_loc_dups

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

```

notice that ``FloatIndex`` was _not_ being checked at all (i.e. #17758 applied to it too), now it is.

In any case, as I wrote above, there are several improvements still to be made, but I want to go gradually because the complexity of this PR is already pretty high for me.

Labels:
name: Bug
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: MultiIndex
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
@jreback @jorisvandenbossche There is some problem with warnings in Python 2.7.

On my machine,

```
 pytest pandas/tests/indexing/test_indexing.py
```

passes and

```
pytest pandas/tests/indexing/test_loc.py
```

passes, but
```
pytest pandas/tests/indexing/test_loc.py  pandas/tests/indexing/test_indexing.py
```

fails with ``Did not see expected warning`` (as in circleci and travis-ci).

Any suggestion (apart from skipping those on Python 2.7)?
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20770?src=pr&el=h1) Report
> Merging [#20770](https://codecov.io/gh/pandas-dev/pandas/pull/20770?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/563a6ad108a79b0192a25d51d743812a18e21b15?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `93.75%`.

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

```diff
@@            Coverage Diff             @@
##           master   #20770      +/-   ##
==========================================
+ Coverage   91.77%   91.79%   +0.01%     
==========================================
  Files         153      153              
  Lines       49313    49354      +41     
==========================================
+ Hits        45259    45306      +47     
+ Misses       4054     4048       -6
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.19% <93.75%> (+0.02%)` | :arrow_up: |
| #single | `41.92% <56.25%> (+0.03%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20770?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/sparse/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2ZyYW1lLnB5) | `94.83% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.64% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `93.55% <93.47%> (+0.41%)` | :arrow_up: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.15% <0%> (-0.07%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.73% <0%> (-0.04%)` | :arrow_down: |
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `92.41% <0%> (ø)` | :arrow_up: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `100% <0%> (ø)` | :arrow_up: |
| [pandas/core/resample.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzYW1wbGUucHk=) | `96.07% <0%> (ø)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.03% <0%> (+0.03%)` | :arrow_up: |
| ... and [4 more](https://codecov.io/gh/pandas-dev/pandas/pull/20770/diff?src=pr&el=tree-more) | |

------

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

Hello @toobaz! Thanks for updating the PR.



















Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on May 01, 2018 at 11:03 Hours UTC
@jreback ready for me
Are we doing this for 0.23?
> Are we doing this for 0.23?

I guess so!
yes we should
need to review again - ok after rc 
> I think this deserves a more elaborate whatsnew notice?

Not sure...  the notice in 0.21.0 (in principle) already covered this case (it didn't mention special cases with specific kinds of ``Index``es)
@toobaz the issue is that I have seen the warnings pop up and signal an actual error multiple times
if things are being forced / ignored it is only adding technical debt
i
> if things are being forced / ignored it is only adding technical debt

In current master they are ignored.
how’s that?


> how’s that?

The bug is clearly in the test suite (because whatever the pandas code does, tests shouldn't change behavior when they are called alone vs. together). So I'm just providing a reproducible example and making it evident... which is the opposite of ignoring.
@toobaz the reason I mentioned the bigger whatsnew notice is because I think this is actually an important change, and one that will have an impact on people's code. Yes, sure, the change was already done for 0.21 for single level indexes, and you can certainly refer to that and keep it shorter as there, but IMO it still deserves a subsection in the API changes section.
@toobaz you argument is not logical. master is passing, this branch is not. if you want to do a smaller PR to show the warning and not do a refactoring ok, otherwise, this PR will be held up.

@toobaz to clarify the scope of the introduced warning: this is only for the case where you pass a list of exact labels? (so fully specified for all levels)
For the test problem: so you could reproduce it locally on python 2.7 by running the two files together? (what I understand from https://github.com/pandas-dev/pandas/pull/20770#issuecomment-383271735). I can't (only removed the single skip from the current branch)
> you argument is not logical.

this is pure rethoric - which part of my argument is not logical?

> master is passing, this branch is not.

... depending on how you call the tests, which proves that it is a problem in the tests

> if you want to do a smaller PR to show the warning and not do a refactoring ok

Feel free to stratify more cruft to indexing code, there's already plenty of it. I won't. And by the way, what makes you think that the refactoring, and not the warning itself, triggers a problem _in the code that catches warnings_?!


> you could reproduce it locally on python 2.7 by running the two files together?

yes

> I can't

Interesting, let me investigate
> @toobaz to clarify the scope of the introduced warning: this is only for the case where you pass a list of exact labels? (so fully specified for all levels)

Yes, partial indexing is not affected, it requires a different approach.
In the meantime I can reproduce it, I only removed one of the two skips
In the meantime I can reproduce it, I only removed one of the two skips
> In the meantime I can reproduce it, I only removed one of the two skips

You are right - I had added the other because the test was failing, but it is not required (at least not to run the two files, let's see how the entire suite goes).
Found the test problem (but not yet solved it ;-)): it's in the catching of all xarnings for Panels in check_result
This fixes it for me (idea is to only catch Panel specific warnings there):

```patch
diff --git a/pandas/tests/indexing/common.py b/pandas/tests/indexing/common.py
index f7fa058..7acb963 100644
--- a/pandas/tests/indexing/common.py
+++ b/pandas/tests/indexing/common.py
@@ -300,7 +300,9 @@ class Base(object):
 
                     # Panel deprecations
                     if isinstance(obj, Panel):
-                        with catch_warnings(record=True):
+                        with catch_warnings():
+                            import warnings
+                            warnings.filterwarnings("ignore", "\nPanel*", FutureWarning)
                             _call()
                     else:
                         _call()
diff --git a/pandas/tests/indexing/test_indexing.py b/pandas/tests/indexing/test_indexing.py
index b965bca..468e164 100644
--- a/pandas/tests/indexing/test_indexing.py
+++ b/pandas/tests/indexing/test_indexing.py
@@ -132,8 +132,8 @@ class TestFancy(Base):
         assert is_float_dtype(left['foo'])
         assert is_float_dtype(left['baz'])
 
-    @pytest.mark.skipif(PY2, reason=("Catching warnings unreliable with "
-                                     "Python 2 (GH #20770)"))
+    #@pytest.mark.skipif(PY2, reason=("Catching warnings unreliable with "
+    #                                 "Python 2 (GH #20770)"))
     def test_dups_fancy_indexing(self):
 
         # GH 3455
diff --git a/pandas/tests/indexing/test_loc.py b/pandas/tests/indexing/test_loc.py
index 5fd3ab1..673307d 100644
--- a/pandas/tests/indexing/test_loc.py
+++ b/pandas/tests/indexing/test_loc.py
@@ -153,8 +153,8 @@ class TestLoc(Base):
                           [Timestamp('20130102'), Timestamp('20130103')],
                           typs=['ts'], axes=0)
 
-    @pytest.mark.skipif(PY2, reason=("Catching warnings unreliable with "
-                                     "Python 2 (GH #20770)"))
+    #@pytest.mark.skipif(PY2, reason=("Catching warnings unreliable with "
+    #                                 "Python 2 (GH #20770)"))
     def test_loc_getitem_label_list_with_missing(self):
         self.check_result('list lbl', 'loc', [0, 1, 2], 'indexer', [0, 1, 2],
                           typs=['empty'], fails=KeyError)
@@ -167,9 +167,10 @@ class TestLoc(Base):
             self.check_result('list lbl', 'loc', [3, 6, 7], 'ix', [3, 6, 7],
                               typs=['ints', 'uints', 'floats'],
                               axes=1, fails=KeyError)
-        self.check_result('list lbl', 'loc', [4, 8, 10], 'ix', [4, 8, 10],
-                          typs=['ints', 'uints', 'floats'],
-                          axes=2, fails=KeyError)
+        with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
+            self.check_result('list lbl', 'loc', [4, 8, 10], 'ix', [4, 8, 10],
+                            typs=['ints', 'uints', 'floats'],
+                            axes=2, fails=KeyError)
 
         # GH 17758 - MultiIndex and missing keys
         with tm.assert_produces_warning(FutureWarning, check_stacklevel=False):
```

But, then I also needed to add an extra assert_warning, and didn't look into the actual test (the `check_result` stuff is rather difficult to understand ..) if this is correct or not.
> But, then I also needed to add an extra assert_warning, and didn't look into the actual test (the check_result stuff is rather difficult to understand ..) if this is correct or not.

To conclude myself: yes the added `with tm.assert_produces_warning(..)` is correct as it is testing a list with missing labels. The fact that the assert was not there was not logical (and a bug in the testing code), and should actually been have investigated when the asserts were added to the test in the first place.

Anyhow, the full test is also a bit bogus. As it only checks that at least one FutureWarning is raised, but the `self.check_result` tests several things at the same time .. (but that is not for this PR to fix :-))
@jorisvandenbossche thanks, I should have applied your fix.
@jorisvandenbossche let me know if you see any obvious mistake in my implementation of your fix...
@jorisvandenbossche for sure your fix is good, but I'm afraid there's something deeper (also because your fix alone should in principle affect Python 2 and 3, and invocation of the entire test suite or of single files, in the same way)
Too bad :-) There might be other similar cases where warnings are catched and hidden in the testing code. 

It's a bit strange that both are failing. Locally it was only one of two that are failing. (and with your updated branch none, at least when only running the two files)

That said, I am totally fine with skipping those for now, given it is probably a bug in existing testing code.
> That said, I am totally fine with skipping those for now, given it is probably a bug in existing testing code.

Agree. So OK for 0.23.0?