-
-
Notifications
You must be signed in to change notification settings - Fork 286
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
Added info for Group and Array #2400
Conversation
src/zarr/_info.py
Outdated
return template.format(**dataclasses.asdict(self)) | ||
|
||
|
||
def human_readable_size(size: int) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was taken from the 2.x implementation.
src/zarr/core/array.py
Outdated
def info(self) -> ArrayInfo: | ||
return self._info() | ||
|
||
async def info_complete(self) -> ArrayInfo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just don't have this in the API till we implement it (since it's new).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A few questions after looking at the initial implementation.
src/zarr/core/array.py
Outdated
) | ||
@property | ||
def info(self) -> ArrayInfo: | ||
return self._async_array.info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add docstrings here and in info_complete
src/zarr/core/_info.py
Outdated
class ArrayInfo: | ||
type: Literal["Array"] = "Array" | ||
zarr_format: Literal[2, 3] | ||
data_type: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, compressor
, filters
, codecs
, and maybe store_type
pick whether we want the string repr as an argument or the concrete value. I think initially I wanted to use concrete values for everything, which gives us a bit more flexiblity at formatting time (we can always call str
then if we just want the string repr).
I'll look into why I switched over to str
s for these.
Zarr's public API. | ||
""" | ||
|
||
_name: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made all these fields private.
IMO, we should encourage things like group.info.zarr_format
. The one place for that information should be group.metadata.zarr_format
.
This class's sole user-facing focus should be the repr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should encourage things like group.info.zarr_format.
Did you mean:
"... we should NOT encourage ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @TomAugspurger !
Introduced by zarr-developers#2400 / e5135f9.
Introduced by zarr-developers#2400 / e5135f9.
Quick pass at adding
.info
to Group and Array. Like 2.x,info
is a property, but it will only contain information that's known statically. I've added aGroup.info_complete
method that actually calls the store to get dynamic information (for group this is children).I've punted on a similar
Array.info_complete
. For that, we'll need to expand the Store ABC to give us annbytes
method to get the size of the array in bytes. Maybe similar for the actual number of chunks written. Doable, but separate from this PR I think .Closes #2135