PERF: MultiIndex._shallow_copy

[topper-123]

• xref PERF: Index._shallow_copy doesn't copy ._engine #28584, PERF: copy cached attributes on index shallow_copy #32568, PERF: copy cached attributes on extension index shallow_copy #32640
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Improves performance of MultiIndex._shallow_copy. Example:

>>> n = 100_000
>>> df = pd.DataFrame({'a': range(n), 'b': range(1, n+1)})
>>> mi = pd.MultiIndex.from_frame(df)
>>> mi.is_lexsorted()
True
>>> mi.get_loc(mi[0])  # also sets up the cache
>>> %timeit mi._shallow_copy().get_loc(mi[0])
8.56 ms ± 127 µs per loop  # master
75.1 µs ± 2.3 µs per loop  # this PR, first commit
46.9 µs ± 792 ns per loop  # this PR, second commit

Also adds tests for _shallow_copy for all index types. This ensures that this issue has been resolved for all index types.

[topper-123]

the levels are actually not readonly, bacise each level's name attribute is writeable.

[WillAyd]

Hmm I thought these were readonly as of 1.0 though?

https://pandas.pydata.org/pandas-docs/version/1.0.0/whatsnew/v1.0.0.html#backwards-incompatible-api-changes

@TomAugspurger

[topper-123]

I actually don't mind treating it as immutable, it was just that the test using pandas/tests/indexes/multi/test_names.py::check_level_names failed.

CategoricalIndex and CategoricalDtype actually have the same issue already:

>>> dtype = pd.CategoricalDtype(['a', 'b', 'c'])
>>> dtype.categories.name = "x"  # dtypes a re immutable, this is not supposed to work
>>> dtype.categories
Index(['a', 'b', 'c'], dtype='object', name='x')

IMO this issue is a wart, but nothing serious, because MultiIndex.names are now seperate from the levels, so it doesn't interfere with anything and the same issue is the same as with CategoricalIndex.categories.name.

[TomAugspurger]

Can you check the history here? Did we deliberately make this cache_readonly to avoid a performance regression.

I also don't see why check_level_names would be failing when this is cache_readonly.

[topper-123]

Yeah it was changed in #31651 because of a performance regression.

I've made a new version that does a shallow_copy of _cache["levels"] also.

[topper-123]

Using cache_readonly is faster than property. I've updated the example to reflect that.

[jbrockmendel]

comment pointing back to this thread?

[topper-123]

ok

[topper-123]

Updated.

It was needlessly complex to do ops in the "levels" in the cache. Simpler to delete the "levels" key and just let it be recreated if/when the .levels attribute is accessed in the new shallow_copied index.

[jreback]

thanks @topper-123 very nice

[topper-123]

Yeah, this potentially makes copying thing around much cheaper. Will be interesting to see if there are some specific use cases where performances improve significantly.

[jacobaustin123]

This PR has introduced a small bug. I'm not sure how deep it goes, but I've encountered it in PeriodIndex objects inside a MultiIndex. Here is a minimal reproduction:

import pandas as pd
idx = pd.MultiIndex.from_arrays([pd.PeriodIndex([pd.Period("2019Q1"), pd.Period("2019Q2")], name='b')])
idx2 = pd.MultiIndex.from_arrays([idx._get_level_values(level) for level in range(idx.nlevels)])
print(all(x.is_monotonic for x in idx2.levels)) # raises an error

There's a simple fix for this, which involves changing pandas/core/indexes/period.py:260 to result._cache = {}. I don't understand the internals enough to know what is causing this issue or whether it extends farther than this case. The problem seems to be that the _cache caches the _engine attribute, and since we no longer clear the cache when copying the object using _simple_new, _engine is left in an invalid state. The error is caused by idx._simple_new()._engine.vgetter() returning None, which causes issues in is_monotonic, among other things.

[topper-123]

Thanks for the report, @jacobaustin123. This is a bit tricky as you mention, but is caused by the use of weakref(self) is PeriodIndex._engine, where the weakreffed self in this case is released too early.

This can be solved by deleting PeriodIndex._engine and falling back to the base Index._engine.

@jbrockmendel , I think you made the new Index._engine method including Index._get_engine_target. Do you agree that PeriodIndex._engine should just be deleted?

[jbrockmendel]

Do you agree that PeriodIndex._engine should just be deleted?

Eventually, yes. The reason why PeriodIndex._engine exists is because PeriodEngine needs to get the .freq attribute of the PeriodIndex. However, the PeriodEngine methods that actually use .freq are never actually reached, because the relevant PeriodIndex methods route around them, and it isn't obvious how intentional that is.

So it should be removed, but there is some untangling to do first.

[topper-123]

Ok. a fix to the current problem seems to be to replace weakref(self) with weakref(self._values) (haven't run the test suite yet, though). We could do that until the fix, I think.