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

Allow rechunking to groups #144

Merged
merged 5 commits into from
Aug 12, 2023
Merged

Conversation

rabernat
Copy link
Member

This PR allows target_store and temp_store to be existing Zarr groups rather than store objects.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #144 (6538513) into master (ba7efc0) will increase coverage by 26.09%.
The diff coverage is 90.00%.

❗ Current head 6538513 differs from pull request most recent head 95fb3af. Consider uploading reports for the commit 95fb3af to get more accurate results

@@             Coverage Diff             @@
##           master     #144       +/-   ##
===========================================
+ Coverage   70.03%   96.12%   +26.09%     
===========================================
  Files          11       11               
  Lines         554      568       +14     
  Branches      106      113        +7     
===========================================
+ Hits          388      546      +158     
+ Misses        149       14      -135     
+ Partials       17        8        -9     
Files Changed Coverage Δ
rechunker/api.py 96.52% <90.00%> (+29.85%) ⬆️

... and 6 files with indirect coverage changes

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

It would be nice to add some tests against the new error states. Other than that, this looks nice and clean.

rechunker/api.py Outdated
@@ -287,6 +289,10 @@ def rechunk(
* python
* pywren

array_name: str, optional
Required when rechunking an array if the any of the targets is a group
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Required when rechunking an array if the any of the targets is a group
Required when rechunking an array if any of the targets are a group

rechunker/api.py Outdated
@@ -142,12 +142,13 @@ def _encode_zarr_attributes(attrs):

def _zarr_empty(shape, store_or_group, chunks, dtype, name=None, **kwargs):
# wrapper that maybe creates the array within a group
if name is not None:
assert isinstance(store_or_group, zarr.hierarchy.Group)
if isinstance(store_or_group, zarr.hierarchy.Group):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a difference between zarr.hierarchy.Group and zarr.Group? Later on you use the latter.

Copy link
Member Author

Choose a reason for hiding this comment

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

No difference.

@rabernat rabernat merged commit 0ac43e8 into pangeo-data:master Aug 12, 2023
2 checks passed
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.

None yet

2 participants