Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug in iloc aligned objects #37728
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 in iloc aligned objects #37728
Changes from 5 commits
9d11511
ca3f47b
f1306aa
c55cd62
ea41407
6bced6a
689a1ab
d870dd6
23dab4a
48e0d25
ea068ba
8ff2430
8a697f7
3bef35a
7375f22
e58fbc8
55e9403
7016977
b965eee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
can you type name here (and above)
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
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.
can this be solved with an appropriate call to _align_frame or _align_series? id really prefer to avoid passing
name
around. by the time we get to setitem_with_indexer we should always be working with positional indexers (i.e. iloc)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.
Hm no not to my knowledge. The issue is, that we want to achieve oposing things between loc and iloc, but the objects do look the same.
setitem_with_indexer
could be solved with a default value, that is not a problem.But I can not see a way to do this without passing some kind of flag to the align calls.
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.
Looked into this a bit again. The indexer in
setitem_with_indexer
already is positional, but the value may not be properly aligned for loc, hence we call the align functions there. This is not necessary for the iloc path, but this information is lost there, if we do not pass it through somehow.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.
it sounds like we should be doing some appropriate alignment within Loc before passing it into iLoc?
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.
That would be an option, but would result in a bigger rewrite and I am not sure if that would introduce more complexity, because the
align_series
method is called in various places. Should I give it a try?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.
it looks like this PR is making it so that we never call align_frame/align_series if setitem_with_indexer was called from iloc, only if it was called from loc. setitem_with_indexer is only called from one place within loc. Can we just do that alignment right before that call?
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.
Hm, we could try, but it is not that simple. For example before calling
setitem_with_indexer
from loc,value
may be aDataFrame
with different dtypes. Currently_align_series
is called for every column to get andarray
with the correct dtype. If we call this before callingsetitem_with_indexer
we would have to call_align_frame
or copy the logic, which decides if we align every colum at once or separately._align_frame
returns an object dtype array, which obviously would introduce a lot of dtype errors. That is what I meant with a bigger rewrite here.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 explaining, i think i have a better handle on it now.
ive got an upcoming PR to make all 2D cases go through split_path. let's revisit this after that goes through and see if we can make it cleaner
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.
That sounds great. Just ping me when merged, then I will have a look.
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.
type
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
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.
if we have a dict here dont we still want to wrap it in a Series?
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.
Yep, have no tests getting here with iloc. Will add one
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.
LMK when these tests are all ready and ill take another look. this one is almost ready
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.
Tests are all in
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.
double check me on this, but i think it might be the case that we only get here with
name = "loc"
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.
Yep, you are right. Iloc raises when indexer is a dict. That is the only way we can get in there. Removed it
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.
are there other indexer/value type combinations that are relevant here? e.g. the value being set here is a DataFrame, would a Series trigger the same bug? what if instead of
[1]
the indexer wasslice(1, 2)
?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.
raises
Did not check slice previously, this triggered the bug too. Will add test