-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DOC: update the pandas.Series/DataFrame.interpolate docstring #20270
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 pandas.Series/DataFrame.interpolate docstring #20270
Conversation
pandas/core/generic.py
Outdated
|
||
* 'linear': ignore the index and treat the values as equally | ||
* 'linear': Ignore the index and treat the values as equally |
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.
Generally shouldn't need periods at the end of bullet points
pandas/core/generic.py
Outdated
* 'time': interpolation works on daily and higher resolution | ||
data to interpolate given length of interval | ||
* 'index', 'values': use the actual numerical values of the index | ||
Default. |
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.
Don't need this
pandas/core/generic.py
Outdated
'polynomial', 'spline', 'piecewise_polynomial', | ||
'from_derivatives', 'pchip', 'akima'} | ||
'polynomial', 'spline', 'piecewise_polynomial', 'pad', | ||
'from_derivatives', 'pchip', 'akima'}, default 'linear' |
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.
Shouldn't need the default designation at end (implied by linear
being the first value)
pandas/core/generic.py
Outdated
data to interpolate given length of interval | ||
* 'index', 'values': use the actual numerical values of the index | ||
Default. | ||
* 'time': Interpolation works on daily and higher resolution |
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.
"Interpolation works...to interpolate" seems unnecessarily verbose. Perhaps just "Works on daily and higher resolution data"?
pandas/core/generic.py
Outdated
* 'nearest', 'zero', 'slinear', 'quadratic', 'cubic', | ||
'barycentric', 'polynomial' is passed to | ||
'barycentric', 'polynomial': Passed to | ||
``scipy.interpolate.interp1d``. Both 'polynomial' and 'spline' | ||
require that you also specify an `order` (int), | ||
e.g. df.interpolate(method='polynomial', order=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.
Seems better served as a dedicated example than crammed into this
pandas/core/generic.py
Outdated
|
||
Examples | ||
-------- | ||
|
||
Filling in NaNs | ||
Filling in NaNs in a Series via linear interpolation. |
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.
:class:`~pandas.Series`
pandas/core/generic.py
Outdated
1 1 | ||
2 2 | ||
3 3 | ||
>>> ser = pd.Series([0, 1, np.nan, 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.
Convention here is s =
instead of ser =
pandas/core/generic.py
Outdated
>>> ser = pd.Series([np.nan, "single_one", np.nan, | ||
... "fill_two_more", np.nan, np.nan, np.nan, | ||
... 4.71, np.nan]) | ||
>>> ser |
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.
To save space you don't need to print the Series
here - should be straightforward based off the constructor directly above it
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 did not include the print for the very small series example (it was straightforward to see), but I'd like to keep this longer one if that's alright - it was encouraged so the differences can be spotted easier.
pandas/core/generic.py
Outdated
|
||
Create a DataFrame with missing values. | ||
|
||
>>> df = pd.DataFrame([[0,1,2,0,4],[1,2,3,-1,8], |
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.
Why not just construct with the missing values?
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.
Mainly so people can see the "expected" Interpolation (I tried to have a pattern column-wise) and they can compare it with what actually happens, e.g. with lin. Interpolation (especially if the last entry is an NA)
pandas/core/generic.py
Outdated
Fill the DataFrame forward (that is, going down) along each column. | ||
Note how the last entry in column `a` is interpolated differently | ||
(because there is no entry after it to use for interpolation). | ||
Note how the first entry in column `b` remains NA (because there |
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.
NaN
Lot's of comments but just wanted to say nice job! This is one of the tougher docstrings |
I hope to finish these great suggestions some time today. |
Thank you for the thorough review, @WillAyd I believe the errors below can be ignored, because they relate to known issues (**kwargs, .. versionadded::, etc.)
|
pandas/core/generic.py
Outdated
|
||
* 'linear': ignore the index and treat the values as equally | ||
* 'linear': Ignore the index and treat the values as equally | ||
spaced. This is the only method supported on MultiIndexes. |
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 understand why you added these, but generally do not put punctuation at the end of bullet points. If you get an error as a result OK to ignore
pandas/core/generic.py
Outdated
* 'index', 'values': use the actual numerical values of the index. | ||
* 'pad': Fill in NaNs using existing values. | ||
* 'nearest', 'zero', 'slinear', 'quadratic', 'cubic', 'spline', | ||
'barycentric', 'polynomial': Passed to | ||
``scipy.interpolate.interp1d``. Both 'polynomial' and 'spline' |
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 do the same thing here you did for 'krogh' and move some of the implementation details down to the Notes section
pandas/core/generic.py
Outdated
<http://docs.scipy.org/doc/scipy/reference/tutorial/interpolate.html>`__ | ||
* 'from_derivatives' refers to BPoly.from_derivatives which | ||
* 'krogh', 'piecewise_polynomial', 'spline', 'pchip', 'akima': | ||
Wrappers around the scipy interpolation methods of similar |
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.
Use SciPy instead of scipy when referring to the package outside of code (couple other places this pops up)
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.
sure
pandas/core/generic.py
Outdated
If limit is specified, consecutive NaNs will be filled in this | ||
direction. | ||
inplace : bool, default False | ||
Update the NDFrame in place if possible. | ||
limit_area : {`None`, 'inside', 'outside'} |
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.
Add ", default None" to the end here and remove the comment about it being the default below
pandas/core/generic.py
Outdated
* None: No fill restriction (default). | ||
* 'inside': Only fill NaNs surrounded by valid values | ||
(interpolate). | ||
* 'outside': Only fill NaNs outside valid values (extrapolate). |
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.
Would be good to add an example for 'outside'
pandas/core/generic.py
Outdated
If the selected `method` is one of 'krogh', 'piecewise_polynomial', | ||
'spline', 'pchip', 'akima': | ||
They are wrappers around the scipy interpolation methods of similar | ||
names. These use the actual numerical values of the 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.
What does "These use the actual numerical values of the index" mean?
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.
"These use the actual numerical values of the index." Better grammar?
pandas/core/generic.py
Outdated
3 8.000000 | ||
dtype: float64 | ||
|
||
Create a :class:`~pandas.DataFrame` with missing values to fill it |
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.
You are explaining here what the below code is going to do, but not really saying what it's important. Would be better worded as "Interpolation can also be applied to DataFrames" or something to the effect
pandas/core/generic.py
Outdated
|
||
>>> df = pd.DataFrame([[0,1,2,0,4],[1,2,3,-1,8], |
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.
Thought I had this comment before but just use the NA values in your constructor - no reason to instantiate the DataFrame with values and then assign them missing values after the fact.
Also make sure you put a space after every comma
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 changed it so one can see how the columns get created - and we have linear values in 3 columns and quadratic on the 4th.
pandas/core/generic.py
Outdated
|
||
Fill the DataFrame forward (that is, going down) along each column. | ||
Note how the last entry in column `a` is interpolated differently | ||
(because there is no entry after it to use for interpolation). |
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.
Don't need the parentheses here (nor on the next line)
pandas/core/generic.py
Outdated
|
||
Returns | ||
------- | ||
Series or DataFrame of same shape interpolated at the NaNs | ||
Series or DataFrame | ||
Same-shape object interpolated at the NaN values |
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.
For the description here say "Returns the same object type as the caller" - that wording has been used by a few other PRs so just want to be consistent
Hello @math-and-data! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on August 19, 2018 at 00:25 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #20270 +/- ##
=======================================
Coverage 92.05% 92.05%
=======================================
Files 169 169
Lines 50713 50713
=======================================
Hits 46683 46683
Misses 4030 4030
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.
@WillAyd if you don't mind reviewing this one too. Made few minor changes like pep8 of the doctest, the type of method couldn't be all options, as sphinx do not let parameter types be multiline and things like this.
Thanks for the docstring @math-and-data, really good work. And sorry for the long wait.
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.
IIUC needs some fixes around backtick usage
pandas/core/generic.py
Outdated
* 'pad': Fill in NaNs using existing values. | ||
* 'nearest', 'zero', 'slinear', 'quadratic', 'cubic', 'spline', | ||
'barycentric', 'polynomial': Passed to | ||
``scipy.interpolate.interp1d``. Both 'polynomial' and 'spline' |
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 should just be single backticks no?
pandas/core/generic.py
Outdated
Wrappers around the SciPy interpolation methods of similar | ||
names. See `Notes`. | ||
* 'from_derivatives': Refers to | ||
``scipy.interpolate.BPoly.from_derivatives`` which |
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.
Single backtick too?
pandas/core/generic.py
Outdated
If limit is specified, consecutive NaNs will be filled with this | ||
restriction. | ||
|
||
* None: No fill restriction. |
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.
Maybe double backticks here to render literal value?
pandas/core/generic.py
Outdated
Series or DataFrame of same shape interpolated at the NaNs | ||
Series or DataFrame | ||
Returns the same object type as the caller, interpolated at | ||
some or all `NaN` values |
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.
Double ticks
pandas/core/generic.py
Outdated
|
||
Examples | ||
-------- | ||
|
||
Filling in NaNs | ||
Filling in `NaN` in a :class:`~pandas.Series` via linear |
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.
Double
pandas/core/generic.py
Outdated
dtype: float64 | ||
|
||
Filling in `NaN` in a Series by padding, but filling at most two |
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.
Double ticks (here and next line)
pandas/core/generic.py
Outdated
8 4.71 | ||
dtype: object | ||
|
||
Filling in `NaN` in a Series via polynomial interpolation or splines: |
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.
Double backtick
pandas/core/generic.py
Outdated
|
||
Note how the last entry in column `a` is interpolated differently, | ||
because there is no entry after it to use for interpolation. | ||
Note how the first entry in column `b` remains `NaN`, because there |
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.
Double backticks
Yep, agree. I think they should be all right now. Thanks @WillAyd ! |
pandas/core/generic.py
Outdated
an `order` (int). | ||
Filling in ``NaN`` in a Series via polynomial interpolation or splines: | ||
Both 'polynomial' and 'spline' methods require that you also specify | ||
an ``order`` (int). |
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.
Parameters should be single backticks - double is only for literals and code samples I think
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.
My understanding is that double backticks is for code, including parts like a single variable None
, an assignment foo=1
... Single backticks is for things that you can refer (link) to, like a function, class, module... And for values just quotes.
For an argument, I'd consider it more code, that something you can link to. That's why I added double backticks. But it's very subtle, I'd be happy with any option (no quoting, single backticks, double backticks and quotes).
Does this make sense?
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.
Yep thanks. I think I've seen other instances where parameters are in single backticks but this is nuanced enough that it shouldn't hold up the PR - can be part of a larger conversation.
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 added a bullet point to #20298 to decide a standard for these cases. I think at the moment there is not much consistency.
Thanks @math-and-data and @datapythonista ! |
interpolator). | ||
scipy.interpolate.PchipInterpolator : PCHIP 1-d monotonic cubic | ||
interpolation. | ||
scipy.interpolate.CubicSpline : Cubic spline data interpolator. |
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.
As this method is referenced here, I expected it to be available just like any of the other ones, but I have combed the source code and I am unable to find the place where this method is used. Was it added because it was planned to add future support for CubicSpline (with a wrapper such as Akima's)?
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>
Errors in the validation script: