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

Strided rolling #3607

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Strided rolling #3607

wants to merge 14 commits into from

Conversation

niowniow
Copy link
Contributor

This PR adds a stride parameter to the rolling function of DataArray and Dataset . It basically extends the stride functionality being available for core.rolling.DataArrayRolling.construct and core.rolling.DatasetRolling.construct to the other methods of DataArrayRolling and DatasetRolling.

Note: it makes the arguments of DataArrayRolling and DatasetRolling inconsistent with the respective rolling arguments of pandas Series and DataFrame (They do not support stride).
Moreover, it does not solve the issue addressed in this pandas issue (Efficient stride computation).

  • Tests added
  • Passes black . && mypy . && flake8
  • Fully documented, including whats-new.rst for all changes and api.rst for new API

@fujiisoup
Copy link
Member

@niowniow
Thank you for your contribution!

I think stride option is a good idea.
One thing is how to implement this efficient nan-reduction method.

Currently, we use 'bottleneck' if it is installed for speeding up nan-ops, but bottleneck does not support stride option.
Another problem is inefficiency of nan-ops of numpy for strided arrays; he copies the strided array into full array and replace np.nan by zero before the reduction.

One way we could do is

  1. skip using 'bottleneck' if stride is other than 1
  2. implement our nan-ops for rolling.
    For example, for nansum, we can replace np.nan by 0 before creating the strided arrays and apply usual sum for the strided array.

In rolling.count, we did a similar thing.

@pep8speaks
Copy link

pep8speaks commented May 31, 2021

Hello @niowniow! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-31 12:39:49 UTC

@headtr1ck
Copy link
Collaborator

Is that still wanted?
It seems like a useful feature to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants