DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth #38701

MarcoGorelli · 2020-12-26T09:49:04Z

closes Bug: groupby with sort=False creates buggy MultiIndex #32259
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

The issue is that is_lexsorted only looks at whether the codes are sorted - the order of the codes doesn't necessarily reflect the sorting of the index values, e.g. if the index is created from a .groupby(sort=False) operation (see the linked issue for an example).

So, instead of is_lexsorted, users should use is_monotonic_increasing. There isn't an equivalent function to point them to for lexsort_depth, right?

MarcoGorelli · 2020-12-26T11:37:47Z

lexsort_depth also probably needs to be made private, although there's already a _lexsort_depth...will think about how to rename things

MarcoGorelli · 2020-12-26T13:11:09Z

pandas/core/indexes/multi.py

+        return self._get_lexsort_depth()
+
+    @cache_readonly
+    def _get_lexsort_depth(self):


there's already a _lexsort_depth function, so am replacing this with _get_lexsort_depth

umm that is extra confusing. pls don't do this

you can just move the .sortorder stuff to _lexsort_depth

Thanks for your review - the difficulty I'm running into is that in pandas/core/indexes/multi.py there's

if self.sortorder is not None: if self.sortorder > self._lexsort_depth(): raise ValueError( "Value for sortorder must be inferior or equal to actual " f"lexsort_depth: sortorder {self.sortorder} " f"with lexsort_depth {self._lexsort_depth()}" )

so I can't just move self.sortorder into self._lexsort_depth else this will never raise, as self._lexsort_depth() would always just return self.sortorder in this snippet.

For now I've factored out the part which is used in the above check into

def _codes_lexsort_depth(self) -> int: int64_codes = [ensure_int64(level_codes) for level_codes in self.codes] for k in range(self.nlevels, 0, -1): if libalgos.is_lexsorted(int64_codes[:k]): return k return 0

but I'd imagine this will be considered equally confusing. Any suggestions?

MarcoGorelli · 2020-12-26T13:11:23Z

pandas/core/indexes/multi.py

-        >>> pd.MultiIndex.from_arrays([['a', 'b', 'c'], ['d', 'e', 'f']]).is_lexsorted()
+        >>> pd.MultiIndex.from_arrays([['a', 'b'], ['d', 'e']])._is_lexsorted()
        True
-        >>> pd.MultiIndex.from_arrays([['a', 'b', 'c'], ['d', 'f', 'e']]).is_lexsorted()
+        >>> pd.MultiIndex.from_arrays([['a', 'b'], ['d', 'f']])._is_lexsorted()


making these a bit shorter so they fit on a single line

moink · 2020-12-26T14:12:22Z

Some of the CI failures caused by #38703

MarcoGorelli · 2020-12-26T14:56:07Z

Some of the CI failures caused by #38703

I haven't checked yet, but thanks for looking into it!

jreback · 2020-12-29T15:10:08Z

pandas/core/indexes/multi.py

+        return self._get_lexsort_depth()
+
+    @cache_readonly
+    def _get_lexsort_depth(self):


umm that is extra confusing. pls don't do this

jreback · 2020-12-29T15:10:36Z

pandas/core/indexes/multi.py

+        return self._get_lexsort_depth()
+
+    @cache_readonly
+    def _get_lexsort_depth(self):


you can just move the .sortorder stuff to _lexsort_depth

jreback · 2020-12-29T15:11:51Z

pandas/tests/frame/methods/test_sort_index.py

@@ -35,15 +35,15 @@ def test_sort_index_and_reconstruction_doc_example(self):
            ),
        )
        result = df.sort_index()
-        assert result.index.is_lexsorted()
+        assert result.index._is_lexsorted()


for the cases where these are the same as is_monotonic (vast majority), ok with removing the _is_lexsorted assert

jreback · 2020-12-29T15:12:41Z

pandas/core/indexes/multi.py


    @cache_readonly
    def lexsort_depth(self):
+        warnings.warn(


metion in the whatsnew as well

MarcoGorelli · 2021-01-01T18:04:52Z

doc/source/whatsnew/v0.20.0.rst

+         value
+   a aa      2
+   bb      1
+   b aa      4
+   bb      3


something went wrong with pasting the indents here, will fix it in the next commit

jreback · 2021-01-01T21:26:19Z

pandas/core/indexes/multi.py

+            return self.sortorder
+        return self._codes_lexsort_depth()
+
+    def _codes_lexsort_depth(self) -> int:


this is very confusing, why do you think you need yet another method here? we want to reduce the api

Yes, I totally agree - see this comment for why I thought it was necessary: #38701 (comment) :

the difficulty I'm running into is that in pandas/core/indexes/multi.py there's

if self.sortorder is not None: if self.sortorder > self._lexsort_depth(): raise ValueError( "Value for sortorder must be inferior or equal to actual " f"lexsort_depth: sortorder {self.sortorder} " f"with lexsort_depth {self._lexsort_depth()}" )

so I can't just move self.sortorder into self._lexsort_depth else this will never raise, as self._lexsort_depth() would always just return self.sortorder in this snippet.

Would it be less confusing if this was a module-level function? There must be some way of calling only this part of _lexsort_depth because it's used in _verify_integrity from pandas/core/indexes/multi.py :

pandas/pandas/core/indexes/multi.py

Lines 393 to 399 in d642b67

if self.sortorder is not None:

if self.sortorder > self._lexsort_depth():

raise ValueError(

"Value for sortorder must be inferior or equal to actual "

f"lexsort_depth: sortorder {self.sortorder} "

f"with lexsort_depth {self._lexsort_depth()}"

)

I've made it a module-level function, is this clearer?

jreback · 2021-01-01T21:26:56Z

pandas/core/indexes/multi.py

-            return self.sortorder
-
-        return self._lexsort_depth()
+        warnings.warn(


you don't need the cache on this one (as its already on _lexsort_depth) and this is now user facing

jreback

lgtm, ping on green.

jreback · 2021-01-03T17:54:44Z

pandas/core/indexes/multi.py

@@ -176,6 +176,15 @@ def new_meth(self_or_cls, *args, **kwargs):
    return new_meth


+def _lexsort_depth(codes: List[np.ndarray], nlevels: int) -> int:


can you move this to almost the end of the file, e.g. where the module level functions are

jreback · 2021-01-04T00:16:01Z

thanks @MarcoGorelli

…#38701)

jbrockmendel · 2022-08-10T18:04:57Z

pandas/core/indexes/multi.py

-
-        return self._lexsort_depth()
+        warnings.warn(
+            "MultiIndex.is_lexsorted is deprecated as a public function, "


@MarcoGorelli should the message here refer to lexsort_depth instead of is_lexsorted?

this warning isn't being caught in a bunch of tests; not sure why that isn't caught by the npdev build

yes, looks like a typo, thanks for catching - we probably want to always use match= when catching warning to help prevent this, i'll fix this now

this warning isn't being caught in a bunch of tests; not sure why that isn't caught by the npdev build

not sure what you mean here sorry

MarcoGorelli force-pushed the deprecate-lexsorted branch from b758956 to c9d1616 Compare December 26, 2020 09:51

deprecate multiindex.is_lexsorted

b5d157a

MarcoGorelli force-pushed the deprecate-lexsorted branch from c9d1616 to b5d157a Compare December 26, 2020 10:12

MarcoGorelli marked this pull request as draft December 26, 2020 11:37

deprecate public usage of MultiIndex.lexsort_depth too

8ca3b3d

MarcoGorelli changed the title ~~DEPR MultiIndex.is_lexsorted~~ DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth Dec 26, 2020

MarcoGorelli added 2 commits December 26, 2020 13:10

make get_lexsort_depth private

109c1ce

test lexsort_depth deprecation

238b568

MarcoGorelli commented Dec 26, 2020

View reviewed changes

MarcoGorelli added 2 commits December 26, 2020 13:18

typo

893ab7c

move tests

b2b1c36

MarcoGorelli marked this pull request as ready for review December 26, 2020 13:39

MarcoGorelli added Deprecate Functionality to remove in pandas MultiIndex labels Dec 26, 2020

jreback requested changes Dec 29, 2020

View reviewed changes

MarcoGorelli added 8 commits December 31, 2020 11:18

move sortorder check to _lexsort_depth

b8bcc5e

note lexsort_depth deprecation in whatsnew note

4ab8981

remove is_lexsorted assertion when it's the same as is_monotonic

72b0abc

fixup property usage in docs

a0a709c

stage unstaged file

be2a692

use as property

7d1d279

Merge remote-tracking branch 'upstream/master' into deprecate-lexsorted

b3c5144

skip sortorder

39d163f

MarcoGorelli marked this pull request as draft January 1, 2021 15:43

factor out _codes_lexsort_depth

6b9c698

MarcoGorelli marked this pull request as ready for review January 1, 2021 17:22

update old release notes

74d8c7e

MarcoGorelli commented Jan 1, 2021

View reviewed changes

jreback requested changes Jan 1, 2021

View reviewed changes

MarcoGorelli added 4 commits January 2, 2021 12:15

use module-level _lexsort_depth

3798cf5

add type hints

dfc779d

fixup indents in old whatsnew note

fdbb366

clarify docstring

fff7297

MarcoGorelli requested a review from jreback January 3, 2021 15:21

jreback added this to the 1.3 milestone Jan 3, 2021

jreback mentioned this pull request Jan 3, 2021

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

jreback approved these changes Jan 3, 2021

View reviewed changes

jreback requested changes Jan 3, 2021

View reviewed changes

move _lexsort_depth to after class definition

fe4aaa6

jreback approved these changes Jan 4, 2021

View reviewed changes

jreback merged commit 586b490 into pandas-dev:master Jan 4, 2021

jreback mentioned this pull request Jan 4, 2021

DOC: MultiIndex.is_lexsorted not linked #38953

Closed

MarcoGorelli deleted the deprecate-lexsorted branch January 5, 2021 08:49

kylekeppler mentioned this pull request Jan 5, 2021

DOC: remove is_lexsorted from MultiIndex docstring #38975

Merged

4 tasks

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021

DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth (pandas-dev…

5f08211

…#38701)

jbrockmendel reviewed Aug 10, 2022

View reviewed changes

lukemanley mentioned this pull request Oct 23, 2022

DEPR: enforce deprecation MultiIndex.is_lexsorted and MultiIndex.lexsort_depth #49260

Merged

1 task

ggold7046 mentioned this pull request Aug 10, 2023

Modified doc/make.py to run sphinx-build -b linkcheck #54265

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth #38701

DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth #38701

MarcoGorelli commented Dec 26, 2020 •

edited

Loading

MarcoGorelli commented Dec 26, 2020

MarcoGorelli Dec 26, 2020

jreback Dec 29, 2020

jreback Dec 29, 2020

MarcoGorelli Jan 1, 2021 •

edited

Loading

MarcoGorelli Dec 26, 2020

moink commented Dec 26, 2020

MarcoGorelli commented Dec 26, 2020

jreback Dec 29, 2020

jreback Dec 29, 2020

jreback Dec 29, 2020

jreback Dec 29, 2020

MarcoGorelli Jan 1, 2021

jreback Jan 1, 2021

MarcoGorelli Jan 2, 2021

MarcoGorelli Jan 2, 2021

jreback Jan 1, 2021

jreback left a comment

jreback Jan 3, 2021

jreback commented Jan 4, 2021

jbrockmendel Aug 10, 2022

MarcoGorelli Aug 10, 2022

	if self.sortorder is not None:
	if self.sortorder > self._lexsort_depth():
	raise ValueError(
	"Value for sortorder must be inferior or equal to actual "
	f"lexsort_depth: sortorder {self.sortorder} "
	f"with lexsort_depth {self._lexsort_depth()}"
	)

		@@ -176,6 +176,15 @@ def new_meth(self_or_cls, args, *kwargs):
		return new_meth


		def _lexsort_depth(codes: List[np.ndarray], nlevels: int) -> int:

DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth #38701

DEPR MultiIndex.is_lexsorted and MultiIndex.lexsort_depth #38701

Conversation

MarcoGorelli commented Dec 26, 2020 • edited Loading

MarcoGorelli commented Dec 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moink commented Dec 26, 2020

MarcoGorelli commented Dec 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli commented Dec 26, 2020 •

edited

Loading

MarcoGorelli Jan 1, 2021 •

edited

Loading