-
-
Notifications
You must be signed in to change notification settings - Fork 304
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
Adds filters, compressors and serializer props to Array #2652
Conversation
src/zarr/core/_info.py
Outdated
|
||
if self._filters is not None: | ||
if len(self._filters) > 0: |
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.
Will this prevent arrays with no filters at all from displaying the fact that they have no filters via the .info
method?
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.
likely out of scope, since my question is really about the pre-existing behavior of this function
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.
The "Filters : ... " line wasn't shown if filters
was None
, but it was shown if filters
was ()
. Now, it is not shown if None
or ()
.
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.
Don't we want to explicitly show the value of filters
, even if it's ()
?
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.
We could. I don't have a strong preference here.
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 it's better to show a static set of properties with this function, but if that's annoying to implement here we can defer for a separate PR
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.
What about "Serializer", which doesn't exist for v2 arrays?
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 None
there? in principle there's nothing stopping someone from implementing a v2-compatible BytesCodec
in numcodecs, in which case we might end up with v2 arrays with a serializer
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.
Well, there is no place in the v2 metadata for a serializer.
I opted for not showing the serializer field in the 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.
happy to see some simplified tests via these changes!
Adds
filters
,compressors
,serializer
properties to theArray
andAsyncArray
classes. These properties mirror the kwargs that users provide increate_array
, i.e. inner codecs in sharding are surfaced.Array.info
is adapted accordingly. Fixes #2633 #2649.I also fixed a bug where strings/bytes dtypes couldn't be used with sharding due to validation errors.
TODO: