Switch to _ARRAY_DIMENSIONS for xarray #3
Conversation
|
This is the latest... more to follow when xarray changes are in main hopefully tomorrow |
| @@ -127,7 +132,7 @@ async def write_virtual_variable_to_icechunk( | |||
| # TODO it would be nice if we could assign directly to the .attrs property | |||
| for k, v in var.attrs.items(): | |||
| arr.attrs[k] = encode_zarr_attr_value(v) | |||
| arr.attrs["DIMENSION_NAMES"] = encode_zarr_attr_value(var.dims) | |||
| arr.attrs["_ARRAY_DIMENSIONS"] = encode_zarr_attr_value(var.dims) | |||
There was a problem hiding this comment.
Did I just make DIMENSION_NAMES up?
There was a problem hiding this comment.
I'm not sure haha but this is what xarray needs
There was a problem hiding this comment.
Zarr V3 actual has native support for dimension names, but it's not supported in Xarray yet. So put a flag that we should change this eventually.
There was a problem hiding this comment.
we are just going to have both for now with this pr.
| arr = group.require_array( | ||
| name=name, | ||
| shape=zarray.shape, | ||
| chunk_shape=zarray.chunks, | ||
| dtype=encode_dtype(zarray.dtype), | ||
| codecs=zarray._v3_codec_pipeline(), | ||
| #codecs=codecs, |
There was a problem hiding this comment.
Does the codec pipeline stuff not work at all?
There was a problem hiding this comment.
Currently crashing when zlib is included. I'm gunna wait till the next cut of zarr v3 to try again there
| @@ -25,7 +25,7 @@ | |||
| } | |||
|
|
|||
|
|
|||
| def dataset_to_icechunk(ds: Dataset, store: "IcechunkStore") -> None: | |||
| async def dataset_to_icechunk_async(ds: Dataset, store: "IcechunkStore") -> None: | |||
There was a problem hiding this comment.
Wouldn't changing this without changing the test break the test?
There was a problem hiding this comment.
Didn't change this, extracted it and made both async and sync versions
There was a problem hiding this comment.
Oh yes - sorry I couldn't see that easily when I looked from my phone haha
docs/releases.rstapi.rst