implement .chunks on v3 arrays #1929
Conversation
| @@ -343,6 +343,16 @@ def ndim(self) -> int: | |||
| def shape(self) -> ChunkCoords: | |||
| return self.metadata.shape | |||
|
|
|||
| @property | |||
| def chunks(self) -> ChunkCoords: | |||
There was a problem hiding this comment.
Should we add a deprecation warning here? I think users should instead look at the chunk grid directly.
There was a problem hiding this comment.
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, ...], ...]
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
My opinion is that having a backwards compatible
.chunksattribute 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?
There was a problem hiding this comment.
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.
src/zarr/array.py
Outdated
| chunk_shape = getattr(self.metadata.chunk_grid, "chunk_shape") # noqa: B009 | ||
| return chunk_shape |
There was a problem hiding this comment.
| chunk_shape = getattr(self.metadata.chunk_grid, "chunk_shape") # noqa: B009 | |
| return chunk_shape | |
| return self.metadata.chunk_grid.chunk_shape |
B009 is actually useful here.
B009: Do not call getattr(x, 'attr'), instead use normal property access: x.attr. Missing a default to getattr will cause an AttributeError to be raised for non-existent properties. There is no additional safety in using getattr if you know the attribute name ahead of time.
|
This is my first PR to the V3 branch. How are we handling docs? Is there anything to update? |
Docs are a (big) tbd, for the moment code changes alone are fine. |
src/zarr/array.py
Outdated
| @property | ||
| def chunks(self) -> ChunkCoords: | ||
| try: | ||
| return self.metadata.chunk_grid.chunk_shape # type: ignore[attr-defined] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I took a swing at fixing this @rabernat. Let me know what you think.
There was a problem hiding this comment.
Thanks Joe! I like it!
…nto v3-implement-chunks-attr
* implement .chunks on v3 arrays * remove noqa: B009 * make mypy happy * only return chunks for regular chunk grids --------- Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com> Co-authored-by: Joseph Hamman <joe@earthmover.io>
Closes #1925
TODO:
Add docstrings and API docs for any new/modified user-facing classes and functionsNew/modified features documented in docs/tutorial.rstChanges documented in docs/release.rst