API: Index.take inconsistently handle fill_value
State: closed

- [x] closes #12631
- [x] tests added / passed
- [x] passes `git diff upstream/master | flake8 --diff`
- [x] whatsnew entry


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

Comments:
@sinhrks as an aside (maybe a new issue). I think we should remove `allow_fill` parameter it duplicates a `fill_value` of `not None`. But would have to check more carefully.

actually, forget what I said above. The `allow_fill=True` is meant as an automatic way of setting the fill value based on the data type (IOW, it automatically looks up the `_na_value`).

moving this comment to main:

Actually I think we need to either make both `Index.take`, and tseries/base take:

either: raise on any negative indexers except -1
or: make a new method to do this, maybe .take_with_fill and restore .take for compat with 'regular' take. That will raise on indices that are not >= -1. These are serving double duty here.

To be compat with `numpy`, how about:

##### `allow_fill=False` or `fill_value=None`

Allow negative indexer, raise `IndexError` if indexer exceeds the range.

```
np.take(np.array([1, 2, 3]), np.array([-2, -1]))
# array([2, 3])
```

##### `allow_fill=True` and `fill_value is not None`

Regard `-1` as NA, raise `ValueError` if any other negative values.

ok if we change the default to `allow_fill=False`, there are a LOT of `.take` (and `.take_nd`) calls in the code-base, so prob need to change a bit for this to work. I agree that is going to be much better to be explicit though. 

So `allow_fill=False and fill_value is None` -> regular taking, esentially `self.values.take(...)`
with `alow_fill=True or fill_value not None` -> -1 == NA's and outside of bounds is a `ValueError`.

@jreback Based on the current impl, I don't think we should change  the default to `allow_fill=False`. The current `DTI/TDI` logic is:
1. `allow_fill=False` **OR** `fill_value is None` -> regular taking, esentially self.values.take(...)
2.  `alow_fill=True` **AND** `fill_value is not None` -> -1 is filled by `pd.NaT`

Because current default is `allow_fill=True` and `fill_value=None` (first path), adding the above logic to all `Index` will not change the existing behavior.

@sinhrks hmm ok, though I think we should have validation though (e.g. if you try something like

```
In [4]: dt = pd.DatetimeIndex([pd.Timestamp('20130101'),pd.NaT,pd.Timestamp('20130103')])

In [5]: dt
Out[5]: DatetimeIndex(['2013-01-01', 'NaT', '2013-01-03'], dtype='datetime64[ns]', freq=None)

In [6]: dt.take([-2,-1,0])
Out[6]: DatetimeIndex(['NaT', '2013-01-03', '2013-01-01'], dtype='datetime64[ns]', freq=None)

In [7]: dt.take([-2,-1,0],allow_fill=True,fill_value=pd.NaT)
Out[7]: DatetimeIndex(['NaT', 'NaT', '2013-01-01'], dtype='datetime64[ns]', freq=None)

In [8]: dt = pd.date_range('20130101',periods=3)

In [9]: dt
Out[9]: DatetimeIndex(['2013-01-01', '2013-01-02', '2013-01-03'], dtype='datetime64[ns]', freq='D')

In [10]: dt.take([-2,-1,0],allow_fill=True,fill_value=pd.NaT)
Out[10]: DatetimeIndex(['2013-01-02', 'NaT', '2013-01-01'], dtype='datetime64[ns]', freq=None)

```

[10] is wrong, this is treating -2 as a positional taker, and -1 as a NA indicator.

so ok with leaving the API, just want to put in some validation tests so that if we are NA filling, then don't allow negative values (except -1)

Yes, added a logic to raise `ValueError` if NA filling and negative indexer less than -1 are specified.

Indexer contains outside of bounds raises `IndexError` (no change, added tests).

```
In [11]: idx
Out[11]: Float64Index([1.0, 2.0, 3.0, 4.0], dtype='float64')

# I think this shoudl give the same as [13]
In [12]: idx.take([2, -5], fill_value=True)
IndexError: index -5 is out of bounds for size 4

In [13]: idx.take([2, -4], fill_value=True)
ValueError: When allow_fill=True and fill_value is not None, all indices must be >= -1
```

@jreback You mean always raising `ValueError`? `IndexError` is to be compat with `numpy`.

```
np.take(np.array([1, 2]), 3)
# IndexError: index 3 is out of bounds for size 2
```

no when in filling mode it should raise ValueError with negative indices ; in 12, the -5 should be caught before the take is done (as its negative and not -1)

Thanks, understood. Fixed and added tests.

@sinhrks thanks! you are fixing some of these internal warts! keep em comin'