-
Notifications
You must be signed in to change notification settings - Fork 116
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
[SNOW-1649147]: Fix inplace
argument for Series objects derived from DataFrames columns
#2209
Conversation
@@ -2752,8 +2752,6 @@ def _update_inplace(self, new_query_compiler): | |||
# Propagate changes back to parent so that column in dataframe had the same contents | |||
if self._parent is not None: | |||
if self._parent_axis == 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.
I removed the else
, because it doesn't seem like self._parent_axis
can be anything other than 1?
In [2]: df = pd.DataFrame([[1, 2, 3], [4, None, 6]], columns=list("ABC"))
In [3]: df['B']._parent_axis
Out[3]: 0
In [4]: df.loc[1]._parent_axis
Out[4]: 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.
@sfc-gh-joshi should we override this code? BTW, in indexing, we never use _parent_axis
before.
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.
In our code base, we never set _parent_axis = 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.
I think we should leave it as-is. We don't use it, but upstream modin uses it in DataFrame._getitem_column
(which we don't use since we use a different code path for indexing).
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.
we now set _parent_axis=1 correctly: #2210
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.
LGTM.
inplace
argument for Series objects derived from DataFramesinplace
argument for Series objects derived from DataFrames columns
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 Rehan!
Fixed by #2210 |
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1649147
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.