Repository: koaning/bulk

Issue:
Color mapping crashes if too many classes defined
Looks like I crashed the color mapping with too many classes.

The palette could be upped to Category20, though this would still limit the number of classes a user can visualize. Whatever the limit is, there should be an error message with a graceful exit if the user tries to define more. I'd be happy to make a PR!

```Traceback (most recent call last):
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/tornado/web.py", line 1713, in _execute
    result = await result
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bokeh/server/views/doc_handler.py", line 54, in get
    session = await self.get_session()
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bokeh/server/views/session_handler.py", line 144, in get_session
    session = await self.application_context.create_session_if_needed(session_id, self.request, token)
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bokeh/server/contexts.py", line 243, in create_session_if_needed
    self._application.initialize_document(doc)
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bokeh/application/application.py", line 194, in initialize_document
    h.modify_document(doc)
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bokeh/application/handlers/function.py", line 143, in modify_document
    self._func(doc)
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bulk/text.py", line 27, in bkapp
    mapper, df = get_color_mapping(df)
  File ".pyenv/versions/rules-env/lib/python3.8/site-packages/bulk/utils.py", line 25, in get_color_mapping
    palette=Category10[len(all_values)],
KeyError: 16```

Discussion:

Patch:
diff --git a/bulk/_bokeh_utils.py b/bulk/_bokeh_utils.py
index 5276d85..67dd7f5 100644
--- a/bulk/_bokeh_utils.py
+++ b/bulk/_bokeh_utils.py
@@ -20,6 +20,9 @@ def get_color_mapping(df: pd.DataFrame) -> Tuple[Optional[bokeh.transform.transf
         all_values = list(df["color"].dropna().unique())
         if len(all_values) == 2:
             all_values.extend([""])
+        elif len(all_values) > len(Category10) + 2:
+            raise ValueError(f"Too many classes defined, the limit for visualisation is {len(Category10) + 2}. "
+                             f"Got {len(all_values)}.")
         mapper = factor_cmap(
             field_name="color",
             palette=Category10[len(all_values)],


Test patch:
diff --git a/tests/test_utils.py b/tests/test_utils.py
new file mode 100644
index 0000000..2b851b4
--- /dev/null
+++ b/tests/test_utils.py
@@ -0,0 +1,33 @@
+import pandas as pd
+import pytest
+
+from bulk._bokeh_utils import get_color_mapping
+
+MAX_DISCRETE_CLASSES = 10
+
+
+def _int_to_alpha(x: int) -> str:
+    # returns int mapped to char: 0: a, 1: b, 2: c
+    return chr(x + 97)
+
+
+def _create_dummy_df() -> pd.DataFrame:
+    df = pd.DataFrame()
+    df["color"] = [_int_to_alpha(i) for i in range(MAX_DISCRETE_CLASSES)]
+    return df
+
+
+def test_get_color_mapping_raises_error_on_too_many_classes():
+    df = _create_dummy_df()
+    additional_class = pd.DataFrame.from_records([{"color": _int_to_alpha(MAX_DISCRETE_CLASSES + 1)}])
+    df = pd.concat([df, additional_class], axis=0, ignore_index=True)
+    with pytest.raises(ValueError):
+        get_color_mapping(df)
+
+
+def test_get_color_mapping_doesnt_raise_error():
+    df = _create_dummy_df()
+    try:
+        get_color_mapping(df)
+    except ValueError as e:
+        assert False, f"get_color_mapping raised a ValueError: {e}"