-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
DOC: Improve the docstring of pandas.core.window.Rolling.median #20260
Conversation
pandas/core/window.py
Outdated
three. | ||
|
||
>>> import pandas as pd | ||
>>> import numpy as np |
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.
Import pandas and numpy is not necessary in the examples.
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.
Good to know. I've removed them.
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.
LGTM after fixing the unneeded imports issue.
pandas/core/window.py
Outdated
|
||
>>> import pandas as pd | ||
>>> import numpy as np | ||
>>> s = pd.Series(np.arange(5)) |
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.
Perhaps construct the series manually to avoid the reader having to think about arange:
s = pd.Series([0, 1, 2, 3, 4])
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.
That make sense. I've changed it from arange
to a simple list
pandas/core/window.py
Outdated
|
||
Examples | ||
-------- | ||
The below example will show a rolling calculation with a window size of |
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'd be more direct and say "Compute the rolling mean of a series with a window size of 3:".
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 agree... but this is median. Copy paste has failed me!
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 yes median, sorry.
Thanks for the feedback everyone. Anything else? |
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 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. |
simplified return type description
Codecov Report
@@ Coverage Diff @@
## master #20260 +/- ##
=========================================
Coverage ? 91.73%
=========================================
Files ? 150
Lines ? 49166
Branches ? 0
=========================================
Hits ? 45100
Misses ? 4066
Partials ? 0
Continue to review full report at Codecov.
|
Thanks @mehemken ! |
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? |
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:
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.