BUG: validate Index data is 1D + deprecate multi-dim indexing
State: closed

- [x] closes #13601
- [x] closes #27125
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

This changes the behavior of `idx[:, None]` to return an ndarray instead of an invalid Index, needed to keep matplotlib tests working.

See also #20285 which this does not entirely close.  That becomes pretty easy to address though once a decision is made on whether to treat `[[0, 1], [2, 3]]` like `[(0, 1), (2, 3)]` (the latter becomes a MultiIndex, the former currently becomes an invalid Index)

Labels:
name: Index
description: Related to the Index class or subclasses
name: Constructors
description: Series/DataFrame/Index/pd.array Constructors

Comments:
There is a feather test in which we do

```
         df.columns = (pd.MultiIndex.from_tuples([("a", 1), ("a", 2), ("b", 1)]),)
         self.check_error_on_write(df, ValueError)
```

and in this PR we are now raising on the `df.columns = ...` instead of getting through to fail on the next line.  Is there a different invalid index we can pass to restore this test?  cc @jorisvandenbossche 
> There is a feather test in which we do

@jbrockmendel that test setup looks bad. We're assigning a length-1 tuple whose only element is a MultiIndex to be the single key?

```python
In [7]: df = pd.DataFrame({"A": [1, 2, 3]})

In [8]: df.columns = pd.MultiIndex.from_tuples([("a", 1), ("a", 2), ("a", 3)]),

In [9]: df
Out[9]: ---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
~/sandbox/pandas/pandas/_libs/hashtable_class_helper.pxi in pandas._libs.hashtable.PyObjectHashTable.map_locations()
   1637             raise KeyError(key)
   1638
-> 1639     def map_locations(self, ndarray[object] values):
   1640         cdef:
   1641             Py_ssize_t i, n = len(values)

ValueError: Buffer has wrong number of dimensions (expected 1, got 3)
Exception ignored in: 'pandas._libs.index.IndexEngine._call_map_locations'
Traceback (most recent call last):
  File "pandas/_libs/hashtable_class_helper.pxi", line 1639, in pandas._libs.hashtable.PyObjectHashTable.map_locations
    def map_locations(self, ndarray[object] values):
ValueError: Buffer has wrong number of dimensions (expected 1, got 3)
```

I think we'd rather have a length-1 MultiIndex

```diff
diff --git a/pandas/tests/io/test_feather.py b/pandas/tests/io/test_feather.py
index e06f2c31a2..3500470035 100644
--- a/pandas/tests/io/test_feather.py
+++ b/pandas/tests/io/test_feather.py
@@ -136,7 +136,7 @@ class TestFeather:
 
         # column multi-index
         df.index = [0, 1, 2]
-        df.columns = (pd.MultiIndex.from_tuples([("a", 1), ("a", 2), ("b", 1)]),)
+        df.columns = pd.MultiIndex.from_tuples([("a", 1)])
         self.check_error_on_write(df, ValueError)
 
     def test_path_pathlib(self):
```
> I think we'd rather have a length-1 MultiIndex

That seems to do the trick, thanks.
ping, whatsnew added
>  This changes the behavior of idx[:, None] to return an ndarray instead of an invalid Index, needed to keep matplotlib tests working.

Do we have tests that ensure `index[2darray]` and `idx[:, None]` returns an ndarray? Did we decide on that as a desirable API, or is that still under discussion?

> Did we decide on that as a desirable API, or is that still under discussion?

It's definitely been decided on for DatetimeIndex, which does that ATM.  For the rest, I think this is #27837.
It seems like in https://github.com/pandas-dev/pandas/issues/27837 the preference of most was to deprecate indexing with 2-D indexers. I'm concerned about changing the behavior here to return an ndarray, only to deprecate it in the future.

I suppose that validating we don't have 2D data necessitates the change to `Index.__getitem__` to return an ndarray?
I guess we _could_ call simple_new from getitem, but I’d prefer to just
return ndarray.  Could also deprecate saying it will raise instead of
returning ndarray in the future.

On Thu, Jan 2, 2020 at 2:11 PM Tom Augspurger <notifications@github.com>
wrote:

> It seems like in #27837
> <https://github.com/pandas-dev/pandas/issues/27837> the preference of
> most was to deprecate indexing with 2-D indexers. I'm concerned about
> changing the behavior here to return an ndarray, only to deprecate it in
> the future.
>
> I suppose that validating we don't have 2D data necessitates the change to
> Index.__getitem__ to return an ndarray?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/30588?email_source=notifications&email_token=AB5UM6HJ66T5NLKXZDNLJ6TQ3ZRA7A5CNFSM4KBX27QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH7TDLA#issuecomment-570372524>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB5UM6D6ELAB6XUXV4N2JSDQ3ZRA7ANCNFSM4KBX27QA>
> .
>

That's also an option. @jreback @jorisvandenbossche thoughts on that?

I'm slightly OK with breaking API to return an ndarray rather than an Index here, since the behavior on master is so clearly broken. But I also don't want to establish `Index.__getitem__` returning an ndarray as a documented / supported behavior.
re-reading the original, I think we are all in favor of deprecate & raise. This PR now raises for construction with a 2D. I don't see any behavior change on getitem, so that *still* returns an ndarray?
> I don't see any behavior change on getitem, so that still returns an ndarray?

This _does_ change `Index.__getitem__` to return ndarray[ndim=2] instead of constructing an invalid Index.
> > I don't see any behavior change on getitem, so that still returns an ndarray?
> 
> This _does_ change `Index.__getitem__` to return ndarray[ndim=2] instead of constructing an invalid Index.

ok and tests are added for this? or changed ones?
> ok and tests are added for this? or changed ones?

Yes, in test_indexing tests are updated to check for the expected exception when indexing with non-1D.
I am personally fine with the 2D Index -> ndarray change in getitem now, if that makes it easier to fix the Index construction from 2D data bug (in https://github.com/pandas-dev/pandas/issues/27837#issuecomment-519826958, I also mentioned such a change probably has not too much impact, at least not for the matplotlib use case). 
But, since we agree we want to deprecate it, I would directly add such a DeprecationWarning as well while changing it.
DeprecationWarning or FutureWarning? I don't have a preference.

We'll need a doc note for the API change and the deprecation.
Let's start with a DeprecationWarning, as this is typically something that will be used by libraries I think. We have plenty of time to change it to FutureWarning before 2.0.
> Let's start with a DeprecationWarning, as this is typically something that will be used by libraries I think. We have plenty of time to change it to FutureWarning before 2.0.

sgtm
The deprecation is breaking some plotting tests that specifically assert that no warnings are issued.  Do we want to update the tests to expect this warning, or do we need to update the plotting code itself?

Also looks like Categorical isn't validating 1D inputs
I'm stuck without a working machine for a couple days, so will need someone to take the baton if we want this in for 1.0
> The deprecation is breaking some plotting tests that specifically assert that no warnings are issued. Do we want to update the tests to expect this warning, or do we need to update the plotting code itself?

I suppose the warnings are coming from matplotlib? Then yes, that is OK to happen (that's the point of the warning for downstream libraries) and the warnings can be ignored.

Actually, it seems they are coming from our own tests, so we can just fix them probably.
> so will need someone to take the baton if we want this in for 1.0

Is there something else left to do than fixing the warnings in the tests ?
> Is there something else left to do than fixing the warnings in the tests ?

The last round of edits changed the behavior of PeriodArray, Categorical, and IntervalArray `__getitem__`.  These need some combination of tests, whatnew, and deprecations.
This is probably a blocker for the RC, if we're doing it for 1.0 (which would be nice). I've tagged it for 1.0 if possible, but understood if we aren't able to get to it.
>This is probably a blocker for the RC, if we're doing it for 1.0 (which would be nice). I've tagged it for 1.0 if possible, but understood if we aren't able to get to it.

I've got my old laptop working well enough that I can probably get this done tomorrow morning.  Are you planning to cut the RC before then?
Planning to do the RC in ~5 hours.
@TomAugspurger I have to head out in about half an hour, which IIRC is about when you wanted to cut the RC.  Anything else to do here? (assuming Travis finished green)
I think we're good. Will let it sit until just before I tag the rc, in case @jreback has a chance to look. But I think we can do followups after the RC.
Taking a look
off topic: running the tests has started leaving behind a bunch of __gibberish__.pickle files.  is that just me?