Skip to content

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Jul 25, 2025

Description:

Adds options to configure the Zarr3 arrays that are created by wklibs. This is required for generating volume annotations, see also #1340

  • Added zarr3_codecs and zarr3_chunk_key_encoding parameters to MagView.add_mag to allow for configuring Zarr3 datasets.
  • The sharding_indexed codec is no longer used if chunk_shape == shard_shape.

Todos:

Make sure to delete unnecessary points or to check all before merging:

  • Updated Changelog
  • Added / Updated Tests

Copy link

github-actions bot commented Jul 28, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
9201 7805 85% 80% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
webknossos/webknossos/dataset/_array.py 88% 🟢
webknossos/webknossos/dataset/layer.py 88% 🟢
webknossos/webknossos/dataset/mag_view.py 91% 🟢
TOTAL 89% 🟢

updated for commit: f23f13e by action🐍

@normanrz normanrz changed the base branch from master to rechunk July 28, 2025 13:45
@normanrz normanrz self-assigned this Jul 28, 2025
@normanrz normanrz requested a review from Tobias314 July 28, 2025 13:45
@normanrz normanrz marked this pull request as ready for review July 28, 2025 13:45
@normanrz
Copy link
Member Author

Adding zarr3_codecs and zarr3_chunk_key_encoding to all these functions is quite verbose. I wonder if we should overload the compress arg to allow for customization instead?

class Zarr3Config:
    codecs: Sequence[Zarr3Codec] | None = None
    chunk_key_encoding: Zarr3ChunkKeyEncoding | None = None

def add_mag(...,
    compress: bool | Zarr3Config = True,
    ...
) -> MagView:
    ...

@normanrz normanrz changed the title Configurable codecs Configurable Zarr3 codecs Jul 28, 2025
@normanrz
Copy link
Member Author

Adding zarr3_codecs and zarr3_chunk_key_encoding to all these functions is quite verbose. I wonder if we should overload the compress arg to allow for customization instead?

class Zarr3Config:
    codecs: Sequence[Zarr3Codec] | None = None
    chunk_key_encoding: Zarr3ChunkKeyEncoding | None = None

def add_mag(...,
    compress: bool | Zarr3Config = True,
    ...
) -> MagView:
    ...

Changed that in e5fb658

Base automatically changed from rechunk to master July 29, 2025 07:12
Copy link
Contributor

@Tobias314 Tobias314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I am not an expert when it comes to the exact details of Zarr3 specification and configuration in tensorstore (dimension/byte order etc.). But as long as reading/writing works which should be covered in the tests everything is correct, I guess. :)

See my small comments.

Comment on lines +765 to +779
if isinstance(compress, Zarr3Config):
if not self.data_format != DataFormat.Zarr3:
raise ValueError(
"Cannot get_or_add_mag: A Zarr3 config can only be supplied for Zarr3 layers."
)
assert isinstance(mag_view.info, Zarr3ArrayInfo)
if mag_view.info.codecs != compress.codecs:
raise ValueError(
f"Cannot get_or_add_mag: The mag {mag} already exists, but the codecs do not match. Expected {mag_view.info.codecs}, got {compress.codecs}."
)
if mag_view.info.chunk_key_encoding != compress.chunk_key_encoding:
raise ValueError(
f"Cannot get_or_add_mag: The mag {mag} already exists, but the chunk key encoding does not match. Expected {mag_view.info.chunk_key_encoding}, got {compress.chunk_key_encoding}."
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add tests for those assertions. Not super important I would say, though.

@normanrz normanrz enabled auto-merge (squash) July 29, 2025 19:22
@normanrz normanrz merged commit 6970039 into master Jul 29, 2025
55 of 57 checks passed
@normanrz normanrz deleted the configurable-codecs branch July 29, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants