Repository: modin-project/modin
Pull request: FIX-#3736: Don't use a 2-d array to set a categorical column.
Signed-off-by: mvashishtha <mahesh@ponder.io>

<!--
Thank you for your contribution!
Please review the contributing docs: https://modin.readthedocs.io/en/latest/CONTRIBUTING.html
if you have questions about contributing.
-->

## What do these changes do?

Work around https://github.com/pandas-dev/pandas/issues/44703 by not converting a categorical scalar to a 2-d array to set a categorical column.

- [x] commit message follows format outlined [here](https://modin.readthedocs.io/en/latest/contributing.html)
- [x] passes `flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py`
- [x] passes `black --check modin/ asv_bench/benchmarks scripts/doc_checker.py`
- [x] signed commit with `git commit -s` <!-- you can amend your commit with a signature via `git commit -amend -s` -->
- [x] Resolves #3736 
- [x] tests added and passing
- [x] module layout described at `docs/developer/architecture.rst` is up-to-date <!-- if you have added, renamed or removed files or directories please update the documentation accordingly -->


Problem statement:
Inconsistent behavior with Categorical.add_categories
### System information
- **OS Platform and Distribution (e.g., Linux Ubuntu 16.04)**: OSX 11.6
- **Modin version** (`modin.__version__`): 0.11.3
- **Python version**: 3.8.11
- **Code we can use to reproduce**:

The following code gives different results between pandas as modin:

```python
import pandas as pd
# import ray
# ray.init()
# import modin.pandas as pd
import uuid

df = pd.DataFrame(
            data=[dict(id=str(uuid.uuid4())) for _ in range(10000)]
        )
df.set_index('id', inplace=True, drop=True)
df['status'] = pd.Series([""]*len(df), dtype="category", index=df.index)
df.status = df.status.cat.add_categories('occupied')
df.loc[list(df.index)[:1000], 'status'] = 'occupied'

print(df)
```

I use `df.status = df.status.cat.add_categories` rather than using the `inplace` param as that is being deprecated in Pandas.

Replace the pandas import is modin and see the following error:

```fish
UserWarning: Distributing <class 'list'> object. This may take some time.
UserWarning: Distributing <class 'list'> object. This may take some time.
UserWarning: `Series.<lambda>` defaulting to pandas implementation.
To request implementation, send an email to feature_requests@modin.org.
FutureWarning: The `inplace` parameter in pandas.Categorical.add_categories is deprecated and will be removed in a future version. Removing unused categories will always return a new Categorical object.
UserWarning: Distributing <class 'pandas.core.series.Series'> object. This may take some time.
Traceback (most recent call last):
  File "test.py", line 15, in <module>
    print(df)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/pandas/base.py", line 3054, in __str__
    return repr(self)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/pandas/dataframe.py", line 210, in __repr__
    result = repr(self._build_repr_df(num_rows, num_cols))
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/pandas/base.py", line 197, in _build_repr_df
    return self.iloc[indexer]._query_compiler.to_pandas()
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/storage_formats/pandas/query_compiler.py", line 255, in to_pandas
    return self._modin_frame.to_pandas()
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/dataframe/pandas/dataframe/dataframe.py", line 2254, in to_pandas
    df = self._partition_mgr_cls.to_pandas(self._partitions)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/dataframe/pandas/partitioning/partition_manager.py", line 644, in to_pandas
    retrieved_objects = [[obj.to_pandas() for obj in part] for part in partitions]
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/dataframe/pandas/partitioning/partition_manager.py", line 644, in <listcomp>
    retrieved_objects = [[obj.to_pandas() for obj in part] for part in partitions]
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/dataframe/pandas/partitioning/partition_manager.py", line 644, in <listcomp>
    retrieved_objects = [[obj.to_pandas() for obj in part] for part in partitions]
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py", line 193, in to_pandas
    dataframe = self.get()
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py", line 75, in get
    return ray.get(self.oid)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/ray/_private/client_mode_hook.py", line 105, in wrapper
    return func(*args, **kwargs)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/ray/worker.py", line 1625, in get
    raise value.as_instanceof_cause()
ray.exceptions.RayTaskError(ValueError): ray::apply_list_of_funcs() (pid=38681, ip=127.0.0.1)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/storage_formats/pandas/query_compiler.py", line 3080, in iloc_mut
    partition.iloc[row_internal_indices, col_internal_indices] = item
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/indexing.py", line 723, in __setitem__
    iloc._setitem_with_indexer(indexer, value, self.name)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/indexing.py", line 1732, in _setitem_with_indexer
    self._setitem_single_block(indexer, value, name)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/indexing.py", line 1968, in _setitem_single_block
    self.obj._mgr = self.obj._mgr.setitem(indexer=indexer, value=value)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/internals/managers.py", line 355, in setitem
    return self.apply("setitem", indexer=indexer, value=value)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/internals/managers.py", line 327, in apply
    applied = getattr(b, f)(**kwargs)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/internals/blocks.py", line 1491, in setitem
    self.values[indexer] = value
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/arrays/_mixins.py", line 182, in __setitem__
    value = self._validate_setitem_value(value)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/arrays/categorical.py", line 2044, in _validate_setitem_value
    raise ValueError(
ValueError: Cannot setitem on a Categorical with a new category, set the categories first

During handling of the above exception, another exception occurred:

ray::apply_list_of_funcs() (pid=38681, ip=127.0.0.1)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/execution/ray/implementations/pandas_on_ray/partitioning/partition.py", line 484, in apply_list_of_funcs
    partition = func(partition.copy(), *args, **kwargs)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/modin/core/storage_formats/pandas/query_compiler.py", line 3080, in iloc_mut
    partition.iloc[row_internal_indices, col_internal_indices] = item
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/indexing.py", line 723, in __setitem__
    iloc._setitem_with_indexer(indexer, value, self.name)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/indexing.py", line 1732, in _setitem_with_indexer
    self._setitem_single_block(indexer, value, name)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/indexing.py", line 1968, in _setitem_single_block
    self.obj._mgr = self.obj._mgr.setitem(indexer=indexer, value=value)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/internals/managers.py", line 355, in setitem
    return self.apply("setitem", indexer=indexer, value=value)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/internals/managers.py", line 327, in apply
    applied = getattr(b, f)(**kwargs)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/internals/blocks.py", line 1491, in setitem
    self.values[indexer] = value
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/arrays/_mixins.py", line 182, in __setitem__
    value = self._validate_setitem_value(value)
  File "/Users/chrisgoddard/Code/modin-test/.env/lib/python3.8/site-packages/pandas/core/arrays/categorical.py", line 2044, in _validate_setitem_value
    raise ValueError(
ValueError: Cannot setitem on a Categorical with a new category, set the categories first
```

Interestingly, when I run the code below and only update a single row, I don't get the error:
```python
import ray
ray.init()
import modin.pandas as pd
import uuid

df = pd.DataFrame(
            data=[dict(id=str(uuid.uuid4())) for _ in range(10000)]
        )
df.set_index('id', inplace=True, drop=True)
df['status'] = pd.Series([""]*len(df), dtype="category", index=df.index)
df.status = df.status.cat.add_categories('occupied')
df.loc[list(df.index)[:1], 'status'] = 'occupied'

print(df)
```

Also, there error doesn't occur when the categorical value is set (df.loc... = 'occupied') - but when the dataframe itself is printed 

Appreciate any help you can provide!

Hints from issue discussion:
Thanks @chrisgoddard for the report!

The updated category does not appear to be propagating in the first case. We'll get someone on this, thanks for the detailed report!

There's also probably a simple workaround, will get that to you so you don't have to wait for the release. Thanks again!
Thanks @devin-petersohn - for now I implemented by own simple workaround using an int-type and my own lookup dictionary but it's not ideal so any other workaround suggestions would be appreciated!
I looked at this for a while and made some progress, but I still don't see how to fix the bug. It turns out that the problem is not with `add_categories`, as this pair of minimal examples shows:

```python
import modin.pandas as pd
df = pd.DataFrame({"status": ["a", "b", "c"]},  dtype="category")
df.loc[:1, "status"] = "a"
df
```

in Modin fails with the error `ValueError: Cannot setitem on a Categorical with a new category, set the categories first`

but in Pandas:

```python
import pandas as pd
df = pd.DataFrame({"status": ["a", "b", "c"]},  dtype="category")
df.loc[:1, "status"] = "a"
df
```

the assignment works fine. Looking deeper into the Modin code, the assigned `"a"` gets converted to two-dimensional array [here](https://github.com/modin-project/modin/blob/75f698cdcb63a027a4fe5032755ac2502ed5c763/modin/pandas/indexing.py#L336). Each partition uses `iloc` to set values on a single pandas dataframe [here](https://github.com/modin-project/modin/blob/75f698cdcb63a027a4fe5032755ac2502ed5c763/modin/core/storage_formats/pandas/query_compiler.py#L3080). For the Modin example above, the row indices are `array([0, 1])`, the column indices are `array([0])`, and the `item` is `array([['a'], ['a']], dtype='<U1')`, where for all variables `array` is `numpy.ndarray`. We can verify that that `iloc` should fail with the same error:

```python
import pandas as pd
from numpy import array
df = pd.DataFrame({"status": ["a", "b", "c"]},  dtype="category")
df.iloc[array([0, 1]), array([0])] = array([['a'], ['a']])
```

This last error could be a bug in pandas. Note that if I just add another column to the dataframe, it works:

```python
import pandas as pd
from numpy import array
df = pd.DataFrame({"status": ["a", "b", "c"], "status2": ["d", "e", "f"]},  dtype="category")
df.iloc[array([0, 1]), array([0])] = array([['a'], ['a']])
```

If this is not a Pandas bug, we need to assign values with something other than `iloc` in `__setitem__`. I filed https://github.com/pandas-dev/pandas/issues/44703 with Pandas for now.

Commit message: FIX-#3736: Don't use a 2-d array to set a categorical column.

Signed-off-by: mvashishtha <mahesh@ponder.io>

Patch to review:
diff --git a/modin/core/dataframe/pandas/partitioning/partition_manager.py b/modin/core/dataframe/pandas/partitioning/partition_manager.py
index 8aa4fc69600..92089d6f8b5 100644
--- a/modin/core/dataframe/pandas/partitioning/partition_manager.py
+++ b/modin/core/dataframe/pandas/partitioning/partition_manager.py
@@ -1201,11 +1201,23 @@ def compute_part_size(indexer, remote_part, part_idx, axis):
                     col_internal_idx, remote_part, col_idx, axis=1
                 )
 
+                # We want to eventually make item_to_distribute an np.ndarray,
+                # but that doesn't work for setting a subset of a categorical
+                # column, as per https://github.com/modin-project/modin/issues/3736.
+                # In that case, `item` is not an ndarray but instead some
+                # categorical variable, which we we don't need to distribute
+                # at all. Note that np.ndarray is not hashable, so it can't
+                # be a categorical variable.
+                # TODO(https://github.com/pandas-dev/pandas/issues/44703): Delete
+                # this special case once the pandas bug is fixed.
                 if item_to_distribute is not None:
-                    item = item_to_distribute[
-                        row_position_counter : row_position_counter + row_offset,
-                        col_position_counter : col_position_counter + col_offset,
-                    ]
+                    if isinstance(item_to_distribute, np.ndarray):
+                        item = item_to_distribute[
+                            row_position_counter : row_position_counter + row_offset,
+                            col_position_counter : col_position_counter + col_offset,
+                        ]
+                    else:
+                        item = item_to_distribute
                     item = {"item": item}
                 else:
                     item = {}
diff --git a/modin/pandas/indexing.py b/modin/pandas/indexing.py
index d1db847213e..02d57b622ad 100644
--- a/modin/pandas/indexing.py
+++ b/modin/pandas/indexing.py
@@ -385,7 +385,34 @@ def __setitem__(self, row_lookup, col_lookup, item, axis=None):
             else:
                 new_col_len = len(col_lookup)
             to_shape = new_row_len, new_col_len
-            item = self._broadcast_item(row_lookup, col_lookup, item, to_shape)
+            # If we are asssigning to a single categorical column, we won't be
+            # able to set the column to a 2-d array due to a bug in Pandas.
+            # Until that bug is fixed, don't convert `item` to a 2-d array
+            # in that case.
+            # TODO(https://github.com/pandas-dev/pandas/issues/44703): Delete
+            # this special case once the pandas bug is fixed.
+            assigning_to_single_category_column = new_col_len == 1 and (
+                # Case 1: df = pd.DataFrame({"status": ["a", "b", "c"]},
+                #                           dtype="category")
+                # Then type(df.dtypes) is pandas.core.series.Series
+                (
+                    type(self.df.dtypes) == pandas.core.series.Series
+                    and self.df.dtypes[col_lookup][0].name == "category"
+                )
+                # Case 2: df = pd.Series( ["a", "b", "c"],  dtype="category")
+                # Then df.dtypes ==  CategoricalDtype(categories=['a', 'b', 'c'],
+                #                                     ordered=False)
+                # and type(df.dtypes) is pandas.core.dtypes.dtypes.CategoricalDtype.
+                # Case 3:  df = pd.Series([0, 1, 2], index=['a', 'b', 'c'])
+                # Then df.dtypes == dtype('int64') and type(df.dtypes) is
+                # numpy.dtype
+                or (
+                    hasattr(type(self.df.dtypes), "name")
+                    and self.df.dtypes.name == "category"
+                )
+            )
+            if not assigning_to_single_category_column:
+                item = self._broadcast_item(row_lookup, col_lookup, item, to_shape)
             self._write_items(row_lookup, col_lookup, item)
 
     def _broadcast_item(self, row_lookup, col_lookup, item, to_shape):
diff --git a/modin/pandas/test/dataframe/test_indexing.py b/modin/pandas/test/dataframe/test_indexing.py
index e7998c9cd84..818d1980e23 100644
--- a/modin/pandas/test/dataframe/test_indexing.py
+++ b/modin/pandas/test/dataframe/test_indexing.py
@@ -372,6 +372,15 @@ def test_loc(data):
         modin_df.loc["NO_EXIST"]
 
 
+# This tests the bug from https://github.com/modin-project/modin/issues/3736
+def test_loc_setting_single_categorical_column():
+    modin_df = pd.DataFrame({"status": ["a", "b", "c"]}, dtype="category")
+    pandas_df = pandas.DataFrame({"status": ["a", "b", "c"]}, dtype="category")
+    modin_df.loc[1:3, "status"] = "a"
+    pandas_df.loc[1:3, "status"] = "a"
+    df_equals(modin_df, pandas_df)
+
+
 def test_loc_multi_index():
     modin_df = pd.read_csv(
         "modin/pandas/test/data/blah.csv", header=[0, 1, 2, 3], index_col=0
diff --git a/modin/pandas/test/test_series.py b/modin/pandas/test/test_series.py
index 887e4c6a8d1..3c03ca243b3 100644
--- a/modin/pandas/test/test_series.py
+++ b/modin/pandas/test/test_series.py
@@ -2105,6 +2105,15 @@ def test_loc(data):
     df_equals(modin_result, pandas_result)
 
 
+# This tests the bug from https://github.com/modin-project/modin/issues/3736
+def test_loc_setting_categorical_series():
+    modin_series = pd.Series(["a", "b", "c"], dtype="category")
+    pandas_series = pandas.Series(["a", "b", "c"], dtype="category")
+    modin_series.loc[1:3] = "a"
+    pandas_series.loc[1:3] = "a"
+    df_equals(modin_series, pandas_series)
+
+
 @pytest.mark.parametrize("data", test_data_values, ids=test_data_keys)
 def test_lt(data):
     modin_series, pandas_series = create_test_series(data)


Code review comments:
diff_hunk: @@ -385,7 +385,34 @@ def __setitem__(self, row_lookup, col_lookup, item, axis=None):
             else:
                 new_col_len = len(col_lookup)
             to_shape = new_row_len, new_col_len
-            item = self._broadcast_item(row_lookup, col_lookup, item, to_shape)
+            # If we are asssigning to a single categorical column, we won't be
+            # able to set the column to a 2-d array due to a bug in Pandas.
+            # Until that bug is fixed, don't convert `item` to a 2-d array
+            # in that case.
+            # TODO(https://github.com/pandas-dev/pandas/issues/44703): Delete
+            # this special case once the pandas bug is fixed.
+            assigning_to_single_category_column = new_col_len == 1 and (
+                # Case 1: df = pd.DataFrame({"status": ["a", "b", "c"]},
+                #                           dtype="category")
+                # Then type(df.dtypes) is pandas.core.series.Series
+                (
+                    type(self.df.dtypes) == pandas.core.series.Series
path: modin/pandas/indexing.py
text: @user1:
```suggestion
                    isinstance(self.df.dtypes, pandas.core.series.Series)
```
?

diff_hunk: @@ -385,7 +385,34 @@ def __setitem__(self, row_lookup, col_lookup, item, axis=None):
             else:
                 new_col_len = len(col_lookup)
             to_shape = new_row_len, new_col_len
-            item = self._broadcast_item(row_lookup, col_lookup, item, to_shape)
+            # If we are asssigning to a single categorical column, we won't be
+            # able to set the column to a 2-d array due to a bug in Pandas.
+            # Until that bug is fixed, don't convert `item` to a 2-d array
+            # in that case.
+            # TODO(https://github.com/pandas-dev/pandas/issues/44703): Delete
+            # this special case once the pandas bug is fixed.
+            assigning_to_single_category_column = new_col_len == 1 and (
+                # Case 1: df = pd.DataFrame({"status": ["a", "b", "c"]},
+                #                           dtype="category")
+                # Then type(df.dtypes) is pandas.core.series.Series
+                (
+                    type(self.df.dtypes) == pandas.core.series.Series
+                    and self.df.dtypes[col_lookup][0].name == "category"
+                )
+                # Case 2: df = pd.Series( ["a", "b", "c"],  dtype="category")
+                # Then df.dtypes ==  CategoricalDtype(categories=['a', 'b', 'c'],
+                #                                     ordered=False)
+                # and type(df.dtypes) is pandas.core.dtypes.dtypes.CategoricalDtype.
+                # Case 3:  df = pd.Series([0, 1, 2], index=['a', 'b', 'c'])
+                # Then df.dtypes == dtype('int64') and type(df.dtypes) is
+                # numpy.dtype
+                or (
+                    hasattr(type(self.df.dtypes), "name")
+                    and self.df.dtypes.name == "category"
path: modin/pandas/indexing.py
text: @user1:
```suggestion
                    getattr(self.df.dtypes, "name", "") == "category"
```
?