Skip to content

DOC: Improve the docstring of pandas.core.window.Rolling.median #20260

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

Conversation

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

# paste output of "scripts/validate_docstrings.py <your-function-or-method>" here
# between the "```" (remove this comment, but keep the "```")

Errors found:
No extended summary found
Errors in parameters section
Parameters {'kwargs'} not documented
Unknown parameters {'**kwargs'}
Parameter "**kwargs" has no type

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.

Median is pretty straightforward. Maybe an extended summary is not really necessary? Kwargs needs further review.

three.

>>> import pandas as pd
>>> import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

Import pandas and numpy is not necessary in the examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. I've removed them.

Copy link
Contributor

@dukebody dukebody left a comment

Choose a reason for hiding this comment

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

LGTM after fixing the unneeded imports issue.


>>> import pandas as pd
>>> import numpy as np
>>> s = pd.Series(np.arange(5))
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps construct the series manually to avoid the reader having to think about arange:

s = pd.Series([0, 1, 2, 3, 4])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That make sense. I've changed it from arange to a simple list


Examples
--------
The below example will show a rolling calculation with a window size of
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be more direct and say "Compute the rolling mean of a series with a window size of 3:".

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 agree... but this is median. Copy paste has failed me!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes median, sorry.

@mehemken
Copy link
Contributor Author

Thanks for the feedback everyone. Anything else?

@dukebody
Copy link
Contributor

LGTM, the only think I'm not sure about is what to put under kwargs. Don't know if we really expect any args there, or it's there only for compatibility purposes.

Looking at https://pandas.pydata.org/pandas-docs/stable/generated/pandas.Series.rolling.html I believe most arguments you could pass to https://pandas.pydata.org/pandas-docs/version/0.17.0/generated/pandas.rolling_median.html are already in the rolling window definition. But in pandas.rolling_median there are also **kwargs...

In the mailing list someone proposed to leave unclear kwargs documented as "Needs Review" and look at them in the future, so this works for me.

@JoanMartin ?

simplified return type description
@codecov
Copy link

codecov bot commented Mar 13, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #20260   +/-   ##
=========================================
  Coverage          ?   91.73%           
=========================================
  Files             ?      150           
  Lines             ?    49166           
  Branches          ?        0           
=========================================
  Hits              ?    45100           
  Misses            ?     4066           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.11% <ø> (?)
#single 41.86% <ø> (?)
Impacted Files Coverage Δ
pandas/core/window.py 96.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 fb556ed...702abc4. Read the comment docs.

@TomAugspurger TomAugspurger merged commit e7e0ea8 into pandas-dev:master Mar 13, 2018
@TomAugspurger
Copy link
Contributor

Thanks @mehemken !

@TomAugspurger TomAugspurger added this to the 0.23.0 milestone Mar 13, 2018
@dukebody
Copy link
Contributor

Stupid question because I'm very surprised: How to you add commits to a PR which is based on a personal fork of the project, if you are not the owner of the fork?

@dukebody
Copy link
Contributor

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.

5 participants