Skip to content
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: Change FutureWarning to DeprecationWarning for inplace setitem with DataFrame.(i)loc #50044

Merged
merged 9 commits into from Jan 18, 2023

Conversation

aneesh98
Copy link
Contributor

@aneesh98 aneesh98 commented Dec 3, 2022

# TODO: how to get future behavior?
# TODO: what if we got here indirectly via loc?
pass
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@aneesh98 aneesh98 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @phofl ,
Can you please check my comments #48673

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phofl if you have an opinion on this, could you chime in on the issue? -> #48673 (to keep the discussion about the exact course of action there)

@github-actions
Copy link
Contributor

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.

@mroeschke
Copy link
Member

@aneesh98 during today's community dev meeting we decided to change this warning to DeprecationWarning instead of fully removing it. Would you be interested in continuing and making that change?

@aneesh98
Copy link
Contributor Author

Hi, @mroeschke I am very much interested in completing the change.

@mroeschke mroeschke mentioned this pull request Jan 13, 2023
2 tasks
@aneesh98
Copy link
Contributor Author

Hi @mroeschke, just to confirm should be any change to text content of the warnings?

pandas/core/indexing.py Outdated Show resolved Hide resolved
@mroeschke
Copy link
Member

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?

@jorisvandenbossche jorisvandenbossche changed the base branch from main to 1.5.x January 16, 2023 08:08
@jorisvandenbossche jorisvandenbossche changed the base branch from 1.5.x to main January 16, 2023 08:09
@datapythonista datapythonista changed the base branch from main to 1.5.x January 17, 2023 09:25
@datapythonista datapythonista changed the base branch from 1.5.x to main January 17, 2023 09:26
@datapythonista datapythonista changed the base branch from main to 1.5.x January 17, 2023 09:34
@datapythonista datapythonista changed the base branch from 1.5.x to main January 17, 2023 09:34
@datapythonista
Copy link
Member

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

@mroeschke mroeschke added this to the 1.5.3 milestone Jan 17, 2023
@mroeschke mroeschke added the Warnings Warnings that appear or should be added to pandas label Jan 17, 2023
@mroeschke
Copy link
Member

Push a commit to fix some tests

@jorisvandenbossche jorisvandenbossche changed the title BUG: Remove FutureWarning for DataFrame.loc BUG: Change FutureWarning to DeprecationWarning for inplace setitem with DataFrame.(i)loc Jan 17, 2023
Copy link
Member

@datapythonista datapythonista left a 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.

@datapythonista datapythonista merged commit 4ea0840 into pandas-dev:1.5.x Jan 18, 2023
@datapythonista
Copy link
Member

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`)
Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Warnings Warnings that appear or should be added to pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Unclear FutureWarning regarding inplace iloc setitem
8 participants