-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
MultiIndex: support isna, fixes #34019 #38558
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
Tested on top of 1.1.5, but seems there are additional failures on master. |
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.
Can you add test(s)? It's the first thing we look at when reviewing
Before putting in any additional time I'd like to know if the chosen approach is acceptable from a functional point of view, that is
is considered the correct approach. |
Well ofc it's completely up to you! Bear in mind that this will need to go through multiple reviews and you will likely get feedback faster if you provide concrete examples of the implementation. FWIW this seems not to break anything which is good. The one failure is an xfailed test which now doesn't raise:
if you remove the xfail the checks should be green. |
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 not a good soln here. rather defining nunique on a MI is better
In the cython class? |
no on MultiIndex |
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
closing as stale. if you want to continue, pls ping and can re-open. |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Note sure what to do with the (smoke) tests here. I suppose an explicit test comparing input and output would be good to have.