TST: add tests for take() on empty arrays
State: closed

Another noticed during geopandas testing: `ExtensionArray.take` needs to be able to handle the case where it is empty.

Added a example implementation for Decimal/JSONArray. 
And apparently, this was also failing for Categorical, so fixed that as well.



Labels:
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20582?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@da33359`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff            @@
##             master   #20582   +/-   ##
=========================================
  Coverage          ?   91.84%           
=========================================
  Files             ?      153           
  Lines             ?    49279           
  Branches          ?        0           
=========================================
  Hits              ?    45261           
  Misses            ?     4018           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.23% <ø> (?)` | |
| #single | `41.9% <ø> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20582?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/20582/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `84.14% <ø> (ø)` | |

------

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

Regarding the discussion whether this "indexer of all -1 in case of empty array"-check should be in `take_1d` or in the array's `take`. 
I agree ideally we should do this in `take_1d`. I just want to note that, currently, we *don't do any* bounds checking in `take_1d` (also not for eg too large indices). So it might be all methods calling `take_1d` already do this and we didn't add it to `take_1d` for performance reasons, there might be other reasons, .. I don't know. 

It's just a bit a bigger endeavour to fix that. Of course I can rather easily add this two-line check to `take_1d`, but then we get into a state where `take_1d` does only a partial incomplete bounds check. So ideally we should decide on how low or high level we see `take_1d` (or eg add another `take` method that is more strict in checking input, to not have to change existing internal code), and if we decide that `take_1d` is a high level function that should guarantee bounds checking, update all code that uses it.
Putting @jreback's comment here to not have it lost in a hidden inline comment:

> I already responsded. `take_1d` can certainly be used in an implementation of this. This is the general implementation of virtually all take behavior, so naturally a check for no indexers should happen there, rather than in each higher level implementation (e.g. EA). The point is that the way you are advocating is simply more complex / buggy and adds more code. Virtually all EA methods should use API's that pandas already has. Should these be public. Sure if possible. But for something like this we don't want to make the actual implementation public as it has certain guarantees. However I don't see any problem with an internal implementation (like EA) using it. it fact I would say they have / should use it. otherwise how else would categorical/DTI/II be implemented at all?

If you can point me to an answer that is not just "this should be in take_1d", I don't find it :)

I removed the changes in this PR regarding Categorical (and opened a separate issue for that: https://github.com/pandas-dev/pandas/issues/20664), so only adding tests for the ExtensionArray and the test implementations. 
So we can keep bigger `take_1d` changes for another PR.

`take_1d` is internally used in many cases where the `indexer` passed to `take_1d` is the result of eg `get_indexer`, so this ensures that the indexer is correct, and for those no out-of-bounds checks are needed.

You advocate for that ExtensionArrays should use pandas take implementation, but at the same time say that you don't want to make it public? That seems to contradict each other.

If we want to expose our `take` implementation to extension array authors (and maybe we are that ourselves as well), I would personally create a new `take` functions which just calls the existing `take_1d`, but additionally does first bounds checking. 
That way, we keep on using the faster `take_1d` functions internally where we know bounds checks are not needed, and we can make the new `take` method public for external usage as well.

@jreback I repeated some of my comments above about `take_1d` / fixing a `take` implementation itself so it can be used in extension arrays, in https://github.com/pandas-dev/pandas/issues/20640. 
So maybe we can continue the discussion on that topic there? And then here only further discuss the actual changes in this PR (I removed everything related to Categorical, I am only adding some generic `take` tests and fixing it for the test example arrays).



lgtm. though seems to be failing the numpy master build?
> though seems to be failing the numpy master build?

Seems to be unrelated clipboard failures
Added one more test (for `reindex`, which was the actual method where this difference in the `take` API surfaced).
Appveyor failure is unrelated and was succeeding before.