-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
CoW: Add warning for mask, where and clip with inplace #56067
CoW: Add warning for mask, where and clip with inplace #56067
Conversation
@@ -1550,6 +1551,23 @@ def test_chained_where_mask(using_copy_on_write, func): | |||
with tm.raises_chained_assignment_error(): | |||
getattr(df[["a"]], func)(df["a"] > 2, 5, inplace=True) | |||
tm.assert_frame_equal(df, df_orig) | |||
else: | |||
with tm.assert_produces_warning( | |||
FutureWarning, match="inplace method", check_stacklevel=False |
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.
Why is the check_stacklevel=False needed here? (the warnings are added to top-level methods, and we are testing those directly, so that should be a simple case?)
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 have no clue, this showed up in a ci build only and only on one, didn't want to spend more time debugging this...
But can try to reproduce locally if you want
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.
Ah, that's just because mask is calling where under the hood. So it was actually raising the warning twice (once with correct and once with incorrect stacklevel).
Pushing a fix to let it only warn once.
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.
Ah interesting
Not clue why this didn't fail locally though
Co-authored-by: Joris Van den Bossche <[email protected]>
xref #56019