Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: MultiIndex.get_indexer with na/missing #48877

Merged
merged 9 commits into from
Nov 9, 2022

Conversation

lukemanley
Copy link
Member

@lukemanley lukemanley commented Sep 30, 2022

Update: #49442 has been merged to fix the underlying bug here. This PR has been updated to add tests for closing the linked issues which are all related to the same underlying bug in #49442.

@lukemanley lukemanley added Bug Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate MultiIndex labels Sep 30, 2022
@@ -4028,6 +4028,11 @@ def get_indexer(
target, method=method, limit=limit, tolerance=tolerance
)

if self._is_multi and self._contains_na_any_level:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id prefer fixing this on the engine level and not special casing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i'll take another look

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably non trivial, not sure what's wrong there

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not seeing an obvious way to fix this on the engine level. If you look a few lines above this, categorical is already special cased to handle nans in a similar way. Are you ok with special casing for now?

@phofl
Copy link
Member

phofl commented Nov 7, 2022

Could you rebase? Hopefully, all tests should pass now. Also, would be good to add the issues to my whatsnew note. wdyt?

@lukemanley lukemanley changed the title BUG: MultiIndex.get_indexer with na/missing TST: MultiIndex.get_indexer with na/missing Nov 8, 2022
@lukemanley
Copy link
Member Author

Could you rebase? Hopefully, all tests should pass now. Also, would be good to add the issues to my whatsnew note. wdyt?

Updated this PR to add tests only and close linked issues. If you think any of these tests are redundant, I'm happy to remove some (all reported for the same underlying bug).

@mroeschke mroeschke added this to the 2.0 milestone Nov 9, 2022
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Merge when ready @phofl

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, I like the tests, since there is now guarantee that the underlying implementation won’t change in the future. So covering our bases should be good

@phofl phofl merged commit 8f869f3 into pandas-dev:main Nov 9, 2022
@lukemanley lukemanley deleted the multiindex-with-na branch November 10, 2022 03:18
codamuse pushed a commit to codamuse/pandas that referenced this pull request Nov 12, 2022
mliu08 pushed a commit to mliu08/pandas that referenced this pull request Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment