Skip to content

Conversation

aurghs
Copy link
Collaborator

@aurghs aurghs commented Oct 7, 2020

I propose this small change with a view to unifying in open_dataset the logic of zarr chunking with that of the other backends.
Currently, the function maybe_chunk is duplicated: it is defined inside the function dataset.chunks and as method of zarr.ZarrStore. This last function has been added with the recent merge of #4187 .
I merged the two functions in a private function _maybe_chunk inside the module dataset.

@@ -359,6 +359,39 @@ def _assert_empty(args: tuple, msg: str = "%s") -> None:
raise ValueError(msg % args)


def _selkeys(dict_, keys):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that you only moved this helper function out, but I would take the occasion to move the logic back into _maybe_chunk.

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

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

This is a nice cleanup, thank you!

@max-sixty
Copy link
Collaborator

@aurghs if you'd like to add an entry to whatsnew, please feel free. We'll plan to merge tomorrow. Thanks!

@shoyer shoyer merged commit 49e3032 into pydata:master Oct 8, 2020
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