BUG: Series.loc raising KeyError for Iterator indexer in case of setitem #39623
Conversation
phofl
commented
Feb 6, 2021
phofl
commented
- closes QST: Why is indexing with a reversed list possible but not assigning a new value? #39614
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
phofl
added
Bug
Indexing
|
can you add any relevant cases to SetitemCastingEquivalents |
|
The functions in there don't work with a generator. Are you referring to adding a new function or to add a parametrization? Parametrization won't work with a generator, while adding a new function would cause a lot of special casing I think |
i was thinking adding to test_slice_key a check with |
|
At thx. I though you were talking about adding another function. Should a Generator work for iloc? iloc.getitem raises this, so I assume no? |
i dont know if we have a specific policy for that. my only opinion here is that loc/iloc/ |
I don't think we accept iterator / generators for indexers, though we do in the constructors. I think its ok that we do, but agree that we should generally test these. Can you open an issue for this. |
|
err rather, I agree with @jbrockmendel that we should systemcially test all setter & getter indexing methods in order to close this (rather than just adding in a specific place). |
|
This would support Iterators for iloc and loc consistently |
|
can you merge master |
|
Done |
There was a problem hiding this comment.
lgtm. @jbrockmendel if you think we are covering all of the testing needed here (can merge this and followup if needed as well).
| @@ -570,6 +573,9 @@ def test_slice_key(self, obj, key, expected, val, indexer_sli, is_inplace): | |||
| indkey = np.array(ilkey) | |||
| self.check_indexer(obj, indkey, expected, val, indexer_sli, is_inplace) | |||
|
|
|||
| genkey = (x for x in indkey) | |||
| self.check_indexer(obj, genkey, expected, val, indexer_sli, is_inplace) | |||
|
|
|||
There was a problem hiding this comment.
these tests are good, but only cover Series. do we need analogous DataFrame tests?
There was a problem hiding this comment.
@phofl i think this was the main outstanding question. if the answer is "no" then this should be good to go (merge master just to be on the safe side)
There was a problem hiding this comment.
Sorry for the long wait. I have more time on my handy in the coming weeks and months again :)
One-dimensional indexers on DataFrames work, but two dimensionals like
df = DataFrame({"a": [1, 2, 3], "b": [4, 5, 6]})
indexer = (x for x in [1, 2])
df.iloc[indexer, 1]
still don't, because we access the tuple elements to check things before doing anything, which leaves an empty generator behind. Only way I am seeing currently is converting this directly in setitem and getitem. Thoughts @jbrockmendel ?
There was a problem hiding this comment.
Only way I am seeing currently is converting this directly in setitem and getitem
I expect this is probably right
There was a problem hiding this comment.
Will Put something up then tomorrow or the day after
|
I have added support for more dimensional iloc indexers. This should cover all cases now |
|
can you rebase |
|
Done |
|
probably needs another rebase, but i think this was ready |
|
rebased |
|
thanks @phofl very nice |
