-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Series.get() with ExtensionArray and integer index #21260
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21260 +/- ##
==========================================
+ Coverage 91.9% 91.92% +0.02%
==========================================
Files 154 153 -1
Lines 49557 49572 +15
==========================================
+ Hits 45544 45568 +24
+ Misses 4013 4004 -9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
I think we should really consider making get
more strict in general (not the integer fallback), xref #17928 but that is again not for this PR
This was not actually tested for Series of datetimetz or categorical type? (as those should also take this path?)
It might be good to add some general tests for Series.get that covers this.
@jorisvandenbossche I added tests for |
doc/source/whatsnew/v0.23.1.txt
Outdated
@@ -81,6 +81,7 @@ Indexing | |||
- Bug in :meth:`Series.reset_index` where appropriate error was not raised with an invalid level name (:issue:`20925`) | |||
- Bug in :func:`interval_range` when ``start``/``periods`` or ``end``/``periods`` are specified with float ``start`` or ``end`` (:issue:`21161`) | |||
- Bug in :meth:`MultiIndex.set_names` where error raised for a ``MultiIndex`` with ``nlevels == 1`` (:issue:`21149`) | |||
- Bug in :meth:`Series.get` for ``Series`` using ``ExtensionArray`` and integer index (:issue:`21257`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to 0.24.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pandas/tests/test_base.py
Outdated
def test_get(self): | ||
for o in self.objs: | ||
if isinstance(o, Series): | ||
s = o.set_axis([2 * i for i in range(len(o))], inplace=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just directly construct a Series with the provided values rather than trying to convolute this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pandas/tests/test_base.py
Outdated
@@ -998,6 +998,42 @@ def test_validate_bool_args(self): | |||
with pytest.raises(ValueError): | |||
self.int_series.drop_duplicates(inplace=value) | |||
|
|||
def test_get(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in pandas/tests/series/test_indexing (and pandas/tests/frame/test_indexing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to pandas/tests/series/indexing/test_indexing and parameterized the test
Note for dataframes, df.get()
has a meaning to get the column, not the row, so I didn't put a test there.
@Dr-Irv this looks good. just rebased. ping on green here. |
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff