DOC/TST: Indexing with NA raises
State: closed

xref https://github.com/pandas-dev/pandas/issues/29556, https://github.com/pandas-dev/pandas/issues/28778

We're already doing the right thing on master. This just documents that behavior, and adds a handful of tests.

I'm not sure if there are existing tests I should be parameterizing. I only found a couple in `tests/indexing/`, which I parametrized over bool and boolean dtype. Will add more if I've missed any. In the meantime, I've written new tests.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
> but not yet for the EAs itself:

Good point. I think I'll add a base test for this, since all EAs should probably be able to handle it. Or is that too opinionated?

> (we should already have some utility somewhere, since it is being done in the indexing machinery)

It's `pandas.core.indexing.check_bool_indexer`, which converts the array-like to a boolean ndarray. I can make public somehow, if we want 3rd party EAs to handle these...


> I think I'll add a base test for this, since all EAs should probably be able to handle it. Or is that too opinionated?

It will probably give failures for external projects that implement EAs .. But maybe that is good, as indeed ideally they would handle this.

>  I can make public somehow, if we want 3rd party EAs to handle these...

That sounds as a good idea to me!
>>  I can make public somehow, if we want 3rd party EAs to handle these...

> That sounds as a good idea to me!

Do people have an initial preference on where to put this? I was thinking a new module in `pandas.api`, perhaps `pandas.api.indexing`. OTOH, this is mostly going to be used by EAs, and we already have `take` in `pandas.api.extensions`, so perhaps we put it with `take`?
I would put it like `take` in api.extensions
Added a couple base tests for `ExtensionArray.__getitem__(booleanarray)`, and updated our EAs to pass. 
Fixed the mypy issues.

On performance, a BooleanArray takes about about 1.50x as long as an ndarray for a 10,000 element array

```python
In [5]: s = pd.Series(np.arange(10000))
   ...:
   ...: m1 = np.zeros(len(s), dtype="bool")
   ...: m2 = pd.array(m1, dtype="boolean")

In [6]: %timeit s[m1]
270 µs ± 10.9 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [7]: %timeit s[m2]
391 µs ± 30.2 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [8]: 391/270
Out[8]: 1.4481481481481482
```

I suspect this is do to the additional `.isna().any()` check we need for BooleanArray, though there may be an unnecessary copy.

Actually... we're doing *two* `isna().any()` checks, one in `is_bool_indexer` and one in `check_bool_array_indexer`. I didn't realize that `is_bool_indexer` also converted to an ndarray and checked for missing values. I'm guessing we'll want to combine those checks.
Edit: reverted this for now. Will investigate more later, but probably out of scope for this PR.

Pushed a perf / bugfix update, will see if it breaks things. I restructured `is_bool_indexer` to not do NA checking. But as I write this, I wonder why we even have `is_bool_indexer`? I'd rather just use `is_bool_dtype` and `check_bool_indexer`. Will look into that now.

Anyway, that turned up a bug in `DatetimeArray.__getitem__` which has been fixed and documented.

I'm probably going to stop looking at perf things now, but there's likely more to be done. We're still about 1.3x as long as ndarray, but only 8% of the time is spent in `check_array` doing the `isna().any()`, which should be the extent of the overhead. We're leaving a lot on the table I think: https://github.com/pandas-dev/pandas/issues/30349
All green.
Planning to merge this later today if there aren't any objections.
Those utilities are useful for anybody implementing an ExtensionArray and who wants proper boolean indexing. Once this PR is merged, the geopandas tests will start failing, and to fix those, I can use those utilities.
I’m a bit concerned about the implementation of those. I think we can do the check and conversion with only one pass over the data (right now we do two). Adding them to the public api will possibly make changing them a bit harder. Not sure. 
I've removed the indexing helpers from `api.extensions`. I think for now 3rd party EAs will be on their own. We can revisit that, but I think it's the right move for now.
(not a blocker) there's been a discussion about implementing an ExtensionIndex for Indexes backed by EAs.  what happens when we have an Index backed by IntegerArray with NAs?
> (not a blocker) there's been a discussion about implementing an ExtensionIndex for Indexes backed by EAs. what happens when we have an Index backed by IntegerArray with NAs?

not sure what you are asking.
this is exactly what an EAI would hold
> not sure what you are asking.

I think I saw how i got mixed up.  never mind.
I am personally -1 on releasing 1.0 without a helper function. IMO it would be a bit stupid to ask of each external EA implementor to implement this logic themselves. 
Taking a step back: Tom, you say you are a bit concerned about the implementation, and the fact that it is now a two-step function (that needs to do potentially two passes over the values). Adding it to the public API might indeed make it more difficult to change this.
Do you have an idea how such a 1-pass API would look like? (we can also try to implement a public function right now still in terms of the two steps, but don't expose it as two separate steps; if we have an idea about the ideal API).

---

So the two steps right now are: 1) check if it is a boolean indexer and then 2) check the validity of the boolean indexer (the length, the nulls). 

So you get a pattern like this:

```
        ...
        elif com.is_bool_indexer(item):
            item = check_bool_array_indexer(self, item)
        ...
```

This can probably be done in a single function, although this single function would only need to raise if it is a boolean indexer, and otherwise pass through the non-boolean indexer (so personally I am not sure a single function would make this easier to read and follow the logic ..). 

Now what is the cost of the two functions? The two passes through the data, where does this happen? I am actually not fully sure: currently, we already check for missing values in `is_bool_indexer` and not in `check_bool_array_indexer`, although in  `check_bool_array_indexer` we do it implicitly again in `asarray(.., dtype=bool)`, as this will raise if there are missing values. 
Can we skip the null checking in `is_bool_indexer` and only do this in `check_bool_array_indexer` ?




> Can we skip the null checking in is_bool_indexer and only do this in check_bool_array_indexer ?

That’s my hope. I don’t know how much work that is off hand. But right now we’re doing the NA checking twice when we should only need to do it once. Will spend some time today or tomorrow on it. 
So I think it’ll still be two functions, one to check the dtype, and a second to check for NA and convert to ndarray. 
The reason that the dtype check is complicated, and not simply the already exposed `is_bool_dtype`, is because we also want to allow object dtype arrays with True/False values, right? 
We should maybe consider deprecating that in the future (boolean indexing with object dtype indexer), so as external EA author, I don't mind not implementing support for object dtype. In such a case, I could use `pd.api.types.is_bool_dtype` for the type check, which would avoid the need to expose a new `is_bool_indexer`, only needing the `check_bool_array_indexer`
@jorisvandenbossche can you take a look at https://github.com/pandas-dev/pandas/pull/30308/commits/21fd589d5003578340314e884b4cdcbbb0d6b71d? That's updated `DecimalArray.__getitem__` to handle boolean masks, and I think is representative of what 3rd party arrays would need to do.

If we're OK with that, I'll add `check_bool_array_indexer` to the public API. I'm not worried about its implementation changing. I'm still not sure where it should go though, but probably `api.extensions`.
we have api.indexers now -
seems like a good place
Yeah, just saw that. It seems reasonable.
All green.
very nice @TomAugspurger 

lots of testing!
While implementing support for this in GeoPandas, I ran into an issue with your example implementation for DecimalArray, basically being:

```
        def __getitem__(self, item):
            ....
            # array, slice.
            if pd.api.types.is_list_like(item):
                if not pd.api.types.is_array_like(item):
                    item = pd.array(item)
                dtype = item.dtype
                if pd.api.types.is_bool_dtype(dtype):
                    item = check_bool_array_indexer(self, item)
            return type(self)(self._data[item])
```

The problem with the above is that this also converts integers to IntegerArray, which is then not supported in the numpy indexing call (not fully sure why this doesn't come up in the decimal array's tests). 
Opened https://github.com/pandas-dev/pandas/issues/30738 for this about indexing with IntegerArray in general.

That issue is maybe not a blocker for 1.0 (it also doesn't work on 0.25, and I can workaround this in GeoPandas), but we should think a moment if this might change how we want to expose an array indexer check function.
> not fully sure why this doesn't come up in the decimal array's tests).

Because you added a fix for IntegerArray in DecimalArray.getitem ;)