Repository navigation
fix: to_datatree reads exact level pixels and survives pickling - #87
Merged
Merged
Conversation
Build each pyramid level with dask map_blocks over reader.get_region instead of a hand-rolled zarr store that zarr never opened. Block origins use ceil(x * ds): readers map level-0 offsets to the level with int(x / ds), so the old rounded downsample read a row or column early at non-integer downsamples. The graph no longer embeds a local lambda, so the image pickles, and it has a fifth of the tasks. The output layout is unchanged; sopa.io.wsi depends on it.
…i-zarr-research-27c871 # Conflicts: # wsidata/reader/__init__.py
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The universal ceiling rule misaligns OpenSlide blocks at non-integer downsample factors.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Replaces the unused virtual Zarr implementation with a smaller, pickle-safe Dask-backed multiscale image reader.
Changes:
- Reads slide levels lazily through
map_blocks. - Adds pixel-equivalence, serialization, layout, and persistence tests.
- Updates the public reader export.
| File | Description |
|---|---|
wsidata/reader/_reader_datatree_zarr_v3.py |
Removes the virtual Zarr implementation. |
wsidata/reader/_datatree.py |
Adds Dask-backed DataTree construction. |
wsidata/reader/__init__.py |
Exports the new implementation. |
tests/test_readers.py |
Adds regression and integration coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
open_wsi(attach_images=True)builds the slide as a lazy multiscale SpatialData image throughwsidata.reader.to_datatree. That path used a hand-rolledSlideZarrStorethat zarr never actually opened: every chunk ranasyncio.runinside adask.delayedlambda, and block origins used a rounded downsample. This replaces it withdask.array.map_blocksoverreader.get_region.int(x / ds)(tiffslide, fastslide, bioformats, isyntax), so block origins now useceil(x * ds). OnGTEX-1117F-0526.svs(level 1 at downsample 4.0005), every TiffSlide level now equals one native read; before, 5.7M of the 24.8M level-1 pixels differed. OpenSlide paints at fractional offsets, so its blocks stay within 1/ds px either way.scale{i}/image), dimsc, y, x,c = ["r", "g", "b"], y/x coords, chunks, per-level transforms andattrs = asdict(reader.properties)are unchanged.sopa.io.wsinow reads slides throughopen_wsi(attach_images=True).to_spatialdata(), so this layout is a downstream contract.Why not a zarr store
spatialdata has no WSI reader or reader plugin system, and
read_zarronly accepts local stores (main raisesTypeErrorfor anything else), so a virtual zarr store over a slide cannot be read by spatialdata anyway. A dask-backedDataTreealready is a native image element. Keeping the reader in themap_blockstask arguments also matches the dask-task soft-link design discussed in scverse/2026_04_hackathon_padua#12, should upstream adopt it. Background: scverse/spatialdata#856, scverse/spatialdata#1161.Tests
New in
tests/test_readers.py:test_to_datatree_levels_match_one_read: every level equals a native whole-level read (fails on the old store)test_to_datatree_pickles(fails on the old store)test_attached_image_layout: the layout sopa relies ontest_attached_image_is_not_written:write()never copies the attached image into the storeuv run pytest: 131 passed, 14 skipped.ruff checkandruff format --checkare clean.