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

Add examples for DataArrayRolling.reduce() #2968

Merged

Conversation

kmsquire
Copy link
Contributor

@kmsquire kmsquire commented May 16, 2019

  • When reducing over a rolling window, DataArrayRolling.reduce()
    only returns values where the complete rolling window is fully
    conatined within the base array; the remaining locations where
    the window overlaps the array border are filled with NaN
  • However, in some situations, it is desirable to obtain values from the
    reducer function for all locations. This commit adds a keyword
    argument, include_border, to DataArrayRolling.reduce() and
    DataSetRolling.reduce() to allow this behavior.

If this seems okay, I have some questions:

1. Should this behavior be added to specific reducing functions (sum, mean, etc.)? As an alternative, all of the specialized functions for DataArrayRolling have corresponding numpy nan* functions, so support could be added for those. (The current PR is still useful for other reducing functions, which is my use case.)
2. I don't use dask, so I don't know if this is needed/desired for the dask versions of these functions.

As @mathause pointed out, the functionality desired already exists by passing min_periods=1 to DataArray.rolling(). So this MR has been updated to simply include some examples for DataArrayRolling.reduce()

@mathause
Copy link
Collaborator

I think you can keep the function signature of reduce but rather allow to set min_periods=0 in __init__ (This is currently disallowed on Line 77).

Then you change your code in reduce to

if self.min_periods == 0:
    return result

btw: If I understand correctly, this does not need to be added for the other functions ds.rolling(...).mean() calls reduce on Line 238.

@kmsquire
Copy link
Contributor Author

@mathause, thank you, I didn't realize that. I'll make those changes.

@kmsquire
Copy link
Contributor Author

kmsquire commented May 17, 2019

Actually, every window has at least 1 valid value, so this change isn't really needed at all.

I'll remove the code changes, and change the added example to show a working example of what I wanted.

I will say that min_periods isn't the most obvious name for this parameter--I'm struggling to figure out how the number of valid observations in a window is related to periods or periodicity.

Would it be reasonable to deprecate min_periods and change it to something like min_observations?

* Adds two examples to DataArrayRolling.reduce(); the second example
  shows the interaction of reduce() and min_periods
@kmsquire kmsquire force-pushed the feature/rolling-reduce-include-border branch from 4ec82e1 to a59d6f6 Compare May 17, 2019 15:30
@kmsquire kmsquire changed the title WIP: allow border results in DataArrayRolling.reduce() Add examples for DataArrayRolling.reduce() May 17, 2019
@kmsquire
Copy link
Contributor Author

Changed to simply add examples to DataArrayRolling.reduce(). I changed the original description accordingly.

@dcherian
Copy link
Contributor

@kmsquire thanks. Can you also add some text to https://xarray.pydata.org/en/stable/computation.html#rolling-window-operations (i.e. computation.rst) describing what min_periods does.

@kmsquire
Copy link
Contributor Author

@dcherian added. Let me know if you want any changes.

doc/computation.rst Outdated Show resolved Hide resolved
@dcherian dcherian mentioned this pull request May 21, 2019
15 tasks
* Add a longer description and examples for the `center` and
  `min_period` parameters to `DataArray.rolling()`
@kmsquire kmsquire force-pushed the feature/rolling-reduce-include-border branch from e1199a7 to 0389060 Compare June 3, 2019 16:15
@dcherian
Copy link
Contributor

dcherian commented Jun 3, 2019

Thanks @kmsquire . The tests failed but they seem unrelated (the docs build passed).

@dcherian dcherian merged commit f84c514 into pydata:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants