Deprecate inplace in Categorical.remove_unused_categories by OlehKSS · Pull Request #37918 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- tests added / passed
- passes
black pandas - passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
Related to #37643
Oleh Kozynets added 2 commits
| @@ -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 |
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
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) |
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)
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
Oleh Kozynets added 2 commits
Hello @OlehKSS! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻
Comment last updated at 2020-11-18 11:57:24 UTC
This was referenced
Nov 18, 2020
thanks @OlehKSS happy to have you do the rest ....