-
Notifications
You must be signed in to change notification settings - Fork 24
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
Dont write _ARRAY_DIMENSIONS to icechunk #286
Conversation
for more information, see https://pre-commit.ci
@@ -144,8 +144,7 @@ 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["_ARRAY_DIMENSIONS"] = encode_zarr_attr_value(var.dims) |
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.
It's not necessary to write these because we already have dimension_names
specified above in
arr = group.require_array(
name=name,
shape=zarray.shape,
chunk_shape=zarray.chunks,
dtype=encode_dtype(zarray.dtype),
codecs=zarray._v3_codec_pipeline(),
dimension_names=var.dims,
fill_value=zarray.fill_value,
# TODO fill_value?
)
…olas/VirtualiZarr into icechunk_dimension_names
Getting the tests to pass required using the most recent version of xarray (for zarr v3 support). Not entirely sure why because the upstream CI was already using a zarrv3 compatibility branch of xarray. I assuming that branch did not have pydata/xarray#9669 in it. Anyway now that xarray 2024.10.0 is released we should just use that, so this PR should be merged immediately after #284. @mpiannucci any issues with this PR from your perspective? |
Looks good to me as long as xarray supports it now! |
Exactly right. I've removed the compat branch so we don't hit this again. |
docs/releases.rst