minor inconsistency in Categorical.remove_categories error message

[punndcoder28]

This pull request fixes minor inconsistency in Categorical.remove_categories error message

• closes minor inconsistency in Categorical.remove_categories error message #28669
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Changed the error message to show invalid removals as a set. Added tests for removal of null from the categories. Parameterized pytest.

[pep8speaks]

Hello @punndcoder28! 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 2019-10-09 17:53:34 UTC

[jbrockmendel]

Nitpick: in the future please give the PR an informative title. Most reviewers won't know off the top of their heads what 28677 is about.

[punndcoder28]

Yes my bad @jbrockmendel. I realized it after opening the PR sorry.

[jreback]

can you add or update the tests on remove_categories to account for this change (since nothing broke we likely are not testing it enough)

[punndcoder28]

I had tests as it were suggested by @simonjayhawkins. But there were errors and I didn't know how to handle that. If I could get some guidance I will surely add those and commit the changes.

[punndcoder28]

If you could tell me what tests I should create I'll create those and commit @jreback.

[simonjayhawkins]

@punndcoder28 suitable tests would be similar to those in pandas/tests/arrays/categorical/test_api.py in 8298620

[simonjayhawkins]

@punndcoder28 Thanks for the PR. can you add a whatsnew entry as well.

[punndcoder28]

assert [NaN, b, c, NaN]\nCategories (2, object): [b < c] is None. This shouldn't cause an error right. As [b < c] is None states the statement assert res is None should evaluate to True and should not raise an AssertionError. Am I understanding this the wrong way?

[simonjayhawkins]

@punndcoder28 generally looks good. a couple of small changes. also need to merge master to resolve merge conflict. ping on green.

[punndcoder28]

@punndcoder28 generally looks good. a couple of small changes. also need to merge master to resolve merge conflict. ping on green.

How do I resolve a conflict? I tried doing it manually but didn't work.

[simonjayhawkins]

How do I resolve a conflict? I tried doing it manually but didn't work.

see https://dev.pandas.io/docs/development/contributing.html#updating-your-pull-request

git fetch upstream
git merge upstream/master

[punndcoder28]

I did what was mentioned here

ci is failing, see https://dev.pandas.io/docs/development/contributing.html#python-pep8-black

But still the tests are failing.

[jbrockmendel]

You need to run isort pandas/tests/arrays/categorical/test_api.py

[simonjayhawkins]

@punndcoder28 lgtm. @jreback

[WillAyd]

Hmm is this actually changing anything on master? I ran one of the test cases but already see the intended result?

>>> pd.Categorical(["a", "b", "a"]).remove_categories(["c", "c"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/williamayd/clones/pandas/pandas/core/arrays/categorical.py", line 1132, in remove_categories
    raise ValueError(msg.format(not_included=not_included))
ValueError: removals must all be in old categories: {'c'}

[simonjayhawkins]

it's changeing the output of cat = cat.remove_categories(["c", np.nan])

from ValueError: removals must all be in old categories: ['c']

to ValueError: removals must all be in old categories: {'c'}

for all other cases the message remain unchanged and tests are added for these. i.e.

cat = cat.remove_categories(["c"])
cat = cat.remove_categories("c")
cat = cat.remove_categories(["c", "c"])

[WillAyd]

Ah OK thanks for clarifying @simonjayhawkins

[WillAyd]

Thanks @punndcoder28