build: require itkwasm-downsample 2.0.0 and repair the baseline comparisons - #628
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR updates ChangesDownsample and Zarr alignment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GaussianMethod
participant castImageToFloat32
participant GaussianFilter
participant castImageToIntegerType
GaussianMethod->>castImageToFloat32: convert integer image
castImageToFloat32->>GaussianFilter: provide float32 image
GaussianFilter->>castImageToIntegerType: return filtered float image
castImageToIntegerType->>GaussianMethod: restore integer component type
sequenceDiagram
participant to_ome_zarr
participant _to_ngff_zarr_impl
participant TensorStore
participant ZarrStore
to_ome_zarr->>_to_ngff_zarr_impl: pass exact scale strategy
_to_ngff_zarr_impl->>TensorStore: pass copied multiscale state and compressors
TensorStore->>ZarrStore: write array data and codec metadata
ZarrStore->>_to_ngff_zarr_impl: complete Zarr array write
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
vboussot
marked this pull request as ready for review
August 5, 2026 11:31
vboussot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py/ngff_zarr/to_multiscales.py`:
- Around line 503-506: Update the NgffImage handling in to_multiscales so the
shallow-copied image receives its own computed_callbacks list before caching can
append cleanup callbacks. Preserve the existing conversion path for
non-NgffImage inputs and avoid mutating the caller’s callback list across
repeated calls.
In `@py/ngff_zarr/to_ngff_zarr.py`:
- Around line 308-336: Update create_compression_codec to detect and serialize
native Zarr v3 codecs through their to_dict() method before attempting the
existing codec_id-based handling. Ensure native codecs such as
zarr.codecs.BloscCodec are returned as the configured compression codec, so the
codec selection in the surrounding writer does not fall back to
default_compression.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b47e5315-3825-41a1-8052-4bc3a5cbbc97
📒 Files selected for processing (5)
py/ngff_zarr/to_multiscales.pypy/ngff_zarr/to_ngff_zarr.pypy/test/_data.pypy/test/test_to_ngff_zarr_sharding.pypy/test/test_to_ngff_zarr_v3_compression.py
vboussot
force-pushed
the
feat/itkwasm-downsample-2
branch
from
August 5, 2026 14:32
2b7bf54 to
c49d6dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
py/ngff_zarr/to_ngff_zarr.py (2)
337-340: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the full
compressorscontract.The TensorStore path reduces a Zarr v3 codec sequence to one codec and treats
compressors=Noneas default Zstd. It also replaces unsupported codecs with default Zstd. Preserve the complete sequence and explicit no-compression sentinel, or reject unsupported sequences instead of changing the stored encoding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py/ngff_zarr/to_ngff_zarr.py` around lines 337 - 340, Update the codec construction around create_compression_codec and inner_codecs to preserve the full compressors sequence and its ordering. Treat compressors=None as the explicit no-compression sentinel, and do not substitute default_compression or silently replace unsupported codecs; reject unsupported codec sequences instead while keeping supported codecs unchanged.
343-351: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd required
index_codecsto sharding_indexed codec configuration.Zarr v3 specification requires
index_codecsas a mandatory configuration member within thesharding_indexedcodec. Current code at lines 343–351 omits this field. Addindex_codecswith a bytes codec (little-endian) followed by a CRC32C checksum codec, matching the specification recommendation:Suggested change
if internal_chunk_shape: codecs.append( { "name": "sharding_indexed", "configuration": { "chunk_shape": internal_chunk_shape, "codecs": inner_codecs, "index_codecs": [ {"name": "bytes", "configuration": {"endian": "little"}}, {"name": "crc32c"}, ], }, } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py/ngff_zarr/to_ngff_zarr.py` around lines 343 - 351, Update the sharding_indexed configuration in the internal_chunk_shape branch to include the required index_codecs list, using a little-endian bytes codec followed by a crc32c codec; preserve the existing chunk_shape and inner_codecs entries.
🧹 Nitpick comments (1)
py/test/test_baseline_guard.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an absolute import for
_data.Replace
from ._data import ...with the package-qualified absolute import used by this repository. The current import depends on pytest collectingpy/testas a package.As per coding guidelines, Python files under
py/must use absolute imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py/test/test_baseline_guard.py` at line 14, Update the imports in test_baseline_guard.py to replace the relative ._data import with the repository’s package-qualified absolute import, preserving the existing store_equals and verify_against_baseline symbols.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/typescript.yml:
- Around line 30-34: Pin the release workflow’s deno-version setting to "2.8.3"
instead of the floating v2.x version, keeping Deno formatting and build behavior
consistent with the test and lint workflows.
In `@py/ngff_zarr/to_ngff_zarr.py`:
- Around line 592-599: Update _is_bytes_codec to handle string codec entries
before object and dictionary introspection, returning true when the string is
"bytes"; alternatively, reject string codec entries consistently at validation.
Preserve existing detection for codec objects and dictionaries so the
TensorStore codec chain creates only one bytes codec.
- Around line 272-291: The Blosc codec evolution in the codec serialization
branch updates typesize without synchronizing shuffle. Update this logic to use
compressor.evolve_from_array_spec() before to_dict(), or manually set both
fields so multi-byte dtypes use the evolved typesize and "shuffle" while
preserving explicit caller settings; add coverage for default and explicitly
provided typesize/shuffle configurations.
---
Outside diff comments:
In `@py/ngff_zarr/to_ngff_zarr.py`:
- Around line 337-340: Update the codec construction around
create_compression_codec and inner_codecs to preserve the full compressors
sequence and its ordering. Treat compressors=None as the explicit no-compression
sentinel, and do not substitute default_compression or silently replace
unsupported codecs; reject unsupported codec sequences instead while keeping
supported codecs unchanged.
- Around line 343-351: Update the sharding_indexed configuration in the
internal_chunk_shape branch to include the required index_codecs list, using a
little-endian bytes codec followed by a crc32c codec; preserve the existing
chunk_shape and inner_codecs entries.
---
Nitpick comments:
In `@py/test/test_baseline_guard.py`:
- Line 14: Update the imports in test_baseline_guard.py to replace the relative
._data import with the repository’s package-qualified absolute import,
preserving the existing store_equals and verify_against_baseline symbols.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d3b2680-9ecc-4f97-87c6-17f297c1a94e
📒 Files selected for processing (5)
.github/workflows/typescript.ymlpy/ngff_zarr/to_multiscales.pypy/ngff_zarr/to_ngff_zarr.pypy/test/_data.pypy/test/test_baseline_guard.py
🚧 Files skipped from review as they are similar to previous changes (2)
- py/ngff_zarr/to_multiscales.py
- py/test/_data.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
vboussot
marked this pull request as draft
August 5, 2026 14:39
vboussot
force-pushed
the
feat/itkwasm-downsample-2
branch
from
August 5, 2026 14:41
c49d6dd to
57c2bfb
Compare
Closes fideus-labs#625. Brings in the information-only output handling (InsightSoftwareConsortium/ITK-Wasm#1545), the origin fix (InsightSoftwareConsortium/ITK-Wasm#1556), and the resample-bounding-box pipeline fideus-labs#626 needs. #1556 shifts the downsample and downsample-label-image grid by half a coarse pixel to match downsample-bin-shrink, which was already correct. ngff-zarr already wrote that shift into its per-scale translation, so pixels and metadata now agree. Every baseline predates it and had to be regenerated.
verify_against_baseline read from baseline/v{version}/ while the writer uses
baseline/zarr{N}/v{version}/. The missing segment opened an empty store, and
store_equals walks the baseline's keys, so an empty baseline compared equal to
anything. Every comparison had been passing vacuously since 8bb29ba.
Turning them back on showed the comparison itself was too strict: chunks were
compared as stored, so compressed. zarr picks the codec and changed its
default between releases, zstd until 3.2 and blosc/lz4 from 3.3, which gives
different bytes for identical data. Compare decompressed arrays instead, parse
the metadata rather than diffing its bytes, and ignore which codec zarr chose.
That also widens coverage: nested .zattrs and .zgroup previously matched
neither branch and were not compared at all, which is where the axes and the
RFC-4 orientation live.
A missing baseline now skips explicitly rather than passing, with tests
pinning that down.
Regenerated every baseline, since #1556 shifted the downsampling grid by half a coarse pixel and the stored ones all predate it. The old archive also carried stores holding both Zarr v2 and v3 metadata side by side, 28 files where a clean write produces 18, and a baseline/zarr2/ layout nothing reads. Dropping those takes it from 329 MB to 273 MB, and the baselines from 274 MB to 56 MB. The flat layout the TypeScript suite reads is preserved. Six tests across three files also verified against cthead1/2_4/RFC3_GAUSSIAN.zarr while writing two different stores: the rfc2 tests use the default chunks, the sharding and compression tests pass chunks=(64,64). Whichever regenerated it last won, so no archive could satisfy all of them. The chunks=(64,64) group now has its own name.
The TensorStore spec only set "codecs" when a compressor or sharding was
requested. With neither, the key was omitted and TensorStore applied its own
defaults, which shard the array and drop compression, so the same call gave a
different store depending on the backend:
zarr-python : chunk_grid [130,128,128], codecs [bytes, zstd]
tensorstore : chunk_grid [130,128,128], codecs [sharding_indexed],
inner chunk [65,64,64], no compression
Reading such a store back reports the inner chunk, so a round trip halved the
chunk size on every axis. A supplied codec was dropped too: the helper keyed
off codec_id, which native Zarr v3 codecs lack, the writer read only
"compressor" and never the "compressors" the API takes, and blosc's typesize
came from to_dict() rather than the dtype.
to_multiscales also replaced ngff_image.data on the object it was handed, so a
caller's image came back rechunked and results depended on what ran before.
Shallow-copy it, with its own computed_callbacks so the cache cleanup does not
pile up once per call.
The baseline comparison works on decompressed values and so covers none of
this, hence the dedicated tests. All four fail without these changes.
vboussot
force-pushed
the
feat/itkwasm-downsample-2
branch
from
August 5, 2026 15:58
57c2bfb to
6121aad
Compare
itkwasm-downsample's Gaussian filter loses precision in integer arithmetic (a float 128.0 comes back 127), a systematic ~-1.2 bias on uint8/int16. The Python port has cast to float32 and rounded back since c93fe01; mirror it in both the Node and browser paths, rounding half to even to match np.rint. Package outputs are otherwise bit-identical between ports.
@itk-wasm/compare-images throws on any 3D image with a non-identity direction, even compared to itself; compare geometry and pixels directly instead, mirroring the Python port's store_equals. The rebuilt archive carries RFC-4 orientation, so stop stripping it on the test side.
…ller to_ngff_zarr's task-graph optimization re-downsamples each scale from the one it just wrote. With scale_strategy="pad", the default, targets that no integer factor reaches are silently missed: for MR-head with factors [2, 3, 4], to_multiscales computes (43, 85, 85) at scale 2 but the store receives (32, 64, 64) under datasets metadata that still declares the factor-3 scale, so the store misdescribes its own geometry by a third. Default to "exact", which falls back to the precomputed image whenever the incremental path cannot hit the target; pad stays available. The write loop also swapped the caller's image data for on-disk views and replaced entries in the caller's multiscales. Work on a shallow copy with per-image computed_callbacks, like to_multiscales since 6121aad.
cthead1 2_3, 2th_cthead1 2_3 and MR-head 2_3_4 were archived through the pad-mode write: 64 pixels wide at the factor-3 level where the metadata says 85. With the exact default both ports produce the same store bit-for-bit; regenerated in the zarr3 layout and the flat layout the TypeScript suite reads. The testing-data-v0.21.0 release asset is replaced in place (20 files changed, none added or removed) since only this branch ever referenced it; CI runs on commits before this one will no longer reproduce.
The ITK and dask-image Gaussian baselines encode x86 floating point; now that the comparison compares, Apple Silicon diverges on both. Skip those two comparisons off x86 rather than loosening them to a tolerance.
vboussot
marked this pull request as ready for review
August 5, 2026 22:53
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
py/test/test_to_ngff_zarr_dask_image.py (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
_on_x86architecture predicate in two test modules. Both files define the same expressionplatform.machine().lower() in ("x86_64", "amd64"). The shared root cause is a missing helper. Put the predicate inpy/test/_data.pynext to the other shared test infrastructure, and import it. A future change to the accepted architecture list then applies to both tests.
py/test/test_to_ngff_zarr_dask_image.py#L11-L11: remove the local_on_x86definition and theplatformimport, then import the shared predicate from._data.py/test/test_to_ngff_zarr_itk.py#L10-L10: remove the local_on_x86definition and theplatformimport, then import the same shared predicate from._data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py/test/test_to_ngff_zarr_dask_image.py` at line 11, Move the shared _on_x86 architecture predicate into py/test/_data.py alongside the existing test infrastructure, then import it from ._data in both py/test/test_to_ngff_zarr_dask_image.py lines 11-11 and py/test/test_to_ngff_zarr_itk.py lines 10-10; remove each local definition and its platform import.py/test/_data.py (1)
174-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_drop_codecsalso hides codecs that this library does choose.The recursion removes
"compressor","compressors", and"codecs"at every depth. That covers the zarr-chosen default, which is the stated goal. It also removes the sharding codec entry and any explicitcompressorspassed by a caller.test_to_ngff_zarr_v3_compression.pyandtest_to_ngff_zarr_sharding.pypass explicit codec settings, so the baseline comparison no longer confirms those settings.Those two tests already assert the codec entries directly from
zarr.json, so coverage is not lost today. Consider narrowing the removal to the top-level"codecs"entry that carries the bytes-to-bytes default, so caller-selected codecs stay in the comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py/test/_data.py` around lines 174 - 188, Update _drop_codecs to remove only the top-level "codecs" entry representing zarr’s bytes-to-bytes default, while preserving nested sharding codec data and explicit caller-provided compressor/codecs settings in recursive structures. Keep the existing list and non-dict recursion behavior, and adjust the filtering logic to distinguish the root node from nested nodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py/ngff_zarr/to_ngff_zarr.py`:
- Around line 619-626: The compressors handling around _write_with_tensorstore
currently keeps only the first non-bytes codec; preserve the full filtered
sequence instead. Update the serialization path, including sharding_indexed
metadata, to retain every codec after the bytes codec in its original order, and
add a regression test asserting codec names and ordering in zarr.json.
In `@py/test/_data.py`:
- Around line 275-279: After creating the baseline store and before comparing it
with the test store, use store_keys, store_contents, and _array_paths to derive
the baseline array paths and assert that the result is non-empty. Keep the
existing missing-directory skip in place, and make the assertion identify the
baseline path and indicate that an empty baseline would pass vacuously.
- Around line 267-271: Add the missing baseline/zarr2/ entries to the testing
archive for the versions and datasets used by verify_against_baseline, ensuring
the expected directory structure matches baseline/zarr3/ so
baseline_path.is_dir() succeeds for Zarr 2 comparisons.
In `@ts/src/methods/itkwasm-shared.ts`:
- Around line 289-294: Update isIntegerComponentType to test whether
componentType is an own property of INTEGER_COMPONENT_RANGES rather than using
the in operator, while preserving the existing string-type guard.
In `@ts/test/baseline_comparison_test.ts`:
- Around line 115-120: Update the diagnostic loop around testData, baselineData,
and componentType to handle int64 and uint64 values as bigint before subtracting
or comparing against numeric zero. Use bigint-safe difference tracking, or skip
mean-difference accumulation for bigint data while preserving difference counts
and firstDiffIndex behavior.
---
Nitpick comments:
In `@py/test/_data.py`:
- Around line 174-188: Update _drop_codecs to remove only the top-level "codecs"
entry representing zarr’s bytes-to-bytes default, while preserving nested
sharding codec data and explicit caller-provided compressor/codecs settings in
recursive structures. Keep the existing list and non-dict recursion behavior,
and adjust the filtering logic to distinguish the root node from nested nodes.
In `@py/test/test_to_ngff_zarr_dask_image.py`:
- Line 11: Move the shared _on_x86 architecture predicate into py/test/_data.py
alongside the existing test infrastructure, then import it from ._data in both
py/test/test_to_ngff_zarr_dask_image.py lines 11-11 and
py/test/test_to_ngff_zarr_itk.py lines 10-10; remove each local definition and
its platform import.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b805c71-e7d7-4599-831e-ec0125b64497
⛔ Files ignored due to path filters (2)
py/pixi.lockis excluded by!**/*.lockts/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
py/ngff_zarr/to_multiscales.pypy/ngff_zarr/to_ngff_zarr.pypy/pyproject.tomlpy/test/_data.pypy/test/test_baseline_guard.pypy/test/test_non_power_of_2_scale_factors.pypy/test/test_to_ngff_zarr_dask_image.pypy/test/test_to_ngff_zarr_itk.pypy/test/test_to_ngff_zarr_sharding.pypy/test/test_to_ngff_zarr_v3_compression.pypy/test/test_writer_isolation.pyts/deno.jsonts/src/methods/itkwasm-browser.tsts/src/methods/itkwasm-node.tsts/src/methods/itkwasm-shared.tsts/test/baseline_comparison_test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- py/ngff_zarr/to_multiscales.py
- py/pyproject.toml
- py/test/test_baseline_guard.py
- py/test/test_to_ngff_zarr_sharding.py
- ts/deno.json
- py/test/test_to_ngff_zarr_v3_compression.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py/test/_data.py`:
- Around line 290-293: Update verify_against_baseline in
py/test/_data.py:290-293 to accept an already-written test store and compare it
directly with baseline_store, removing the internal default MemoryStore write.
In py/test/test_to_ngff_zarr_sharding.py:59-61, 81-83, 103-105, and 142-144,
pass each scalar-, tuple-, mapping-sharded, and TensorStore output; in
py/test/test_to_ngff_zarr_v3_compression.py:66-68 and 127-129, pass each
compressed output to verify_against_baseline.
- Around line 238-264: The store_equals logic in py/test/_data.py#L238-L264 must
reject any mismatch between baseline_keys and test_keys before metadata or array
comparison, reporting unexpected test-store keys as well as missing keys. Update
py/test/test_baseline_guard.py#L28-L39 so the empty-versus-populated assertion
expects False and add coverage proving an extra test-store key is rejected.
In `@py/test/test_to_ngff_zarr_dask_image.py`:
- Around line 14-18: Apply the existing _on_x86 skip marker to both Dask
Gaussian tests, including test_gaussian_isotropic_scale_factors_two_components,
or move it to module scope so it covers both tests. Preserve the current skip
reason and ensure tests using Methods.DASK_IMAGE_GAUSSIAN are skipped on non-x86
architectures.
In `@py/test/test_writer_isolation.py`:
- Around line 71-73: Update the assertion in the test around _scale0_metadata to
locate the blosc codec and verify its cname is "zlib", clevel is 5, and
serialized shuffle is "shuffle", rather than checking only that "blosc" appears
in names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 309cd5e3-9c8f-4c30-b0de-bb71c8d198a5
⛔ Files ignored due to path filters (2)
py/pixi.lockis excluded by!**/*.lockts/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
py/ngff_zarr/to_multiscales.pypy/ngff_zarr/to_ngff_zarr.pypy/pyproject.tomlpy/test/_data.pypy/test/test_baseline_guard.pypy/test/test_non_power_of_2_scale_factors.pypy/test/test_to_ngff_zarr_dask_image.pypy/test/test_to_ngff_zarr_itk.pypy/test/test_to_ngff_zarr_sharding.pypy/test/test_to_ngff_zarr_v3_compression.pypy/test/test_writer_isolation.pyts/deno.jsonts/src/methods/itkwasm-browser.tsts/src/methods/itkwasm-node.tsts/src/methods/itkwasm-shared.tsts/test/baseline_comparison_test.ts
Three review findings that measure as real: - A native Zarr v3 codec passed as compressor on the sharding path was silently swapped for the zstd default: _numcodecs_to_zarr_v3_codec returned None and the caller substituted ZstdCodec(). Pass it through unchanged. - A constructor-defaulted BloscCodec serialized with shuffle=bitshuffle where zarr-python evolves shuffle from the dtype at write time, so the two backends wrote different configurations for the same request. Mirror zarr's _tunable_attrs evolution. The compressor test now compares full codec configurations between backends, not just names, and covers the defaulted case. - _is_bytes_codec did not recognize the codec given as the string "bytes", so a string codec chain picked "bytes" as the compressor.
store_equals only walked the baseline's keys, so an extra array in the test store passed unnoticed; require the key sets to match in both directions. A baseline directory that exists but holds no arrays (a truncated extraction) also compared vacuously; demand at least one array. The guard test now pins both.
'constructor' passed the `in` prototype-chain check in isIntegerComponentType; use an own-property check. The comparison loop now compares raw values, so bigint-backed arrays work, and converts to Number only for the diagnostics.
The Python 3.10 matrix resolves zarr 2.18 (zarr 3 requires 3.11), where zarr.codecs has no BloscCodec: building the codec inside a parametrize decorator broke collection for the whole suite. Construct it lazily in the test, imported from zarr.codecs.blosc, and drop the zarr_format kwarg zarr 2 does not know from the extra-key guard case.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates both the Python and TypeScript implementations to require itkwasm-downsample 2.0.0, and makes baseline verification meaningful again by comparing decompressed array values and parsed metadata (instead of bytewise store contents), with regenerated baselines and targeted regression tests to catch backend-default and geometry mismatches.
Changes:
- Bumped
itkwasm-downsampleto 2.0.0 (Python + TypeScript) and updated lockfiles accordingly. - Reworked baseline comparison to be non-vacuous and robust to codec defaults by comparing decoded arrays + normalized metadata; missing baselines now explicitly skip.
- Fixed/standardized downsampling behavior and writer behavior (exact scale strategy by default, Gaussian integer workaround in TS, and isolation from caller-owned objects) and added regression tests for these invariants.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ts/test/baseline_comparison_test.ts | Replaces ITK-Wasm compare helper with strict element-wise equality aligned with Python’s baseline semantics; updates orientation assumptions. |
| ts/src/methods/itkwasm-shared.ts | Adds integer Gaussian float32 workaround helpers (cast + rint-half-to-even + clamp) for cross-port bit-identical output. |
| ts/src/methods/itkwasm-node.ts | Applies the integer Gaussian workaround in the Node downsampling path. |
| ts/src/methods/itkwasm-browser.ts | Applies the integer Gaussian workaround in the browser downsampling path. |
| ts/deno.lock | Locks updated npm dependencies (including @itk-wasm/downsample@2.0.0). |
| ts/deno.json | Bumps @itk-wasm/downsample import to ^2.0.0. |
| py/test/test_writer_isolation.py | Adds regression tests ensuring TensorStore vs zarr-python parity and that writers don’t mutate caller-owned objects. |
| py/test/test_to_ngff_zarr_v3_compression.py | Updates baseline name/path to match regenerated baseline layout. |
| py/test/test_to_ngff_zarr_sharding.py | Updates baseline name/path to match regenerated baseline layout. |
| py/test/test_to_ngff_zarr_itk.py | Skips native ITK Gaussian baseline checks off x86 due to platform floating-point divergence. |
| py/test/test_to_ngff_zarr_dask_image.py | Skips dask-image/scipy Gaussian baseline checks off x86 due to platform floating-point divergence. |
| py/test/test_non_power_of_2_scale_factors.py | Updates tests to reflect scale_strategy="exact" as the default; keeps "pad" as opt-in. |
| py/test/test_baseline_guard.py | Adds explicit tests preventing vacuous baseline passes (missing/empty baselines and keyset mismatches). |
| py/test/_data.py | Fixes baseline pathing, enforces non-empty baseline stores, and compares decoded arrays + normalized metadata (dropping codec fields). |
| py/pyproject.toml | Requires itkwasm-downsample >= 2.0.0. |
| py/pixi.lock | Updates locked Python artifacts for itkwasm_downsample / wasi companion to 2.0.0. |
| py/ngff_zarr/to_ngff_zarr.py | Switches default scale_strategy to "exact", improves TensorStore codec handling, and avoids mutating caller multiscales. |
| py/ngff_zarr/to_multiscales.py | Avoids mutating caller NgffImage (shallow copy + independent callbacks list). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reverts the default back to "pad"; "exact" stays available opt-in. Updates the default-behaviour test and the parameter docs accordingly.
The 2_3 and 2_3_4 baselines were regenerated to the exact geometry, but "pad" stayed the default because it downsamples from the previous level instead of the full-resolution source. Measured on a 1024 cubed volume with factors 2, 3 and 4, that is 13 s against 25 s, so the default is worth keeping. The three baseline tests that use non-power-of-2 factors now ask for the exact strategy explicitly. Pad's own geometry is already asserted in test_non_power_of_2_scale_factors.py, so nothing loses coverage, and the TypeScript port has no strategy and always writes the exact geometry, so the shared baselines stay in parity. Also puts the sentence about a store misdescribing its own geometry back on "pad", where it belongs.
… order Merging main brought fideus-labs#623, which normalizes generated axes to the spec order (t, c, z, y, x). The brain_two_components DASK_IMAGE_GAUSSIAN baseline in the v0.21.0 testing-data archive was generated before that change, with the component axis last, so the key sets no longer match. Regenerate that baseline against the merged code and pin the updated archive.
thewtex
requested changes
Aug 10, 2026
thewtex
left a comment
thewtex
left a comment
Member
There was a problem hiding this comment.
Looking good!
A request inline.
Requested in review: castImage carries the optimized conversion paths. The integer restore still rounds half-to-even and clamps first, since castImage truncates raw floats and the values must match the Python port's np.clip(np.rint(...)). The rounded intermediate is float64 so every uint32/int32 value survives the trip exactly.
The decorator covered only the single-component test; both compare scipy Gaussian baselines generated on x86_64.
A supplied compressors sequence was reduced to its first non-bytes codec. Keep the whole bytes-to-bytes chain in order after the leading bytes codec, treat an explicit empty chain as no compression to match zarr-python, and apply the default zstd only when no codec option is given. Cover default, empty, bytes-only and multi-codec chains.
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.
Closes #625. Closes #627.
Bumps
itkwasm-downsampleto 2.0.0 in both ports. Verifying a downsampling engine requires a verification apparatus that works; repairing it surfaced real bugs. The problems, and the choices made:The baseline comparisons had been a no-op for 20 months. A path typo (8bb29ba) opened an empty store, and every comparison passed vacuously. → Path fixed; a missing baseline now skips explicitly instead of passing; comparisons work on decompressed values and parsed metadata, so the codec zarr picks per release doesn't matter.
The archive was stale. Every baseline predated the #1556 half-pixel grid shift; stores carried v2+v3 metadata side by side; a zarr2 layout nothing reads since dask ≥ 2026.1.2. → Everything regenerated, dead weight dropped (329 → 273 MB).
TensorStore wrote different stores than zarr-python. With no codecs declared it applied its own defaults (sharded, uncompressed — round trips halved chunk sizes), and requested codecs could be dropped. → Codecs always declared, compressors honoured; the four dedicated tests fail without the fix.
The TypeScript Gaussian was biased ~−1.2 on integer images. itkwasm's filter truncates in integer arithmetic; Python has cast to float32 and rounded back since c93fe01, TS never did. → Same workaround in TS, rounding half to even like np.rint. Both ports now produce bit-identical stores at every scale, data and metadata — verified.
to_ngff_zarr rewrote scales at write time and missed targets.
scale_strategy="pad"(the default) re-downsampled incrementally and silently wrote (32, 64, 64) where the metadata declared the factor-3 (43, 85, 85): stores misdescribing their own geometry. Three baselines had archived exactly that. → Default is now"exact", pad stays opt-in, the three baselines were regenerated (the v0.21.0 asset is replaced in place — only this branch ever referenced it). Both writers also mutated their caller's objects; they work on shallow copies now.Native-method baselines encode x86 floating point. Now that the comparison compares, Apple Silicon diverges on the ITK and dask-image Gaussians. → Those two comparisons skip explicitly off x86; the ITKWASM methods stay exact on every architecture — wasm is deterministic across them, and that is the guarantee that matters.
Suites: Python 813 passed / 3 skipped · TypeScript 466 passed / 0 failed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests