-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Deprecate inplace in Categorical.remove_unused_categories #37918
Conversation
pandas/core/arrays/categorical.py
Outdated
@@ -1083,8 +1093,7 @@ def remove_unused_categories(self, inplace=False): | |||
cat._dtype = new_dtype | |||
cat._codes = coerce_indexer_dtype(inv, new_dtype.categories) | |||
|
|||
if not inplace: | |||
return cat |
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 doesnt get changed until we actually enforce the deprecation a few versions from now
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
@@ -354,7 +354,7 @@ def test_validate_inplace_raises(self, value): | |||
with pytest.raises(ValueError, match=msg): | |||
cat.remove_categories(removals=["D", "E", "F"], inplace=value) | |||
|
|||
with pytest.raises(ValueError, match=msg): | |||
with tm.assert_produces_warning(FutureWarning): | |||
cat.remove_unused_categories(inplace=value) |
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.
comment inside this block about why we're getting a FutureWarning.
im guessing this actually needs both the raises and the produces_warning, ie.
with pytest.raises(ValueError, match=msg):
with tm.assert_produces_warning(FutureWarning):
# GH#37918 inplace kwarg deprecated
cat.remove_unused_categories(inplace=value)
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
This is a very good start. For the checkbox at the top, since this doesn't handle all the relevant methods, it doesn't close the issue. Instead write below the checkboxes "xref #37643" |
@jbrockmendel Thanks for a quick review, I have updated this pull request according to your suggestions |
thanks @OlehKSS happy to have you do the rest .... |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Related to #37643