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

Dispatch categorical Series ops to Categorical #19582

Merged
merged 5 commits into from Feb 18, 2018

Conversation

Projects
None yet
3 participants
@jbrockmendel
Member

jbrockmendel commented Feb 7, 2018

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

@pep8speaks

This comment has been minimized.

pep8speaks commented Feb 7, 2018

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

@jbrockmendel jbrockmendel changed the title from WIP: Dispatch categorical Series ops to Categorical to Dispatch categorical Series ops to Categorical Feb 7, 2018

@codecov

This comment has been minimized.

codecov bot commented Feb 7, 2018

Codecov Report

Merging #19582 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19582      +/-   ##
==========================================
- Coverage   91.62%   91.61%   -0.02%     
==========================================
  Files         150      150              
  Lines       48773    48872      +99     
==========================================
+ Hits        44687    44772      +85     
- Misses       4086     4100      +14
Flag Coverage Δ
#multiple 89.98% <100%> (-0.01%) ⬇️
#single 41.75% <60%> (+0.01%) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/category.py 97.31% <100%> (-0.21%) ⬇️
pandas/core/ops.py 96.84% <100%> (+1.62%) ⬆️
pandas/core/arrays/categorical.py 94.9% <100%> (+0.16%) ⬆️
pandas/io/s3.py 72.72% <0%> (-13.64%) ⬇️
pandas/io/packers.py 87.31% <0%> (-1.34%) ⬇️
pandas/io/json/json.py 92.23% <0%> (-0.37%) ⬇️
pandas/io/common.py 68.77% <0%> (-0.3%) ⬇️
pandas/io/parquet.py 71.79% <0%> (-0.28%) ⬇️
pandas/core/frame.py 97.16% <0%> (-0.27%) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5052842...06bd5bb. Read the comment docs.

@jreback

This comment has been minimized.

Contributor

jreback commented Feb 8, 2018

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

This comment has been minimized.

@jreback

jreback Feb 8, 2018

Contributor

did any of these raise / not work before?

This comment has been minimized.

@jbrockmendel

jbrockmendel Feb 8, 2018

Member

None of these work at the moment. They currently retain the series's name.

This comment has been minimized.

@jreback

jreback Feb 13, 2018

Contributor

so if you are effectively enabling something, then pls add a whatsnew note

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Feb 8, 2018

I didn't see any issues this resolves. #19513 needs some rulings on whether Categorical behavior should change to match CategoricalIndex or vice-versa.

@jreback

This comment has been minimized.

Contributor

jreback commented Feb 10, 2018

I think #19513 should be fixed first. This will simplify.

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Feb 10, 2018

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.

@jreback

This comment has been minimized.

Contributor

jreback commented Feb 10, 2018

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.

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Feb 12, 2018

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

This comment has been minimized.

@jreback

jreback Feb 13, 2018

Contributor

so if you are effectively enabling something, then pls add a whatsnew note

@jreback jreback merged commit c4770c7 into pandas-dev:master Feb 18, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Feb 18, 2018

thanks. pls circle back and see if you can simplify the categorical / CI logic in here (e.g. make consistent / uniform)

@jbrockmendel

This comment has been minimized.

Member

jbrockmendel commented Feb 18, 2018

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.

@jbrockmendel jbrockmendel deleted the jbrockmendel:ops-cats branch Feb 18, 2018

@jreback

This comment has been minimized.

Contributor

jreback commented Feb 18, 2018

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 Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment