-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Change FutureWarning to DeprecationWarning for inplace setitem with DataFrame.(i)loc #50044
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
Conversation
pandas/core/indexing.py
Outdated
# TODO: how to get future behavior? | ||
# TODO: what if we got here indirectly via loc? | ||
pass |
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.
Simply removing the warning is not the correct approach. We have many cases that we want to warn about. You have to figure out what's going wrong there and fix the root cause
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.
Ok Sorry, let me do some anaylsis and comment on the Open Issue.
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.
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.
@phofl do you think it's possible to tell when it's appropriate to raise this warning without it being noisy?
From #48673 (comment) , it seems @jbrockmendel was on board with just removing 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.
Hm no this is tricky. But not sure if removing in 1.5.3 is a good idea either.
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.
could make it a DeprecationWarning?
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.
Sounds good
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.
Okay, should the contents of the warning be changed? since now it only mention .iloc but the warning gets dsiplayed even when .loc is used.
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'd keep it as is, but no strong preference
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.
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
@aneesh98 during today's community dev meeting we decided to change this warning to |
Hi, @mroeschke I am very much interested in completing the change. |
Hi @mroeschke, just to confirm should be any change to text content of the warnings? |
No change to the text. Also, could you reopen this PR targeting the 1.5.x branch directly? |
fda6b17
to
a21ea6a
Compare
a21ea6a
to
ccaf6ae
Compare
Sorry for the noise, but changing the base branch was a bit trickier than expected. I also addressed the pending comments here. If CI finishes green, I think this is ready to be merged. If this and #50682 are ready in the next hours, happy to start the release of 1.5.3 tomorrow. CC: @pandas-dev/pandas-core |
Push a commit to fix some tests |
DataFrame.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.
Thanks @mroeschke for continuing the work on this. Seems like CI should finish green now, if it does I'll merge and start the release.
Thanks @aneesh98 for the work on this. As you've seen we finished the last couple of issues, as we need this for the 1.5.3 release which we are going to release today. And thanks @mroeschke for finishing it. |
@@ -48,6 +48,7 @@ Other | |||
as pandas works toward compatibility with SQLAlchemy 2.0. | |||
|
|||
- Reverted deprecation (:issue:`45324`) of behavior of :meth:`Series.__getitem__` and :meth:`Series.__setitem__` slicing with an integer :class:`Index`; this will remain positional (:issue:`49612`) | |||
- A ``FutureWarning`` raised when attempting to set values inplace with :meth:`DataFrame.loc` or :meth:`DataFrame.loc` has been changed to a ``DeprecationWarning`` (:issue:`48673`) |
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.
Should this bo loc
or 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.
thx
doc/source/whatsnew/v1.5.3.rst
file.