DOC: update DF.set_index
State: closed

Split off from #24697 by request of @jorisvandenbossche & @jreback 

I kept the change for the whatsnew of #22486, to at least not *emphasize* that there are now ambiguous list-likes available for `DataFrame.set_index` (which haven't seen a release yet and would be removed again by #24697), which would/will make moving forward on this a bit easier. @toobaz 


Labels:
name: Docs

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24762?src=pr&el=h1) Report
> Merging [#24762](https://codecov.io/gh/pandas-dev/pandas/pull/24762?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/453fa85a8b88ca22c7b878a3fcf97e068f11b6c4?src=pr&el=desc) will **decrease** coverage by `49.47%`.
> The diff coverage is `n/a`.

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

```diff
@@             Coverage Diff             @@
##           master   #24762       +/-   ##
===========================================
- Coverage   92.38%   42.91%   -49.48%     
===========================================
  Files         166      166               
  Lines       52363    52363               
===========================================
- Hits        48376    22471    -25905     
- Misses       3987    29892    +25905
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.91% <ø> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24762?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `35.74% <ø> (-61.19%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| ... and [124 more](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24762?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/24762?src=pr&el=footer). Last update [453fa85...faf8bcc](https://codecov.io/gh/pandas-dev/pandas/pull/24762?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/24762?src=pr&el=h1) Report
> Merging [#24762](https://codecov.io/gh/pandas-dev/pandas/pull/24762?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/512830b46e678a1c6bd86cc60f54d74ff75792db?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #24762      +/-   ##
==========================================
- Coverage   92.38%   92.38%   -0.01%     
==========================================
  Files         166      166              
  Lines       52379    52377       -2     
==========================================
- Hits        48392    48389       -3     
- Misses       3987     3988       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.81% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.92% <55.55%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24762?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.92% <100%> (-0.01%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24762/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.04% <0%> (-0.1%)` | :arrow_down: |

------

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

> @jreback: is this a change in current behavior? or just original text?

Before #22486, `df.set_index` only allowed explicitly enumerated types (keys, `Series`, `Index`, `MultiIndex`, `np.ndarray` and `list`). You required me to change that to `is_list_like` - I cautioned that this was changing behaviour (see [here](https://github.com/pandas-dev/pandas/pull/22486#discussion_r219704650)), but complied with your review.

Strictly speaking this was orthogonal to the goal of fixing #22484, which is likely part of the reason why other devs like @toobaz missed it and now [object](https://github.com/pandas-dev/pandas/pull/24697#issuecomment-453025682).

#24697 would rectify this, but since that is on hold, the least one can do (as noted in the OP) is to not *advertise* the addition of these ambiguous (and contested) list-likes. Hence, I'm changing the whatsnew-note added by #22486 to *only* reflect what it was supposed to fix: the three points from #22484.
@TomAugspurger can you have a look here, I think we may need to revert #22486 to avoid any changes
Sorry I can't keep this straight. What behavior changed? Something to do with `set_index` and tuples?
I thought there was *no* changes, but I guess there are in #22486. @h-vetinari can you give show what changed. There was a lot of back and forth on that PR. I don't think anything *should* have changed but it seems it did.
> @jreback: @h-vetinari can you give show what changed

For the sake of discussion let's assume we have a list `keys` and elements `col`

* before #22486: elif-chain of instance-checks for `col` (specifically: `MultiIndex, Index, Series, np.ndarray, list`), ending in `frame[col]` (hence `KeyErrors` for types that were not tested, see #22484)
* first approach for #22486 (no change in the allowed types, except for using the ABC versions, as you had asked)
```python
if (is_scalar(col) or isinstance(col, tuple)) and col in frame:
    # all good here
elif isinstance(col, (ABCSeries, ABCIndexClass, np.ndarray, list)):
    # all good here as well (ABCIndexClass includes MultiIndex)
else:
    # raise
```
* you required me to use `is_list_like` instead of instance-checks in [this thread](https://github.com/pandas-dev/pandas/pull/22486#discussion_r219704650). I warned about the tuple case and change in behaviour, but eventually wanted to move forward and complied.
* then I used `if is_list_like(col) and not isinstance(col, set)` to avoid sets (which would be broken from the start), which you also struck down, requiring me to implement #23065
* final form used in #22486, clearly mentioning the tuple-ambiguity, resp. how it is checked first as a key and then as a list-like.
```python
        for col in keys:
            if (is_scalar(col) or isinstance(col, tuple)) and col in self:
                # tuples can be both column keys or list-likes
                # if they are valid column keys, everything is fine
                continue
            elif is_scalar(col) and col not in self:
                # tuples that are not column keys are considered list-like,
                # not considered missing
                missing.append(col)
            elif (not is_list_like(col, allow_sets=False)
                  or getattr(col, 'ndim', 1) > 1):
                raise TypeError('The parameter "keys" may only contain a '
                                'combination of valid column keys and '
                                'one-dimensional list-likes')
```

The source of the back-and-forth in #22486 was your orthogonal review requirement. I warned about this and went to great length (i.e. #23065) to avoid introducing something fundamentally broken (i.e. sets), but you were crystal-clear about wanting to avoid (the pre-existing) instance checks. But ok, you're reviewing basically everything here - oversights and misunderstandings can happen.

To me, the right approach would be merging #24697, which is a [small change](https://github.com/pandas-dev/pandas/pull/24697#issuecomment-453917331), and also paves the way to (eventually) solve #24046 and #22225.
@h-vetinari do you have a short example that behaves differently between 0.23.4 and master?
 @TomAugspurger 

Let's start with: 
```python
>>> df = pd.DataFrame(np.arange(9).reshape((3, 3)), columns=['A', 'B', ('t', 'p', 'l')])
>>> df
   A  B  (t, p, l)
0  0  1          2
1  3  4          5
2  6  7          8
```

call | 0.23.4 | master | comment
----|-------|---------|----------
`df.set_index(['A', 'A'], drop=False)` | works | works |
`df.set_index(['A', 'A'], drop=True)` | `KeyError` | works | #22484
`df.set_index(['C', 'D', 'E'])` | `KeyError: 'C'`  (big stacktrace) | `KeyError: 'C', 'D', 'E'` | #22484
`df.set_index(frozenset('A'))` | cryptic `KeyError` | reasonable `TypeError` | #22484 (any input type outside of a handful is tested as a key in 0.23.4) 
`df.set_index(iter([1, 2, 3]))` | cryptic `KeyError` | sets `Index([1, 2 ,3])` | #22484; list-likes (of correct length) now pass
`df.set_index(('t', 'p', 'l'))` | sets index to column `('t', 'p', 'l')` | sets index to column `('t', 'p', 'l')` | here and below, it doesn't matter if tuple is wrapped in list
`df.set_index(('t', 'p', 'm'))` | `KeyError: ('t', 'p', 'm')`  | sets `Index(['t', 'p', 'm'])` | on master, tuples get tried first as key, then as list-likes

It's the tuples that are now ambiguous (although with well-defined precedence; length must match of course for the list-like case). #24697 would take the fixes of #22484, keep the master-behaviour for tuples, and solve the list-ambiguity:

call | 0.23.4 | #24697 | comment
----|-------|---------|----------
`df.set_index(['A', 'A'], drop=False)` | works | works |
`df.set_index(['A', 'A'], drop=True)` | `KeyError` | works | #22484
`df.set_index(['C', 'D', 'E'])` | `KeyError: 'C'`  (big stacktrace) | `KeyError: 'C', 'D', 'E'` | #22484
`df.set_index(frozenset('A'))` | cryptic `KeyError` | reasonable `ValueError` | #22484
`df.set_index(iter([1, 2, 3]))` | cryptic `KeyError` | reasonable `ValueError` | #22484
`df.set_index(('t', 'p', 'l'))` | sets index to column `('t', 'p', 'l')` | same | 
`df.set_index(('t', 'p', 'm'))` | `KeyError: ('t', 'p', 'm')`  | same | 
`df.set_index(['A', 'B', 'A'])` | sets to MultiIndex of columns `['A', 'B', 'A']` | same |
`df.set_index([['A', 'B', 'A']])` | sets to `Index(['A', 'B', 'A'])` | deprecated |
It's still a bit hard to follow, but I don't see anything in there that needs to be reverted.
> It's still a bit hard to follow, but I don't see anything in there that needs to be reverted.

I also didn't follow everything, but

```
In [3]: pd.DataFrame([[1, 2], [3, 4]]).set_index([('a', 'b')])
Out[3]: 
   0  1
a  1  2
b  3  4
```
while this used (until last release) to raise ``KeyError: ('a', 'b')``, which is the desired behavior because we don't want to encourage using ``tuples`` as list-likes.
> > @TomAugspurger: It's still a bit hard to follow, but I don't see anything in there that needs to be reverted.

> @toobaz: I also didn't follow everything

Not sure how I can make it easier. I gave single line examples with behaviour before/after. Another way to look at it is that:
* before #22486, the list items were applied as the list was unpacked (within an elif-chain based on type)
* with #22486, there's an inspection step for the (outer) list that comes first, and which raises in case columns are missing or the wrong types are used. That check does not fundamentally lead to the ambiguity of tuples (or lists) - this was due to the request to change from roughly `isinstance(col, (ABCIndexClass, ABCSeries, np.ndarray, list))` to `is_list_like(col, allow_sets=False)`.

I'd suggest to *keep* that inspection step, but *change* the requirements the list-elements have to satisfy.

I agree with you that there's nothing fundamentally broken (behaviour has well-defined rules, plus DFs are usually way longer than tuples), just that now there's more ambiguity instead of less (there's already `list_of_scalars` as keys vs `[list_of_scalars]` as an array; plus now `tuple_as_key` vs `tuple_as_array`). #24697 would get rid of both those ambiguities.
> > @toobaz: I also didn't follow everything
> 
> Not sure how I can make it easier.

Sorry, I should have more honestly written "I didn't have time to read everything". Will do it before the end of the week.
@h-vetinari ok so this looks like this change slipped thru. Pls update to switch back to the original inspection code: isinstance(col, (ABCIndexClass, ABCSeries, np.ndarray, list)) to is_list_like(col, allow_sets=False), and *only* this change.
> @jreback: @h-vetinari  ok so this looks like this change slipped thru. Pls update to switch  back to the original inspection code: isinstance(col, (ABCIndexClass,  ABCSeries, np.ndarray, list)) to is_list_like(col, allow_sets=False),  and only this change.

I pondered opening a new PR for this, but in any case, this would have to be coupled with doc changes, so why not do it here.

I removed the types that were added by #22486, and reinstated the instance-checks. This needs to come in two points though - first for the case that `Index/Series/np.ndarray` are passed bare (wrapping them in a list to be able to iterate over the container), and then when inspecting the container.

The changes in the test are just for removing the `iter/tuple` tests from the ones that aren't supposed to pass anymore, and move them down to the tests that should fail.



@jreback 
I added the requested changes. PTAL.
thanks @h-vetinari 
Glad we could fix this before the release. Thanks.