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

DEPR: Remove SettingWithCopyWarning #56614

Merged
merged 13 commits into from
Feb 4, 2024
Merged

Conversation

phofl
Copy link
Member

@phofl phofl commented Dec 24, 2023

🥳

We shouldn't merge before the actual release is out

_item_cache removal
closes #50547
closes #29411
closes #21391

SettingWIthCopyWarning removal
closes #18752
closes #9767
closes #14150
closes #16550
closes #17505
closes #38270
closes #39418
closes #39448
closes #41891
closes #45513
closes #50209
closes #55451

Others Matt thinks
closes #19102

@phofl phofl marked this pull request as draft December 24, 2023 18:17
@jreback
Copy link
Contributor

jreback commented Dec 24, 2023

wow this kills giant hacks that i wrote from a while back!

+1000000

"A value is trying to be set on a copy of a slice from a "
"DataFrame\n\n"
"See the caveats in the documentation: "
"https://pandas.pydata.org/pandas-docs/stable/user_guide/"
Copy link
Contributor

Choose a reason for hiding this comment

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

are these docs still there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I’ll continue this pr later or
Tomorrow, the code things should be gone, docs not yet

phofl and others added 6 commits December 25, 2023 02:10
# Conflicts:
#	pandas/tests/copy_view/test_indexing.py
#	pandas/tests/copy_view/test_internals.py
#	pandas/tests/copy_view/test_methods.py
#	pandas/tests/frame/indexing/test_xs.py
#	pandas/tests/indexing/multiindex/test_chaining_and_caching.py
#	pandas/tests/indexing/multiindex/test_setitem.py
#	pandas/tests/indexing/test_chaining_and_caching.py
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.

@github-actions github-actions bot added the Stale label Jan 27, 2024
@phofl phofl removed the Stale label Feb 1, 2024
# Conflicts:
#	ci/code_checks.sh
#	pandas/core/generic.py
#	pandas/core/groupby/groupby.py
#	pandas/errors/__init__.py
#	pandas/tests/copy_view/test_chained_assignment_deprecation.py
#	pandas/tests/copy_view/test_indexing.py
#	pandas/tests/indexing/test_chaining_and_caching.py
#	pandas/tests/series/test_ufunc.py
@phofl phofl marked this pull request as ready for review February 1, 2024 22:29
@phofl phofl added this to the 3.0 milestone Feb 2, 2024
@@ -402,6 +395,7 @@ slicers on a single axis.
Furthermore, you can *set* the values using the following methods.

.. ipython:: python
:okwarning:
Copy link
Member

Choose a reason for hiding this comment

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

Is this the pdep warning? Best to just change this example or remove if it won't work anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't seem to be necessary anymore, lets see what ci has to say

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll keep this in for now, I can't reproduce locally but it shows up on ci, it's weird

reported.
:ref:`Copy-on-Write <copy_on_write>` is the new default with pandas 3.0.
This means than chained indexing will never work. As a consequence,
the ``SettingWithCopyWarning`` is not necessary anymore.
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the As a consequence, the ``SettingWithCopyWarning`` is not necessary anymore. sentence since that warning will no longer exist

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -4136,7 +4121,7 @@ def _get_value(self, index, col, takeable: bool = False) -> Scalar:
series = self._ixs(col, axis=1)
return series._values[index]

series = self._get_item_cache(col)
series = self[col]
Copy link
Member

Choose a reason for hiding this comment

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

self._get_item(col)?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm yeah this should work, thx

if len(self._mgr.blocks) != blocks_before:
self._clear_item_cache()
return result
return f()
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need _protect_consolidate anymore right?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, no

@mroeschke
Copy link
Member

Could you see if this works?

#44866

@@ -58,8 +58,6 @@ Exceptions and warnings
errors.PossiblePrecisionLoss
errors.PyperclipException
errors.PyperclipWindowsException
errors.SettingWithCopyError
Copy link
Member

Choose a reason for hiding this comment

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

If you have another PR with the whatsnew note, please make sure that the removal of these are mentioned

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create a separate PR with a whatsnew

@phofl
Copy link
Member Author

phofl commented Feb 3, 2024

Could you see if this works?

#44866

It works if you use it on a series but it will never work if you do

df["a"].foo = "something
df["a"].foo

because both df["a"] are different objects with CoW, so this can't work

We can close the issue though, the case doesn't make sense under CoW

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Feel free to merge once you fix the merge conflict

# Conflicts:
#	pandas/_config/__init__.py
#	pandas/core/frame.py
#	pandas/core/generic.py
#	pandas/core/series.py
#	pandas/tests/copy_view/test_chained_assignment_deprecation.py
#	pandas/tests/copy_view/test_indexing.py
#	pandas/tests/copy_view/test_interp_fillna.py
#	pandas/tests/copy_view/test_methods.py
#	pandas/tests/copy_view/test_replace.py
#	pandas/tests/frame/indexing/test_indexing.py
#	pandas/tests/frame/indexing/test_xs.py
#	pandas/tests/indexing/multiindex/test_chaining_and_caching.py
#	pandas/tests/indexing/multiindex/test_setitem.py
#	pandas/tests/indexing/test_chaining_and_caching.py
#	pandas/tests/series/accessors/test_dt_accessor.py
@phofl phofl merged commit bc58fe5 into pandas-dev:main Feb 4, 2024
46 of 47 checks passed
@phofl phofl deleted the settingwithcopywarning branch February 4, 2024 15:18
@bersbersbers
Copy link

This PR closed an issue I subscribed to (#9767), and I had no idea why and how until I found https://pandas.pydata.org/pdeps/0007-copy-on-write.html. Leaving this here for others.

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
* DEPR: Remove SettingWithCopyWarning

* Fixup

* Remove docs

* CoW: Boolean indexer in MultiIndex raising read-only error

* Update

* Update

* Update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment