Add itk_transform_resample_bounding_box - #632
Merged
vboussot merged 10 commits intoAug 11, 2026
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 adds Python and TypeScript transform-resampling bounding-box APIs. It updates Zarr v3 codec and baseline handling, isolates writer inputs, and adds integer-safe ITK-Wasm Gaussian downsampling. ChangesTransform resampling
Zarr writer and baseline validation
Integer Gaussian downsampling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant itkTransformResampleBoundingBox
participant resampleBoundingBoxShared
participant ResampleBoundingBox
Caller->>itkTransformResampleBoundingBox: provide transform and image metadata
itkTransformResampleBoundingBox->>resampleBoundingBoxShared: select environment pipeline
resampleBoundingBoxShared->>ResampleBoundingBox: create validated region
ResampleBoundingBox-->>Caller: return lazy selection and crop metadata
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
py/ngff_zarr/itk_transform_resample_bounding_box.py (3)
351-356: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe "4 spatial dims" branch is unreachable.
_spatial_dimsfilters against_SPATIAL_DIMS = ("x", "y", "z"), sofixed_spatialnever has more than 3 entries. The upper bound of 4 and the message "only 2, 3 and 4 are supported" cannot be satisfied. Either addtto the spatial set for 4D transforms, or change the bound and message to 2 and 3.🤖 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/itk_transform_resample_bounding_box.py` around lines 351 - 356, Update the spatial-dimension validation around fixed_spatial to match the dimensions produced by _spatial_dims and _SPATIAL_DIMS: either support 4D by adding t consistently, or restrict the check and error message to the currently supported 2D and 3D cases.
38-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
moving_shapea required field.
ResampleBoundingBoxis exported publicly.moving_shapedefaults to{}, soclamped(),is_empty,slices()andcrop()raiseKeyErroron a manually constructed instance. The library itself always passes it. Making the field required removes the invalid state.🤖 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/itk_transform_resample_bounding_box.py` around lines 38 - 53, Make ResampleBoundingBox.moving_shape a required constructor field by removing its default_factory empty-dictionary value, while preserving its existing type and use in clamped(). Update any internal instantiations to explicitly provide the moving shape so manually constructed instances cannot omit required bounds.
331-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
:raises:docstring with the implemented checks.Two mismatches exist. The docstring lists "if the transform couples spatial and non-spatial axes", but no such check exists in the function. The function also raises
TypeErrorfrom_as_itk_transform_listfor an unsupported transform type, which the docstring does not document.🤖 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/itk_transform_resample_bounding_box.py` around lines 331 - 336, Update the raises documentation for the affected bounding-box resampling function to remove the unsupported claim about rejecting transforms that couple spatial and non-spatial axes, and document that TypeError may be raised by _as_itk_transform_list for unsupported transform types. Keep the other documented ValueError conditions unchanged.docs/itk.md (1)
117-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the TypeScript heading.
## TypeScriptsits at the same level as## ITK Pythonand## ITK-Wasm Python, so it reads as general TypeScript interoperability. The section only documentsitkTransformResampleBoundingBox. Use### TypeScriptunder "Out-of-core resampling", or rename it to "Out-of-core resampling in TypeScript".🤖 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 `@docs/itk.md` around lines 117 - 121, Adjust the heading for the section describing itkTransformResampleBoundingBox so it is scoped under “Out-of-core resampling”: make it a level-three “TypeScript” heading or rename it to “Out-of-core resampling in TypeScript,” without changing the documented content.py/test/test_itk_transform_resample_bounding_box.py (1)
71-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_oracle_regionis never called.No test in this file uses
_oracle_region. The module docstring states that expected regions come either from the ITK-Wasm documentation or from_oracle_region, so the helper and the docstring do not match the tests. Add a test that compares a linear-transform region against_oracle_region, or remove the helper and the docstring claim. The TypeScript counterpart has the same unused helper, and there it fails the lint job.🤖 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_itk_transform_resample_bounding_box.py` around lines 71 - 94, Resolve the unused `_oracle_region` helper and its module-docstring claim by either adding a test that computes a linear-transform region and compares it with `_oracle_region`, or removing both the helper and the claim. Keep the Python and TypeScript test suites consistent, applying the same choice to the corresponding unused helper in the TypeScript counterpart.Source: Linters/SAST tools
ts/test/itk_transform_resample_bounding_box_test.ts (1)
209-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the spatial entries of
selection().The test checks only that the
tandcentries arenull. The Python counterpart also asserts the spatial slices(1, 5),(2, 10)and(3, 11). Add the equivalent assertions onselection[2]throughselection[4]so the two ports cover the same behavior.🤖 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 `@ts/test/itk_transform_resample_bounding_box_test.ts` around lines 209 - 215, Extend the test around boundingBox.selection(moving.dims) to assert selection[2], selection[3], and selection[4] contain the expected spatial slices corresponding to (1, 5), (2, 10), and (3, 11), while preserving the existing null assertions for t and c.
🤖 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 1503-1511: Make exact the default scale_strategy in the
multiscale-writing configuration so written array shapes match their declared
coordinate transforms; alternatively, derive transforms from actual written
shapes. Update py/test/test_non_power_of_2_scale_factors.py lines 173-180 to
assert default output geometry matches metadata, and retain explicit pad
coverage at lines 183-204 only if pad emits matching metadata or rejects
nonrepresentable scales.
- Around line 342-368: Update codec construction around create_compression_codec
and inner_codecs to consume the full compressors chain, preserving one
BytesCodec followed by every supplied codec in order. Apply default zstd only
when no codec option is provided; an explicitly empty chain should not add
compression, while bytes-only and multi-codec chains must remain intact for both
sharded and unsharded output. Add coverage for these three cases.
In `@py/test/test_itk_transform_resample_bounding_box.py`:
- Around line 359-366: Update test_degenerate_fixed_grid_yields_an_empty_region
to pass a 2D identity transform via _identity(2), matching the fixed and moving
images’ ("y", "x") dimensions while preserving the existing empty-region
assertions.
In `@py/test/test_to_ngff_zarr_dask_image.py`:
- Around line 14-18: Apply the existing x86-only pytest.mark.skipif decorator to
both test_gaussian_isotropic_scale_factors and
test_gaussian_isotropic_scale_factors_two_components, so both Gaussian baseline
comparisons are skipped when _on_x86 is false.
In `@ts/src/methods/itkwasm-shared.ts`:
- Around line 260-294: Update the Gaussian workaround paths that currently
convert eligible images to Float32Array so int32 and uint32 values are processed
with Float64Array instead, preserving values beyond float32 precision through
filtering and integer conversion. Use the existing IntegerComponentType and
INTEGER_TYPED_ARRAY_CTORS symbols to select the appropriate precision, and add
constant-image coverage for 16_777_217, -16_777_217, and 4_294_967_295.
In `@ts/test/itk_transform_resample_bounding_box_test.ts`:
- Around line 76-113: The independent oracle helpers are unused, so either
exercise them in linear-transform tests or remove them and their matching
docstring claims. In ts/test/itk_transform_resample_bounding_box_test.ts lines
76-113, add a test comparing itkTransformResampleBoundingBox with oracleRegion;
in py/test/test_itk_transform_resample_bounding_box.py lines 71-94, add the
equivalent comparison with _oracle_region. If not adding those tests, remove
both helpers and the associated documentation claims to resolve lint failures.
---
Nitpick comments:
In `@docs/itk.md`:
- Around line 117-121: Adjust the heading for the section describing
itkTransformResampleBoundingBox so it is scoped under “Out-of-core resampling”:
make it a level-three “TypeScript” heading or rename it to “Out-of-core
resampling in TypeScript,” without changing the documented content.
In `@py/ngff_zarr/itk_transform_resample_bounding_box.py`:
- Around line 351-356: Update the spatial-dimension validation around
fixed_spatial to match the dimensions produced by _spatial_dims and
_SPATIAL_DIMS: either support 4D by adding t consistently, or restrict the check
and error message to the currently supported 2D and 3D cases.
- Around line 38-53: Make ResampleBoundingBox.moving_shape a required
constructor field by removing its default_factory empty-dictionary value, while
preserving its existing type and use in clamped(). Update any internal
instantiations to explicitly provide the moving shape so manually constructed
instances cannot omit required bounds.
- Around line 331-336: Update the raises documentation for the affected
bounding-box resampling function to remove the unsupported claim about rejecting
transforms that couple spatial and non-spatial axes, and document that TypeError
may be raised by _as_itk_transform_list for unsupported transform types. Keep
the other documented ValueError conditions unchanged.
In `@py/test/test_itk_transform_resample_bounding_box.py`:
- Around line 71-94: Resolve the unused `_oracle_region` helper and its
module-docstring claim by either adding a test that computes a linear-transform
region and compares it with `_oracle_region`, or removing both the helper and
the claim. Keep the Python and TypeScript test suites consistent, applying the
same choice to the corresponding unused helper in the TypeScript counterpart.
In `@ts/test/itk_transform_resample_bounding_box_test.ts`:
- Around line 209-215: Extend the test around boundingBox.selection(moving.dims)
to assert selection[2], selection[3], and selection[4] contain the expected
spatial slices corresponding to (1, 5), (2, 10), and (3, 11), while preserving
the existing null assertions for t and c.
🪄 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: 8ae309e2-8006-4c6b-8e7b-28c841d01749
⛔ Files ignored due to path filters (2)
py/pixi.lockis excluded by!**/*.lockts/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
docs/itk.mdpy/ngff_zarr/__init__.pypy/ngff_zarr/itk_transform_resample_bounding_box.pypy/ngff_zarr/to_multiscales.pypy/ngff_zarr/to_ngff_zarr.pypy/pyproject.tomlpy/test/_data.pypy/test/test_baseline_guard.pypy/test/test_itk_transform_resample_bounding_box.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/browser-mod.tsts/src/io/itk_transform_resample_bounding_box-browser.tsts/src/io/itk_transform_resample_bounding_box-node.tsts/src/io/itk_transform_resample_bounding_box-shared.tsts/src/io/itk_transform_resample_bounding_box.tsts/src/methods/itkwasm-browser.tsts/src/methods/itkwasm-node.tsts/src/methods/itkwasm-shared.tsts/src/mod.tsts/test/baseline_comparison_test.tsts/test/itk_transform_resample_bounding_box_test.ts
vboussot
force-pushed
the
feat/issue-626-resample-bounding-box
branch
2 times, most recently
from
August 7, 2026 14:25
ebb5fd8 to
2213079
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ts/test/itk_transform_resample_bounding_box_test.ts (2)
483-521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an async test callback and derive the oracle inputs from shared constants.
Two points in this test:
The callback is synchronous and returns an immediately-invoked async function. Every other test in the file uses
async () => { ... }directly. Movematrixandoffsetinside and drop the IIFE.Lines 511-515 restate the geometry literals that lines 490-499 already pass to
geometryImage. The two copies can drift. If someone changes the fixed shape or the moving scale in one place only, the oracle compares against the wrong reference and the result is a spurious pass or failure. Bind the geometry to named constants and pass the same constants to both.♻️ Proposed refactor
-Deno.test("an asymmetric 3D affine matches the oracle", () => { +Deno.test("an asymmetric 3D affine matches the oracle", async () => { // Stated in NGFF (z, y, x) order for the oracle; the transform itself is // built in ITK order, so both row and column ordering reverse. const matrix = [[1, 0.2, 0], [0, 2, 0.3], [0.5, 0, 1]]; const offset = [4, -6, 11]; - return (async () => { - const dims = ["z", "y", "x"]; - const fixed = await geometryImage(dims, { z: 4, y: 8, x: 16 }, { - z: 3, - y: 2, - x: 1, - }, { z: 30, y: 20, x: 10 }); - const moving = await geometryImage(dims, { z: 64, y: 128, x: 256 }, { - z: 1.5, - y: 0.5, - x: 0.25, - }, { z: -5, y: 7, x: 3 }); + const dims = ["z", "y", "x"]; + const fixedShape = { z: 4, y: 8, x: 16 }; + const fixedScale = { z: 3, y: 2, x: 1 }; + const fixedTranslation = { z: 30, y: 20, x: 10 }; + const movingScale = { z: 1.5, y: 0.5, x: 0.25 }; + const movingTranslation = { z: -5, y: 7, x: 3 }; + const byDim = (r: Record<string, number>) => dims.map((d) => r[d]); - const boundingBox = await itkTransformResampleBoundingBox( - itkAffine(reversed(matrix), [...offset].reverse()), - fixed, - moving, - { padding: 2 }, - ); + const fixed = await geometryImage( + dims, + fixedShape, + fixedScale, + fixedTranslation, + ); + const moving = await geometryImage( + dims, + { z: 64, y: 128, x: 256 }, + movingScale, + movingTranslation, + ); - const expected = oracleRegion( - matrix, - offset, - [4, 8, 16], - [3, 2, 1], - [30, 20, 10], - [1.5, 0.5, 0.25], - [-5, 7, 3], - 2, - ); - assertEquals(dims.map((d) => boundingBox.startIndex[d]), expected.start); - assertEquals(dims.map((d) => boundingBox.size[d]), expected.size); - })(); + const boundingBox = await itkTransformResampleBoundingBox( + itkAffine(reversed(matrix), [...offset].reverse()), + fixed, + moving, + { padding: 2 }, + ); + + const expected = oracleRegion( + matrix, + offset, + byDim(fixedShape), + byDim(fixedScale), + byDim(fixedTranslation), + byDim(movingScale), + byDim(movingTranslation), + 2, + ); + assertEquals(dims.map((d) => boundingBox.startIndex[d]), expected.start); + assertEquals(dims.map((d) => boundingBox.size[d]), expected.size); });🤖 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 `@ts/test/itk_transform_resample_bounding_box_test.ts` around lines 483 - 521, Refactor the test callback to use async () => { ... } directly, moving matrix and offset inside it and removing the immediately invoked async function. In the an asymmetric 3D affine matches the oracle test, define named constants for the fixed and moving geometry values, then reuse those constants in both geometryImage calls and oracleRegion instead of repeating the literals.
21-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the ITK transform helpers with
TransformList.Import
TransformListfrom"itk-wasm"and annotateitkTranslation,identity,itkAffine, and the inline transform literal. This removes theanysuppressions andas nevercasts while checking the transform shape at compile time.🤖 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 `@ts/test/itk_transform_resample_bounding_box_test.ts` around lines 21 - 66, Import TransformList from "itk-wasm" and use it to type itkTranslation, identity, itkAffine, and the inline transform literal. Remove the no-explicit-any suppressions and any as never casts, preserving the existing transform data while ensuring it satisfies TransformList at compile time.
🤖 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/test_itk_transform_resample_bounding_box.py`:
- Around line 359-362: Update the unsupported-dimensionality rejection tests to
use transforms matching their 1D images: change _identity(2) to _identity(1) in
py/test/test_itk_transform_resample_bounding_box.py lines 359-362, and
identity(2) to identity(1) in
ts/test/itk_transform_resample_bounding_box_test.ts lines 368-375.
In `@ts/test/itk_transform_resample_bounding_box_test.ts`:
- Around line 74-101: Update geometryImage’s axesOrientations parameter to use
Record<string, AnatomicalOrientation>, importing AnatomicalOrientation from
../src/types/rfc4.ts. At the call site, pass RAS directly and remove the unsafe
“as never” cast.
- Around line 240-243: Extend the assertions in the boundingBox selection test
after the existing t and c checks to validate selection[2], selection[3], and
selection[4]. Assert that these entries represent z from 1..5, y from 2..10, and
x from 3..11, respectively.
---
Nitpick comments:
In `@ts/test/itk_transform_resample_bounding_box_test.ts`:
- Around line 483-521: Refactor the test callback to use async () => { ... }
directly, moving matrix and offset inside it and removing the immediately
invoked async function. In the an asymmetric 3D affine matches the oracle test,
define named constants for the fixed and moving geometry values, then reuse
those constants in both geometryImage calls and oracleRegion instead of
repeating the literals.
- Around line 21-66: Import TransformList from "itk-wasm" and use it to type
itkTranslation, identity, itkAffine, and the inline transform literal. Remove
the no-explicit-any suppressions and any as never casts, preserving the existing
transform data while ensuring it satisfies TransformList at compile time.
🪄 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: ee4b3f4c-c45b-446d-8b55-63c22cda19a9
📒 Files selected for processing (2)
py/test/test_itk_transform_resample_bounding_box.pyts/test/itk_transform_resample_bounding_box_test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ts/src/io/itk_transform_resample_bounding_box-shared.ts (3)
42-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a factory function instead of an exported class.
The repository style prefers function-based exports.
ResampleBoundingBoxis a data holder with derived helpers, so acreateResampleBoundingBox(options)factory that returns a frozen object withclamped,isEmpty,selection, andcroppedTranslationkeeps the same surface. Keep the exported type as aninterfacefor callers.This is optional if the class shape intentionally mirrors the Python API.
As per coding guidelines: "Prefer function-based exports rather than classes".
🤖 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 `@ts/src/io/itk_transform_resample_bounding_box-shared.ts` around lines 42 - 82, The exported ResampleBoundingBox data holder should follow the repository’s function-based export style. Replace the class constructor with a createResampleBoundingBox(options) factory returning a frozen object, preserve the existing clamped, isEmpty, selection, and croppedTranslation helpers, and expose ResampleBoundingBox as an interface so callers retain the same public surface.Source: Coding guidelines
331-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe 4-dimension branch is unreachable.
SPATIAL_DIMScontains onlyx,y, andz, sofixedSpatial.lengthis at most 3. The conditionfixedSpatial.length > 4never holds, and the message states that 4 spatial dims are supported. Align the check and the message with the supported set, or add the fourth spatial dim name toSPATIAL_DIMS.🤖 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 `@ts/src/io/itk_transform_resample_bounding_box-shared.ts` around lines 331 - 336, Update the spatial-dimension validation around fixedSpatial and SPATIAL_DIMS so it matches the actually supported dimensions: either remove 4-dimensional support from the bound and error message, or add the required fourth dimension name and implementation support consistently.
284-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed
Imageobject.Declare the object as
const itkImage: Imageand return it.image.nameis a string, andUint8Array(0)satisfiesImage.data; the double cast is unnecessary.🤖 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 `@ts/src/io/itk_transform_resample_bounding_box-shared.ts` around lines 284 - 298, In the return-object construction, declare the object as const itkImage: Image, then return itkImage. Remove the as unknown as Image cast while preserving the existing fields, including image.name and the empty Uint8Array data.Source: Coding guidelines
ts/src/io/itk_transform_resample_bounding_box-browser.ts (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
npm:specifiers for npm dependencies.Lines 6-7 use bare package specifiers for
@itk-wasm/downsampleanditk-wasm. Usenpm:@itk-wasm/downsample`` andnpm:itk-wasm, unless an explicit repository import-map exception exists.As per coding guidelines,
ts/**/*.tsfiles must use thenpm:prefix for npm packages.🤖 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 `@ts/src/io/itk_transform_resample_bounding_box-browser.ts` around lines 6 - 7, Update the imports in the browser resampling module to use the npm: prefix for both `@itk-wasm/downsample` and itk-wasm, preserving their existing imported symbols and applying no other import changes.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 `@ts/src/io/itk_transform_resample_bounding_box-shared.ts`:
- Around line 318-321: Update the padding validation near the padding
initialization to reject NaN, infinities, and non-integer values, allowing only
finite non-negative integers. Revise the thrown error message to state that
padding must be a non-negative integer while preserving the existing padding
value in the message.
---
Nitpick comments:
In `@ts/src/io/itk_transform_resample_bounding_box-browser.ts`:
- Around line 6-7: Update the imports in the browser resampling module to use
the npm: prefix for both `@itk-wasm/downsample` and itk-wasm, preserving their
existing imported symbols and applying no other import changes.
In `@ts/src/io/itk_transform_resample_bounding_box-shared.ts`:
- Around line 42-82: The exported ResampleBoundingBox data holder should follow
the repository’s function-based export style. Replace the class constructor with
a createResampleBoundingBox(options) factory returning a frozen object, preserve
the existing clamped, isEmpty, selection, and croppedTranslation helpers, and
expose ResampleBoundingBox as an interface so callers retain the same public
surface.
- Around line 331-336: Update the spatial-dimension validation around
fixedSpatial and SPATIAL_DIMS so it matches the actually supported dimensions:
either remove 4-dimensional support from the bound and error message, or add the
required fourth dimension name and implementation support consistently.
- Around line 284-298: In the return-object construction, declare the object as
const itkImage: Image, then return itkImage. Remove the as unknown as Image cast
while preserving the existing fields, including image.name and the empty
Uint8Array data.
🪄 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: 3e626032-d968-4b35-b8a9-79223c589004
⛔ Files ignored due to path filters (2)
py/pixi.lockis excluded by!**/*.lockts/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
docs/itk.mdpy/ngff_zarr/__init__.pypy/ngff_zarr/itk_transform_resample_bounding_box.pypy/ngff_zarr/to_multiscales.pypy/ngff_zarr/to_ngff_zarr.pypy/pyproject.tomlpy/test/_data.pypy/test/test_baseline_guard.pypy/test/test_itk_transform_resample_bounding_box.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/browser-mod.tsts/src/io/itk_transform_resample_bounding_box-browser.tsts/src/io/itk_transform_resample_bounding_box-node.tsts/src/io/itk_transform_resample_bounding_box-shared.tsts/src/io/itk_transform_resample_bounding_box.tsts/src/methods/itkwasm-browser.tsts/src/methods/itkwasm-node.tsts/src/methods/itkwasm-shared.tsts/src/mod.tsts/test/baseline_comparison_test.tsts/test/itk_transform_resample_bounding_box_test.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- py/test/test_to_ngff_zarr_sharding.py
- py/test/test_to_ngff_zarr_v3_compression.py
- py/ngff_zarr/to_multiscales.py
- py/test/test_baseline_guard.py
- py/pyproject.toml
- py/ngff_zarr/init.py
- ts/test/baseline_comparison_test.ts
- py/test/test_non_power_of_2_scale_factors.py
- py/test/test_writer_isolation.py
- ts/src/browser-mod.ts
- py/test/test_to_ngff_zarr_itk.py
- ts/src/methods/itkwasm-node.ts
- docs/itk.md
- py/ngff_zarr/itk_transform_resample_bounding_box.py
- ts/src/mod.ts
- py/test/test_itk_transform_resample_bounding_box.py
- ts/src/methods/itkwasm-browser.ts
- py/ngff_zarr/to_ngff_zarr.py
- py/test/test_to_ngff_zarr_dask_image.py
- ts/src/io/itk_transform_resample_bounding_box.ts
- ts/src/io/itk_transform_resample_bounding_box-node.ts
- ts/src/methods/itkwasm-shared.ts
- ts/test/itk_transform_resample_bounding_box_test.ts
- py/test/_data.py
- ts/deno.json
vboussot
force-pushed
the
feat/issue-626-resample-bounding-box
branch
from
August 7, 2026 17:21
2213079 to
3223e22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ts/src/io/itk_transform_resample_bounding_box-shared.ts`:
- Around line 381-395: Validate paddedStartIndex and paddedSize before the byDim
helper builds records: require each array to contain exactly one finite numeric
value per itkDims dimension, and reject invalid or short pipeline results. Keep
the existing checkRegionContainsCorners validation and only proceed to
clamped/selection after both arrays pass these checks.
🪄 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: 945b4039-8470-4c3b-a0e6-e279d0f83d4a
📒 Files selected for processing (9)
py/ngff_zarr/itk_transform_resample_bounding_box.pypy/ngff_zarr/to_ngff_zarr.pypy/test/_data.pypy/test/test_itk_transform_resample_bounding_box.pypy/test/test_to_ngff_zarr_itkwasm.pypy/test/test_to_ngff_zarr_tensorstore.pyts/scripts/build_npm.tsts/src/io/itk_transform_resample_bounding_box-shared.tsts/test/itk_transform_resample_bounding_box_test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ts/test/itk_transform_resample_bounding_box_test.ts
- py/test/test_itk_transform_resample_bounding_box.py
- py/ngff_zarr/itk_transform_resample_bounding_box.py
- py/ngff_zarr/to_ngff_zarr.py
- py/test/_data.py
…ampling Compute the region of a moving image needed to resample a fixed image grid, from geometry alone: the pixel buffers are never read and the Dask graphs are never computed. That is the point -- describe two images and a transform with a few numbers, learn exactly which block a resample will touch, and only then move pixels. The transform is an ITK one, including the CompositeTransform an Elastix registration returns, and it maps fixed points into moving space. The image geometry is built the way ngff_image_to_itk_image builds it, RFC-4 direction included, so the transform is applied in the space it was produced in. ResampleBoundingBox keys everything by dimension name -- the pipeline reports arrays fastest-axis-first, the reverse of the Zarr order -- clamps negative start indices rather than letting them wrap, and crop() returns a lazily sliced NgffImage with a corrected translation.
TypeScript counterpart of the Python implementation, following the existing node/browser split. ResampleBoundingBox exposes selection() for a zarrita selection and croppedTranslation() for the shifted origin.
vboussot
force-pushed
the
feat/issue-626-resample-bounding-box
branch
from
August 7, 2026 17:32
3223e22 to
c71e5e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ts/test/itk_transform_resample_bounding_box_test.ts (3)
315-318: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a static import for
itkDirection.Line 19 already imports
resampleBoundingBoxSharedstatically from the same module. AdditkDirectionto that import and remove the dynamicawait import(...).♻️ Proposed change
-import { resampleBoundingBoxShared } from "../src/io/itk_transform_resample_bounding_box-shared.ts"; +import { + itkDirection, + resampleBoundingBoxShared, +} from "../src/io/itk_transform_resample_bounding_box-shared.ts";Deno.test("RAS orientation yields a non-identity direction", async () => { - const { itkDirection } = await import( - "../src/io/itk_transform_resample_bounding_box-shared.ts" - ); const image = await geometryImage(🤖 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 `@ts/test/itk_transform_resample_bounding_box_test.ts` around lines 315 - 318, Update the static import from the shared resampling module to include itkDirection, then remove the dynamic await import inside the “RAS orientation yields a non-identity direction” test while preserving its existing usage.
22-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typing the transform helpers as
TransformList.The three helpers return
anyand each needs adeno-lint-ignore no-explicit-anydirective.itkTransformResampleBoundingBoxalready acceptsTransformList(seets/src/io/itk_transform_resample_bounding_box-node.tslines 34-39). If you import that type and use it as the return type, you can drop the three lint-ignore directives and theas nevercast at line 476.🤖 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 `@ts/test/itk_transform_resample_bounding_box_test.ts` around lines 22 - 67, Type itkTranslation, identity, and itkAffine as returning TransformList, importing that type from the existing ITK transform definitions; remove their no-explicit-any directives and the unnecessary as never cast at the itkTransformResampleBoundingBox call. Preserve the current transform object structures and behavior.Source: Coding guidelines
486-523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the test body and derive the oracle inputs.
Two points:
- The test callback is synchronous and returns an async IIFE. Deno supports an
asynctest callback directly. Movematrixandoffsetinside anasync () =>body.- Lines 514-518 repeat the shape, scale, and translation literals from lines 493-502. If one list changes, the oracle silently disagrees with the images. Derive the arrays from
fixedandmoving.♻️ Proposed change
-Deno.test("an asymmetric 3D affine matches the oracle", () => { - // Stated in NGFF (z, y, x) order for the oracle; the transform itself is - // built in ITK order, so both row and column ordering reverse. - const matrix = [[1, 0.2, 0], [0, 2, 0.3], [0.5, 0, 1]]; - const offset = [4, -6, 11]; - return (async () => { - const dims = ["z", "y", "x"]; - const fixed = await geometryImage(dims, { z: 4, y: 8, x: 16 }, { - z: 3, - y: 2, - x: 1, - }, { z: 30, y: 20, x: 10 }); - const moving = await geometryImage(dims, { z: 64, y: 128, x: 256 }, { - z: 1.5, - y: 0.5, - x: 0.25, - }, { z: -5, y: 7, x: 3 }); - - const boundingBox = await itkTransformResampleBoundingBox( - itkAffine(reversed(matrix), [...offset].reverse()), - fixed, - moving, - { padding: 2 }, - ); - - const expected = oracleRegion( - matrix, - offset, - [4, 8, 16], - [3, 2, 1], - [30, 20, 10], - [1.5, 0.5, 0.25], - [-5, 7, 3], - 2, - ); - assertEquals(dims.map((d) => boundingBox.startIndex[d]), expected.start); - assertEquals(dims.map((d) => boundingBox.size[d]), expected.size); - })(); -}); +Deno.test("an asymmetric 3D affine matches the oracle", async () => { + // Stated in NGFF (z, y, x) order for the oracle; the transform itself is + // built in ITK order, so both row and column ordering reverse. + const matrix = [[1, 0.2, 0], [0, 2, 0.3], [0.5, 0, 1]]; + const offset = [4, -6, 11]; + const dims = ["z", "y", "x"]; + const fixedShape = { z: 4, y: 8, x: 16 }; + const fixedScale = { z: 3, y: 2, x: 1 }; + const fixedTranslation = { z: 30, y: 20, x: 10 }; + const movingScale = { z: 1.5, y: 0.5, x: 0.25 }; + const movingTranslation = { z: -5, y: 7, x: 3 }; + const fixed = await geometryImage( + dims, + fixedShape, + fixedScale, + fixedTranslation, + ); + const moving = await geometryImage( + dims, + { z: 64, y: 128, x: 256 }, + movingScale, + movingTranslation, + ); + + const boundingBox = await itkTransformResampleBoundingBox( + itkAffine(reversed(matrix), [...offset].reverse()), + fixed, + moving, + { padding: 2 }, + ); + + const expected = oracleRegion( + matrix, + offset, + dims.map((d) => fixedShape[d as keyof typeof fixedShape]), + dims.map((d) => fixedScale[d as keyof typeof fixedScale]), + dims.map((d) => fixedTranslation[d as keyof typeof fixedTranslation]), + dims.map((d) => movingScale[d as keyof typeof movingScale]), + dims.map((d) => movingTranslation[d as keyof typeof movingTranslation]), + 2, + ); + assertEquals(dims.map((d) => boundingBox.startIndex[d]), expected.start); + assertEquals(dims.map((d) => boundingBox.size[d]), expected.size); +});🤖 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 `@ts/test/itk_transform_resample_bounding_box_test.ts` around lines 486 - 523, Update the “an asymmetric 3D affine matches the oracle” test to use an async callback directly, moving matrix and offset declarations inside it and removing the async IIFE. Derive the oracle’s fixed and moving shape, scale, and translation arrays from the corresponding geometry image definitions or their resulting objects instead of repeating literals, so the oracle inputs stay synchronized with fixed and moving.
🤖 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 `@ts/src/io/itk_transform_resample_bounding_box-shared.ts`:
- Around line 281-295: Update the direction construction around
anatomicalOrientationToItkDirection to validate 2D orientations before copying
columns: when dimension is 2, reject any converted orientation column with a
nonzero LPS z component and return the existing identity direction fallback.
Preserve the current column-copy behavior for valid 2D and 3D orientations.
---
Nitpick comments:
In `@ts/test/itk_transform_resample_bounding_box_test.ts`:
- Around line 315-318: Update the static import from the shared resampling
module to include itkDirection, then remove the dynamic await import inside the
“RAS orientation yields a non-identity direction” test while preserving its
existing usage.
- Around line 22-67: Type itkTranslation, identity, and itkAffine as returning
TransformList, importing that type from the existing ITK transform definitions;
remove their no-explicit-any directives and the unnecessary as never cast at the
itkTransformResampleBoundingBox call. Preserve the current transform object
structures and behavior.
- Around line 486-523: Update the “an asymmetric 3D affine matches the oracle”
test to use an async callback directly, moving matrix and offset declarations
inside it and removing the async IIFE. Derive the oracle’s fixed and moving
shape, scale, and translation arrays from the corresponding geometry image
definitions or their resulting objects instead of repeating literals, so the
oracle inputs stay synchronized with fixed and moving.
🪄 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: c5cd3a69-e9f4-4a82-a48e-ab70b840472a
📒 Files selected for processing (4)
py/ngff_zarr/itk_transform_resample_bounding_box.pypy/test/test_itk_transform_resample_bounding_box.pyts/src/io/itk_transform_resample_bounding_box-shared.tsts/test/itk_transform_resample_bounding_box_test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- py/ngff_zarr/itk_transform_resample_bounding_box.py
… 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.
A superior/inferior orientation points along LPS z; truncating its column into a 2D direction matrix produced a singular matrix instead of the identity fallback.
Use AnatomicalOrientation for geometryImage's axesOrientations instead of casting RAS through never, and check the z/y/x selection entries in the non-spatial pass-through test.
vboussot
marked this pull request as draft
August 10, 2026 20:56
vboussot
marked this pull request as ready for review
August 10, 2026 21:02
itk.dict_from_transform materializes the parameters as float64 while keeping the transform's declared float32 value type, so the pipeline read the buffer as raw float32 and computed a region from garbage for e.g. DisplacementFieldTransform[itk.F, 2]. Declare the type the buffer actually has.
vboussot
added a commit
to InsightSoftwareConsortium/ITKElastix
that referenced
this pull request
Aug 11, 2026
Use ngff_zarr.itk_transform_resample_bounding_box to compute, from image geometry alone, the region of the moving image the full-resolution resample will read, and materialize only that region instead of the whole moving image. The composite transform is sampled into a displacement field over the fixed domain for the bounding-box pipeline, which reconstructs transforms itself and does not yet support itk.BSplineTransform. Requires ngff-zarr >= 0.42 (fideus-labs/ngff-zarr#632); the notebook executes end-to-end against that branch.
Open
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 #626.
Stacked on #628, which has merged; the base is back on
main.Given an ITK transform (including the
CompositeTransforman Elastixregistration returns) plus a fixed and a moving
NgffImage, this returns theregion of the moving image a resample would read. Everything comes from
geometry: no pixels are read and no Dask graph is computed.
cropis what removes the whole-moving-image materialization inInsightSoftwareConsortium/ITKElastix#375. It returns
Nonewhen the transformedgrid does not overlap, so a tiling loop can skip that block. Python and
TypeScript.
Conventions
The result is keyed by dimension name, because the underlying pipeline reports
arrays fastest-axis-first, the reverse of the Zarr order. Start indices may be
negative when the grid extends past the moving origin, so
crop,slicesandclampedclamp rather than letting a negative index wrap. The image geometry isbuilt the way
ngff_image_to_itk_imagebuilds it, RFC-4 direction included, sothe transform is applied in the space a registration produced it in.
Expected regions in the tests do not come from the pipeline under test: they
come either from the worked examples in the ITK-Wasm documentation or from a
NumPy oracle that recomputes the region from first principles in NGFF axis
order.
Deformable transforms
Supported: a linear transform is bounded exactly by its transformed grid
corners, while a non-linear one needs the full grid boundary walked, which the
pipeline does.
itk.BSplineTransformcurrently aborts insideitkwasm-downsample. That is anupstream defect, fixed in InsightSoftwareConsortium/ITK-Wasm#1581; displacement
fields and every linear parameterization work today. The limitation is noted in
the docs.
Summary by CodeRabbit