Skip to content

BUG: intersection of decreasing RangeIndexes #17374

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 7, 2017
Merged

BUG: intersection of decreasing RangeIndexes #17374

merged 1 commit into from
Sep 7, 2017

Conversation

toobaz
Copy link
Member

@toobaz toobaz commented Aug 29, 2017

@gfyoung gfyoung added Bug Indexing Related to indexing on series/frames, not to indexes themselves labels Aug 29, 2017
gfyoung
gfyoung reviewed Aug 29, 2017
View reviewed changes
doc/source/whatsnew/v0.21.0.txt Outdated
@@ -359,6 +359,7 @@ Indexing
- Bug in ``.iloc`` when used with inplace addition or assignment and an int indexer on a ``MultiIndex`` causing the wrong indexes to be read from and written to (:issue:`17148`)
- Bug in ``.isin()`` in which checking membership in empty ``Series`` objects raised an error (:issue:`16991`)
- Bug in ``CategoricalIndex`` reindexing in which specified indices containing duplicates were not being respected (:issue:`17323`)
- Bug in intersection of ``RangeIndex``es with negative step (:issue:`17296`)
Copy link
Member

Choose a reason for hiding this comment

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

Drop the "es" in front of RangeIndex

gfyoung
gfyoung reviewed Aug 29, 2017
View reviewed changes
pandas/tests/indexes/test_range.py Outdated
@@ -609,6 +609,19 @@ def test_intersection(self):
expected = Index(np.sort(np.intersect1d(self.index.values,
other.values)))
tm.assert_index_equal(result, expected)
# reversed
Copy link
Member

Choose a reason for hiding this comment

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

Reference the issue number.

@toobaz toobaz force-pushed the range_intersection branch from fd46d82 to 8d2038c Compare August 29, 2017 21:24
jschendel
jschendel reviewed Aug 30, 2017
View reviewed changes
pandas/core/indexes/range.py Outdated
@@ -544,6 +548,14 @@ def __floordiv__(self, other):
fastpath=True)
return self._int64index // other

def _reverse(self):
Copy link
Member

Choose a reason for hiding this comment

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

This is already defined in terms of slicing, i.e. RangeIndex(...)[::-1] will return a RangeIndex with the opposite step and is performant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, nice, thanks!

@codecov
Copy link

codecov bot commented Aug 30, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17374      +/-   ##
==========================================
- Coverage   91.01%   90.99%   -0.02%     
==========================================
  Files         163      163              
  Lines       49567    49574       +7     
==========================================
- Hits        45113    45111       -2     
- Misses       4454     4463       +9
Flag Coverage Δ
#multiple 88.78% <100%> (ø) ⬆️
#single 40.25% <7.69%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/range.py 92.35% <100%> (+0.16%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.72% <0%> (-0.1%) ⬇️

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 0d676a3...8d2038c. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 30, 2017 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17374      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files         163      163              
  Lines       49587    49591       +4     
==========================================
- Hits        45203    45198       -5     
- Misses       4384     4393       +9
Flag Coverage Δ
#multiple 88.92% <100%> (ø) ⬆️
#single 40.25% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/range.py 92.28% <100%> (+0.09%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.72% <0%> (-0.1%) ⬇️

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 84a39f9...457e7ff. Read the comment docs.

@jschendel
Copy link
Member

FYI: I had to mark a test as xfailing in #17272 due to this bug. The PR is still open, but depending on when it gets merged you may need to rebase and remove the xfail.

@toobaz toobaz force-pushed the range_intersection branch from 8d2038c to 680dd4b Compare August 30, 2017 05:40
@toobaz toobaz mentioned this pull request Aug 30, 2017
Closed
jreback
jreback approved these changes Aug 30, 2017
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

very minor comment. ping on green.

pandas/tests/indexes/test_range.py Outdated
expected = first.astype(int).intersection(other.astype(int))
result = first.intersection(other).astype(int)
tm.assert_index_equal(result, expected)
# reversed
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put blank line here

pandas/tests/indexes/test_range.py Outdated
@@ -609,6 +609,19 @@ def test_intersection(self):
expected = Index(np.sort(np.intersect1d(self.index.values,
other.values)))
tm.assert_index_equal(result, expected)
# reversed (GH 17296)
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well

@jreback jreback added this to the 0.21.0 milestone Aug 30, 2017
@toobaz toobaz force-pushed the range_intersection branch from 680dd4b to cc0bd29 Compare August 30, 2017 12:43
@toobaz
Copy link
Member Author

toobaz commented Aug 30, 2017

@jreback ping

@jreback
Copy link
Contributor

jreback commented Sep 6, 2017

lgtm. can you rebase and ping on green just to make sure.

@toobaz toobaz force-pushed the range_intersection branch from cc0bd29 to 457e7ff Compare September 6, 2017 12:51
@toobaz
Copy link
Member Author

toobaz commented Sep 6, 2017

@jreback ping

@jreback jreback merged commit fd137f5 into pandas-dev:master Sep 7, 2017
@jreback
Copy link
Contributor

jreback commented Sep 7, 2017

thanks!

@toobaz toobaz deleted the range_intersection branch September 7, 2017 05:06
jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Sep 10, 2017
@toobaz @jbrockmendel
BUG: intersection of decreasing RangeIndexes (pandas-dev#17374)
c877772 
closes pandas-dev#17296
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
@toobaz @alanbato
BUG: intersection of decreasing RangeIndexes (pandas-dev#17374)
372f483 
closes pandas-dev#17296
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
@toobaz @No-Stream
BUG: intersection of decreasing RangeIndexes (pandas-dev#17374)
e1d782a 
closes pandas-dev#17296
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschendel jschendel jschendel left review comments

@gfyoung gfyoung gfyoung left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Bug Indexing Related to indexing on series/frames, not to indexes themselves
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

BUG: Intersection of RangeIndexes fails when the step sign is negative
4 participants
@toobaz @jschendel @jreback @gfyoung