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
BUG: Raise proper TypeError for scalar indexer in Index.take #42886
Conversation
Hello @radioactive11! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-09-25 12:57:16 UTC |
you will have to add a test for this |
@jreback From what I understand, I should add a test to check whether this exception is being raised on passing a scalar value, right? |
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.
Thanks for the PR!
From what I understand, I should add a test to check whether this exception is being raised on passing a scalar value, right?
Yes, indeed.
There is a TestTake
class in pandas/tests/indexes/test_indexing.py
. You can add a test there.
@jorisvandenbossche I have made the following changes as per your suggestions
|
@@ -66,6 +66,10 @@ def test_take(self, index): | |||
with pytest.raises(AttributeError, match=msg): | |||
index.freq | |||
|
|||
scalar_index = 1 | |||
with pytest.raises(TypeError): |
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.
i think you will have to add the match
parameter with the error msg. And maybe also point back to the issue.
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.
match
param should be in pytest.raises
. Take a look at how they are setup in other tests
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 is still incorrect
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.
and maybe the msg should say that it can't be scalar (which you are actually checking)
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.
try also with a 2D array
@jreback I have made changes as per your suggestions. I also added |
@debnathshoham I have made the changes as per your suggestions and the tests are passing as well. Please have a look and do suggest if further changes need to be made. |
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.
also pls add a whatsnew note in 1.4 bug fixes in indexing section.
pandas/core/indexes/base.py
Outdated
@@ -958,6 +958,10 @@ def take( | |||
if kwargs: | |||
nv.validate_take((), kwargs) | |||
indices = ensure_platform_int(indices) | |||
|
|||
if indices.ndim == 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.
how about indices.ndim != 1
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.
@jreback can you please help me with the whatsnew note? I do not know how & where to add it 😅
@@ -66,6 +66,10 @@ def test_take(self, index): | |||
with pytest.raises(AttributeError, match=msg): | |||
index.freq | |||
|
|||
scalar_index = 1 | |||
with pytest.raises(TypeError): |
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.
try also with a 2D array
@jreback I changed it to |
there are a number of failing tests |
@jreback I removed a blank line which was causing a flake8 test to fail. However I can still see a number of cancelled tests. When I run a subset of the tests which deal with the indexer ( |
@@ -70,6 +70,17 @@ def test_take(self, index): | |||
with pytest.raises(AttributeError, match=msg): | |||
index.freq | |||
|
|||
scalar_index = 1 |
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.
put this in a new test
your change as small as it is, is possibly affecting other tests, you will need to investigate |
Appears this PR has been dormant for a while and the CI is still failing so closing. If you're interested in continuing please merge master, ensure the tests are passing and we can reopen |
Fixes: #42875 (comment)
TypeError is being raised on passing scalar value in DataFrame.take