Repository: pyjanitor-devs/pyjanitor (python)

Issue:
Incorrectly modified output of `count_cumulative_unique` when `case_sensitive` is turned off
# Brief Description

The `case_sensitive` parameter of `count_cumulative_unique` when set to False, would modify the column which is being counted, and this change is then (wrongly) propagated to the output.

The implementation is currently written like so:
```python
    if not case_sensitive:
        # Make it so that the the same uppercase and lowercase
        # letter are treated as one unique value
        df[column_name] = df[column_name].astype(str).map(str.lower)
```
Which modifies the column_name for the intermediate calculation of `count_cumulative_unique`, which is incorrect. See MWE.

# Minimally Reproducible Code

```python
In [4]: df = pd.DataFrame({"ccu": np.array(["a", "b", "c", "A", "B", "C", "a", "b", "c"])})

In [5]: df
Out[5]: 
  ccu
0   a
1   b
2   c
3   A
4   B
5   C
6   a
7   b
8   c

In [6]: df.count_cumulative_unique("ccu", "counting", case_sensitive=False)
Out[6]: 
  ccu  counting
0   a         1
1   b         2
2   c         3
3   a         3
4   b         3
5   c         3
6   a         3
7   b         3
8   c         3
```
Note that the `ccu` column in the original df has upper case letters, but the `ccu` column in the output does not. Changes made to the dataframe for intermediate calculations (i.e., `str.lower()`) should not be propagated to the final output.

Pull request:
[ENH/DOC] Fix case_sensitive bug in `count_cumulative_unique` and add MWE
# PR Description

Please describe the changes proposed in the pull request:

- Fixed bug: `count_cumulative_unique` now no longer modifies the column being counted in the output when `case_sensitive` argument is set to False, resolves #1065. See the bug description in the linked issue.
- And add tests to avoid similar regression in the future.
- Add MWE, related to #972 .



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 53e9a2c..3a9b715 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,6 +24,7 @@
 -   [ENH] Let `filter_string` suit parameters of `Series.str.contains` Issue #1003 and #1047. @Zeroto521
 -   [ENH] `names_glue` in `pivot_wider` now takes a string form, using str.format_map under the hood. `levels_order` is also deprecated. @samukweku
 -   [BUG] Fixed bug in `transform_columns` which ignored the `column_names` specification when `new_column_names` dictionary was provided as an argument, issue #1063. @thatlittleboy
+-   [BUG] `count_cumulative_unique` no longer modifies the column being counted in the output when `case_sensitive` argument is set to False, issue #1065. @thatlittleboy
 
 ## [v0.22.0] - 2021-11-21
 
diff --git a/janitor/functions/count_cumulative_unique.py b/janitor/functions/count_cumulative_unique.py
index f18281e..7ae48b2 100644
--- a/janitor/functions/count_cumulative_unique.py
+++ b/janitor/functions/count_cumulative_unique.py
@@ -1,7 +1,12 @@
+"""Implementation of count_cumulative_unique."""
 from typing import Hashable
+
+import numpy as np
 import pandas_flavor as pf
 import pandas as pd
 
+from janitor.utils import check_column
+
 
 @pf.register_dataframe_method
 def count_cumulative_unique(
@@ -12,73 +17,91 @@ def count_cumulative_unique(
 ) -> pd.DataFrame:
     """Generates a running total of cumulative unique values in a given column.
 
-    Functional usage syntax:
-
-    ```python
-        import pandas as pd
-        import janitor as jn
-
-        df = pd.DataFrame(...)
-
-        df = jn.functions.count_cumulative_unique(
-            df=df,
-            column_name='animals',
-            dest_column_name='animals_unique_count',
-            case_sensitive=True
-        )
-    ```
-
-    Method chaining usage example:
-
-    ```python
-        import pandas as pd
-        import janitor
-
-        df = pd.DataFrame(...)
-
-        df = df.count_cumulative_unique(
-            column_name='animals',
-            dest_column_name='animals_unique_count',
-            case_sensitive=True
-        )
-    ```
-
     A new column will be created containing a running
     count of unique values in the specified column.
     If `case_sensitive` is `True`, then the case of
     any letters will matter (i.e., `a != A`);
     otherwise, the case of any letters will not matter.
 
-    This method mutates the original DataFrame.
-
-    :param df: A pandas dataframe.
-    :param column_name: Name of the column containing
-        values from which a running count of unique values
-        will be created.
+    This method does not mutate the original DataFrame.
+
+    Examples:
+
+        >>> import pandas as pd
+        >>> import janitor
+        >>> df = pd.DataFrame({
+        ...     "letters": list("aabABb"),
+        ...     "numbers": range(4, 10),
+        ... })
+        >>> df
+          letters  numbers
+        0       a        4
+        1       a        5
+        2       b        6
+        3       A        7
+        4       B        8
+        5       b        9
+        >>> df.count_cumulative_unique(
+        ...     column_name="letters",
+        ...     dest_column_name="letters_unique_count",
+        ... )
+          letters  numbers  letters_unique_count
+        0       a        4                     1
+        1       a        5                     1
+        2       b        6                     2
+        3       A        7                     3
+        4       B        8                     4
+        5       b        9                     4
+
+    Example: Cumulative counts, ignoring casing.
+
+        >>> df.count_cumulative_unique(
+        ...     column_name="letters",
+        ...     dest_column_name="letters_unique_count",
+        ...     case_sensitive=False,
+        ... )
+          letters  numbers  letters_unique_count
+        0       a        4                     1
+        1       a        5                     1
+        2       b        6                     2
+        3       A        7                     2
+        4       B        8                     2
+        5       b        9                     2
+
+    :param df: A pandas DataFrame.
+    :param column_name: Name of the column containing values from which a
+        running count of unique values will be created.
     :param dest_column_name: The name of the new column containing the
         cumulative count of unique values that will be created.
     :param case_sensitive: Whether or not uppercase and lowercase letters
-        will be considered equal (e.g., 'A' != 'a' if `True`).
-
+        will be considered equal. Only valid with string-like columns.
     :returns: A pandas DataFrame with a new column containing a cumulative
         count of unique values from another column.
+    :raises TypeError: If `case_sensitive` is False when counting a non-string
+        `column_name`.
     """
+    check_column(df, column_name)
+    check_column(df, dest_column_name, present=False)
 
+    counter = df[column_name]
     if not case_sensitive:
-        # Make it so that the the same uppercase and lowercase
-        # letter are treated as one unique value
-        df[column_name] = df[column_name].astype(str).map(str.lower)
-
-    df[dest_column_name] = (
-        (
-            df[[column_name]]
-            .drop_duplicates()
-            .assign(dummyabcxyz=1)
-            .dummyabcxyz.cumsum()
-        )
-        .reindex(df.index)
-        .ffill()
-        .astype(int)
+        try:
+            # Make it so that the the same uppercase and lowercase
+            # letter are treated as one unique value
+            counter = counter.str.lower()
+        except (AttributeError, TypeError) as e:
+            # AttributeError is raised by pandas when .str is used on
+            # non-string types, e.g. int.
+            # TypeError is raised by pandas when .str.lower is used on a
+            # forbidden string type, e.g. bytes.
+            raise TypeError(
+                "case_sensitive=False can only be used with a string-like "
+                f"type. Column {column_name} is {counter.dtype} type."
+            ) from e
+
+    counter = (
+        counter.groupby(counter, sort=False).cumcount().to_numpy(copy=False)
     )
+    counter = np.cumsum(counter == 0)
 
-    return df
+    return df.assign(**{dest_column_name: counter})


Test patch:
diff --git a/tests/functions/test_count_cumulative_unique.py b/tests/functions/test_count_cumulative_unique.py
index e15ad56..da934a2 100644
--- a/tests/functions/test_count_cumulative_unique.py
+++ b/tests/functions/test_count_cumulative_unique.py
@@ -1,23 +1,100 @@
+import numpy as np
+import pandas as pd
 import pytest
 
-from janitor.functions import count_cumulative_unique  # noqa: F401
+
+@pytest.mark.functions
+def test_column_name_not_exists(dataframe):
+    """Checks that an error is raised if the column to be counted does not exist
+    in df.
+    """
+    with pytest.raises(
+        ValueError,
+        match="_foo_ not present in dataframe columns",
+    ):
+        dataframe.count_cumulative_unique(
+            "_foo_",
+            dest_column_name="foo_count",
+        )
+
+
+@pytest.mark.functions
+@pytest.mark.parametrize("case_sensitive", [True, False])
+def test_original_column_values_not_altered(dataframe, case_sensitive):
+    """Checks that the column to be counted is not altered by the case-switching
+    logic implemented by `count_cumulative_unique`.
+    """
+    before = np.array(["a", "b", "c", "A", "B", "C", "a", "b", "c"])
+    dataframe["ccu"] = before
+
+    df = dataframe.count_cumulative_unique(
+        "ccu",
+        dest_column_name="ccu_count",
+        case_sensitive=case_sensitive,
+    )
+    after = df["ccu"].to_numpy()
+
+    assert (before == after).all()
 
 
 @pytest.mark.functions
 def test_case_sensitive(dataframe):
+    """Checks a new column is added containing the correct distinct count,
+    when case sensitive is True.
+    """
     dataframe["ccu"] = ["a", "b", "c", "A", "B", "C", "a", "b", "c"]
     df = dataframe.count_cumulative_unique(
-        "ccu", dest_column_name="ccu_count", case_sensitive=True
+        "ccu",
+        dest_column_name="ccu_count",
+        case_sensitive=True,
     )
-    # column ccu_count should contain [1,2,3,4,5,6,6,6,6]
+
     assert all(df["ccu_count"] == [1, 2, 3, 4, 5, 6, 6, 6, 6])
 
 
 @pytest.mark.functions
 def test_not_case_sensitive(dataframe):
+    """Checks a new column is added containing the correct distinct count,
+    when case sensitive is False.
+    """
     dataframe["ccu"] = ["a", "b", "c", "A", "B", "C", "a", "b", "c"]
     df = dataframe.count_cumulative_unique(
-        "ccu", dest_column_name="ccu_count", case_sensitive=False
+        "ccu",
+        dest_column_name="ccu_count",
+        case_sensitive=False,
     )
-    # column ccu_count should contain [1,2,3,3,3,3,3,3,3]
+
     assert all(df["ccu_count"] == [1, 2, 3, 3, 3, 3, 3, 3, 3])
+
+
+@pytest.mark.functions
+def test_not_case_sensitive_but_nonstring():
+    """Checks TypeError is raised if case sensitive is explicitly set to
+    False but the column being counted does not support `.str.lower()`.
+    """
+    df = pd.DataFrame(
+        {
+            "ok1": ["ABC", None, "zzz"],
+            "ok2": pd.Categorical(["A", "b", "A"], ordered=False),
+            "notok1": [1, 2, 3],
+            "notok2": [b"ABC", None, b"zzz"],
+        }
+    )
+
+    # acceptable string types
+    for okcol in ["ok1", "ok2"]:
+        _ = df.count_cumulative_unique(
+            okcol,
+            dest_column_name="ok_count",
+            case_sensitive=False,
+        )
+
+    # forbidden data types
+    msg = "case_sensitive=False can only be used with a string-like type.*"
+    for notokcol in ["notok1", "notok2"]:
+        with pytest.raises(TypeError, match=msg):
+            _ = df.count_cumulative_unique(
+                notokcol,
+                dest_column_name="notok_count",
+                case_sensitive=False,
+            )