-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
DOC: Added examples to rolling.py #1574
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
Conversation
Added a comprehensive rolling example
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.
The right place to put this documentation is actually probably on the Dataset.rolling
/DataArray.rolling
method:
Line 452 in f9464fd
def rolling(self, min_periods=None, center=False, **windows): |
Users don't create Rolling
objects directly, so this docstring actually does not appear in the built docs.
xarray/core/rolling.py
Outdated
@@ -50,6 +50,80 @@ def __init__(self, obj, min_periods=None, center=False, **windows): | |||
Returns | |||
------- | |||
rolling : type of input argument | |||
|
|||
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.
This should have one more -
.
xarray/core/rolling.py
Outdated
>>> a = np.repeat(np.repeat(np.linspace(0,11,num=12)[:, np.newaxis], | ||
len(lat), axis=1)[:, :, np.newaxis], len(lon), axis=2) | ||
>>> da = xr.DataArray(a, coords=[time, lat, lon], dims=['time','lat','lon']) | ||
>>> da |
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 is a fine example, but it takes a lot of space to show it because of the unused lat/lon dimensions. Maybe it would be better to drop those dimensions and make the array only go along time?
xarray/core/rolling.py
Outdated
* time (time) datetime64[ns] 1999-12-15 2000-01-15 2000-02-15 ... | ||
* lat (lat) float64 -90.0 -89.0 -88.0 -87.0 -86.0 -85.0 -84.0 -83.0 ... | ||
* lon (lon) float64 0.0 1.0 2.0 3.0 4.0 5.0 6.0 7.0 8.0 9.0 10.0 11.0 ... | ||
>>> da_seasonal_avg = da.rolling(time=3).mean().dropna('time') |
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 examples in docstrings, we usually try to keep things as simple as possible. The dropna()
and reassignment below are good tips for restructuring the result of rolling, but we should show the intermediate output first, e.g., by directly printing da.rolling(time=3).mean()
.
Removed my example
Thanks for your help with this. Agree with all of your suggestions. |
@raybellwaves - can you remove all changes to |
Shed a few lines of my example
xarray/core/common.py
Outdated
Create rolling seasonal average of monthly data e.g. DJF, JFM, ..., SON: | ||
|
||
>>> da = xr.DataArray(np.linspace(0,11,num=12), | ||
coords=[pd.date_range('15/12/1999', |
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 ...
to continue a line (notice that this is what the python
interpreter does).
xarray/core/common.py
Outdated
array([ nan, nan, 1., 2., 3., 4., 5., 6., 7., 8., 9., 10.]) | ||
Coordinates: | ||
* time (time) datetime64[ns] 1999-12-15 2000-01-15 2000-02-15 ... | ||
>>> da_avg = da.rolling(time=3).mean().dropna('time') |
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.
Either add a little more exposition here to make it clear why you're showing this or drop 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.
Are you referring to the output of da.rolling(time=3).mean()
or the da.rolling(time=3).mean().dropna('time')
statement?
You originally said it may be worth showing the output before restructuring the result of rolling. I wasn't sure about adding a comment in the middle of an example so i've opted to expand the description of the example. I decided to cut out the da.rolling(time=3).mean()
line as a believe the output of da.rolling(time=3).mean().dropna('time')
is cleaner but I understand if you would prefer not to add another function in the example.
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 can definitely add comments in the middle of examples (e.g., see "Example" for np.matmul
). Just add line breaks before/after and don't preface those lines with >>>
, e.g.,
Add two numbers:
>>> 1 + 1
2
Subtract two numbers:
>>> 1 - 1
0
Even though the output is not as clean, my preference is for showing "minimal units" in docstrings so it's entirely clear to users how a function works. That way they'll know to expect NaNs, for example.
So you can also include the example using dropna()
(with a comment) but we should have the simpler version without it first.
cleaned up rolling example
Extended the description of the rolling.py example
Realized I didn't need to create a variable, as I can simply show the output
Cleaned up the example text to add a comment in the middle
Thanks @raybellwaves ! |
No problem. Feel free to add 'novice' labels to any issues that require grunt work (such as documentation) and i'll be happy to have a stab at them to get more familiar with xarray |
This idea for this PR was discussed here: https://stackoverflow.com/questions/45992983/python-xarray-rolling-mean-example
I wasn't able to drop the nans using min_periods so I used the dropna method.
If there is a more eloquent method to my example feel free to point me to it.
git diff upstream/master | flake8 --diff
whats-new.rst
for all changes andapi.rst
for new API