-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DOC: update the str_cat() docstring (Delhi) #20171
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
DOC: update the str_cat() docstring (Delhi) #20171
Conversation
Please show the output of the validation script as asked |
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.
Thanks for the PR! Added some first comments
pandas/core/strings.py
Outdated
This function concatenates elements of Series/Index and elements | ||
of lists or list-like objects having same length. | ||
The concatenation is done at corresponding indices of | ||
iterable specified by `Series` when two or more iterables |
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.
This part is not fully clear to me: " corresponding indices of iterable"
Can you give a small example of what you want to explain 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.
Corresponding indices indicates that the concatenation is being done in one to one correspondence with the indices i.e, index of iterable, let say list.
For example :
pd.Series(['a', 'b', 'c']).str.cat(['A', 'B', 'C'], sep=',')
index
0 a,A (a with A i.e, 0th index of list in Series and 0th index of list in cat(list...) )
1 b,B (b with B)
2 c,C (c with C)
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.
Ah, OK. In that case, I think it is fine to say that "concatenates the Series/Index and other
element-wise"
pandas/core/strings.py
Outdated
@@ -68,29 +76,29 @@ def str_cat(arr, others=None, sep=None, na_rep=None): | |||
When ``na_rep`` is `None` (default behavior), NaN value(s) | |||
in the Series are ignored. | |||
|
|||
>>> Series(['a','b',np.nan,'c']).str.cat(sep=' ') | |||
>>> pd.Series(['a','b',np.nan,'c']).str.cat(sep=' ') |
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 split this example in two steps?
s = pd.Series(...)
s.str.cat(sep=..)
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.
Yes, I can do this way as well.
pandas/core/strings.py
Outdated
@@ -68,29 +76,29 @@ def str_cat(arr, others=None, sep=None, na_rep=None): | |||
When ``na_rep`` is `None` (default behavior), NaN value(s) |
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.
I would also say something here about not passing other
that then all values in the Series are concatenated in a single string (so to indicate you first give an example of this part of the behaviour of the method)
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.
Added :-)
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.
Did you add this?
pandas/core/strings.py
Outdated
iterable specified by `Series` when two or more iterables | ||
are present. The final concatenation is done with a given `sep` | ||
and a string is returned. | ||
|
||
Parameters | ||
---------- | ||
others : list-like, or list of list-likes |
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 add , optional
at the end of the line?
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.
Yes. Adding.
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.
Thanks for the updates! Added some additional comments
pandas/core/strings.py
Outdated
na_rep : string or None, default None | ||
If None, NA in the series are ignored. | ||
|
||
Returns | ||
------- | ||
concat : Series/Index of objects or str | ||
|
||
See Also | ||
-------- | ||
str_split : Split each string in the Series/Index |
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.
str_split -> split (str_split
is an implementation detail in this case, split
is the user-facing interface)
pandas/core/strings.py
Outdated
Examples | ||
-------- | ||
When ``na_rep`` is `None` (default behavior), NaN value(s) | ||
in the Series are ignored. | ||
|
||
>>> Series(['a','b',np.nan,'c']).str.cat(sep=' ') | ||
>>> s = pd.Series(['a','b',np.nan,'c']) |
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 add spaces after the comma (PEP8)
(same for the examples below)
pandas/core/strings.py
Outdated
This function concatenates elements of Series/Index and elements | ||
of lists or list-like objects having same length. | ||
The concatenation is done at corresponding indices of | ||
iterable specified by `Series` when two or more iterables |
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.
Ah, OK. In that case, I think it is fine to say that "concatenates the Series/Index and other
element-wise"
Codecov Report
@@ Coverage Diff @@
## master #20171 +/- ##
=========================================
Coverage ? 91.72%
=========================================
Files ? 150
Lines ? 49152
Branches ? 0
=========================================
Hits ? 45086
Misses ? 4066
Partials ? 0
Continue to review full report at Codecov.
|
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.
Simplified the extended summary a bit (see my feedback), looks good for the rest!
@vipinkjonwal Thanks for the PR! |
Checklist for the pandas documentation sprint (ignore this if you are doing
an unrelated PR):
scripts/validate_docstrings.py <your-function-or-method>
git diff upstream/master -u -- "*.py" | flake8 --diff
python doc/make.py --single <your-function-or-method>
Please include the output of the validation script below between the "```" ticks:
If the validation script still gives errors, but you think there is a good reason
to deviate in this case (and there are certainly such cases), please state this
explicitly.
Checklist for other PRs (remove this part if you are doing a PR for the pandas documentation sprint):
git diff upstream/master -u -- "*.py" | flake8 --diff