OME-ZARR Reader - #53

@claude review

Claude finished @Artur-man's task in 5m 20s —— View job

Claude's Review

Todo list:

• Gather context (read diff, changed files, repo guidelines)
• Review R/omezarr.R (new OME-Zarr reader)
• Review R/AllClasses.R and R/ImageArray.R changes
• Review NAMESPACE changes
• Review tests/testthat/test-omezarr.R
• Post final review

Overall this is a clean, well-structured addition that mirrors the existing BFPath/BFArray pattern closely (constructor, resolution/path accessors, show method, createImageList dispatch). Nice test coverage for OZPath construction, resolution selection, and error paths.

Potential bug: axes may be mis-assigned for non-square OME-Zarr images

createListFromOZPath (R/ImageArray.R:249-268) determines axes via:

axes <- .check_axes(image_list[[1]], axes = axes)

.check_axes (R/ImageArray.R:921+) guesses 2D axes as c("x", "y") — i.e. it assumes dim(image)[1] is the x (width) axis and dim(image)[2] is y (height), which matches EBImage's native convention (used for PNG/TIFF/magick-loaded images).

However, OME-NGFF zarr arrays are stored per the NGFF spec's axis order, where the array's dims follow (..., c, z, y, x) — i.e. y (height) comes before x (width), the opposite of EBImage's convention. If ZarrArray exposes dim() in the same order as the underlying .zarray/zarr.json shape (as Rarr/ZarrArray do, with no NGFF-aware reordering), then for a non-square image this would swap width and height, e.g. in getImageInfo() (R/image.R:31-36), which reads dim_image["x"]/dim_image["y"] based on this guessed axis mapping.

The included test fixture is 512×512 (square), so this swap — if it occurs — would be invisible in expect_equal(getImageInfo(img), data.frame(width = 512, height = 512)) (tests/testthat/test-omezarr.R:75) since width == height either way.

Suggestions:

• Verify with a non-square OME-Zarr fixture (even a tiny synthetic one) that width/height come out correctly.
• More robust long-term fix: rather than guessing axes generically for zarr arrays, read the axis names/order from the store's NGFF multiscales metadata (.zattrs/zarr.json's "axes" field), which explicitly encodes this per the spec, instead of relying on the same heuristic used for plain PNG/TIFF images.

Fix this →

Minor: copy-pasted roxygen doc

R/ImageArray.R:237-248 — the @noRd doc block for createListFromOZPath still has the title createListFromBFPath (copy-pasted from the function above it) and documents a resolution parameter that doesn't exist in this function's signature (image, axes, scales, n.levels, max.pixel.threshold, verbose). Purely cosmetic since it's @noRd, but worth tidying.

Fix this →

Other observations (no action needed)

• Deleting inst/extdata/bird.png/sample.png is safe — a repo-wide search confirms every example/test that references sample.png uses EBImage's own copy (system.file("images", "sample.png", package = "EBImage")), not the package's extdata file.
• OZPath()'s handling of .zarray/zarr.json/.zgroup to distinguish arrays from groups (dropping the "labels" group, rejecting mixed-dimensionality pyramids) is well tested and looks correct for the given fixture.
• DESCRIPTION, resolution/path S4 generics, and ZarrArray import were already in place on devel, so no dependency wiring is missing.

Note: R is not available in this sandbox, so I could not actually run R CMD check/testthat to confirm the axes-order concern empirically — it's based on reading .check_axes, getImageInfo, and the OME-NGFF axis-order convention. Recommend the author double check with a rectangular test image.

@Bisaloo: I'm not quite sure what the best solution is but what I was thinking is that ImageArray would provide a virtual class that we re-use, e.g., in romeo. But also if there is any other pyramid formats that a third-party developer wants to support, they can just write their own class that inherit from ImageArray

Makes sense, perhaps this constructor has to be defined in romeo instead. I will leave it here.