Fix: proper return types for MultiIndex.swaplevel & MultiIndex.union #1437
Conversation
|
Thank you for the feedback! I will work on that shortly. |
|
I've tried my best to spend a bit of time unpacking this. Ultimately, I didn't think it was best to include all types of Index in the overload because not all types of Index are valid to union with MultiIndex (only those with tuples). Referring to tuples within Indexes wasn't supported so I had to add that support in Happy to adjust recognising that Dr-Irv never mentioned any issue with using the complete Index type. Additionally, I've attempted to add a test in |
|
I can see now that all checks have failed, looks related to my extension of S1 (in adding support for |
There was a problem hiding this comment.
- You can mark the PR as a draft if you plan to work on it for a while.
- Just click
Re-request reviewon the top-right corner of the PR when you are done.
- Just click
- Please run
pre-commitlocally to avoid simple mistakes - Please use
poeto do further tests locally, e.g.poe test_all
|
Ended up removing MultiIndex-Index unions from assumption for returning a MultiIndex - was causing too many problems and Dr-Irv said it was okay to ignore. It was either violating the overload standard or confusing mypy in testing. swaplevel tests added, MultiIndex tests removed some cases relating to different types of Indexes (since we no longer try checking for those), and S1 changes reverted. |
|
I'm not sure why it failed. I ran Running the test case locally individually it failed, seems to be a runtime error because Pandas won't accept an Index unionising a MultiIndex (as it should). Removing that test case. |
pandas-stubs/core/indexes/multi.pyi
Outdated
| @overload | ||
| def union(self, other: Index | list[HashableT], sort: bool | None = ...,) -> Index: ... |
There was a problem hiding this comment.
If MultiIndex.union does not produce an Index, please remove this overload
| | datetime.datetime # includes pd.Timestamp | ||
| | datetime.timedelta # includes pd.Timedelta | ||
| ) | ||
|
|
There was a problem hiding this comment.
This extra empty line can also be removed
My mistake. I had ran a pre-commit after committing, and didn't commit the refactoring it made. |
| self, | ||
| other: Self | Index | list[HashableT], | ||
| sort: bool | None = ..., |
There was a problem hiding this comment.
I suppose Index should be removed. It is not tested, either.
| self, | |
| other: Self | Index | list[HashableT], | |
| sort: bool | None = ..., | |
| self, other: list[HashableT] | Self, sort: bool | None = ... |
| | datetime.datetime # includes pd.Timestamp | ||
| | datetime.timedelta # includes pd.Timedelta | ||
| ) | ||
|
|
| def test_multiindex_swaplevel_rettype() -> None: | ||
| """Test that union returns MultiIndex on MultiIndex input and swaplevel returns Self""" | ||
| mi = pd.MultiIndex.from_product([["a", "b"], [1, 2]], names=["let", "num"]) | ||
| mi2 = pd.MultiIndex.from_product([["a", "b"], [3, 4]], names=["let", "num"]) | ||
|
|
||
| check( | ||
| assert_type(mi.swaplevel(0, 1), "pd.MultiIndex"), | ||
| pd.MultiIndex, | ||
| ) | ||
| check( | ||
| assert_type(mi.union(mi2), "pd.MultiIndex"), | ||
| pd.MultiIndex, | ||
| ) | ||
| check( | ||
| assert_type(mi.union([("c", 3), ("d", 4)]), "pd.MultiIndex"), | ||
| pd.MultiIndex, | ||
| ) |
There was a problem hiding this comment.
Please make two tests, since swaplevel and union are two independent functionalities:
| def test_multiindex_swaplevel_rettype() -> None: | |
| """Test that union returns MultiIndex on MultiIndex input and swaplevel returns Self""" | |
| mi = pd.MultiIndex.from_product([["a", "b"], [1, 2]], names=["let", "num"]) | |
| mi2 = pd.MultiIndex.from_product([["a", "b"], [3, 4]], names=["let", "num"]) | |
| check( | |
| assert_type(mi.swaplevel(0, 1), "pd.MultiIndex"), | |
| pd.MultiIndex, | |
| ) | |
| check( | |
| assert_type(mi.union(mi2), "pd.MultiIndex"), | |
| pd.MultiIndex, | |
| ) | |
| check( | |
| assert_type(mi.union([("c", 3), ("d", 4)]), "pd.MultiIndex"), | |
| pd.MultiIndex, | |
| ) | |
| def test_multiindex_union() -> None: | |
| """Test that MultiIndex.union returns MultiIndex""" | |
| mi = pd.MultiIndex.from_product([["a", "b"], [1, 2]], names=["let", "num"]) | |
| mi2 = pd.MultiIndex.from_product([["a", "b"], [3, 4]], names=["let", "num"]) | |
| check( | |
| assert_type(mi.union(mi2), "pd.MultiIndex"), pd.MultiIndex | |
| ) | |
| check( | |
| assert_type(mi.union([("c", 3), ("d", 4)]), "pd.MultiIndex"), pd.MultiIndex | |
| ) | |
| def test_multiindex_swaplevel() -> None: | |
| """Test that MultiIndex.swaplevel returns MultiIndex""" | |
| mi = pd.MultiIndex.from_product([["a", "b"], [1, 2]], names=["let", "num"]) | |
| check( | |
| assert_type(mi.swaplevel(0, 1), "pd.MultiIndex"), pd.MultiIndex | |
| ) |
3 participants
This is my first contribution so I would appreciate your guidance. I think I have fixed the issue, but I'm confused on where to add test(s), or if I need to at all. Would it be acceptable to use Dr-Irv's reproduction script, or perhaps a modified copy?