Repository: pandas-dev/pandas
Pull request: Better error for str.cat with listlike of wrong dtype.
- [x] closes #22722
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This had been blocked on #23167.

Problem statement:
Improve TypeError message for str.cat
Currently, 

```
s = pd.Series(['a', 'b', 'c'])
s.str.cat([1, 2, 3])
```
yields
```
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Axel Obermeier\eclipse-workspace\pddev\pandas\core\strings.py", line 2222, in cat
    res = str_cat(data, others=others, sep=sep, na_rep=na_rep)
  File "C:\Users\Axel Obermeier\eclipse-workspace\pddev\pandas\core\strings.py", line 111, in str_cat
    cats = [sep.join(tup) for tup in tuples]
  File "C:\Users\Axel Obermeier\eclipse-workspace\pddev\pandas\core\strings.py", line 111, in <listcomp>
    cats = [sep.join(tup) for tup in tuples]
TypeError: sequence item 1: expected str instance, int found
```

IMO, this should be improved to have a better error message, and shallower stack trace.

Hints from issue discussion:
What are you suggesting exactly? Error message reflects what you would get from a standard Python operation:

```python
>>> "".join(['foo', 1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: sequence item 1: expected str instance, int found
```
@WillAyd 

That's of course correct, but users are not dealing with str/int instances, but usually with Series/ndarray, etc., and so the mentioned warning gives the right hint, but is not necessarily the best we can do. I opened this issue as it was a side product of cleaning up some internals/tests for #22725.
@jreback 

I'll open another PR for this, but this will not be closed by #22725 (was split off due to review)
@WillAyd The code for this was splitt off of #22725 to focus that PR (I had edited the OP to reflect that) - this issue is still open.

Commit message: Better TypeError for wrong dtype in str.cat

Patch to review:
diff --git a/doc/source/whatsnew/v0.25.0.rst b/doc/source/whatsnew/v0.25.0.rst
index 1619ba1a45739..3e8383c66abc2 100644
--- a/doc/source/whatsnew/v0.25.0.rst
+++ b/doc/source/whatsnew/v0.25.0.rst
@@ -575,7 +575,7 @@ Strings
 ^^^^^^^
 
 - Bug in the ``__name__`` attribute of several methods of :class:`Series.str`, which were set incorrectly (:issue:`23551`)
--
+- Improved error message when passing ``Series`` of wrong dtype to :meth:`Series.str.cat` (:issue:`22722`)
 -
 
 
diff --git a/pandas/core/strings.py b/pandas/core/strings.py
index bd756491abd2f..218577016fd0e 100644
--- a/pandas/core/strings.py
+++ b/pandas/core/strings.py
@@ -2280,6 +2280,23 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):
                                  'must all be of the same length as the '
                                  'calling Series/Index.')
 
+        # data has already been checked by _validate to be of correct dtype,
+        # but others could still have Series of dtypes (e.g. integers) which
+        # will necessarily fail in concatenation. To avoid deep and confusing
+        # traces, we raise here for anything that's not object or all-NA float.
+        def _legal_dtype(series):
+            # unify dtype handling between categorical/non-categorical
+            dtype = (series.dtype if not is_categorical_dtype(series)
+                     else series.cat.categories.dtype)
+            legal = dtype == 'O' or (dtype == 'float' and series.isna().all())
+            return legal
+        err_wrong_dtype = ('Can only concatenate list-likes containing only '
+                           'strings (or missing values).')
+        if any(not _legal_dtype(x) for x in others):
+            raise TypeError(err_wrong_dtype + ' Received list-like of dtype: '
+                            '{}'.format([x.dtype for x in others
+                                         if not _legal_dtype(x)][0]))
+
         if join is None and warn:
             warnings.warn("A future version of pandas will perform index "
                           "alignment when `others` is a Series/Index/"
@@ -2307,23 +2324,28 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):
         na_masks = np.array([isna(x) for x in all_cols])
         union_mask = np.logical_or.reduce(na_masks, axis=0)
 
-        if na_rep is None and union_mask.any():
-            # no na_rep means NaNs for all rows where any column has a NaN
-            # only necessary if there are actually any NaNs
-            result = np.empty(len(data), dtype=object)
-            np.putmask(result, union_mask, np.nan)
-
-            not_masked = ~union_mask
-            result[not_masked] = cat_core([x[not_masked] for x in all_cols],
-                                          sep)
-        elif na_rep is not None and union_mask.any():
-            # fill NaNs with na_rep in case there are actually any NaNs
-            all_cols = [np.where(nm, na_rep, col)
-                        for nm, col in zip(na_masks, all_cols)]
-            result = cat_core(all_cols, sep)
-        else:
-            # no NaNs - can just concatenate
-            result = cat_core(all_cols, sep)
+        # if there are any non-string, non-null values hidden within an object
+        # dtype, cat_core will fail; catch error and return with better message
+        try:
+            if na_rep is None and union_mask.any():
+                # no na_rep means NaNs for all rows where any column has a NaN
+                # only necessary if there are actually any NaNs
+                result = np.empty(len(data), dtype=object)
+                np.putmask(result, union_mask, np.nan)
+
+                not_masked = ~union_mask
+                result[not_masked] = cat_core([x[not_masked]
+                                               for x in all_cols], sep)
+            elif na_rep is not None and union_mask.any():
+                # fill NaNs with na_rep in case there are actually any NaNs
+                all_cols = [np.where(nm, na_rep, col)
+                            for nm, col in zip(na_masks, all_cols)]
+                result = cat_core(all_cols, sep)
+            else:
+                # no NaNs - can just concatenate
+                result = cat_core(all_cols, sep)
+        except TypeError:
+            raise TypeError(err_wrong_dtype)
 
         if isinstance(self._orig, Index):
             # add dtype for case that result is all-NA
diff --git a/pandas/tests/test_strings.py b/pandas/tests/test_strings.py
index 1ba0ef3918fb7..5011cead017c1 100644
--- a/pandas/tests/test_strings.py
+++ b/pandas/tests/test_strings.py
@@ -420,6 +420,22 @@ def test_str_cat_categorical(self, box, dtype_caller, dtype_target, sep):
             result = s.str.cat(t, sep=sep)
             assert_series_or_index_equal(result, expected)
 
+    # test integer/float dtypes (inferred by constructor) and mixed
+    @pytest.mark.parametrize('data', [[1, 2, 3], [.1, .2, .3], [1, 2, 'b']],
+                             ids=['integers', 'floats', 'mixed'])
+    # without dtype=object, np.array would cast [1, 2, 'b'] to ['1', '2', 'b']
+    @pytest.mark.parametrize('box', [Series, Index, list,
+                                     lambda x: np.array(x, dtype=object)],
+                             ids=['Series', 'Index', 'list', 'np.array'])
+    def test_str_cat_wrong_dtype_raises(self, box, data):
+        # GH 22722
+        s = Series(['a', 'b', 'c'])
+        t = box(data)
+
+        msg = 'Can only concatenate list-likes containing only strings.*'
+        with pytest.raises(TypeError, match=msg):
+            s.str.cat(t, join='left')
+
     @pytest.mark.parametrize('box', [Series, Index])
     def test_str_cat_mixed_inputs(self, box):
         s = Index(['a', 'b', 'c', 'd'])


Code review comments:
diff_hunk: @@ -575,7 +575,7 @@ Strings
 ^^^^^^^
 
 - Bug in the ``__name__`` attribute of several methods of :class:`Series.str`, which were set incorrectly (:issue:`23551`)
--
+- Improved error message when passing ``Series`` of wrong dtype to :meth:`Series.str.cat` (:issue:`22722`)
path: doc/source/whatsnew/v0.25.0.rst
text: @user1:
use ``:class:`Series` ``

@author:
No problem

diff_hunk: @@ -2307,23 +2324,28 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):
         na_masks = np.array([isna(x) for x in all_cols])
         union_mask = np.logical_or.reduce(na_masks, axis=0)
 
-        if na_rep is None and union_mask.any():
-            # no na_rep means NaNs for all rows where any column has a NaN
-            # only necessary if there are actually any NaNs
-            result = np.empty(len(data), dtype=object)
-            np.putmask(result, union_mask, np.nan)
-
-            not_masked = ~union_mask
-            result[not_masked] = cat_core([x[not_masked] for x in all_cols],
-                                          sep)
-        elif na_rep is not None and union_mask.any():
-            # fill NaNs with na_rep in case there are actually any NaNs
-            all_cols = [np.where(nm, na_rep, col)
-                        for nm, col in zip(na_masks, all_cols)]
-            result = cat_core(all_cols, sep)
-        else:
-            # no NaNs - can just concatenate
-            result = cat_core(all_cols, sep)
+        # if there are any non-string, non-null values hidden within an object
+        # dtype, cat_core will fail; catch error and return with better message
+        try:
+            if na_rep is None and union_mask.any():
+                # no na_rep means NaNs for all rows where any column has a NaN
+                # only necessary if there are actually any NaNs
+                result = np.empty(len(data), dtype=object)
+                np.putmask(result, union_mask, np.nan)
+
+                not_masked = ~union_mask
+                result[not_masked] = cat_core([x[not_masked]
+                                               for x in all_cols], sep)
path: pandas/core/strings.py
text: @user1:
can you limit the try/except to the relevant code

@author:
I am; `cat_core` is called in all three branches, and may raise in any one of them.

@user1:
so rather than having a giant try/except, either add this check in the cat_core, or write a function which *calls* cat_core and catches and formats a nicer error

maybe try to combine the _legal_dtype check with this, IOW. you can just try to cat them then catch an error, at which point you can then infer and give a nice message.

@author:
> @user1: so rather than having a giant try/except, either add this check in the cat_core, or write a function which calls cat_core and catches and formats a nicer error
maybe try to combine the _legal_dtype check with this, IOW. you can just try to cat them then catch an error, at which point you can then infer and give a nice message.

Moved try/except to `cat_safe`, which wraps around `cat_core` to do what you describe in the second sentence.

diff_hunk: @@ -2280,6 +2280,23 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):
                                  'must all be of the same length as the '
                                  'calling Series/Index.')
 
+        # data has already been checked by _validate to be of correct dtype,
+        # but others could still have Series of dtypes (e.g. integers) which
+        # will necessarily fail in concatenation. To avoid deep and confusing
+        # traces, we raise here for anything that's not object or all-NA float.
+        def _legal_dtype(series):
path: pandas/core/strings.py
text: @user1:
this is way complicated, what exactly are you trying to do here

@author:
I originally had an inline condition within `any`, but @user2 found this too complex, so I broke out that condition into a function.

Basically, I want to fail early for any Series that will necessarily fail concatenation (based on dtype). Object must obviously be allowed, but also all-NA float (which can happen if two Series completely misalign), plus needs handling of categorical.

@user1:
do you not already have the inferred types at this point? and if you don't, why not just infer them, then this condition becomes easier

@user2:
> I originally had an inline condition within `any`, but @user2 found this too complex, so I broke out that condition into a function.

the any is applied to a generator expesssion with a for loop and then raising. so i'm not sure the use of any here is a benefit. could you not just use a for loop and do away with the separate function?

@author:
> @user1: do you not already have the inferred types at this point? and if you don't, why not just infer them, then this condition becomes easier

The dtype has only been inferred for `data` at this point, not for `others`. I want to avoid inferring for `other`, as that would lead to another (not insignificant) perf-hit, whereas reading out the dtypes is trivial.