-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEPR: list-likes of list-likes in str.cat #22264
Conversation
0eeff33
to
372cc24
Compare
doc/source/text.rst
Outdated
@@ -306,20 +306,20 @@ The same alignment can be used when ``others`` is a ``DataFrame``: | |||
Concatenating a Series and many objects into a Series | |||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
|
|||
All one-dimensional list-likes can be arbitrarily combined in a list-like container (including iterators, ``dict``-views, etc.): | |||
Several objects of type ``Series``, ``Index`` or ``np.ndarray`` can be arbitrarily combined in a list-like container (including iterators, ``dict``-views, etc.): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Several objects of type Series
, Index
, or np.ndarray
" - are you sure that's what you meant to say? I think that should be reworded.
pandas/core/strings.py
Outdated
join_warn = join_warn or wnx | ||
|
||
if depr_warn: | ||
warnings.warn('list-likes other than Series, Index or ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add a comma after "Index"
Hello @h-vetinari! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on August 28, 2018 at 17:09 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #22264 +/- ##
=========================================
Coverage ? 92.03%
=========================================
Files ? 169
Lines ? 50785
Branches ? 0
=========================================
Hits ? 46742
Misses ? 4043
Partials ? 0
Continue to review full report at Codecov.
|
16b0b03
to
1ab981d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine, ex comments on the text.rst
doc/source/text.rst
Outdated
|
||
.. ipython:: python | ||
|
||
s | ||
u | ||
s.str.cat([u.values, ['A', 'B', 'C', 'D'], map(str, u.index)], na_rep='-') | ||
s.str.cat([u.values, u, u.index.astype(str)], na_rep='-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you save these in another variable, rather than doing the combination in-line. its easier to see it printed out (with comments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "comments"?
doc/source/text.rst
Outdated
|
||
All elements must match in length to the calling ``Series`` (or ``Index``), except those having an index if ``join`` is not None: | ||
|
||
.. ipython:: python | ||
|
||
v | ||
s.str.cat([u, v, ['A', 'B', 'C', 'D']], join='outer', na_rep='-') | ||
s.str.cat([u, v, u.values], join='outer', na_rep='-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, u
and u.values
will have been shown 3 lines above, and v
directly above. I think this should be plenty, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use w as you defined above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point here is to show how join
works with objects that have no index, so can't use w
(which has an index).
doc/source/text.rst
Outdated
|
||
.. ipython:: python | ||
|
||
s | ||
u | ||
s.str.cat([u.values, ['A', 'B', 'C', 'D'], map(str, u.index)], na_rep='-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put some comments here, this is just too much code in a row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my POV, it's just one line of code, while showing all the ingredients. But I'm gonna change it back to just using variants of u
in different "boxes".
I think it's fair enough to show just u
and not u.values
and pd.Index(u)
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so pls show u.values, I have no idea what it actually is, and as I said, pls either add comments or split up showing s, u, u.values, this is just too long
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback u
is shown directly above...?! Also in the screenshot I posted - in context it's IMO very clear (especially as it's at the end of the concatenation section, where the reader has seen the same objects 10 times already).
I can of course show both u
and u.values
, but I thought that understanding the relationship between a Series and its .values
would be safe to expect of a users pandas-fu.
doc/source/text.rst
Outdated
|
||
All elements must match in length to the calling ``Series`` (or ``Index``), except those having an index if ``join`` is not None: | ||
|
||
.. ipython:: python | ||
|
||
v | ||
s.str.cat([u, v, ['A', 'B', 'C', 'D']], join='outer', na_rep='-') | ||
s.str.cat([u, v, u.values], join='outer', na_rep='-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use w as you defined above
maybe would be helpful to show a rendered version here of text.rst |
@jreback here you go: |
@jreback @TomAugspurger ping :) |
doc/source/text.rst
Outdated
|
||
.. ipython:: python | ||
|
||
s | ||
u | ||
s.str.cat([u.values, ['A', 'B', 'C', 'D'], map(str, u.index)], na_rep='-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so pls show u.values, I have no idea what it actually is, and as I said, pls either add comments or split up showing s, u, u.values, this is just too long
@@ -474,12 +474,14 @@ Other API Changes | |||
Deprecations | |||
~~~~~~~~~~~~ | |||
|
|||
- :meth:`DataFrame.to_stata`, :meth:`read_stata`, :class:`StataReader` and :class:`StataWriter` have deprecated the ``encoding`` argument. The encoding of a Stata dta file is determined by the file type and cannot be changed (:issue:`21244`). | |||
- :meth:`MultiIndex.to_hierarchical` is deprecated and will be removed in a future version (:issue:`21613`) | |||
- :meth:`DataFrame.to_stata`, :meth:`read_stata`, :class:`StataReader` and :class:`StataWriter` have deprecated the ``encoding`` argument. The encoding of a Stata dta file is determined by the file type and cannot be changed (:issue:`21244`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls don't edit other things. The longer the PR the more time things will take. I guess since its already done leave it.
doc/source/whatsnew/v0.24.0.txt
Outdated
- :meth:`Series.ptp` is deprecated. Use ``numpy.ptp`` instead (:issue:`21614`) | ||
- :meth:`Series.compress` is deprecated. Use ``Series[condition]`` instead (:issue:`18262`) | ||
- :meth:`Categorical.from_codes` has deprecated providing float values for the ``codes`` argument. (:issue:`21767`) | ||
- :func:`pandas.read_table` is deprecated. Instead, use :func:`pandas.read_csv` passing ``sep='\t'`` if necessary (:issue:`21948`) | ||
- :meth:`Series.str.cat` has deprecated using arbitrary list-likes *within* list-likes. A list-like container may still contain | ||
arbitrarily many ``Series``, ``Index`` or 1-dimenstional ``np.ndarray``, or alternatively, only scalar values. (:issue:`21950`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the word arbitrarily (and in text.rst)
pandas/core/strings.py
Outdated
others : Series, DataFrame, np.ndarray, list-like or list-like of | ||
objects that are either Series, np.ndarray (1-dim) or list-like | ||
others : Series, Index, DataFrame, np.ndarray, list-like or list-like | ||
of objects that are Series, Index, np.ndarray (1-dim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or np.ndarray
@jreback Incorporated feedback, except the
In context it's IMO very clear (especially as it's at the end of the concatenation section, where the reader has seen the same objects 10 times already). |
@jreback Same screenshot as above. IMO the relationship |
ok this is fine. can you rebase once more and ping on green. |
dd74c2e
to
a3ea6fc
Compare
@jreback All green. |
730ab79
to
80547eb
Compare
80547eb
to
d6b8a68
Compare
All green. Just fixed an indentation error on the way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jreback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny typo, ping on push.
doc/source/whatsnew/v0.24.0.txt
Outdated
- :meth:`Series.ptp` is deprecated. Use ``numpy.ptp`` instead (:issue:`21614`) | ||
- :meth:`Series.compress` is deprecated. Use ``Series[condition]`` instead (:issue:`18262`) | ||
- The signature of :meth:`Series.to_csv` has been uniformed to that of doc:meth:`DataFrame.to_csv`: the name of the first argument is now 'path_or_buf', the order of subsequent arguments has changed, the 'header' argument now defaults to True. (:issue:`19715`) | ||
- :meth:`Categorical.from_codes` has deprecated providing float values for the ``codes`` argument. (:issue:`21767`) | ||
- :func:`pandas.read_table` is deprecated. Instead, use :func:`pandas.read_csv` passing ``sep='\t'`` if necessary (:issue:`21948`) | ||
- :meth:`Series.str.cat` has deprecated using arbitrary list-likes *within* list-likes. A list-like container may still contain | ||
many ``Series``, ``Index`` or 1-dimenstional ``np.ndarray``, or alternatively, only scalar values. (:issue:`21950`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dimensional
bf66ce3
to
4a2b33b
Compare
Side note: |
4a2b33b
to
7e74d80
Compare
This is all done, approved and green. |
I don't like these skip's generally. they are not necessary as all of the CI's cancel prior builds anyhow. |
thanks @h-vetinari |
This was not about cancelling prior builds but not having to run the full CI suite if (say) a single comment (like for the GH number) gets added. I view the CI as somewhat limited resources (appveyor recently having something like 12h waiting time), so |
git diff upstream/master -u -- "*.py" | flake8 --diff
As mentioned in #21950, my suggestion is to modify the allowed combinations (as of v0.23) as follows:
In other words, if the user wants sequential concatenation, there are many possibilities available, and list-of-lists does not have to be one of them, IMO. This would substantially simplify (post-deprecation) the code for
str.cat._get_series_list
: