Skip to content

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

Merged
merged 6 commits into from
Mar 14, 2018

Conversation

vipinkjonwal
Copy link
Contributor

@vipinkjonwal vipinkjonwal commented Mar 10, 2018

Checklist for the pandas documentation sprint (ignore this if you are doing
an unrelated PR):

  • PR title is "DOC: update the docstring"
  • The validation script passes: scripts/validate_docstrings.py <your-function-or-method>
  • The PEP8 style check passes: git diff upstream/master -u -- "*.py" | flake8 --diff
  • The html version looks good: python doc/make.py --single <your-function-or-method>
  • It has been proofread on language by another sprint participant

Please include the output of the validation script below between the "```" ticks:

# paste output of "scripts/validate_docstrings.py <your-function-or-method>" here
"Docstring for "pandas.Series.str.cat" correct. :)"

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):

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@jorisvandenbossche
Copy link
Member

Please show the output of the validation script as asked

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a 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

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
Copy link
Member

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?

Copy link
Contributor Author

@vipinkjonwal vipinkjonwal Mar 10, 2018

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)

Copy link
Member

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"

@@ -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=' ')
Copy link
Member

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=..)

Copy link
Contributor Author

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.

@@ -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)
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you add this?

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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Adding.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a 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

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
Copy link
Member

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, splitis the user-facing interface)

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'])
Copy link
Member

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)

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
Copy link
Member

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
Copy link

codecov bot commented Mar 14, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@d7bcb22). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #20171   +/-   ##
=========================================
  Coverage          ?   91.72%           
=========================================
  Files             ?      150           
  Lines             ?    49152           
  Branches          ?        0           
=========================================
  Hits              ?    45086           
  Misses            ?     4066           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.11% <ø> (?)
#single 41.84% <ø> (?)
Impacted Files Coverage Δ
pandas/core/strings.py 98.32% <ø> (ø)

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 d7bcb22...7aa5ea7. Read the comment docs.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a 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!

@jorisvandenbossche jorisvandenbossche merged commit e1fb30b into pandas-dev:master Mar 14, 2018
@jorisvandenbossche
Copy link
Member

@vipinkjonwal Thanks for the PR!

@jorisvandenbossche jorisvandenbossche added this to the 0.23.0 milestone Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants