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

ENH: Implement subtraction for object-dtype Index #21981

Merged
merged 4 commits into from
Jul 23, 2018

Conversation

jbrockmendel
Copy link
Member

@jreback @jorisvandenbossche discussed briefly at the sprint. Merits more thorough testing, but I'd like to get the go-ahead to separate out arithmetic tests that are common to EA/Index/Series/Frame[1col] that are highly duplicative first.

@gfyoung gfyoung added Enhancement Indexing Related to indexing on series/frames, not to indexes themselves labels Jul 20, 2018

result = index - pd.Index([Decimal(1), Decimal(1)])
tm.assert_index_equal(result, expected)

Copy link
Member

Choose a reason for hiding this comment

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

Can we also add tests where element-wise subtraction should fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are already some in the test just above this, but more couldn’t hurt. Will do.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, thanks for pointing that out. Two things would be good there:

  • We should rename that test above to be test_sub_fail
  • Add a couple of tests where element-wise subtraction should fail

Copy link
Member Author

Choose a reason for hiding this comment

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

You got it.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Yes, I am +1 on this.

Regarding testing, I think you should only add rsub case to the test?

@jorisvandenbossche jorisvandenbossche changed the title [ENH] Implement subtraction for object-dtype Index ENH: Implement subtraction for object-dtype Index Jul 20, 2018
@jorisvandenbossche
Copy link
Member

but I'd like to get the go-ahead to separate out arithmetic tests that are common to EA/Index/Series/Frame[1col] that are highly duplicative first.

You will need to give some more details, but let's do such a change in a separate PR anyway, so I would simply fix up and merge this one first.

@jorisvandenbossche jorisvandenbossche added API Design and removed Indexing Related to indexing on series/frames, not to indexes themselves labels Jul 20, 2018
@@ -871,6 +871,18 @@ def test_sub(self):
pytest.raises(TypeError, lambda: index - index.tolist())
pytest.raises(TypeError, lambda: index.tolist() - index)

def test_sub_object(self):
# GH#19369
from decimal import Decimal
Copy link
Contributor

Choose a reason for hiding this comment

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

imports at the top

index = pd.Index([Decimal(1), Decimal(2)])
expected = pd.Index([Decimal(0), Decimal(1)])

result = index - Decimal(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you are exercising rsub?

@jbrockmendel
Copy link
Member Author

Regarding testing, I think you should only add rsub case to the test?

I don't think you are exercising rsub?

rsub is tested implicitly in the result = index - pd.Index([Decimal(1), Decimal(1)]) line. I'll make that explicit.

@codecov
Copy link

codecov bot commented Jul 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@27ebb3e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #21981   +/-   ##
=========================================
  Coverage          ?   91.96%           
=========================================
  Files             ?      166           
  Lines             ?    50331           
  Branches          ?        0           
=========================================
  Hits              ?    46289           
  Misses            ?     4042           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.36% <100%> (?)
#single 42.23% <33.33%> (?)
Impacted Files Coverage Δ
pandas/core/indexes/base.py 96.36% <100%> (ø)

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 27ebb3e...bd6e88c. Read the comment docs.

@jorisvandenbossche jorisvandenbossche merged commit 68ac291 into pandas-dev:master Jul 23, 2018
@jorisvandenbossche
Copy link
Member

@jbrockmendel thanks!

@jorisvandenbossche jorisvandenbossche added this to the 0.24.0 milestone Jul 23, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@jbrockmendel jbrockmendel deleted the oindex branch April 5, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Index.__add__ OK but __sub__ not?
4 participants