Dispatch categorical Series ops to Categorical by jbrockmendel · Pull Request #19582 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Moving towards the One Implementation To Rule Them All, this has Series[timedelta64].__cmp__ ops wrap TimedeltaIndex.__cmp__ and Series[category].__cmp__ wrap Categorical.__cmp__
Fixes (and tests) incorrectly named results when comparing Series/Index objects.
Ref #19513
Hello @jbrockmendel! Thanks for updating the PR.
Cheers ! There are no PEP8 issues in this Pull Request. 🍻
Comment last updated on February 13, 2018 at 16:33 Hours UTC
WIP: Dispatch categorical Series ops to Categorical Dispatch categorical Series ops to Categorical
I suspect this would close an issue or 2 can you have a look.
| ('foo', 'bar', None), |
|---|
| ('baz', 'baz', 'baz')]) |
| def test_ser_cmp_result_names(self, names, op): |
| # datetime64 dtype |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did any of these raise / not work before?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these work at the moment. They currently retain the series's name.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if you are effectively enabling something, then pls add a whatsnew note
I didn't see any issues this resolves. #19513 needs some rulings on whether Categorical behavior should change to match CategoricalIndex or vice-versa.
I think #19513 should be fixed first. This will simplify.
I think #19513 should be fixed first. This will simplify.
OK. That will affect the edits here a bit but not a whole lot. Given I'd like to move forward with fixing comp_method_SERIES for the datetime64 case and dont want multiple PRs touching this code, my preference would be to merge this and follow up when #19513 is resolved.
OK. That will affect the edits here a bit but not a whole lot. Given I'd like to move forward with fixing comp_method_SERIES for the datetime64 case and dont want multiple PRs touching this code, my preference would be to merge this and follow up when #19513 is resolved.
no I think this will have to wait. moving around code just to move it around doesn't advance things. I would just rebase this after.
There are non-Categorical-specific parts of this I'd like to push through to avoid re-doing when implementing other dtypes (mainly datetime64). Plus this fixes+tests some currently-incorrect names. Would this be easier to finish off if I reverted some/all of the Categorical edits?
| ('foo', 'bar', None), |
|---|
| ('baz', 'baz', 'baz')]) |
| def test_ser_cmp_result_names(self, names, op): |
| # datetime64 dtype |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if you are effectively enabling something, then pls add a whatsnew note
This was referenced
Feb 15, 2018
thanks. pls circle back and see if you can simplify the categorical / CI logic in here (e.g. make consistent / uniform)
thanks. pls circle back and see if you can simplify the categorical / CI logic in here (e.g. make consistent / uniform)
Yah, this is definitely a goal, is largely dependent on the canonical behavior being clarified in #19513.
i would suggest making ci and categorical have a similar behavior is the first step
harisbal pushed a commit to harisbal/pandas that referenced this pull request