-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
virtualizarr/zarr.py
Outdated
@@ -4,6 +4,7 @@ | |||
import numcodecs | |||
import numpy as np | |||
import ujson # type: ignore | |||
from zarr.core.metadata.v3 import parse_codecs |
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 break compatibility with non-v3 zarr-python? In fact what's the best way to approach that in general? I don't mind updating virtualizarr to require bleeding edge zarr-python.
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 can try to import it and if not the pipeline function can just error?
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.
Sure - so let's move the import inside the pipeline function.
I have the codecs resolving correctly, but currently data isn’t loading right so going to take a beat to look into why |
@@ -213,4 +217,5 @@ def _num_codec_config_to_configurable(num_codec: dict) -> dict: | |||
Convert a numcodecs codec into a zarr v3 configurable. | |||
""" | |||
num_codec_copy = num_codec.copy() | |||
return {"name": num_codec_copy.pop("id"), "configuration": num_codec_copy} | |||
name = "numcodecs." + num_codec_copy.pop("id") |
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 is subject to zarr-developers/numcodecs#597 being installed.. but it works
Im going to merge this in, because without it the icechunk branch isnt super useful |
docs/releases.rst
api.rst