Separate MultiIndex names from levels
State: closed

- [x] progress towards #27138 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

In #27138 I proposed doing some changes to ``MultiIndex``, so that the index type can have its data collected in ``_data`` as type ``List[Categorical]``,+ adding ``MultiIndex.arrays`` in order to access each full level as zero-copy ``Categorical``.

This is the first part of that proposal, and drops setting the names on the  ``levels[x].name`` attribute and instead sets the names on the ``MultiIndex._names`` attribute.

This PR is a minorly backward-breaking change (so would be good to get into 0.25), while the followup will not break anything.

Labels:
name: API Design
name: MultiIndex

Comments:
I’ve changed how the name is set. This is a bit slower (many checks that are not needed), but than could be fixed seperately in a later PR.
Is this ok? I'd like to get this in 0.25, as this is a breaking change. I'll add a 0.25 label as a reminder.

The rest of #27138 will be non-breaking, so can go in later, if needed.
I haven’t looked, but we shouldn’t merge breaking changes in the release candidate.


________________________________
From: Terji Petersen <notifications@github.com>
Sent: Monday, July 8, 2019 6:25 PM
To: pandas-dev/pandas
Cc: Subscribed
Subject: Re: [pandas-dev/pandas] Separate MultiIndex names from levels (#27242)


Is this ok? I'd like to get this in 0.25, as this is a breaking change. I'll add a 0.25 label as a reminder.

The rest of #27138<https://github.com/pandas-dev/pandas/issues/27138> will be non-breaking, so can go in later, if needed.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/27242?email_source=notifications&email_token=AAKAOIV7NXDLCTHW6VWPXTDP6PEINA5CNFSM4H6F75WKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZOUESY#issuecomment-509428299>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAKAOIX2TJDHTZTBXNWDJZDP6PEINANCNFSM4H6F75WA>.

Yeah, I understand that, but the followups to this PR will in addition to the benefits mentioned in #27138 also allow some nice simplifications of MultiIndex (by delegating all single-level checks to Categorical) and I assume release of 0.25 will mean a stop to breaking changes for a while,, because next up will be 1.0?

 
@TomAugspurger I don't believe we have held off on merging even breaking changes to an RC. I don't see this as a big deal and would merge as is.
I haven't had a chance to look (and won't this week), but if we're merging
API changes in RC0 then we'll need a second RC.

On Tue, Jul 9, 2019 at 3:50 PM Jeff Reback <notifications@github.com> wrote:

> @TomAugspurger <https://github.com/TomAugspurger> I don't believe we have
> held off on merging even breaking changes to an RC. I don't see this as a
> big deal and would merge as is.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27242?email_source=notifications&email_token=AAKAOITD5YHRMJ3JRADT4TDP6UB4FA5CNFSM4H6F75WKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZRUSAA#issuecomment-509823232>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIUYKYEC5G4OUG647ZLP6UB4FANCNFSM4H6F75WA>
> .
>

Ping.
I've moved the whatsnew section to 1.0.
@topper-123 lgtm (tiny comment). @TomAugspurger any objections?
Question on the broad goal of https://github.com/pandas-dev/pandas/issues/27138: IIUC, the motivation is for MultiIndex to be backed by a `_data: List[Categorical]`. I don't necessarily agree that that goal requires API breaking changes here.

Why can `.levels` not return something like

```python
@property
def levels(self):
    return FrozenList(pd.Index(self._data[i], name=self.names[i] for i in idx.nlevels))
```
Your example is not equivalent to ``levels``, as ``._data[i]`` will be a ``Categorical``. Rather each level should be equivalent to ``._data[i].categories``. It does not make sense that categories have a ``.name``. So it will be better in terms of having clear code paths, if we avoid having a ``.name`` in the levels.

Your example would probably work, but we'd always have to be mindful that levels are different than categories, and we shoul never use levels internally (because they'd be more expensive to construct than accessing the categories). And for that extra complexity we would't get any gain in return, so I think it's better to just seperate names and levels.

> Your example is not equivalent to levels, as ._data[i] will be a Categorical. Rather each level should be equivalent to ._data[i].categories.

You're right, my apologies. I think the broader point about `.levels` wrapping the source of truth `._data[I]` in an Index with a name stands.

> but we'd always have to be mindful that levels are different than categories, and we shoul never use levels internally

Isn't the plan to use `MutliIndex._data` internally rather than `MultiIndex.levels`?

> And for that extra complexity we would't get any gain in return, so I think it's better to just seperate names and levels.

But we don't break API, which IMO is worth it when the change *seems* unnecessary.
> Isn't the plan to use MutliIndex._data internally rather than MultiIndex.levels?

Yes, that's the plan. My point was that if ``name`` can be accessed through the levels, we'd have to be careful not to use levels in the future, because it's a list of indexes and indexes are expensive to construct compared to a pure access to ``_data[i].categories``.

Also, when constructing MultiIndex instances we'd have three places to check the names (``name``, ``names`` and in ``levels``) compared to two if we drop having names in levels.

> But we don't break API, which IMO is worth it when the change seems unnecessary.

That's a fair point, though I think this is a worthwhile simplification of the API.

This perhaps leads to a question if the API is the right one and if multiIndex should accept categories instead of levels, so concepts known from other places of pandas could be reused in MultiIndex.

> Also, when constructing MultiIndex instances we'd have three places to check the names (name, names and in levels) compared to two if we drop having names in levels.

That's the status quo right? If we want to deprecate passing names in one of those places then by all means let's have that conversation.

> This perhaps leads to a question if the API is the right one and if multiIndex should accept categories instead of levels, so concepts known from other places of pandas could be reused in MultiIndex.

Perhaps, I don't have an opinion at this point.
This makes sense to me - should we fix up merge conflict and keep this moving or are there outstanding objections?
Are there any remaining API changes here? I want to make sure https://github.com/pandas-dev/pandas/pull/27242#issuecomment-516076989 is addressed before merging.

It's been a while since I've looked, but IIRC we can achieve the desired refactor without needing to break API.
This is a simplification of the code base, as it gives better seperation of concern.

Also, MultiIndex gets more similar to other index types (and Series etc.), where the name is NOT part of the underlying data structure, as it currently is for MultiIndex. E.g. we in general have ``idx._data`` storing the data and ``idx.name`` (or ``._name``) storing the name and the name is (correctly) not stored in the data.

> This is a simplification of the code base, as it gives better seperation of concern.

I don't disagree, but is it an API change?
> I don't disagree, but is it an API change?

Do you refer to the text in the whatsnew? I would say it's an API change, as previously you could access names by ``midx.levels[1].name``, which no longer will work.

OTOH perhaps it's only me who has been practically aware of this and others have seen it as an implementation detail? It does not seem to be documented for example [in the docs](https://dev.pandas.io/user_guide/advanced.html#hierarchical-indexing-multiindex)
> OTOH perhaps it's only me who has been practically aware of this and others have seen it as an implementation detail?

No, it's a public attribute. I don't think we can call that an implementation detail. I know I've used it in the past.
So, do you agree with this code change overall and pull it in, @TomAugspurger?
I'm not too familiar with MultiIndex's internals, but it seems like a reasonable refactor. We just need to deprecate accessing `.levels[x].name` with something like https://github.com/pandas-dev/pandas/pull/27242#issuecomment-516076989
Oh, doing that will likely cause large slowdowns, as we will need to create new indexes for each new ``levels`` access, and index instantiation is very slow.

Can you measure the slowdown?

Presumably that can be cached to offset some of the slowdown, if it's actually a problem.
```python
>>>  x = ['a', 'b', 'c', 'd']
>>> %timeit pd.Index(x)
91.7 µs ± 2.72 µs
>>> %timeit pd.CategoricalIndex(x)
507 µs ± 9.98 µs
>>> %timeit pd.date_range(2010, periods=4)
238 µs ± 4.53 µs
```

So for a 4-level MultiIndex, probably costs a millisecond each time ``levels`` would be accessed which seems too much, given that this is index operations.
 
Architecturally, we'd also have to consider that ``midx.levels[2].name = 'abc'`` is currently possible, but would not work under your proposal (without adding complexity).

Is that a representative benchmark? We're not passing a list to the index constructor, we already have a valid index, we're just adding a name.

```python
In [53]: idx = pd.MultiIndex.from_product([['a', 'b', 'c'], [1, 2, 3]], names=['a', 'b'])

In [54]: %timeit idx.levels[0]
626 ns ± 19.4 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [55]: %timeit idx.levels[0]._shallow_copy(name='a')
4.98 µs ± 101 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
```

For user-facing code, I think that's acceptable. If it proves to be a slowdown internally, we can have a private `._levels` attribute that doesn't do the wrapping with the name. But I suspect it's not going to be a problem in practice.

> Architecturally, we'd also have to consider that midx.levels[2].name = 'abc' is currently possible, but would not work under your proposal (without adding complexity).

Is the additional complexity reflecting that update in MultiIndex.names? That doesn't seem too difficult, and I'm quite happy to deprecate this behavior in favor of updating names.
Ok, hadn't considered ``_shallow_copy``, though there will still be an extra step for instantiating the ``._engine``, which is O(N) for the size of the level, so will be noticable for large levels, but not small ones:

```python
>>> x = pd.CategoricalIndex(np.arange(1_000_000))
>>> %timeit x.get_loc(999_999)
21.7 µs ± 319 ns per loop
>>> %timeit x._shallow_copy().get_loc(999_999)
2.61 ms ± 31.7 µs per loop
```

> Is the additional complexity reflecting that update in MultiIndex.names? That doesn't seem too difficult, and I'm quite happy to deprecate this behavior in favor of updating names.

Not sure I understand you here. Do you mean drop the behaviour?
I mean we can deprecate setting of the names via the levels. 
But a level is just an index instance, so we can't deprecate setting the name on a level without doing the same for indexes. Are we miscommunicating?
No, I was likely just wrong about being able to deprecate setting `.levels[i].name` and have it be reflected in `MultiIndex.names`.

> Architecturally, we'd also have to consider that midx.levels[2].name = 'abc' is currently possible, but would not work under your proposal (without adding complexity).

Do you have a plan for how to achieve that? I'm curious how complex it is.