REF: No need to delegate to index check of whether an int is an int #21982

toobaz · 2018-07-19T21:04:16Z

tests passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Just fixing the following nonsensical error:

In [2]: pd.Series(range(10)).iloc['a']
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-4a32011255fe> in <module>()
----> 1 pd.Series(range(10)).iloc['a']
[...]
TypeError: cannot do positional indexing on <class 'pandas.core.indexes.range.RangeIndex'> with these indexers [a] of <class 'str'>

(you just cannot do positional indexing with a str, regardless of the index)

... and adding a docstring while I was at it.

codecov · 2018-07-20T06:05:04Z

Codecov Report

Merging #21982 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21982      +/-   ##
==========================================
- Coverage   91.96%   91.96%   -0.01%     
==========================================
  Files         166      166              
  Lines       50334    50333       -1     
==========================================
- Hits        46292    46291       -1     
  Misses       4042     4042

Flag	Coverage Δ
#multiple	`90.36% <ø> (-0.01%)`	⬇️
#single	`42.21% <ø> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexing.py	`93.82% <ø> (+0.08%)`	⬆️
pandas/core/indexes/interval.py	`94.12% <0%> (-0.19%)`	⬇️
pandas/tseries/offsets.py	`97.15% <0%> (ø)`	⬆️

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 b9c1786...34cdc8e. Read the comment docs.

jorisvandenbossche

Good change! (I have also always been annoyed if I saw that error :-))

jorisvandenbossche · 2018-07-20T09:26:56Z

pandas/core/indexing.py

@@ -2215,8 +2233,6 @@ def _getitem_axis(self, key, axis=None):

        # a single integer
        else:
-            key = self._convert_scalar_indexer(key, axis)


Can you check if there is any remaining occasion where _convert_scalar_indexer is used for iloc (in principle it shouldn't?). Because if not, _convert_scalar_indexer has some redundant code

Yes, there is, and it was related to a commit I pushed but then removed, because unfortunately it broke some tests and I don't have time to check them in the next days. So I went for the obvious change only, will add the rest in some other PR (which I think will be quite large, as I will also touch code in indexes).

OK. Can you point to where it is currently used? (to have this on record)
Is it worth to add a todo comment there?

I'll rebase my other PR on this once merged, push it and open an issue referring to it

jorisvandenbossche · 2018-07-20T09:27:29Z

doc/source/whatsnew/v0.23.4.txt

@@ -32,6 +32,7 @@ Bug Fixes

 - Bug where calling :func:`DataFrameGroupBy.agg` with a list of functions including ``ohlc`` as the non-initial element would raise a ``ValueError`` (:issue:`21716`)
 - Bug in ``roll_quantile`` caused a memory leak when calling ``.rolling(...).quantile(q)`` with ``q`` in (0,1) (:issue:`21965`)
+- Passing a non-int scalar to ``.iloc`` now raises a more appropriate error message (:issue:`21982`)


Not sure if it is worth to add a whatsnew for this.

OK, removing

toobaz · 2018-07-20T11:28:13Z

See #21989 for the more general issue. Ready to merge if there are no objections.

jreback · 2018-07-20T12:20:39Z

pandas/tests/indexing/test_floats.py

@@ -50,7 +50,7 @@ def test_scalar_error(self):
            def f():
                s.iloc[3.0]
            tm.assert_raises_regex(TypeError,
-                                   'cannot do positional indexing',
+                                   'Cannot index by location index',


side issue, we have 2 different versions of this error message (the original also happens), so these should be the same (future PR). I don't think we actually need to distinguish them.

We already have both messages in the code (I did not write a new message).

The only difference is that now this case raises the correct one.

jreback

can you add the test in top of the PR?

pep8speaks · 2018-07-20T13:22:17Z

Hello @toobaz! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 20, 2018 at 13:23 Hours UTC

jreback · 2018-07-20T13:26:01Z

lgtm. merge on green.

…andas-dev#21982)

toobaz added Indexing Related to indexing on series/frames, not to indexes themselves Error Reporting Incorrect or improved errors from pandas labels Jul 19, 2018

toobaz force-pushed the iloc_with_non_int branch from eec4dd5 to 06141c0 Compare July 20, 2018 06:50

jorisvandenbossche reviewed Jul 20, 2018

View reviewed changes

REF: No need to delegate to index check of whether an int is an int

3c07893

toobaz force-pushed the iloc_with_non_int branch from 06141c0 to 3c07893 Compare July 20, 2018 09:39

toobaz mentioned this pull request Jul 20, 2018

iloc calls should not reach indexes code #21989

Closed

jreback reviewed Jul 20, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jul 20, 2018

jreback requested changes Jul 20, 2018

View reviewed changes

toobaz added a commit to toobaz/pandas that referenced this pull request Jul 20, 2018

TST: test for pandas-dev#21982

86c2733

TST: test for pandas-dev#21982

34cdc8e

toobaz force-pushed the iloc_with_non_int branch from 86c2733 to 34cdc8e Compare July 20, 2018 13:23

jreback approved these changes Jul 20, 2018

View reviewed changes

toobaz merged commit f8f0e61 into pandas-dev:master Jul 20, 2018

toobaz deleted the iloc_with_non_int branch July 20, 2018 19:20

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

REF: No need to delegate to index check of whether an int is an int (p…

903a6ea

…andas-dev#21982)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: No need to delegate to index check of whether an int is an int #21982

REF: No need to delegate to index check of whether an int is an int #21982

toobaz commented Jul 19, 2018

codecov bot commented Jul 20, 2018 •

edited

Loading

jorisvandenbossche left a comment •

edited

Loading

jorisvandenbossche Jul 20, 2018

toobaz Jul 20, 2018

jorisvandenbossche Jul 20, 2018

toobaz Jul 20, 2018

jorisvandenbossche Jul 20, 2018

toobaz Jul 20, 2018

toobaz commented Jul 20, 2018

jreback Jul 20, 2018

toobaz Jul 20, 2018

jreback left a comment

pep8speaks commented Jul 20, 2018 •

edited

Loading

jreback commented Jul 20, 2018

REF: No need to delegate to index check of whether an int is an int #21982

REF: No need to delegate to index check of whether an int is an int #21982

Conversation

toobaz commented Jul 19, 2018

codecov bot commented Jul 20, 2018 • edited Loading

Codecov Report

jorisvandenbossche left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toobaz commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

pep8speaks commented Jul 20, 2018 • edited Loading

Comment last updated on July 20, 2018 at 13:23 Hours UTC

jreback commented Jul 20, 2018

codecov bot commented Jul 20, 2018 •

edited

Loading

jorisvandenbossche left a comment •

edited

Loading

pep8speaks commented Jul 20, 2018 •

edited

Loading