BUG: np array indexer modifed in iloc #21867
Conversation
pandas/core/indexing.py
Outdated
| @@ -2586,6 +2587,7 @@ def maybe_convert_indices(indices, n): | |||
| IndexError : one of the converted indices either exceeded the number | |||
| of elements (specified by `n`) OR was still negative. | |||
| """ | |||
| indices = deepcopy(indices) | |||
There was a problem hiding this comment.
Can this be done down on line 2601? Avoid a copy if at all possible
There was a problem hiding this comment.
Ok moving deepcopy to 2601 makes sense; will do. Can't see a way to avoid copying altogether.
There was a problem hiding this comment.
Should be able to just use .copy() instead of importing deepcopy, no?
There was a problem hiding this comment.
Yes, true. Will change to .copy()
|
Pls implement a test for this. |
Codecov Report
@@ Coverage Diff @@
## master #21867 +/- ##
==========================================
+ Coverage 91.91% 91.91% +<.01%
==========================================
Files 164 164
Lines 49992 49993 +1
==========================================
+ Hits 45951 45952 +1
Misses 4041 4041
Continue to review full report at Codecov.
|
…henko/pandas into array-modification-on-iloc
|
Hello @ydovzhenko! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on July 13, 2018 at 19:38 Hours UTC |
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
pandas/core/indexing.py
Outdated
| @@ -2596,6 +2598,8 @@ def maybe_convert_indices(indices, n): | |||
|
|
|||
| mask = indices < 0 | |||
| if mask.any(): | |||
| # indices is np.array, so we don't use deepcopy | |||
| indices = np.copy(indices) | |||
pandas/core/indexing.py
Outdated
| @@ -2596,6 +2598,8 @@ def maybe_convert_indices(indices, n): | |||
|
|
|||
| mask = indices < 0 | |||
| if mask.any(): | |||
| # indices is np.array, so we don't use deepcopy | |||
There was a problem hiding this comment.
you don't need the comment
| @@ -126,6 +126,24 @@ def test_iloc_getitem_neg_int(self): | |||
| typs=['labels', 'mixed', 'ts', 'floats', 'empty'], | |||
| fails=IndexError) | |||
|
|
|||
| def test_iloc_array_not_mutating_negative_indices(self): | |||
There was a problem hiding this comment.
can you add the issue number here as a comment
jreback
added
Bug
Indexing
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
pandas/tests/indexing/test_iloc.py
Outdated
| @@ -126,6 +126,24 @@ def test_iloc_getitem_neg_int(self): | |||
| typs=['labels', 'mixed', 'ts', 'floats', 'empty'], | |||
| fails=IndexError) | |||
|
|
|||
| def test_iloc_array_not_mutating_negative_indices(self): | |||
|
|
|||
| # Test for issue 21867. Assert that iloc does not modify index array if | |||
There was a problem hiding this comment.
Can simplify this to just GH 21867
There was a problem hiding this comment.
Do you mean change the function name or the comment? Thanks!
pandas/tests/indexing/test_iloc.py
Outdated
| 'C': [106, 107, 108]}, | ||
| index=[1, 2, 3]) | ||
| df.iloc[array_with_neg_numbers] | ||
| assert np.all(array_with_neg_numbers == array_copy) |
There was a problem hiding this comment.
Use tm.assert_numpy_array_equal instead both here and below
pandas/core/indexing.py
Outdated
| @@ -2586,6 +2587,7 @@ def maybe_convert_indices(indices, n): | |||
| IndexError : one of the converted indices either exceeded the number | |||
| of elements (specified by `n`) OR was still negative. | |||
| """ | |||
| indices = deepcopy(indices) | |||
pandas/core/indexing.py
Outdated
| @@ -1,4 +1,5 @@ | |||
| # pylint: disable=W0223 | |||
| from copy import deepcopy | |||
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
doc/source/whatsnew/v0.24.0.txt
Outdated
| @@ -363,8 +363,7 @@ Indexing | |||
| - ``DataFrame.__getitem__`` now accepts dictionaries and dictionary keys as list-likes of labels, consistently with ``Series.__getitem__`` (:issue:`21294`) | |||
| - Fixed ``DataFrame[np.nan]`` when columns are non-unique (:issue:`21428`) | |||
| - Bug when indexing :class:`DatetimeIndex` with nanosecond resolution dates and timezones (:issue:`11679`) | |||
|
|
|||
| - | |||
| - Bug when indexing with a numpy array containing negative values, fixed so the array is not mutated (:issue:`21867`) | |||
There was a problem hiding this comment.
Can you be sure to capitalize NumPy? Can also reword to Bug where indexing with a Numpy array containing negative values would mutate the indexer
pandas/tests/indexing/test_iloc.py
Outdated
| @@ -126,6 +126,23 @@ def test_iloc_getitem_neg_int(self): | |||
| typs=['labels', 'mixed', 'ts', 'floats', 'empty'], | |||
| fails=IndexError) | |||
|
|
|||
| def test_iloc_array_not_mutating_negative_indices(self): | |||
|
|
|||
| # GH 21867. | |||
There was a problem hiding this comment.
Minor nit but don't need the period
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
pandas/tests/indexing/test_iloc.py
Outdated
|
|
||
| df.iloc[:, array_with_neg_numbers] | ||
| tm.assert_numpy_array_equal(array_with_neg_numbers, array_copy) | ||
|
|
There was a problem hiding this comment.
Should only need one blank line here (think this will fail LINTing)
There was a problem hiding this comment.
I can't find LINTing in the contributing guide. Can I LINT myself and how exactly do you guys do that?
There was a problem hiding this comment.
http://pandas-docs.github.io/pandas-docs-travis/contributing.html#python-pep8 has the quick version, but that doesn't check everything.
The actual command run during CI is LINT=1 ci/lint.sh, but that takes longer and may have additional dependencies.
https://travis-ci.org/pandas-dev/pandas/jobs/403382274#L2928 is the line causing this PR to fail.
There was a problem hiding this comment.
flake8 pandas/tests/indexing/test_iloc.py should work for you.
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
…henko/pandas into array-modification-on-iloc
|
thanks @ydovzhenko |
git diff upstream/master -u -- "*.py" | flake8 --diff