-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Updates for Zarr 3 Dtypes #10456
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
Updates for Zarr 3 Dtypes #10456
Changes from 17 commits
47359bd
218098b
841ed36
99cfd78
5da1124
cb18495
3e035a6
7a037f1
c597782
3877091
6d3e9cd
fde770d
5da6cca
f8a45ba
12c4943
f2e917a
b1c6809
8ee46b1
05c8aa6
3f7a557
6568023
8d61d17
d9e04eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -816,11 +816,29 @@ def open_store_variable(self, name): | |
if zarr_array.fill_value is not None: | ||
attributes["_FillValue"] = zarr_array.fill_value | ||
elif "_FillValue" in attributes: | ||
attributes["_FillValue"] = FillValueCoder.decode( | ||
attributes["_FillValue"], zarr_array.dtype | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this code is correct and should be restored |
||
# TODO update version check for the released version with dtypes | ||
# probably be 3.1 | ||
|
||
# if Version(zarr.__version__) > Version("3.0.10"): | ||
if hasattr(zarr_array.metadata.data_type, "to_native_dtype"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had to check by attrs here because of version confusion. Though maybe this is a better check here anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better check than version comparison* There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this check ever be used for arrays using Zarr 2.x? In 2.x didn't have the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good callout, I'm a little surprised the tests didn't pick that up. For merging we can do >=3.1.0 i think, but I ran into problems testing with version checking because the release of 3.0.10 is higher than 3.0.9.dev (and for some reason the instlalation from git was not turning it into 3.0.10.dev) |
||
native_dtype = zarr_array.metadata.data_type.to_native_dtype() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zarr_array.dtype is still a regular numpy dtype after my changes. so I don't think you need to go through the metadata |
||
attributes["_FillValue"] = ( | ||
# Use the new dtype infrastructure instead of doing xarray | ||
# specific fill value decoding | ||
FillValueCoder.decode( | ||
attributes["_FillValue"], | ||
native_dtype, | ||
) | ||
) | ||
else: | ||
dtype_value = zarr_array.metadata.data_type.value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here -- I don't think you need to go through the metadata to get the numpy dtype There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also I'm confused because I thought I fixed this already There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had this since my initial test, never thought to try to remove it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when was the last time you pulled in the latest changes from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok this PR started by overwriting my fix :) I would recommend reverting that change |
||
attributes["_FillValue"] = FillValueCoder.decode( | ||
attributes["_FillValue"], dtype_value | ||
) | ||
|
||
variable = Variable(dimensions, data, attributes, encoding) | ||
|
||
return Variable(dimensions, data, attributes, encoding) | ||
return variable | ||
|
||
def get_variables(self): | ||
return FrozenDict((k, self.open_store_variable(k)) for k in self.array_keys()) | ||
|
Uh oh!
There was an error while loading. Please reload this page.