CLN/ERR: str.cat internals
State: closed

This is mainly a clean-up of internal methods for `str.cat` that I didn't want to touch within #20347.

~~As a side benefit of changing the implementation, this also solves #22721. Finally, I've also added a better message for TypeErrors (closes #22722)~~

~~closes #22721~~
~~closes #22722~~
- [x] tests modified / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Here's the ASV output (the original implementation of this PR (see first commit) that used more higher-level pandas-functions like `fillna`, `drop_na`, etc. was up to three times slower, so I tweaked it some more, and actually believe that the last solution with `interleave_sep` is the most elegant anyway):
```
       before           after         ratio
     [37455764]       [4d1710f1]
         10.9±1ms         9.11±1ms    ~0.83  strings.Cat.time_cat(0, ',', '-', 0.0)
         9.55±1ms         10.9±0ms    ~1.15  strings.Cat.time_cat(0, ',', '-', 0.001)
         12.5±2ms         14.1±2ms    ~1.12  strings.Cat.time_cat(0, ',', '-', 0.15)
         9.94±1ms       9.23±0.7ms     0.93  strings.Cat.time_cat(0, ',', None, 0.0)
         14.3±2ms         8.68±1ms    ~0.61  strings.Cat.time_cat(0, ',', None, 0.001)
-        13.7±1ms       11.7±0.8ms     0.86  strings.Cat.time_cat(0, ',', None, 0.15)
       9.11±0.7ms         7.81±2ms    ~0.86  strings.Cat.time_cat(0, None, '-', 0.0)
       9.38±0.8ms         10.9±1ms    ~1.17  strings.Cat.time_cat(0, None, '-', 0.001)
         11.4±2ms         10.9±2ms     0.96  strings.Cat.time_cat(0, None, '-', 0.15)
         13.4±2ms       9.38±0.6ms    ~0.70  strings.Cat.time_cat(0, None, None, 0.0)
         9.23±2ms         11.7±1ms    ~1.27  strings.Cat.time_cat(0, None, None, 0.001)
         10.2±2ms         10.9±1ms     1.08  strings.Cat.time_cat(0, None, None, 0.15)
-        70.3±4ms         54.7±4ms     0.78  strings.Cat.time_cat(3, ',', '-', 0.0)
         62.5±4ms        46.9±20ms    ~0.75  strings.Cat.time_cat(3, ',', '-', 0.001)
        93.8±10ms        66.4±10ms    ~0.71  strings.Cat.time_cat(3, ',', '-', 0.15)
         62.5±4ms         62.5±4ms     1.00  strings.Cat.time_cat(3, ',', None, 0.0)
+        46.9±4ms         85.9±4ms     1.83  strings.Cat.time_cat(3, ',', None, 0.001)
         52.1±2ms         50.8±6ms     0.97  strings.Cat.time_cat(3, ',', None, 0.15)
         50.8±8ms         54.7±6ms     1.08  strings.Cat.time_cat(3, None, '-', 0.0)
         54.7±4ms         62.5±4ms    ~1.14  strings.Cat.time_cat(3, None, '-', 0.001)
         62.5±5ms         54.7±3ms    ~0.88  strings.Cat.time_cat(3, None, '-', 0.15)
         46.9±4ms         39.1±0ms    ~0.83  strings.Cat.time_cat(3, None, None, 0.0)
         46.9±8ms         58.6±6ms    ~1.25  strings.Cat.time_cat(3, None, None, 0.001)
         46.9±9ms         54.7±4ms    ~1.17  strings.Cat.time_cat(3, None, None, 0.15)
                                                                  ^  ^     ^     ^
                                                                  |  |     |     |
                                                         other_cols  |   na_rep  |
                                                                     |           |
                                                                    sep        na_frac
```

There's a bunch of noise in there, but by and large, things don't look so bad IMO. Especially, when one excludes the not-so-common worst-case scenario of a very small but non-zero amount of NaNs (`na_frac=0.001`):
```
       before           after         ratio
     [37455764]       [4d1710f1]
         10.9±1ms         9.11±1ms    ~0.83  strings.Cat.time_cat(0, ',', '-', 0.0)
         12.5±2ms         14.1±2ms    ~1.12  strings.Cat.time_cat(0, ',', '-', 0.15)
         9.94±1ms       9.23±0.7ms     0.93  strings.Cat.time_cat(0, ',', None, 0.0)
-        13.7±1ms       11.7±0.8ms     0.86  strings.Cat.time_cat(0, ',', None, 0.15)
       9.11±0.7ms         7.81±2ms    ~0.86  strings.Cat.time_cat(0, None, '-', 0.0)
         11.4±2ms         10.9±2ms     0.96  strings.Cat.time_cat(0, None, '-', 0.15)
         13.4±2ms       9.38±0.6ms    ~0.70  strings.Cat.time_cat(0, None, None, 0.0)
         10.2±2ms         10.9±1ms     1.08  strings.Cat.time_cat(0, None, None, 0.15)
-        70.3±4ms         54.7±4ms     0.78  strings.Cat.time_cat(3, ',', '-', 0.0)
         62.5±4ms        46.9±20ms    ~0.75  strings.Cat.time_cat(3, ',', '-', 0.001)
        93.8±10ms        66.4±10ms    ~0.71  strings.Cat.time_cat(3, ',', '-', 0.15)
         62.5±4ms         62.5±4ms     1.00  strings.Cat.time_cat(3, ',', None, 0.0)
         52.1±2ms         50.8±6ms     0.97  strings.Cat.time_cat(3, ',', None, 0.15)
         50.8±8ms         54.7±6ms     1.08  strings.Cat.time_cat(3, None, '-', 0.0)
         62.5±5ms         54.7±3ms    ~0.88  strings.Cat.time_cat(3, None, '-', 0.15)
         46.9±4ms         39.1±0ms    ~0.83  strings.Cat.time_cat(3, None, None, 0.0)
         46.9±9ms         54.7±4ms    ~1.17  strings.Cat.time_cat(3, None, None, 0.15)
                                                                  ^  ^     ^     ^
                                                                  |  |     |     |
                                                         other_cols  |   na_rep  |
                                                                     |           |
                                                                    sep        na_frac
```

Labels:
name: Error Reporting
description: Incorrect or improved errors from pandas
name: Strings
description: String extension data type and string data
name: Clean

Comments:
Hello @h-vetinari! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`pandas/core/strings.py`](https://github.com/h-vetinari/pandas/blob/f79c707b3d72d93a387e89dd22172afee835edd5/pandas/core/strings.py) !

 - There are no PEP8 issues in the file [`pandas/tests/test_strings.py`](https://github.com/h-vetinari/pandas/blob/f79c707b3d72d93a387e89dd22172afee835edd5/pandas/tests/test_strings.py) !



##### Comment last updated on September 17, 2018 at 09:16 Hours UTC
@WillAyd @jreback : The conversations went stale a little in the original issues, and I'm not sure how well aligned these changes with what you guys were suggesting or saying in them.
@gfyoung @WillAyd @jreback 
I opened #22721 because the behaviour was changed through this refactor. As I said in the issue, I'm happy to disable it, but that would need adding in some check against binary data (because `np.sum` -- contrary to the previous implementation -- doesn't throw an error for binary data).

I do however stand by:
> Beyond that, it's IMO the `.str`-accessor that should be enforcing the correct types, but since there's no dedicated string-dtype yet, the methods that *do* work for other object data (e.g. lists) are also used like that (e.g. people use `.str.len()` to get the different length of a Series of lists).

In other words, I'm +epsilon on allowing (consistent, reasonable) off-label use of the `.str`-accessor until there is a string dtype.

#22722 is equally something that I'm not going to fight for. I got some less than ideal error messages while testing, and decided this could/should be improved. If you disagree, it's easy to remove the offending lines.
@h-vetinari : Not a problem.  I was only reading through the issues and your PR and matching up what you were doing with what was being said.  Just pinging to get their eyes on this.
I'm still very much against setting the expectation that the `.str` accessor will work with byte objects.

Also generally don't think that a "cleanup" should be performed in a same PR that changes the expected functionality of the codebase. Would be much easier to stick to one thing at a time, i.e. a clean up that doesn't introduce or change any existing behavior
@WillAyd 
> Would be much easier to stick to one thing at a time, i.e. a clean up that doesn't introduce or change any existing behavior

I reverted the (unrelated) fix for #22721, but the situation for #22722 is more delicate. The genesis was as follows:
1. refactor
1. see that behaviour changed
1. (sorta) agree with new behaviour
1. open issue so that changed behaviour in PR is explained

Turns out, that the situation is even a bit more delicate, as currently on master, bytes *can be* successfully concatenated as long as `sep` is explicitly set:
```
(pandas-dev) C:\Users\[...]\pddev>python
Python 3.6.6 |Anaconda, Inc.| (default, Jun 28 2018, 11:27:44) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import pandas as pd
>>> import numpy as np
>>> pd.__version__
'0.24.0.dev0+586.g8a1c8ad4b'
>>> s = pd.Series(np.array(list('abc'), 'S1').astype(object))
>>> t = pd.Series(np.array(list('def'), 'S1').astype(object))
>>> s.str.cat(t)
TypeError: sequence item 0: expected str instance, bytes found
>>> s.str.cat(t, sep=b'')
0    b'ad'
1    b'be'
2    b'cf'
dtype: object
>>> s.str.cat(t, sep=b',')
0    b'a,d'
1    b'b,e'
2    b'c,f'
dtype: object
```

The problem was that `tests/frame/test_strings.test_method_on_bytes` only tests `sep=None`.

This is the sort of thing I was talking about with the missing string dtype. Without one, there are legitimate off-label uses for the `.str`-accessor, and concatenating bytes workes like a charm *already* (not to mention several other methods from `.str`). The only real change here then would be that `sep=None` does not automatically trigger the `TypeError`.
@h-vetinari pls pls pls 1 thing per PR. We do NOT handle bytes in ``.str`` if you want to add tests and raise, pls do so, but not going to 'make it work better'. It is amazingly confusing and causes all sorts of errors. We probably don't have explicit checks on this (though I *thought* that we always infer on the strings that must be string/unicode and *never* bytes).
@jreback
> We do NOT handle bytes in .str

Yes you (currently) do. Just try the code I posted above.

> pls pls pls 1 thing per PR.

The whatsnew-note notwithstanding, this PR only changes the implementation (you'll see in the test that I've not changed anything substantial)

I understand that you don't want people using `.str` for byte data, but it works currently. The problem is that there's no good dtype distinction, and inspecting every element of a Series when calling `.str` would come with a big perf hit.
> Yes you (currently) do. Just try the code I posted above.

It may happen to work. Instead of refactoring this as I said above, would prefer tests / and better error messages with bytes inputs.


@jreback 
> It may happen to work. Instead of refactoring this as I said above, would prefer tests / and better error messages with bytes inputs.

The point is that this PR *does not change the current behaviour* and should stand on its merits, unrelated to the fact that you'd like to disallow `.str` on bytes.
@h-vetinari you removed tests, so clearly you are changing things.
@jreback 
> you removed tests, so clearly you are changing things.

I removed *one* test for the internal method that has been factored away. Furthermore, this removed test (`test_cat`) is exactly replicated in the test directly below (`test_str_cat`).
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=h1) Report
> Merging [#22725](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/c8ce3d01e9ffafc24c6f9dd568cd9eb7e42c610c?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22725/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22725      +/-   ##
==========================================
- Coverage    92.2%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50924    50900      -24     
==========================================
- Hits        46952    46928      -24     
  Misses       3972     3972
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.32% <6.45%> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/22725/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.58% <100%> (-0.05%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=footer). Last update [c8ce3d0...e58ec9d](https://codecov.io/gh/pandas-dev/pandas/pull/22725?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jreback 
While you're at it with the reviewing, please don't forget this one. :)
@jreback 

PTAL
@jreback 

PTAL
lgtm. @jorisvandenbossche @TomAugspurger @WillAyd if someone else would have a look
Away for a few days but can take a look Monday. If someone approves in the interim feel free to merge
Something seems off here (points to a gap in test coverage). Take the following as an example on your branch:

```python
In [4]: import pandas as pd

In [5]: ser1 = pd.Series(list(range(3)), dtype=object)

In [6]: ser2 = pd.Series(list(range(3, 6)), dtype=object)

In [7]: ser1.str.cat(ser2)
Out[7]: 
0    3
1    5
2    7
dtype: object
```

On master this raises:

```python
TypeError: sequence item 0: expected str instance, int found
```

Can you see what may be causing that difference in behavior? The new behavior is incorrect
@WIllAyd,

good catch. Will have to bite the bullet and always add `sep`, even if it's `''`, as this is the only way to catch these cases (barring a proper string dtype).

I have another PR corresponding to #22722 (a part of this PR that I removed for the purpose of stream-lining it), which will raise when the entered columns have the wrong dtype (but wouldn't catch ints masquerading as objects).

Also added a test for this.
Build failure unrelated - an error due to hypthesis internals/health checks:
```
______________ TestDataFrameAggregate.test_frequency_is_original _______________
[gw1] linux -- Python 3.7.0 /home/travis/miniconda3/envs/pandas/bin/python
self = <pandas.tests.frame.test_apply.TestDataFrameAggregate object at 0x7f9b7025db70>
    @given(index=indices(5), num_columns=integers(0, 5))
>   def test_frequency_is_original(self, index, num_columns):
pandas/tests/frame/test_apply.py:1159: 
[...]
E           hypothesis.errors.HypothesisDeprecationWarning: Test took 295.55ms to run.
In future the default deadline setting will be 200ms, which will make this an error.
You can set deadline to an explicit value of e.g. 300 to turn tests slower than this into
an error, or you can set it to None to disable this check entirely.
```
@WillAyd 

No idea why, the other numpy1.9 didn't fail. In any case, I skipped right to bigger guns (#23062) to solve this. ;-)
> No idea why, the other numpy1.9 didn't fail. In any case, I skipped right to bigger guns (#23062) to solve this. ;-)

@h-vetinari we generally like to fix issues independently, you can wait on this if you want, but changing deps PR is far from ready.
@jreback 
> @h-vetinari we generally like to fix issues independently, you can wait on this if you want, but changing deps PR is far from ready.

IMO no point to spend time working around something that will be obsolete very soon.
@h-vetinari why don't you just use the np less than 1.11 check here and resize the empty arrays as appropriate for the lower version? 

The other PR seems to be a larger scope than when it first started so may take time. Using a condition here allows this to move independently and is an easy change on the other PR when the time is right
Nevermind the previous post I immediately deleted, it ran on the wrong branch... Ugh.

There's gonna be a bit of speed lost due to always adding in `sep`, even if it's `''`, but I know how to deal with that in a follow-up (or *maybe* predecessor)...
ok this looks fine. pending another asv run, can merge.
Got delayed by #23085. Will try again tonight.
@WillAyd @jreback 
Unfortunately, I have bad news. I started out in this PR with a very idiomatic solution (see the first couple commits), and it was just too slow.

Here's the ASV for the last commit:
```
All benchmarks:

       before           after         ratio
     [b28cf5aa]       [a97fe67e]
     <master>         <cln_str_cat>
         9.38±0ms         9.38±0ms     1.00  strings.Cat.time_cat(0, ',', '-', 0.0)
       9.38±0.8ms         10.9±0ms    ~1.17  strings.Cat.time_cat(0, ',', '-', 0.001)
         10.9±0ms       12.5±0.6ms    ~1.14  strings.Cat.time_cat(0, ',', '-', 0.15)
       9.38±0.6ms       8.59±0.8ms     0.92  strings.Cat.time_cat(0, ',', None, 0.0)
-      12.5±0.8ms         10.9±0ms     0.87  strings.Cat.time_cat(0, ',', None, 0.001)
-        14.1±0ms         10.9±0ms     0.78  strings.Cat.time_cat(0, ',', None, 0.15)
         9.38±0ms       9.38±0.8ms     1.00  strings.Cat.time_cat(0, None, '-', 0.0)
         9.38±0ms       9.38±0.8ms     1.00  strings.Cat.time_cat(0, None, '-', 0.001)
       10.9±0.8ms       12.5±0.8ms    ~1.14  strings.Cat.time_cat(0, None, '-', 0.15)
         10.9±2ms       7.81±0.8ms    ~0.71  strings.Cat.time_cat(0, None, None, 0.0)
         7.81±0ms       10.9±0.8ms    ~1.40  strings.Cat.time_cat(0, None, None, 0.001)
         10.9±2ms       10.9±0.8ms     1.00  strings.Cat.time_cat(0, None, None, 0.15)
         78.1±8ms         93.8±8ms    ~1.20  strings.Cat.time_cat(3, ',', '-', 0.0)
+        62.5±8ms          109±0ms     1.75  strings.Cat.time_cat(3, ',', '-', 0.001)
+        78.1±8ms          125±8ms     1.60  strings.Cat.time_cat(3, ',', '-', 0.15)
+        46.9±8ms         93.8±0ms     2.00  strings.Cat.time_cat(3, ',', None, 0.0)
         62.5±8ms          109±0ms    ~1.75  strings.Cat.time_cat(3, ',', None, 0.001)
+        46.9±8ms          102±8ms     2.17  strings.Cat.time_cat(3, ',', None, 0.15)
         62.5±0ms         78.1±6ms    ~1.25  strings.Cat.time_cat(3, None, '-', 0.0)
+        46.9±8ms         93.8±0ms     2.00  strings.Cat.time_cat(3, None, '-', 0.001)
+        62.5±8ms          125±6ms     2.00  strings.Cat.time_cat(3, None, '-', 0.15)
+        62.5±8ms         85.9±8ms     1.38  strings.Cat.time_cat(3, None, None, 0.0)
         46.9±6ms         93.8±0ms    ~2.00  strings.Cat.time_cat(3, None, None, 0.001)
         46.9±0ms         93.8±0ms    ~2.00  strings.Cat.time_cat(3, None, None, 0.15)
```
So especially when `others` is not None (all the `pd.concat` and dealing with `DataFrames`) we lose perf.
As a comparison, here's the ASV before the idiomatic changes @WillAyd requested:

```
All benchmarks:

       before           after         ratio
     [b28cf5aa]       [0d3c6d21]
     <master>         <cln_str_cat~2>
         9.38±0ms         9.38±0ms     1.00  strings.Cat.time_cat(0, ',', '-', 0.0)
         9.38±0ms         10.9±0ms    ~1.17  strings.Cat.time_cat(0, ',', '-', 0.001)
         10.9±0ms         12.5±0ms    ~1.14  strings.Cat.time_cat(0, ',', '-', 0.15)
       9.38±0.6ms         9.38±0ms     1.00  strings.Cat.time_cat(0, ',', None, 0.0)
       14.1±0.8ms         10.9±0ms    ~0.78  strings.Cat.time_cat(0, ',', None, 0.001)
       14.1±0.6ms       11.7±0.8ms    ~0.83  strings.Cat.time_cat(0, ',', None, 0.15)
         9.38±0ms         9.38±0ms     1.00  strings.Cat.time_cat(0, None, '-', 0.0)
       9.38±0.6ms       10.9±0.6ms    ~1.17  strings.Cat.time_cat(0, None, '-', 0.001)
         10.9±0ms         12.5±0ms    ~1.14  strings.Cat.time_cat(0, None, '-', 0.15)
         10.9±0ms         7.81±2ms    ~0.71  strings.Cat.time_cat(0, None, None, 0.0)
         9.38±0ms       10.9±0.8ms    ~1.17  strings.Cat.time_cat(0, None, None, 0.001)
       10.9±0.8ms       11.7±0.8ms     1.07  strings.Cat.time_cat(0, None, None, 0.15)
         78.1±0ms         78.1±8ms     1.00  strings.Cat.time_cat(3, ',', '-', 0.0)
         70.3±8ms         78.1±0ms    ~1.11  strings.Cat.time_cat(3, ',', '-', 0.001)
         93.8±8ms         93.8±0ms     1.00  strings.Cat.time_cat(3, ',', '-', 0.15)
         46.9±6ms         78.1±0ms    ~1.67  strings.Cat.time_cat(3, ',', None, 0.0)
         46.9±8ms         78.1±0ms    ~1.67  strings.Cat.time_cat(3, ',', None, 0.001)
         46.9±6ms         62.5±8ms    ~1.33  strings.Cat.time_cat(3, ',', None, 0.15)
         54.7±8ms         54.7±8ms     1.00  strings.Cat.time_cat(3, None, '-', 0.0)
         46.9±8ms         62.5±8ms    ~1.33  strings.Cat.time_cat(3, None, '-', 0.001)
         62.5±0ms        78.1±10ms    ~1.25  strings.Cat.time_cat(3, None, '-', 0.15)
         62.5±6ms         46.9±8ms    ~0.75  strings.Cat.time_cat(3, None, None, 0.0)
         46.9±6ms         62.5±0ms    ~1.33  strings.Cat.time_cat(3, None, None, 0.001)
         46.9±0ms         62.5±6ms    ~1.33  strings.Cat.time_cat(3, None, None, 0.15)
```

This isn't great, but not too bad IMO. Obviously it costs us to uselessly add in `sep=''` just to catch TypeErrors that should already be caught in the `.str`-accessor. I have something in mind there as well.
The direct comparison:
```
                        HEAD~2 vs. master  HEAD vs. master HEAD vs. HEAD~2 HvH2 increase
(3, ',', '-', 0.0)                   1.00             1.20            1.20       +20.00%
(3, ',', '-', 0.001)                 1.11             1.75            1.58       +57.66%
(3, ',', '-', 0.15)                  1.00             1.60            1.60       +60.00%
(3, ',', None, 0.0)                  1.67             2.00            1.20       +19.76%
(3, ',', None, 0.001)                1.67             1.75            1.05        +4.79%
(3, ',', None, 0.15)                 1.33             2.17            1.63       +63.16%
(3, None, '-', 0.0)                  1.00             1.25            1.25       +25.00%
(3, None, '-', 0.001)                1.33             2.00            1.50       +50.38%
(3, None, '-', 0.15)                 1.25             2.00            1.60       +60.00%
(3, None, None, 0.0)                 0.75             1.38            1.84       +84.00%
(3, None, None, 0.001)               1.33             2.00            1.50       +50.38%
(3, None, None, 0.15)                1.33             2.00            1.50       +50.38%
```

Finally, as a general warning about the results right after the run with `SOME BENCHMARKS CHANGED SIGNIFICANTLY` etc.: all **benchmarks with a `~` in their ratio are falsely omitted** from those results. I've opened airspeed-velocity/asv#752 for that.

 
IIUC you are saying the last batch of changes requested are causing performance to suffer anywhere between 20-80%? I've been wrong before but at the same time I've never seen instances where applying a function via list comprehension would be significantly faster than applying to the entire frame. Would be helpful if you could profile and debug further