Issue 28518 multiindex interesection #28735
Conversation
* test_lexsort_depth verify that lexsort_depth return the correct depth when sortorder is passed to the MultiIndex constructor * test_raise_invalid_sortorder test that the MultiIndex constructor raise when passing an incorrect sortorder * test_merge_multiindex_columns test the original issue
This fix issue pandas-dev#28518, where the label of the merge index where invalid due to inconsistent lexsort_depth property of the intersection of the indexes
pandas/core/indexes/multi.py
Outdated
| return self.nlevels | ||
| else: | ||
| return 0 | ||
| return int(self.sortorder) |
There was a problem hiding this comment.
Is there a reason for int(...) here?
There was a problem hiding this comment.
I took the line from the constructor, line 278, but if it is already cast in the constructor (maybe to handle passing False for sortorder), it may no be necessary here. I look if it passes the test without it.
| for n in numbers: | ||
| index_tuples.append([l, n]) | ||
|
|
||
| index = pd.MultiIndex.from_tuples(index_tuples, names=["outer", "inner"]) |
There was a problem hiding this comment.
Can you not just use from_product here instead to replace the two loops above?
| r_suf = "_y" | ||
| expected_labels = sum(([l + l_suf, l + r_suf] for l in letters), []) | ||
| merged_frame = frame_x.merge(frame_y, on="id", suffixes=((l_suf, r_suf))).columns | ||
| for label in expected_labels: |
There was a problem hiding this comment.
Instead of asserting within a loop would be easier if you built a result and expected frame and used tm.assert_frame_equal to compare. You will find examples of that in other tests
There was a problem hiding this comment.
Did'nt think about this way. It does make more sense.
There was a problem hiding this comment.
looks good. can you add a whatsnew note in 1.0, in the Reshaping section (also a note about validing sortorder in the MultiIndex bug fix section).
side note, do you see an actual usecase for sortorder in the constructor? I think we should deprecate this. (can you create an issue)
nrebena
force-pushed
the
issue_28518_multiindex_interesection
branch
from
e89ac18 to
716820e
Compare
pandas/core/indexes/multi.py
Outdated
|
|
||
| def _lexsort_depth(self): |
There was a problem hiding this comment.
right was wanting a
def _lexsort_depth(self) -> int: here
There was a problem hiding this comment.
Ok sorry i did'nt get the "hint".
Joke aside, is it related to the caching of the result that the function needs typing?
|
@jreback Changes are made and checks are green. 🙂 |
|
@jreback Green and ready for takeoff |
|
thanks @nrebena very nice! |
MultiIndex lexsort_depth now return sortorder if sortorder is not set to None
At first the error come from the intersection of the two indexes, that resulted in an unsorted index with an invalid lexsort_depth value. Then when looking at the lexsort_depth function I found the deeper error, where the returned lexsort_depth value were the number of level instead of the self.sortorder value.
After fixing that I implemented a check in the MultiIndex._verify_integrity function when a sortorder is given to raise if it no coherent with the lexsort_depth.
I added tests for the issue, for the lexsort_depth function, and fo raising error in the _verify_integrity function.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff