Skip to content
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 docstring of pandas.Series.add_prefix docstring #20313

Merged
merged 8 commits into from
Mar 13, 2018

Conversation

astrastefania
Copy link
Contributor

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:

################################################################################
##################### Docstring (pandas.Series.add_prefix) #####################
################################################################################

Concatenate prefix string with panel items names.

Parameters
----------
prefix : str
    The string to add before each item name.

Returns
-------
Series
    Original Series with updated item names.

See Also
--------
pandas.Series.add_suffix: Add a suffix string to panel items names.

Examples
--------
>>> s = pd.Series([1,2,3,4])
>>> s
0    1
1    2
2    3
3    4
dtype: int64
>>> s.add_prefix('item_')
item_0    1
item_1    2
item_2    3
item_3    4
dtype: int64

################################################################################
################################## Validation ##################################
################################################################################

Errors found:
	No extended summary found

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.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Nice PR. Added couple of comments.


Examples
--------
>>> s = pd.Series([1,2,3,4])
Copy link
Member

Choose a reason for hiding this comment

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

Missing spaces after commas to pass PEP-8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, the PEP-8 passed with any comments, it could be updated with this case?


See Also
--------
pandas.Series.add_suffix: Add a suffix string to panel items names.
Copy link
Member

Choose a reason for hiding this comment

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

I think the preferred option is to not user the pandas. prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took it away. :)

@@ -2967,11 +2967,33 @@ def add_prefix(self, prefix):

Copy link
Member

Choose a reason for hiding this comment

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

Panel is deprecated, we may want to use Series or DataFrame, or something generic.

I'd try to find uses cases on the internet for this method, and add in the extended summary when this method can be useful, if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I used Series as it is a Series module in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this and #20315, these apply to both Series and DataFrame (and Panel, but we don't care about that). So perhaps

Prefix row labels with a string `prefix`.

I want to avoid items, as (to me) I think dict.items so key-value pairs. But we're just touching the row labels here.

That summary is strange since we use Prefix as a verb and nound. Maybe "Prepend" would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thank you, my doubt is that on DataFrames add_prefix() adds a prefix on columns names not on rows, for example: df = pd.DataFrame({'A':[1,2,3,4], 'B':[3,4,5,6]}), df.add_suffix('_item'). For this motivations I decided to leave on Series.

Perhaps I could updated the docstring for pandas.DataFrame.add_prefix with the relative example to avoid confusion and let this as it is.

Let me know what sound better. :)

Copy link
Contributor

@TomAugspurger TomAugspurger Mar 13, 2018

Choose a reason for hiding this comment

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

Ahh, good catch! I would say splitting the docstring is more complexity that warranted. How about somehting like

"""
Prefix labels with string `prefix`.

For Series, the row labels are prefixed. For DataFrame, the column labels are prefixed

...
"""

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 do agree, thanks!

I didn't know about the issue of splitting docstring, so in this case is better have the same for both pandas.Series.add_prefix and pandas.DataFrame.add_prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the docstring you've been editing in core/generic.py is used by both Series.add_prefix and DataFrame.add_prefix.

@astrastefania
Copy link
Contributor Author

Docstring updated, thanks for the comments!

@codecov
Copy link

codecov bot commented Mar 13, 2018

Codecov Report

Merging #20313 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20313      +/-   ##
==========================================
+ Coverage    91.7%   91.73%   +0.02%     
==========================================
  Files         150      150              
  Lines       49165    49168       +3     
==========================================
+ Hits        45087    45102      +15     
+ Misses       4078     4066      -12
Flag Coverage Δ
#multiple 90.11% <ø> (+0.02%) ⬆️
#single 41.86% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 95.85% <ø> (ø) ⬆️
pandas/core/algorithms.py 94.17% <0%> (-0.01%) ⬇️
pandas/core/frame.py 97.18% <0%> (ø) ⬆️
pandas/core/indexes/base.py 96.66% <0%> (ø) ⬆️
pandas/plotting/_converter.py 66.81% <0%> (+1.73%) ⬆️
pandas/core/arrays/base.py 76.74% <0%> (+2.38%) ⬆️

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 bfe6ebc...6d599ad. Read the comment docs.

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Thanks. Do you have time to update this and #20315 with similar changes @astrastefania?

@@ -2967,11 +2967,33 @@ def add_prefix(self, prefix):

Copy link
Contributor

Choose a reason for hiding this comment

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

For this and #20315, these apply to both Series and DataFrame (and Panel, but we don't care about that). So perhaps

Prefix row labels with a string `prefix`.

I want to avoid items, as (to me) I think dict.items so key-value pairs. But we're just touching the row labels here.

That summary is strange since we use Prefix as a verb and nound. Maybe "Prepend" would be better?


Returns
-------
with_prefix : type of caller
Series
Copy link
Contributor

Choose a reason for hiding this comment

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

Series or DataFrame
    Same type as the calling object, with updated row labels.


See Also
--------
Series.add_suffix: Add a suffix string to Series items names.
Copy link
Contributor

Choose a reason for hiding this comment

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

"items names" -> "row labels"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point for "item names", please check the comment above related to include DataFrame or not (if not could make the change to "row labels" suggested here).

Copy link
Contributor

Choose a reason for hiding this comment

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

So anywhere we say "item names", let's say "labels." or "row labels" or "column labels".

item_1 2
item_2 3
item_3 4
dtype: int64
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line and then an example with DataFrame.


Returns
-------
with_prefix : type of caller
Series or DataFrame
Original Series or DataFrame with updated labels.
Copy link
Contributor

Choose a reason for hiding this comment

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

This make it sound a bit like the object is modified inplace. Could you instead say

Series or DataFrame
    New Series or DataFrame with updated labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually is modified inplace, should it be specified?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't think so

In [6]: s = pd.Series(1, ['a', 'b'])

In [7]: s
Out[7]:
a    1
b    1
dtype: int64

In [8]: s.add_prefix('foo_')
Out[8]:
foo_a    1
foo_b    1
dtype: int64

In [9]: s
Out[9]:
a    1
b    1
dtype: int64

s is unmodified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant it's unmodified! I agree is not modified inplace.

1 2 4
2 3 5
3 4 6
>>> df.add_suffix('_item')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the add_prefix example I think :)

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!!! 😄

@TomAugspurger
Copy link
Contributor

LGTM other than those two minor issues.

@astrastefania
Copy link
Contributor Author

@TomAugspurger @datapythonista, if it's all fine in here now I'll proceed to modify #20315 (otherwise I'll update this one first)

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Mar 13, 2018

Looks great! Just added a tiny commit adjusting the spacing. Thanks!

@TomAugspurger TomAugspurger merged commit 7c5bae3 into pandas-dev:master Mar 13, 2018
@TomAugspurger TomAugspurger added this to the 0.23.0 milestone Mar 13, 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.

4 participants