Follow-up #20347: incorporate review about _get_series_list #20923

h-vetinari · 2018-05-02T12:28:24Z

Closes #20922

recap of #20347 :
Enabling alignment on top of the (v.0.22-)legal list of lists was relatively complex. The allowed argument types implemented in #20347 are as follows:

Type of (element of) "others"        | alone | within list-like
---------------------------------------------------------------------
pd.Series                            |  yes  | yes
pd.Index                             |  yes  | yes
np.ndarray (1-dim)                   |  yes  | yes
DataFrame                            |  yes  | no
np.ndarray (2-dim)                   |  yes  | no
iterators, dict-views, etc.          |  yes  | **
anything else w/ is_list_like = True |  yes  | **

** to be permitted, list-likes (say nxt) within a list-like others must pass (essentially)
all(not is_list_like(x) for x in nxt)
and not be a DF.

Open review points from #20347 from @jreback (and my responses) reproduced in review comments.

PS. Managed to have a brainfart and there's a bug in the RC docs - very sorry. Fixed in this PR.

h-vetinari

Responding to review of @jreback

h-vetinari · 2018-05-02T12:31:30Z

pandas/core/strings.py

                # without copy, this could change "others"
                # that was passed to str.cat
                others = others.copy()
                others.index = idx
-            return ([others[x] for x in others], fu_wrn)
+            return ([others[x] for x in others], warn)
        elif isinstance(others, np.ndarray) and others.ndim == 2:


@jreback :

this is wrong
i don’t think we can align a ndarray at all like this
let’s can ndarray a that are > 1 dim

My response:
The DF-constructor works as expected for a 2-dim ndarray, but I haven't checked if this is tested behaviour. (essentially, df == DataFrame(df.values, columns=df.columns, index=df.index))

I would suggest not to can 2-dim ndarrays, because they are necessary to avoid alignment on the deprecation path for join:

[...] To disable alignment (the behavior before v.0.23) and silence this warning, use .values on any Series/Index/DataFrame in others. [...]

can you add a test for this, basically see what happens when I have a non-default index with the Series (e.g. 2,3, 4) or something and it gets aligned with the 0,1,2 of the ndarray-converted-to-DataFrame. It will 'work' but is wrong.

I added a general comment before all the cases for others because it seems this point wasn't clear:

# Generally speaking, all objects without an index inherit the index # `idx` of the calling Series/Index - i.e. must have matching length. # Objects with an index (i.e. Series/Index/DataFrame) keep their own # index, *unless* ignore_index is set to True.

So, the case you mention does not happen, because an ndarray is always automatically aligned with the calling Series (of course, the lengths must match for this to work).

There are several tests with objects with different indexes, both with join is None (so with force-realign; successful but raising a warning), as well as with the different join-types.

i c, ok then

h-vetinari · 2018-05-02T12:32:11Z

pandas/core/strings.py

        elif isinstance(others, np.ndarray) and others.ndim == 2:
            others = DataFrame(others, index=idx)
            return ([others[x] for x in others], False)
        elif is_list_like(others):
            others = list(others)  # ensure iterators do not get read twice etc
            if all(is_list_like(x) for x in others):
                los = []
-                fu_wrn = False


@jreback :

can u name this parameter just warn

Done

h-vetinari · 2018-05-02T12:32:16Z

pandas/core/strings.py

-                    fu_wrn = fu_wrn or fwn
-                return (los, fu_wrn)
-            # test if there is a mix of list-like and non-list-like (e.g. str)
-            elif (any(is_list_like(x) for x in others)


@jreback

you can make this simpler by just checking for all is not list like (eg strings)
anything else will fail thru to the TypeError

Done

h-vetinari · 2018-05-02T12:33:00Z

pandas/core/strings.py

                while others:
-                    nxt = others.pop(0)  # list-like as per check above
-                    # safety for iterators and other non-persistent list-likes


@jreback :

this whole section needs some work it’s way too hard to read and follow

I tried to comment in detail what's happening - which part is unclear?

h-vetinari · 2018-05-02T12:34:37Z

pandas/core/strings.py

                    is_legal = ((no_deep and nxt.dtype == object)
-                                or all((isinstance(x, compat.string_types)
-                                        or (not is_list_like(x) and isnull(x))


@jreback :

isnull already checks for None
only 1d objects are valid here (or all scalars)
do this check up front

I simplified the check, but I can't put it any further upfront (unless I remove the fast track). Need to convert iterators first, and the checking for fast-track for Series/Index etc. also needs to be done before

h-vetinari · 2018-05-02T12:37:51Z

@TomAugspurger from #20922

Some requests for simplification. Most of the complexity seems to be in the code inferring what kind of regime we're in (one or more alignable objects vs. one or more unalignable objects vs. mix). Being strict here (after deprecating) or requiring additional user input may be worthwhile from a maintenance perspective.

What do you mean by being strict. Could you maybe edit the table from above to reflect what you think should be allowed?

h-vetinari · 2018-05-02T13:56:15Z

@TomAugspurger
Since I saw you're doing an RC2 due to #20924, would you consider merging this PR (or a separate one) for fixing the embarassing (for me) doc-error?

This PR will (almost certainly) be fully green in ~1h.

TomAugspurger · 2018-05-02T14:03:50Z

Nope, just doing build stuff right now.

codecov · 2018-05-02T15:00:56Z

Codecov Report

Merging #20923 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #20923      +/-   ##
==========================================
- Coverage   91.81%   91.81%   -0.01%     
==========================================
  Files         153      153              
  Lines       49471    49470       -1     
==========================================
- Hits        45422    45421       -1     
  Misses       4049     4049

Flag	Coverage Δ
#multiple	`90.21% <100%> (-0.01%)`	⬇️
#single	`41.84% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/strings.py	`98.62% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce4ab82...3bf38a0. Read the comment docs.

jreback · 2018-05-03T00:38:04Z

pandas/core/strings.py

                # without copy, this could change "others"
                # that was passed to str.cat
                others = others.copy()
                others.index = idx
-            return ([others[x] for x in others], fu_wrn)
+            return ([others[x] for x in others], warn)
        elif isinstance(others, np.ndarray) and others.ndim == 2:


can you add a test for this, basically see what happens when I have a non-default index with the Series (e.g. 2,3, 4) or something and it gets aligned with the 0,1,2 of the ndarray-converted-to-DataFrame. It will 'work' but is wrong.

jreback · 2018-05-03T00:38:27Z

other than my comment above the cleanup looks good.

h-vetinari

Thanks for the feedback. I think the point you mention is a misunderstanding, but I pushed another commit to make it clearer what's happening.

h-vetinari · 2018-05-03T05:08:48Z

pandas/core/strings.py

                # without copy, this could change "others"
                # that was passed to str.cat
                others = others.copy()
                others.index = idx
-            return ([others[x] for x in others], fu_wrn)
+            return ([others[x] for x in others], warn)
        elif isinstance(others, np.ndarray) and others.ndim == 2:


I added a general comment before all the cases for others because it seems this point wasn't clear:

# Generally speaking, all objects without an index inherit the index # `idx` of the calling Series/Index - i.e. must have matching length. # Objects with an index (i.e. Series/Index/DataFrame) keep their own # index, *unless* ignore_index is set to True.

So, the case you mention does not happen, because an ndarray is always automatically aligned with the calling Series (of course, the lengths must match for this to work).

There are several tests with objects with different indexes, both with join is None (so with force-realign; successful but raising a warning), as well as with the different join-types.

h-vetinari

Before we close this, maybe this question from the last PR merits some further discussion. @TomAugspurger @jreback @jorisvandenbossche

h-vetinari · 2018-05-03T05:37:37Z

doc/source/text.rst

@@ -311,7 +311,7 @@ All one-dimensional list-likes can be arbitrarily combined in a list-like contai



Another point of @TomAugspurger in #20347, responding to the line
All one-dimensional list-likes can be arbitrarily combined in a list-like container (including iterators, ``dict``-views, etc.):
(which is just above this line pointer):

@TomAugspurger

question: what happens with a dict? Do we use the keys, or does it transform to a series and get aligned?

Response 1:

I was talking about d.keys() or d.values(). For passing a dict directly, currently the keys would get read (as that's what x in d would return).

Response 2:

I could of course add another safety that maps d to d.values().
However, I tend to think that this could be left to the python-skills of the user as well -- a dictionary is not "list-like" from the POV of normal python usage (whereas its keys and values are, see d = dict(zip(keys, values))).

a dict is treated consistently here how we handle list-likes else where, we effectively call list on it

jreback · 2018-05-03T09:57:39Z

doc/source/text.rst

@@ -311,7 +311,7 @@ All one-dimensional list-likes can be arbitrarily combined in a list-like contai



a dict is treated consistently here how we handle list-likes else where, we effectively call list on it

jreback · 2018-05-03T09:57:56Z

pandas/core/strings.py

                # without copy, this could change "others"
                # that was passed to str.cat
                others = others.copy()
                others.index = idx
-            return ([others[x] for x in others], fu_wrn)
+            return ([others[x] for x in others], warn)
        elif isinstance(others, np.ndarray) and others.ndim == 2:


i c, ok then

h-vetinari · 2018-05-03T15:09:59Z

a dict is treated consistently here how we handle list-likes else where, we effectively call list on it

In that case, I think this PR is good to go.

h-vetinari · 2018-05-04T05:43:49Z

@jreback @TomAugspurger @jorisvandenbossche
re: treatment of dicts:
FWIW, concat does a very similar kind of operation to str.cat, and does treat dicts differently (than calling list on them):

d = {'a' : pd.Series([0, 1]), 'b': pd.Series(['c', 'd'])}
pd.concat(d, axis=1)
#    a  b
# 0  0  c
# 1  1  d
pd.concat(d.values(), axis=1)
#    0  1
# 0  0  c
# 1  1  d
pd.concat(list(d), axis=1)
# TypeError

OTOH, I'm honestly fine to leave this PR as it is. Special treatment for dicts should be a separate issue, IMHO.

jreback · 2018-05-04T10:12:05Z

thanks @h-vetinari good followup!

as far as dicts go, certainly welcome clarification if needed (PR / issue all ok), or can just wait and see if that usecase is seen in real life

h-vetinari · 2018-05-04T11:00:25Z

@jreback
Glad this PR is done, TBH. Thanks for all the reviews and feedback!

Follow-up: incorporate review about _get_series_list

34cce91

h-vetinari commented May 2, 2018

View reviewed changes

Further comment improvements

08593e0

jreback added Strings String extension data type and string data Clean labels May 3, 2018

jreback requested changes May 3, 2018

View reviewed changes

h-vetinari commented May 3, 2018

View reviewed changes

One more round of review

3bf38a0

h-vetinari force-pushed the str_cat_align branch from d270178 to 3bf38a0 Compare May 3, 2018 05:11

h-vetinari commented May 3, 2018

View reviewed changes

jreback approved these changes May 3, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone May 3, 2018

jreback merged commit ef019fa into pandas-dev:master May 4, 2018

h-vetinari mentioned this pull request Jul 26, 2019

DEPR: execute deprecations for str.cat in v1.0 #27611

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow-up #20347: incorporate review about _get_series_list #20923

Follow-up #20347: incorporate review about _get_series_list #20923

h-vetinari commented May 2, 2018 •

edited

Loading

h-vetinari left a comment

h-vetinari May 2, 2018

jreback May 3, 2018

h-vetinari May 3, 2018 •

edited

Loading

jreback May 3, 2018

h-vetinari May 2, 2018

h-vetinari May 2, 2018

h-vetinari May 2, 2018 •

edited

Loading

h-vetinari May 2, 2018

h-vetinari commented May 2, 2018

h-vetinari commented May 2, 2018 •

edited

Loading

TomAugspurger commented May 2, 2018

codecov bot commented May 2, 2018 •

edited

Loading

jreback May 3, 2018

jreback commented May 3, 2018

h-vetinari left a comment

h-vetinari May 3, 2018 •

edited

Loading

h-vetinari left a comment

h-vetinari May 3, 2018

jreback May 3, 2018

jreback May 3, 2018

jreback May 3, 2018

h-vetinari commented May 3, 2018

h-vetinari commented May 4, 2018

jreback commented May 4, 2018

h-vetinari commented May 4, 2018

		@@ -311,7 +311,7 @@ All one-dimensional list-likes can be arbitrarily combined in a list-like contai

Follow-up #20347: incorporate review about _get_series_list #20923

Follow-up #20347: incorporate review about _get_series_list #20923

Conversation

h-vetinari commented May 2, 2018 • edited Loading

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented May 2, 2018

h-vetinari commented May 2, 2018 • edited Loading

TomAugspurger commented May 2, 2018

codecov bot commented May 2, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jreback commented May 3, 2018

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari May 3, 2018 • edited Loading

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented May 3, 2018

h-vetinari commented May 4, 2018

jreback commented May 4, 2018

h-vetinari commented May 4, 2018

h-vetinari commented May 2, 2018 •

edited

Loading

h-vetinari May 3, 2018 •

edited

Loading

h-vetinari May 2, 2018 •

edited

Loading

h-vetinari commented May 2, 2018 •

edited

Loading

codecov bot commented May 2, 2018 •

edited

Loading

h-vetinari May 3, 2018 •

edited

Loading