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

rolling.construct: Add sliding_window_kwargs to pipe arguments down to sliding_window_view #9720

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

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Nov 5, 2024

Closes #9550
xref #4325

  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

cc @phofl

Blocked till the next dask release, so we can verify tests

@dcherian dcherian force-pushed the rolling-auto-rechunk branch 4 times, most recently from 223390c to ae9e8c0 Compare November 5, 2024 21:12
Copy link
Contributor

@phofl phofl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@max-sixty
Copy link
Collaborator

I don't have much context here, but should we always do this if possible, rather than add an option? Or we want to allow an escape hatch for instances where copies are made?

@dcherian
Copy link
Contributor Author

dcherian commented Nov 5, 2024

It is on by default in the public APIs (construct and reduce). Advanced users can opt-out. For internal uses, where copies are not made, I set it to False by default (in _reduce_method) and opt-in to rechunking when necessary for each individual method.

I think we should expose it in construct since that is a thin wrapper around sliding_window_view. Are you concerned about exposing it in reduce?

@max-sixty
Copy link
Collaborator

No! Generally keen on keeping the API small but totally reasonable here!

@dcherian
Copy link
Contributor Author

dcherian commented Nov 6, 2024

@max-sixty one option would be to add sliding_window_view_kwargs to forward arbitrary kwargs down to the implementing library. For example, numpy has writeable which someone might want to set (we do not allow this currently)

@dcherian dcherian changed the title sliding_window_view: add new automatic_rechunk kwarg rolling.construct: Add sliding_window_kwargs to pipe arguments down to sliding_window_view Nov 9, 2024
@dcherian dcherian changed the title rolling.construct: Add sliding_window_kwargs to pipe arguments down to sliding_window_view rolling.construct: Add sliding_window_kwargs to pipe arguments down to sliding_window_view Nov 9, 2024
* main:
  Compress PNG files (pydata#9747)
  Dispatch to Dask if nanquantile is available (pydata#9719)
@dcherian
Copy link
Contributor Author

dcherian commented Nov 9, 2024

I went with sliding_window_kwargs to save some typing...

@dcherian dcherian marked this pull request as ready for review November 9, 2024 19:21
@dcherian dcherian added needs review topic-rolling topic-chunked-arrays Managing different chunked backends, e.g. dask labels Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review topic-chunked-arrays Managing different chunked backends, e.g. dask topic-rolling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rolling(...).construct(...) blows up chunk size
3 participants