Skip to content

implement .chunks on v3 arrays #1929

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 3, 2024
Merged

implement .chunks on v3 arrays #1929

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9d3bdc5
implement .chunks on v3 arrays
rabernat May 29, 2024
355d24f
remove noqa: B009
rabernat Jun 2, 2024
cfee2e8
Merge branch 'v3' into v3-implement-chunks-attr
d-v-b Jun 2, 2024
ab79442
make mypy happy
rabernat Jun 2, 2024
4faba8f
Merge remote-tracking branch 'origin/v3-implement-chunks-attr' into v…
rabernat Jun 2, 2024
28363fa
Merge branch 'v3' of https://github.com/zarr-developers/zarr-python i…
jhamman Jun 3, 2024
9d946ac
only return chunks for regular chunk grids
jhamman Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/zarr/array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,15 @@ def ndim(self) -> int:
def shape(self) -> ChunkCoords:
return self.metadata.shape

@property
def chunks(self) -> ChunkCoords:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a deprecation warning here? I think users should instead look at the chunk grid directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the chunk grid object is guaranteed to have a consistent structure, beyond {name: str, configuration: dict[str, JSON]}? We might instead want users to look at a normalized (explicit) representation of the chunks, i.e. tuple[tuple[int, ...], ...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My opinion is that having a backwards compatible .chunks attribute is very important. Deprecating this or changing its behavior will have far-reaching consequences for libraries that depend on Zarr. Given that we only support regular chunk grids so far, this solution feels very reasonable to me. Once other chunk grids are developed this will raise an attribute error, and the user code can handle it some other way (i.e. looking at the chunk grid directly).

Copy link
Contributor

Choose a reason for hiding this comment

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

@rabernat I think your reasoning makes sense, given that irregular chunks are prospective at this point.

Thinking about future changes to support irregular chunks, do you think it would make sense to transform the type of chunks to tuple[tuple[int, ...], ...] (matching the chunks attribute of a dask array), assuming we give a long deprecation warning?

Copy link
Member

Choose a reason for hiding this comment

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

My opinion is that having a backwards compatible .chunks attribute is very important. Deprecating this or changing its behavior will have far-reaching consequences for libraries that depend on Zarr. Given that we only support regular chunk grids so far, this solution feels very reasonable to me. Once other chunk grids are developed this will raise an attribute error, and the user code can handle it some other way (i.e. looking at the chunk grid directly).

Fair enough, we can revisit this once we have other chunk grids.

tuple[tuple[int, ...], ...]

How would that scale if you have billions of chunks?

Copy link
Contributor

Choose a reason for hiding this comment

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

How would that scale if you have billions of chunks?

In that case, we would probably need to switch to a generator of tuple of ints. But I think someone with billions of chunks will have lots of other problems they run into first.

try:
return self.metadata.chunk_grid.chunk_shape # type: ignore[attr-defined]
Copy link
Contributor Author

@rabernat rabernat Jun 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am fighting with mypy here. My local pre-commit env does not like

# type: ignore[attr-defined, no-any-return]

(says src/zarr/array.py:349: error: Unused "type: ignore[no-any-return]" comment [unused-ignore])

but that seems to be what the CI-based pre-commit linter wants

Copy link
Member

Choose a reason for hiding this comment

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

I took a swing at fixing this @rabernat. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Joe! I like it!

except AttributeError as err: # pragma: no cover
raise AttributeError(
f"Chunk grid {self.metadata.chunk_grid} array does not have a chunk shape."
) from err

@property
def size(self) -> int:
return np.prod(self.metadata.shape).item()
Expand Down Expand Up @@ -558,6 +567,10 @@ def ndim(self) -> int:
def shape(self) -> ChunkCoords:
return self._async_array.shape

@property
def chunks(self) -> ChunkCoords:
return self._async_array.chunks

@property
def size(self) -> int:
return self._async_array.size
Expand Down
2 changes: 1 addition & 1 deletion tests/v3/test_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def test_group(store: MemoryStore | LocalStore) -> None:
assert arr.dtype == data.dtype

# TODO: update this once the array api settles down
# assert arr.chunk_shape == (2, 2)
assert arr.chunks == (2, 2)

bar2 = foo["bar"]
assert dict(bar2.attrs) == {"baz": "qux"}
Expand Down