BUG: when Index is numeric and indexer is boolean (#16877)
State: closed

- [ X ] closes #16877 
- [ X ] tests added / passed
- [ X ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ X ] whatsnew entry

This warns for both tickets:
https://github.com/pandas-dev/pandas/issues/6189
https://github.com/pandas-dev/pandas/issues/16877

There's one particular flaw: non-unique indices never run get_indexer and deals with dropping in a way where it is difficult to inspect types. I think it should be OK to leave that one for now though.

One of the reasons I chose to warn instead of Error is that running some_numeric_index.difference([True, False]) should possibly still be allowed, while still warning the user that the difference they are getting may just be due to conversion from True to 1 (or 1.0) and False to 0 (or 0.0). This probably is rare enough that an Error might still be the best approach.

The questions remaining to me are:
Is this severe enough that an Error should be raised? What type in this case?
Is the message sufficient?

Thanks.

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

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=h1) Report
> Merging [#17343](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/473a7f3c186f6b0bfd9d3ce413fb627cf7a8f111?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/17343/graphs/tree.svg?height=150&width=650&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17343      +/-   ##
==========================================
- Coverage   91.03%   91.02%   -0.02%     
==========================================
  Files         162      162              
  Lines       49567    49569       +2     
==========================================
- Hits        45125    45118       -7     
- Misses       4442     4451       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.8% <100%> (ø)` | :arrow_up: |
| #single | `40.24% <50%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `95.88% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.72% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17343?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/17343?src=pr&el=footer). Last update [473a7f3...44ffaf1](https://codecov.io/gh/pandas-dev/pandas/pull/17343?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/17343?src=pr&el=h1) Report
> Merging [#17343](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/83c9205971bd173692286187024ae30aae9ffc39?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/17343/graphs/tree.svg?token=eZ4WkYLtcO&width=650&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17343      +/-   ##
==========================================
- Coverage   91.23%   91.21%   -0.02%     
==========================================
  Files         163      163              
  Lines       49808    49810       +2     
==========================================
- Hits        45442    45435       -7     
- Misses       4366     4375       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.01% <100%> (ø)` | :arrow_up: |
| #single | `40.32% <50%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.29% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17343?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.73% <0%> (-0.1%)` | :arrow_down: |

------

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

>  Is this severe enough that an Error should be raised? What type in this case?

I'm a bit in favour of raising an error (``KeyError``). Actually, I think the error shouldn't necessarily illustrate the dtype problem... just reporting "keys not found" would be enough.

But more in general, I wonder whether a ``BooleanIndex`` would be worth introducing, even just as an internal class. I at least didn't expect
``` python
In [2]: pd.MultiIndex.from_product([[1,2], [True, False]]).levels[1].dtype
Out[2]: dtype('O')
```
but that's probably just coherent with BooleanIndex not existing (``MultiIndex`` levels of Booleans make sense, while admittedly flat indexes of Booleans don't).

Anyway, I'm afraid the current proposal is missing the following:

```python
In [2]: pd.Series([1,2,3], index=[0, 1, False]).drop([False])
Out[2]: 
1    2
dtype: int64
```
We generally try to avoid going from no error message / warning to direct error message because for downstream users, that could be too drastic of a change.  A warning should suffice for now I would think.
> We generally try to avoid going from no error message / warning to direct error message because for downstream users, that could be too drastic of a change.

OK: let me just emphasize that in this specific case it's _very_ likely that anyone indexing a numeric index with boolean labels is doing something wrong/assuming a behaviour (e.g. in ``.drop()``) which is actually not supported.
i am ok in raising in this case
this is a clear error
@toobaz The example index [0, 1, False] is a duplicate index (0 and False are equivalent), which I described as not being covered by this PR. However, you are right that the PR does not cover the case with unique mixed-integer indices (e.g. [0, True, 2]). I will add a "self.holds_integer()" to the condition to cover this, since the function is already implemented. 

This would not cover "mixed-float" (e.g. [1.5, True]), which is 'mixed' inferred type in pandas but 'mixed' could also be ['ab', True] in which case it would be an incorrect warning. So I'll leave that one out.

Edit: I will only add a self.holds_integer() condition in case it's a warning. Since a user with [0, True, 2] index should be able to drop the True without an Error. Even if that user may drop the 0 by passing [False].
> I will add a "self.holds_integer()" to the condition to cover this, since the function is already implemented.

Actually, thinking to my example again I realize that you are right... but that this PR might not be the fix we need. My example should _not_ raise an error (neither a warning); however, it should give a different result (not dropping the 0). So I'm afraid the fix is a bit deeper (wherever booleans are automatically casted to integers).

That said, waiting for a better fix, your PR certainly won't harm.
Just for context:

```python
In [2]: pd.Series([1,2,3], index=[0, 1, 's']).loc[True]
Out[2]: 2

In [3]: pd.Series([1,2,3], index=[0, 1, True]).loc[True]
Out[3]: 
1       2
True    3
dtype: int64

In [4]: pd.Series([1,2,3], index=[0, 1, 2]).loc[True]
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
/home/nobackup/repo/pandas/pandas/core/indexes/base.py in get_loc(self, key, method, tolerance)
   2457             try:
-> 2458                 return self._engine.get_loc(key)
   2459             except KeyError:

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc (pandas/_libs/index.c:5272)()
    128 
--> 129     cpdef get_loc(self, object val):
    130         if is_definitely_invalid_key(val):

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc (pandas/_libs/index.c:5077)()
    147 
--> 148         self._check_type(val)
    149 

/home/nobackup/repo/pandas/pandas/_libs/index_class_helper.pxi in pandas._libs.index.Int64Engine._check_type (pandas/_libs/index.c:16899)()
    135         if util.is_bool_object(val):
--> 136             raise KeyError(val)
    137         elif util.is_float_object(val):

KeyError: True

During handling of the above exception, another exception occurred:

KeyError                                  Traceback (most recent call last)
/home/nobackup/repo/pandas/pandas/core/indexing.py in _get_label(self, label, axis)
    129             try:
--> 130                 return self.obj._xs(label, axis=axis)
    131             except:

/home/nobackup/repo/pandas/pandas/core/generic.py in xs(self, key, axis, level, drop_level)
   2242         else:
-> 2243             loc = self.index.get_loc(key)
   2244 

/home/nobackup/repo/pandas/pandas/core/indexes/base.py in get_loc(self, key, method, tolerance)
   2459             except KeyError:
-> 2460                 return self._engine.get_loc(self._maybe_cast_indexer(key))
   2461 

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc (pandas/_libs/index.c:5272)()
    128 
--> 129     cpdef get_loc(self, object val):
    130         if is_definitely_invalid_key(val):

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc (pandas/_libs/index.c:5077)()
    147 
--> 148         self._check_type(val)
    149 

/home/nobackup/repo/pandas/pandas/_libs/index_class_helper.pxi in pandas._libs.index.Int64Engine._check_type (pandas/_libs/index.c:16899)()
    135         if util.is_bool_object(val):
--> 136             raise KeyError(val)
    137         elif util.is_float_object(val):

KeyError: True

During handling of the above exception, another exception occurred:

KeyError                                  Traceback (most recent call last)
<ipython-input-4-b1a86c8195a6> in <module>()
----> 1 pd.Series([1,2,3], index=[0, 1, 2]).loc[True]

/home/nobackup/repo/pandas/pandas/core/indexing.py in __getitem__(self, key)
   1333         else:
   1334             key = com._apply_if_callable(key, self.obj)
-> 1335             return self._getitem_axis(key, axis=0)
   1336 
   1337     def _is_scalar_access(self, key):

/home/nobackup/repo/pandas/pandas/core/indexing.py in _getitem_axis(self, key, axis)
   1557         # fall thru to straight lookup
   1558         self._has_valid_type(key, axis)
-> 1559         return self._get_label(key, axis=axis)
   1560 
   1561 

/home/nobackup/repo/pandas/pandas/core/indexing.py in _get_label(self, label, axis)
    130                 return self.obj._xs(label, axis=axis)
    131             except:
--> 132                 return self.obj[label]
    133         elif isinstance(label, tuple) and isinstance(label[axis], slice):
    134             raise IndexingError('no slices here, handle elsewhere')

/home/nobackup/repo/pandas/pandas/core/series.py in __getitem__(self, key)
    617         key = com._apply_if_callable(key, self)
    618         try:
--> 619             result = self.index.get_value(self, key)
    620 
    621             if not is_scalar(result):

/home/nobackup/repo/pandas/pandas/core/indexes/base.py in get_value(self, series, key)
   2491         try:
   2492             return self._engine.get_value(s, k,
-> 2493                                           tz=getattr(series.dtype, 'tz', None))
   2494         except KeyError as e1:
   2495             if len(self) > 0 and self.inferred_type in ['integer', 'boolean']:

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_value (pandas/_libs/index.c:4396)()
     93         return val in self.mapping
     94 
---> 95     cpdef get_value(self, ndarray arr, object key, object tz=None):
     96         """
     97         arr : 1-dimensional ndarray

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_value (pandas/_libs/index.c:4079)()
    101             void* data_ptr
    102 
--> 103         loc = self.get_loc(key)
    104         if PySlice_Check(loc) or cnp.PyArray_Check(loc):
    105             return arr[loc]

/home/nobackup/repo/pandas/pandas/_libs/index.pyx in pandas._libs.index.IndexEngine.get_loc (pandas/_libs/index.c:5077)()
    146             return self._get_loc_duplicates(val)
    147 
--> 148         self._check_type(val)
    149 
    150         try:

/home/nobackup/repo/pandas/pandas/_libs/index_class_helper.pxi in pandas._libs.index.Int64Engine._check_type (pandas/_libs/index.c:16899)()
    134         hash(val)
    135         if util.is_bool_object(val):
--> 136             raise KeyError(val)
    137         elif util.is_float_object(val):
    138             raise KeyError(val)

KeyError: True
```
Hello @andrejonasson! Thanks for updating the PR.







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

##### Comment last updated on September 25, 2017 at 14:07 Hours UTC
@toobaz I agree. This PR can prevent cases like the issues referenced until the core of the problem is solved.

I have changed it to correctly return an array of repeated -1 when the Index is numeric and the indexer is boolean.
``` python
>>> a = pd.Index([0,1,2,3])
>>> a.drop([True,False])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/andredotj/Projects/pandas/pandas/pandas/core/indexes/base.py", line 3641, in drop
    labels[mask])
ValueError: labels [ True False] not contained in axis
>>> a.difference([True, False])
Int64Index([0, 1, 2, 3], dtype='int64')
```

New update let's the client functions handle the missing values as they see fit e.g. ```difference``` correctly does not remove ```[0, 1]```.   

For the example in #16877 
``` python
>>> df = pd.DataFrame( data = {
...                          'acol'  : np.arange(4),
...                          'bcol' :  2*np.arange(4)
...                         })
>>> df.drop(df.bcol > 2, axis=0, inplace=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/andredotj/Projects/pandas/pandas/pandas/core/generic.py", line 2402, in drop
    new_axis = axis.drop(labels, errors=errors)
  File "/home/andredotj/Projects/pandas/pandas/pandas/core/indexes/base.py", line 3641, in drop
    labels[mask])
ValueError: labels [False False  True  True] not contained in axis
```
It correctly raises when passed the boolean target.
Just wanted to say "shouldn't this rather return -1 than raising an error", but I see you already did that! 
This looks good to me.

> In [3]: pd.Series([1,2,3], index=[0, 1, True]).loc[True]
  Out[3]: 
  1       2
  True    3
  dtype: int64

@toobaz I am not sure we will ever be able to solve this. When you have an 'object' index, I think we can only rely on the python object's equality / hashing, and True and 1 *are* equal for Python (only their identity is different, but we don't rely on that)
Is there anything that should be amended in the updated PR? Also, do I just push again to get AppVeyor to pass or will it automatically rebuild at some point?

Thanks!

@andrejonasson : Don't worry about `AppVeyor`.  We'll take care of that for you.  I left a couple of comments on your PR though.
> @toobaz I am not sure we will ever be able to solve this. When you have an 'object' index, I think we can only rely on the python object's equality / hashing

Indeed, you're right... after all,

```python
In [1]: [True].index(1)
Out[1]: 0
```
Hi, thanks for the feedback everyone.

I removed `test_get_indexer_boolean_index_boolean_target` - since when I tried to make it fail (by returning wrong indexer for instance) it was already covered by other tests. If redundancy is desirable I'll add it back in.

I added a direct test to `pd.Series` `.drop()` since it uses the same underlying function as `pd.DataFrame`.
@jreback Is there anything else that needs editing? 

Also is there some way to make sure I don't get merge conflicts due to the whatsnew line I've added (keeps on being the reason for my conflicts)?
> Also is there some way to make sure I don't get merge conflicts due to the whatsnew line I've added

Really not much you can do here.  People will modify the `whatsnew` in other PR's, causing the merge conflicts to occur.  Just remain diligent to resolve them when they pop up.
can you rebase / update
There is a windows failure (int32 vs int64)
I restarted the failed build (compile error, that I think has been fixed on master). Ping on green.
@andrejonasson Thanks!