-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: MultiIndex.get_indexer with na/missing #48877
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
Conversation
pandas/core/indexes/base.py
Outdated
@@ -4028,6 +4028,11 @@ def get_indexer( | |||
target, method=method, limit=limit, tolerance=tolerance | |||
) | |||
|
|||
if self._is_multi and self._contains_na_any_level: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
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). |
There was a problem hiding this 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
There was a problem hiding this 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
doc/source/whatsnew/v2.0.0.rst
file if fixing a bug or adding a new feature.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.