Add fix to raise error when category value 'x' is not predefined but is assigned through df.loc[..]=x
- [X] closes #25383
- [X] tests passed, new test added
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
To be honest I'm not quite happy with my solution. Do you think there's a nicer way to include this fix specifically to the categorical module instead? It feels like that would require some major refactoring though. Any opinion? > To be honest I'm not quite happy with my solution. Do you think there's a nicer way to include this fix specifically to the categorical module instead? It feels like that would require some major refactoring though. Any opinion?

see https://github.com/pandas-dev/pandas/issues/25383#issuecomment-465630713

from #25383, the expected behaviour is 

> Keep the categorical dtype if the added value is in the list of categories, throw an error/warning otherwise.

along with the test added here to confirm the an error raised, could also add test to confirm that unused categories are retained.  Hello @chrispe! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2021-03-13 20:30:26 UTC > > To be honest I'm not quite happy with my solution. Do you think there's a nicer way to include this fix specifically to the categorical module instead? It feels like that would require some major refactoring though. Any opinion?
> 
> see [#25383 (comment)](https://github.com/pandas-dev/pandas/issues/25383#issuecomment-465630713)
> 
> from #25383, the expected behaviour is
> 
> > Keep the categorical dtype if the added value is in the list of categories, throw an error/warning otherwise.
> 
> along with the test added here to confirm the an error raised, could also add test to confirm that unused categories are retained.

I've included the test case you described. What are we looking to achieve with this PR at the end? My current changes resolve the issue of assigning a non-existing categorical value to a Categorical series, but then of course that doesn't resolve the core issue mentioned at https://github.com/pandas-dev/pandas/issues/25383. > I've included the test case you described. What are we looking to achieve with this PR at the end? My current changes resolve the issue of assigning a non-existing categorical value to a Categorical series, but then of course that doesn't resolve the core issue mentioned at #25383.

Thinking some more, I'm not sure if raising should be the expected behaviour. This happens at the array level, which is fine for the array as the dtype holds the categories and assigning values directly to the array that are not in the categories should raise.

however, for Series, if the dtype cannot hold an element a new array is created.

```
>>> ser = pd.Series([1, 2])
>>> ser
0    1
1    2
dtype: int64
>>>
>>> ser.values
array([1, 2], dtype=int64)
>>>
>>> id1 = id(ser.values)
>>> id1
2687613117264
>>>
>>> ser.loc[1] = 42
>>> ser
0     1
1    42
dtype: int64
>>>
>>> id(ser.values) == id1
True
>>>
>>> ser.loc[1] = "42"
>>> ser
0     1
1    42
dtype: object
>>>
>>> id(ser.values) == id1
False
>>>
```

For a performant operation, that does not create a new array, but updates the array in place and errors if the underlying array cannot hold the elements is Series.at. 

I would therefore expect Series.at to raise ValueError: Cannot setitem on a Categorical with a new...

but I would expect Series.loc to create a new array that can hold the new element with the correct categories. This issue needs further discussion IMO.

 I'm a bit confused about the expected behaviour. In the current version this piece of code has the following result:
```python
>>> import pandas as pd
>>> from pandas import CategoricalDtype
>>> s_data = list("abcd")
>>> s_dtype = CategoricalDtype(list("abc"))
>>> s = pd.Series(s_data, dtype=s_dtype)
>>> s
0      a
1      b
2      c
3    NaN
dtype: category
Categories (3, object): [a, b, c]

>>> # This should raise an exception after the PR
>>> s.loc[4] = "d"
>>> s
0      a
1      b
2      c
3    NaN
4      d
dtype: object
```
Given that the `s` expects only values of `[a,b,c]` because of it's dtype, should we instead raise the same `ValueError` about non-invalid category value? If not, what's the logic behind the current behaviour? It's a bit contradictory to what we are trying to achieve with raising an exception (instead of replacing the new value with nan) when a non-existing category is inserted through index expansion. > Given that the `s` expects only values of `[a,b,c]` because of it's dtype, should we instead raise the same `ValueError` about non-invalid category value? If not, what's the logic behind the current behaviour? It's a bit contradictory to what we are trying to achieve with raising an exception (instead of replacing the new value with nan) when a non-existing category is inserted through index expansion.

so we have three possible behaviours?

1. raise if values not in categories
2. assign np.nan if values not in categories.
3. create a new array with new categories to hold new value.

and we have at least three operations where this applies

1. Series construction
2. assigning a new value to an existing Series element
3. Series expansion

The Series construction case is explicitly passing a dtype and so coercion of the values is expected. Assigning np.nan does seem reasonable but also raising is also a reasonable alternative. There are a couple of open issues regarding constructors, #24435 and #21311.

For this issue, we should maybe ignore the Series constructor for now, particularly since we are explicitly passing a dtype and therefore it is reasonable to allow a different behaviour and just focus on the two Series.\_\_setitem__ cases.

we need to step back slightly and consider a Series to to dictionary like, where the index is the keys, whereas the underlying CategorialArray is a sequence.

on master, the current behaviour is

setting an item for an existing key with a value not in the categories raises `ValueError: Cannot setitem on a Categorical with a new category, set the categories first`

as mentioned in https://github.com/pandas-dev/pandas/pull/34011#issuecomment-632150450, this behaviour for categorical is inconsistent with a numeric Series.

setting an item for an non-existing key with a value not in the categories, returns a Series with object dtype.

as mentioned in #25383, the current behaviour is unexpected.

issue #33952 was raised because of the inconsistency that currently exists, but in order to be consistent, we should also need to establish to correct behaviour.

> 1. raise if values not in categories

this is stated as the expected behaviour in both #33952 and #25383

> 2. assign np.nan if values not in categories.

This _may_ be OK in Series construction when a dtype is explicitly given, but for Series.\_\_setitem__, silently converting values to np.nan is probably not wanted.

> 3. create a new array with new categories to hold new value.

this would be consistent with numeric Series


 > > Given that the `s` expects only values of `[a,b,c]` because of it's dtype, should we instead raise the same `ValueError` about non-invalid category value? If not, what's the logic behind the current behaviour? It's a bit contradictory to what we are trying to achieve with raising an exception (instead of replacing the new value with nan) when a non-existing category is inserted through index expansion.
> 
> so we have three possible behaviours?
> 
> 1. raise if values not in categories
> 2. assign np.nan if values not in categories.
> 3. create a new array with new categories to hold new value.
> 
> and we have at least three operations where this applies
> 
> 1. Series construction
> 2. assigning a new value to an existing Series element
> 3. Series expansion
> 
> The Series construction case is explicitly passing a dtype and so coercion of the values is expected. Assigning np.nan does seem reasonable but also raising is also a reasonable alternative. There are a couple of open issues regarding constructors, #24435 and #21311.
> 
> For this issue, we should maybe ignore the Series constructor for now, particularly since we are explicitly passing a dtype and therefore it is reasonable to allow a different behaviour and just focus on the two Series.__setitem__ cases.
> 
> we need to step back slightly and consider a Series to to dictionary like, where the index is the keys, whereas the underlying CategorialArray is a sequence.
> 
> on master, the current behaviour is
> 
> setting an item for an existing key with a value not in the categories raises `ValueError: Cannot setitem on a Categorical with a new category, set the categories first`
> 
> as mentioned in [#34011 (comment)](https://github.com/pandas-dev/pandas/pull/34011#issuecomment-632150450), this behaviour for categorical is inconsistent with a numeric Series.
> 
> setting an item for an non-existing key with a value not in the categories, returns a Series with object dtype.
> 
> as mentioned in #25383, the current behaviour is unexpected.
> 
> issue #33952 was raised because of the inconsistency that currently exists, but in order to be consistent, we should also need to establish to correct behaviour.
> 
> > 1. raise if values not in categories
> 
> this is stated as the expected behaviour in both #33952 and #25383
> 
> > 1. assign np.nan if values not in categories.
> 
> This _may_ be OK in Series construction when a dtype is explicitly given, but for Series.__setitem__, silently converting values to np.nan is probably not wanted.
> 
> > 1. create a new array with new categories to hold new value.
> 
> this would be consistent with numeric Series

When applying index expansion the `Categorical` constructor is called (through the `_setitem_with_indexer_missing`; no call is made to `setitem`). As you mentioned, the constructor by default replaces the non-existing categories with `np.nan`. Therefore, it's not able to catch the case of including a non-existing categorical value through the index expansion, but to instead replace it with a missing value. And given that, I think it makes sense to stick to a single behaviour for all of the three cases you mentioned.

Index expansion also makes it very difficult to identify for which of the solutions we should go for (during runtime), since at any of the operations (series construction, expansion etc.) the same constructor function is used and in that phase you already luck the information of if that new object is initiated to construct a new Category series, or to "expand" an existing one (which means you cannot handle those two cases differently in the constructor).

(this is all assuming https://github.com/pandas-dev/pandas/issues/25383 is resolved: category dtypes don't turn to object dtypes through index expansion). @chrispe92 Can you merge master to fix the conflict? > @chrispe92 Can you merge master to fix the conflict?

Hi @dsaxton, it's done @chrispe92 also a number of comments. > @chrispe92 also a number of comments.

@jreback Most of them should be resolved now. Sorry it took so long. 

I've also changed the PR to point to https://github.com/pandas-dev/pandas/issues/25383 instead of https://github.com/pandas-dev/pandas/issues/33952, because that was considered a duplicate. 

The tests are still relevant, but the current approach to resolving the issue has to change as you also mentioned. 
Any suggestions about that? @simonjayhawkins also purposed three different ways to approach this, but I don't think we've ended up to any decision. Made some new changes. Please check my last comment https://github.com/pandas-dev/pandas/pull/34011#discussion_r507150195 about them. can  you merge master this is *way* too complicated and a fragile code change. 

You might be able to not touch much of the indexing code and instead have concat correctly handling appending a value not in the category. These are de-facto the same case. > this is _way_ too complicated and a fragile code change.
> 
> You might be able to not touch much of the indexing code and instead have concat correctly handling appending a value not in the category. These are de-facto the same case.

Ok, will try that instead and will keep you posted. @jreback : I have noticed a couple of tests in the master which partially contradict what we are looking to achieve with this MR (at least through the concat approach). Here is an example from `pandas.tests.reshape.concat.test_append_common`:

```python
# category + not-category => not-category
s1 = Series([1, 2, np.nan], dtype="category")
s2 = Series([2, 1, 2])

exp = Series([1, 2, np.nan, 2, 1, 2], dtype="object")
tm.assert_series_equal(pd.concat([s1, s2], ignore_index=True), exp)
```
The above test shows that preserving the categorical dtype is not expected when concatenating. 
So, why should this behaviour be different in the case of index expansion (which at the end is also a concat 
with an array of a single element)? 

Do we want to achieve both behaviours or convert only to one of them? If both, then it would mean that arrays of a single element should be handled differently in the `concat_compat` function (or revert back to the solution through indexing). Is that right, or am I missing something? > @jreback : I have noticed a couple of tests in the master which partially contradict what we are looking to achieve with this MR (at least through the concat approach). Here is an example from `pandas.tests.reshape.concat.test_append_common`:
> 
> ```python
> # category + not-category => not-category
> s1 = Series([1, 2, np.nan], dtype="category")
> s2 = Series([2, 1, 2])
> 
> exp = Series([1, 2, np.nan, 2, 1, 2], dtype="object")
> tm.assert_series_equal(pd.concat([s1, s2], ignore_index=True), exp)
> ```
> 
> The above test shows that preserving the categorical dtype is not expected when concatenating.
> So, why should this behaviour be different in the case of index expansion (which at the end is also a concat
> with an array of a single element)?
> 
> Do we want to achieve both behaviours or convert only to one of them? If both, then it would mean that arrays of a single element should be handled differently in the `concat_compat` function (or revert back to the solution through indexing). Is that right, or am I missing something?

concatenation is different that indexing. I think raising on indexing is ok. @chrispe in order to merge this if you can respond to my comments > @chrispe in order to merge this if you can respond to my comments

Hi @jreback, I'm not sure I completely understand your last comment about the raising. Can you elaborate a bit more? Thanks. if u r trying to add a non existent category (eg a value not in the categories) it should raise rather than coerce to object Hi @jreback. I did change my approach, but I does feel like I'm going in circles in terms of complexity, so I may need some further guidance. The thing is that the `find_common_type` does not take into account the values of the arrays, so we cannot conclude if we should raise an error given only their dtypes.

For example, this should raise an error:
```python
ser = pd.Series(Categorical(["a", "b", "c"]))
ser.loc[3] = "d"
# ValueError "Cannot setitem on a Categorical with a new category, set the categories first"
```
While this should be fine:
```python
ser = pd.Series(Categorical(["a", "b", "c"]))
ser.loc[3] = "a"
``` closing as stale, if you want to continue working, please ping.