fix(ts): write toNgffImage's buffer as the type it holds - #672
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: 8 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthrough
ChangesTyped-array dtype preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change preserves typed-array data types through image creation and processing, with reported coverage for supported inputs and downstream paths. No actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00b78af595
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await toNgffZarr(store, multiscales, { version: "0.5" }); | ||
|
|
||
| const read = await fromNgffZarr(store); |
There was a problem hiding this comment.
This newly added integration test calls toNgffZarr and fromNgffZarr, which are retained only as compatibility aliases; use toOmeZarr and fromOmeZarr instead so new tests exercise and promote the canonical API names.
AGENTS.md reference: AGENTS.md:L301-L308
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Renamed to toOmeZarr / fromOmeZarr in 3a07a0c.
The zarr array was created with `data_type: "float32"` whatever came in, while a caller's `Uint8Array` or `Uint16Array` was kept and written through an `as Float32Array` cast. A chunk is copied as raw bytes sized by the destination data type, so the buffer was reinterpreted: a 24-element `Uint8Array` of 1..24 read back as 6 float32 values (1.54e-36, 4.06e-34, ...) followed by 18 zeros, and a `Uint16Array` of the same length as 12 values followed by 12 zeros. The other typed arrays were coerced rather than corrupted. `Int8Array`, `Int16Array`, `Int32Array`, `Uint32Array` and `Float64Array` went through `new Float32Array(data)`, which keeps the values, loses precision past float32's exact range, and drops the type: 1/3 came back as 0.3333333432674408, and a uint32 of 4294967295 as 4294967296, outside the range of the type it was written as. Python's `to_ngff_image` preserves the input dtype end to end. This was the only string literal in a `data_type` position under ts/src: every other `zarr.create` that writes a buffer takes its type from that buffer or from the source array's dtype, and the one that writes no data, `utils/factory.ts`, takes it as a parameter. `typedInputOf` pairs a typed array with the zarr data type of its elements, and that type reaches `defaultCodecs` as well as `zarr.create`, so the blosc parameters match the data too. uint8 gets noshuffle and typesize 1: byte shuffle on 1-byte elements interleaves unrelated values instead of separating one element's bytes, which costs 14% (cthead1) to 71% (bat-cochlea-volume) on this repository's own uint8 test images. `Uint8ClampedArray` is not a `Uint8Array`, so it has its own branch and is stored as uint8. Plain JavaScript arrays and any other `ArrayLike` become float32; Python reads `data.ndim` and rejects lists, so there is nothing to mirror there. `calculateStride` comes from `utils/transpose.ts`, which exports the same function. `to_ngff_image_dtype_test.ts` reads the values back for all nine typed-array types, pins the blosc parameters for uint8, uint16 and float64, pins the float32 default for plain arrays, and carries a uint8 buffer through `toMultiscales` and `toOmeZarr` into a store. The dtype assertions in `typed_array_support_test.ts` cover both pyramid levels, where the shape assertions alone hold even for a corrupted buffer. Two itk-wasm failures stop being masked by the float32 coercion, and neither is a defect of this package alone. `downsampleLabelImage` has no float64 instantiation and `downsampleBinShrink` no VariableLengthVector instantiation for int8, int32 or uint32, in the Python binding exactly as in the JS one. Python still downsamples 3-channel int8/int32/uint32 images because `methods/_itkwasm.py` enters vector mode only when `c` is the last dim, which `_canonical_axis_order` has already made false; the TypeScript gate omits that condition and takes the vector path where Python iterates channels. That divergence deserves its own issue. 597 passed, 0 failed. Closes fideus-labs#665
00b78af to
3a07a0c
Compare
1 participant
Fixes #665, found in #664.
toNgffImagecreated its zarr array withdata_type: "float32"whatever camein, kept a caller's
Uint8ArrayorUint16Array, and wrote it through anas Float32Arraycast. A chunk is copied as raw bytes sized by the destinationdata type, so the buffer was reinterpreted:
The other typed arrays were coerced rather than corrupted:
Int8Array,Int16Array,Int32Array,Uint32ArrayandFloat64Arraywent throughnew Float32Array(data), which keeps the values but drops the type and theprecision past float32's exact range. 1/3 read back as 0.3333333432674408, and
a uint32 of 4294967295 as 4294967296, outside the range of the type it was
written as.
The change
typedInputOfpairs a typed array with the zarr data type of its elements,and that type reaches
defaultCodecsas well aszarr.create:// dtype: "uint8", values: 1, 2, 3, ..., 24Python's
to_ngff_imagepreserves the input dtype end to end, and this wasthe only string literal in a
data_typeposition underts/src; every otherzarr.createthat writes a buffer derives its type from the data. Derivingthe codecs from the same type matters on its own: byte shuffle on 1-byte
elements interleaves unrelated values, which costs 14% (cthead1) to 71%
(bat-cochlea-volume) of encoded size on this repository's uint8 test images,
so uint8 now gets
noshuffleandtypesize: 1.Uint8ClampedArray, whatcanvas.getImageData()returns, is not aUint8Array, so it has its own branch and is stored as uint8. PlainJavaScript arrays and any other
ArrayLikebecome float32; Python readsdata.ndimand rejects lists, so there is no behaviour to mirror there.Integer Gaussian downsampling for these images goes through the existing
cast-to-float32, round, cast-back path in
itkwasm-shared.ts, the port of thePython precision workaround, which a float32-typed image never entered. The
downsampled pixel values now match Python exactly: for the same 64x64 uint8
input, level 1 is byte-identical between the two ports.
Tests
to_ngff_image_dtype_test.tsreads the values back for all nine typed-arraytypes, pins the blosc parameters for uint8, uint16 and float64, pins the
float32 default for plain arrays, and carries a uint8 buffer through
toMultiscalesandtoOmeZarrinto a store. Nine of its eleven testsrequire the source change; the two that do not pin the plain-array default and
the float32 round-trip. The dtype assertions added in
typed_array_support_test.tscover both pyramid levels, where shapeassertions alone hold even for a corrupted buffer.
597 passed, 0 failed.
itk-wasm limitations this uncovers, left out of scope
The float32 coercion was hiding missing type instantiations in the downsample
pipelines. Called directly,
@itk-wasm/downsample2.0.2 (JS) anditkwasm-downsample2.0.1 (Python) fail identically, so the gaps are in theshared wasm, not in either binding:
downsampledownsampleBinShrinkdownsampleLabelImageI did not find these reported in InsightSoftwareConsortium/ITK-Wasm; they are
worth an upstream issue.
What reaches them differs between the ports.
methods/_itkwasm.pyentersvector mode only when
cis the last dim, which_canonical_axis_orderhasalready made false, so Python iterates channels on the scalar path and only
ever hits the float64
label_imagegap, which fails there too. The TypeScriptgate (
itkwasm-node.ts:201and its twins) omits thec-last condition andtakes the vector path for any
caxis of 8 channels or fewer. Twoconsequences, both reachable before this change on images built by
fromNgffZarroritkImageToNgffImageand unchanged by it:ITKWASM_LABEL_IMAGEwith 2 to 8 channels throws for every dtype,float32 included.
ITKWASM_BIN_SHRINKwith 2 to 8 channels throws for int8, int32 anduint32, where Python succeeds.
Aligning the TypeScript gate on the Python one, or falling back to the
per-channel loop that already exists below it, deserves its own issue.
Summary by CodeRabbit
Bug Fixes
uint8values through serialization and deserialization.Tests