Skip to content

BUG: positional getitem indexing with list on Series with duplicate integer index fails #26083

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
jreback merged 8 commits into from
Apr 16, 2019
Merged

BUG: positional getitem indexing with list on Series with duplicate integer index fails #26083

jreback merged 8 commits into from
Apr 16, 2019

Conversation

@tranctan
Copy link
Contributor

@tranctan tranctan commented Apr 14, 2019 •
edited
Loading

Create test case for issue GH17347

TST GH17347
c0fe2e0 
Create test case for issue GH17347
@pep8speaks
Copy link

pep8speaks commented Apr 14, 2019 •
edited
Loading

Hello @tranctan96! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-15 17:04:32 UTC

@codecov
Copy link

codecov bot commented Apr 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@190a69e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #26083   +/-   ##
=========================================
  Coverage          ?   91.94%           
=========================================
  Files             ?      175           
  Lines             ?    52444           
  Branches          ?        0           
=========================================
  Hits              ?    48221           
  Misses            ?     4223           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.5% <ø> (?)
#single 40.75% <ø> (?)

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 190a69e...c0fe2e0. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 14, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@190a69e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #26083   +/-   ##
=========================================
  Coverage          ?   91.95%           
=========================================
  Files             ?      175           
  Lines             ?    52412           
  Branches          ?        0           
=========================================
  Hits              ?    48195           
  Misses            ?     4217           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.51% <ø> (?)
#single 40.73% <ø> (?)

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 190a69e...d25af61. Read the comment docs.

TST GH17347
312e082 
Create test case for issue GH17347
mroeschke
mroeschke requested changes Apr 14, 2019
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Some comments.

pandas/tests/indexing/test_indexing.py Outdated

# Assert the duplicate indexes
for idx in dup_idxes:
assert s[idx].equals(s[[idx]])
Copy link
Member

@mroeschke mroeschke Apr 14, 2019

Choose a reason for hiding this comment

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

Use tm.assert_series_equal

pandas/tests/indexing/test_indexing.py Outdated
def test_duplicate_int_indexing(self, idx):
# GH 17347
s = pd.Series(range(len(idx)), idx)
dup_idx_filter = s.index.duplicated(keep=False)
Copy link
Member

@mroeschke mroeschke Apr 14, 2019

Choose a reason for hiding this comment

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

What's the purpose for all this duplicated logic? You can just copy the example from the original issue.

Copy link
Contributor Author

@tranctan tranctan Apr 14, 2019

Choose a reason for hiding this comment

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

Hi ! Thank you for your review. This is my first time making a test case contribution, so i just figured it out that i overdid it. I'm fixing it now!

@mroeschke mroeschke added Indexing Related to indexing on series/frames, not to indexes themselves Testing pandas testing functions or related to the test suite labels Apr 14, 2019
mroeschke
mroeschke reviewed Apr 14, 2019
View reviewed changes
pandas/tests/indexing/test_indexing.py Outdated
def test_duplicate_int_indexing(self):
# GH 17347
s = pd.Series(range(3), index=[1, 1, 3])
tm.assert_series_equal(s[1], s[[1]])
Copy link
Member

@mroeschke mroeschke Apr 14, 2019

Choose a reason for hiding this comment

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

Can you also test loc?

And format the test like

result = ...
expected = ...
tm.assert_series_equal(result, expected)

@jreback jreback changed the title TST GH17347 BUG: positional getitem indexing with list on Series with duplicate integer index fails Apr 15, 2019
@jreback jreback added this to the 0.25.0 milestone Apr 15, 2019
jreback
jreback requested changes Apr 15, 2019
View reviewed changes
pandas/tests/indexing/test_indexing.py Outdated
idxr(s2)['0'] = 0
assert s2.index.is_object()

def test_duplicate_int_indexing(self):
Copy link
Contributor

@jreback jreback Apr 15, 2019

Choose a reason for hiding this comment

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

use parameterize like

@pytest.parameterize('case', [lambda s: s.__getitem__, lambda: s.loc])
def test_duplicate_int_indexing(self, case):
     expected = s.iloc[[1]]
     result = case(s)[[1]]

pandas/tests/indexing/test_indexing.py Outdated
def test_duplicate_int_indexing(self):
# GH 17347
s = pd.Series(range(3), index=[1, 1, 3])
expected = s[1]
Copy link
Contributor

@jreback jreback Apr 15, 2019

Choose a reason for hiding this comment

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

move this after def test_dups_fancy_indexing2(self)

@jreback jreback merged commit 4b1624e into pandas-dev:master Apr 16, 2019
@jreback
Copy link
Contributor

jreback commented Apr 16, 2019

thanks @tranctan96

@tranctan tranctan deleted the dup-indexing-test-case branch April 18, 2019 07:08
yhaque1213 pushed a commit to yhaque1213/pandas that referenced this pull request Apr 22, 2019
@tranctan @yhaque1213
BUG: positional getitem indexing with list on Series with duplicate i…
f8bc4c5 
…nteger index fails (pandas-dev#26083)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mroeschke mroeschke mroeschke requested changes

@jreback jreback jreback approved these changes

Assignees

No one assigned

Labels

Indexing Related to indexing on series/frames, not to indexes themselves Testing pandas testing functions or related to the test suite

Projects

None yet

Milestone

0.25.0

Development

Successfully merging this pull request may close these issues.

BUG: positional getitem indexing with list on Series with duplicate integer index fails

4 participants

@tranctan @pep8speaks @jreback @mroeschke