API: str.cat will align on Series
State: closed

Fixes issue #18657, fixed existing tests, added new test; all pass.

After I pushed everything and thought about it some more, I realised that one may argue about the default alignment-behavior, and whether it should be changed to `join=outer`. The behavior as implemented is compatible with the current requirement that everything be of the same length. To me, it is more intuitive that the concatenated `other` is added to the current series without enlarging it, but I can also see the argument why that restriction is unnecessary.

PS. This is my first PR, tried to follow all the rules. Sorry if I overlooked something.

Edit: Also fixes #20842 

Labels:
name: API Design
name: Strings
description: String extension data type and string data

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





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on May 02, 2018 at 05:33 Hours UTC
I just realised that due to the API-change, a what's-new entry is probably needed somewhere?
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20347?src=pr&el=h1) Report
> Merging [#20347](https://codecov.io/gh/pandas-dev/pandas/pull/20347?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/c4da79b5b322c73d8e61d1cb98ac4ab1e2438b40?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #20347      +/-   ##
==========================================
+ Coverage   91.79%   91.81%   +0.01%     
==========================================
  Files         153      153              
  Lines       49411    49478      +67     
==========================================
+ Hits        45359    45429      +70     
+ Misses       4052     4049       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.21% <100%> (+0.01%)` | :arrow_up: |
| #single | `41.85% <4%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20347?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/20347/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.63% <100%> (+0.28%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20347/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `84.59% <0%> (+0.2%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20347?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/20347?src=pr&el=footer). Last update [c4da79b...3f77b80](https://codecov.io/gh/pandas-dev/pandas/pull/20347?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

There are some useless commits in the appveyor-queue - how can those be cancelled? I'm guessing I don't have sufficient rights to do it. If someone who can should see this, you can cancel builds for commits (starting with): `41daef8d` and `d1c5543f`.
@jreback, I assume you meant `text.rst` and not `io.rst`. Added tests (changed alignment to `join='outer'`), added some tests, updated `v0.23.0.txt` and `text.rst`.
@TomAugspurger @jreback Can one of you (tell me how to) remove all but the last of my commits from the appveyor-queue? It's quite far behind as it is, so no need to choke it with unnecessary commits (in Travis new commits automatically supersede old ones - why not in appveyor...?).
Appveyor will auto cancel a build if there are newer commits on the PR
branch, but it doesn't show up as canceled until its turn in the queue.

On Thu, Mar 15, 2018 at 10:47 AM, h-vetinari <notifications@github.com>
wrote:

> @TomAugspurger <https://github.com/tomaugspurger> @jreback
> <https://github.com/jreback> Can one of you (tell me how to) remove all
> but the last of my commits from the appveyor-queue? It's quite far behind
> as it is, so no need to choke it with unnecessary commits (in Travis new
> commits automatically supersede old ones - why not in appveyor...?).
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/20347#issuecomment-373423692>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHItmN8d6A8SplKgALB5ivkC4OwDPSks5teo0UgaJpZM4SqkE_>
> .
>

@TomAugspurger , re:appveyor: this is not the case, at least not immediately. I can see in https://ci.appveyor.com/project/pandas-dev/pandas/history that some old commits were tried (while the new ones already existed) and ran for 1-2 minutes. I asked because I saw that some other commits were explicitly cancelled by users, but I don't know how to do that.
@jorisvandenbossche I wasn't even aware that's a legal signature? I'm guessing all Series would be concatenated with the same ``sep`` (and ``na_rep``, etc.)? And it is only legal if all are Series? What about a mix of Series and ``ndarray``?
> I'm guessing all Series would be concatenated with the same sep (and na_rep, etc.)? 

Yes, I think so

> And it is only legal if all are Series? What about a mix of Series and ndarray?

I suppose we should just process each element in the list separately, so then it does not really matter if it is a mixture.



Is there some pandas default how often a ``FutureWarning`` shows up? I'm not getting the align-warning a second time, even I verified the code runs through the corresponding ``if``.
@jorisvandenbossche 
I added the functionality you wanted, but it's potentially a bit unintuitive if there's a mix of ``ndarray`` and ``Series`` (because treating the elements sequentually would mean that the lengths of an array would have to match the length of the union of the indexes of all previous seriers-like elements). In this case, I demand that the ``ndarray`` elements match in length to the Series that's calling ``str.cat``.

It was a substantial rewrite, but the code got better through it (found some bugs, and now its cleaner).
I added *a lot* of tests about the expected behavior - please have a look at those tests if something is unclear. Maybe some of these examples should make it into whatsnew or ``text.rst``, but I'm too tired now. Feedback welcome.
@jreback 
Just saw your comments now (thanks!), will work on them tomorrow (if I can). Writing good docs is surely an important part.
The code changed a fair bit with the last commit -- initially, I left the structure as it was (i.e. ``str_cat`` and ``_get_array_list``), but that was very much not optimal (and the reason for some of my comments).

Is ``str_cat`` part of the public API or just used to construct ``str.cat``? Because I shifted all the relevant logic to ``str.cat`` now, and ``str_cat`` just provides the doc-string (i.e. align does not do anything for ``str_cat``).
@jorisvandenbossche @TomAugspurger @jreback 
regarding legal inputs for ``others`` - if list of series are allowed, why not DataFrames too? It would be a very simple addition.

I added functionality and tests for it. If you like how everything so far works, I'll update the docs (just wanna leave the doc-writing for when the code has converged).
> Is there some pandas default how often a FutureWarning shows up? I'm not getting the align-warning a second time, even I verified the code runs through the corresponding if.

That's normal. FutureWarnings only show up the first time you use something (you can warnings.filterwarnings to have it show always)
@jreback @TomAugspurger @jorisvandenbossche 
I added documentation (also for things I haven't touched, like ``_get_array_list``), and wrote and extensive section in ``text.rst`` outlining the ``str.cat``-method in general, and the expanded usability in particular.

If you would be so kind, please compile and read ``text.rst`` first, before you have a look at the code or tests; it should make for easier reading. I could document the code some more, but it should be quite understandable. Despite the lengths, I think it is not wasteful in terms of lines. It's just not that easy to cover all the desired cases *plus*  all the alignment options.

Feedback welcome.
The failure in the Travis-CI is an artefact -- the linter (only in the 2.7 run) complained about ``pandas/_libs/tslibs/period.pyx:1358:12: W291 trailing whitespace``, but neither did I touch this file, nor is there any trailing whitespace around the line where the failure supposedly happened.
@jorisvandenbossche 
> I suppose we should just process each element in the list separately, so then it does not really matter if it is a mixture.

A comment why this is not so easy (and why I align all elements in a list before starting concatenation):
If one starts concatenating right away, then one might get a problem with index changes induced by later list elements.

```
>>> s = pd.Series(['a', 'b', 'c', 'd'])
>>> t = pd.Series(['d', 'a', 'e', 'b'], index=[3, 0, 4, 1])
>>>
>>> # the following would be equivalent to
>>> # s.str.cat([t.values, t], align=True, join='outer', na_rep='-')
>>> # if list elements in `other` would be concatenated sequentially
>>> s.str.cat(t.values).str.cat(t, align=True, join='outer', na_rep='-')
0    ada
1    bab
2    ce-
3    dbd
4     -e  # <-- missing a NaN for first column!
dtype: object
>>> s.str.cat([t.values, t], align=True, join='outer', na_rep='-')
0    ada
1    bab
2    ce-
3    dbd
4    --e  # <-- this is more sensible behavior, IMO
dtype: object
```

Even worse, this would mean that any other arrays following in the list would be of the wrong length and trigger a warning...
OK, let's try to take a step back and first decide what behaviour we want, as I think the code now it getting too complicated:

- I mentioned I would find a "left align" more logical, so the returned series always preserves length and index. But, although I still find that sensible, on a second thought, that was maybe not a good suggestion? 
  Do we have some similar methods in pandas to see how they do it, so we could have a consistency argument?
- I am not sure we want to add an `align` or `join` keyword here. IMO, we should just choose a default alignment (bullet point above), and stick to that. If people want a different alignment, they can always first do an `s1.algin(s2)` manually before feeding the data to `str.cat`. Adding it here, just complicates things further? 
- you are right the lists of series/arrays gives some additional problems (also eg if you have first a series that realigns the data, and then an array that does not fit the length anymore due to this re-alignment). But, maybe we can be just more restrictive here. Eg disallow different types (either only series, or either only lists/arrays), might simplify things?
@jorisvandenbossche 
Thanks for the comment.

pandas advertises itself as (http://pandas.pydata.org/pandas-docs/stable/):
> Automatic and explicit **data alignment**: objects can be explicitly aligned to a set of labels, or the user can simply ignore the labels and let Series, DataFrame, etc. automatically align the data for you in computations

Therefore, I do not think that uses should have to align their Series themselves - the expectation that pandas evokes everywhere else is that things will be aligned automatically (in fact, I didn't know about ``Series.align`` for a long time, because it was never necessary). The easiest reference for me is something like boolean operations, which are 'outer' by default,
```
In [0]: s = pd.Series(['a', 'b'])
In [1]: t = pd.Series(['a', 'b'], index=[1, 2])
In [2]: s.astype(bool) & t.astype(bool)
Out[2]:
0    False
1     True
2    False
dtype: bool
```
but also things like ``.eq``. OTOH, methods like  ``.merge`` gives full control over the join-type.

I agree that ``join='left'`` is a sensible default behavior for ``str.cat``, but it is very important IMHO to allow the other options as well (see also the original issue #18657, which needs at least 'outer'). And since there are *no other methods*  for the ``.str``-accessor that take another Series as an argument, this does not break any consistency. Therefore, the proposal would be:
```
In [3]: s.str.cat(t, align=True, na_rep='-')
Out[3]: 
0    a-
1    ba
dtype: object
In [4]: s.str.cat(t, align=True, join='outer', na_rep='-')
Out[4]: 
0    a-
1    ba
2    -b
dtype: object
```

A bit further up, @TomAugspurger agreed with the ``join``-keyword as well. Finally, the join-types have completely negligible impact on code complexity (see below).

> But, maybe we can be just more restrictive here. Eg disallow different types (either only series, or either only lists/arrays), might simplify things?

I don't think that this would make things easier -- assuming we don't want to break existing code like
`` s.str.cat([t, ['A', 'B'], ['1', '2']])``. Basically all of the complexity comes from allowing lists of Series and arrays, but since those are legal currently, it's a strong argument to keep that, no?

And as soon as the logic is there to deal with such mixed lists (i.e. align arrays with the calling Series and proceed from there), then the other additions I made are really cheap:
My thought was, "if we allow list of Series, why not DataFrames?"
Same for two-dimensional ndarrays and an iterable as the outer container. Those can easily be removed, but I consider them cheap (in terms of line-count) and *useful* additions to the capability that is already needed to deal with the list of Series/arrays.

PS. have you seen a compiled version of the ``text.rst`` I updated? I think the exposition I made makes the case reasonably well. Particularly, I think it's easy to understand "All elements [in a list] must match in length to the calling Series, except those having an Index if align=True."
> Therefore, I do not think that uses should have to align their Series themselves - the expectation that pandas evokes everywhere else is that things will be aligned automatically (in fact, I didn't know about Series.align for a long time, because it was never necessary)

Sure, I agree we want to do alignment if a Series is provided (the bug you are fixing, completely agree with that), but that doesn't mean we should give a keyword to specify the *kind* of alignment. 
We *can* choose the default alignment which we think will make most sense in most of the cases, and only leave the manual alignment for advanced corner cases.

> The easiest reference for me is something like boolean operations, which are 'outer' by default,

Yep, indeed, and that's the reason I mentioned I am also not sure that I find 'left' alignment better, as we indeed have many cases where 'outer' is the default. 
But it is not everywhere, there are also methods where a 'left' alignment is more natural, eg `fillna`:

```
In [15]: s = pd.Series([0, 1, 2, np.nan, 4])

In [17]: s.fillna(pd.Series([10, 20, 30, 40, 50], index=[1, 2, 3, 4, 5]))
Out[17]: 
0     0.0
1     1.0
2     2.0
3    30.0
4     4.0
dtype: float64
```

So when filling element-wise, it aligns the `other` series, but not using 'outer' alignment (in this case it would also be very strange to do 'outer' alignment, as you only want to fill the calling series). Another example is the `where` method.

The question of course is a bit how we see `str.cat`, to which of the methods it is more similar to.

> see also the original issue #18657, which needs at least 'outer

Of course that goes into the very specifics of that code examples and is not a general answer, but there you could also do `col.loc[ss1] + col.loc[ss2]` and you get the outer alignment.
 



@jorisvandenbossche 
> We *can* choose the default alignment which we think will make most sense in most of the cases, and only leave the manual alignment for advanced corner cases.

If I had to choose one, I would choose 'outer'. OTOH, I see only benefits in providing the 'join'-options, but will defer to (plural-)your experience on that. Maybe @TomAugspurger can elaborate why he was in favor of a 'join'-keyword?

> So when filling element-wise, it aligns the other series, but not using 'outer' alignment (in this case it would also be very strange to do 'outer' alignment, as you only want to fill the calling series).
[...]
The question of course is a bit how we see str.cat, to which of the methods it is more similar to.

I think ``str.cat`` sort of lives in the middle where both the 'outer'-POV (like booleans) and 'left'-POV (like fillna) can make more sense, depending on context. Kind of like ``merge``, where all these options are needed as well - and isn't the concatenation between Series essentially a merging operation?




@jreback
I shortened ``text.rst``, mainly by reducing somewhat redundant examples - please re-review. I left the align/join-kwargs as they are for the moment, pending further discussion (there seem to be 3 different opinions from the core-devs at the moment).
Re: build failure: this is due to ``curl: (7) Failed to connect to s3.amazonaws.com port 443: Connection timed out``

Nothing build-relevant changed to the previous (passing) commit, and sphinx ran fine for me.
I haven't looked at the code changes yet, so I can't gauge the complexity tradeoff here yet.
  
left-align seems the most natural to me because most other accessor methods are shape-preserving. I could also see wanting an outer join, hence my preference for a keyword argument.

Do we want to change the default behavior (with a deprecation warning)? If so, what's the default?

I don't think we need both `align` and `join`. We can achieve everything with a single keyword argument.
@TomAugspurger 
As a comment: the join-style & -default (as well as whether there's one or two keywords) have almost no bearing on the complexity. The complexity comes from dealing with lists containing combinations of Series/ndarray/etc.

Re:
> Do we want to change the default behavior (with a deprecation warning)? If so, what's the default?

In your response to #18657, you wrote 
>I'd favor a keyword like align for controlling it. For now it'll be None by default and if others is a series it will not align, but warn that the default will change in the future to align.

which was my starting point for implementing it. But again, the necessary changes for different keyword(-combination)s would all be minor. My only preference is giving the option of different joins in some form.
> Do we want to change the default behavior (with a deprecation warning)? If so, what's the default?

I think the default should (eventually) be alignment, but will need a transition period?
As join-type-default, I would use 'outer' *if* only one option is available. Otherwise, I would use 'left' as default (if all the other join-options are also available), to stay "closer" to the ``.str``-accessor-feel, while having full flexibility.
If I try to summarize the positions (as I understand them) so far:

@jorisvandenbossche prefers a single join-style (potentially 'left' or 'outer'), and asks (paraphrasing) "what other methods is ``str.cat`` similar to``?
@TomAugspurger prefers having a kwarg for different joins, but only one kwarg in total, and asks about how to transition the API with Deprecation (or Future?) Warnings
@jreback says only one kwarg should suffice.

I believe that ``str.cat`` is more similar to ``merge`` than to the other ``str``-accessors, in that it should support all different join styles. In order to not break currently legal code (but maybe issue a warning on *every* non-alignment right away - i.e. from v.0.23), how about the following suggestion:
- v.0.23: ``join=None(->FuWa)|'left'|'outer'|'inner'|'right'``, with default None. The warning would be emitted only when necessary - i.e. if others contains an (element with an) index.
- v.0.2x: ``join='left'|'outer'|'inner'|'right'``, with default tbd ('left'/'outer'?). If the user wants no alignment, they could still use ``.values``.
Thanks for the summary. I think your proposal is reasonable. For the future default, I'd favor `join='left'`, because it preserves the shape of `self`.
@TomAugspurger I agree with the (future) default of ``join='left'`` as well.

@jorisvandenbossche Can you OK (or critique) the above proposal, before I start rewriting code/tests/docs?