Add resample pipeline to @itk-wasm/downsample (transform + selectable interpolator) - #1548
Conversation
|
I thought this would be a new These are the things Fable found Findings (most severe first)
|
@sedghi yes, that was the original intention. However, at least for now, we have the system downsample python package on conda-forge, and I am vendoring these with downsample. |
|
Sounds good to me let's do it |
…olator validation, docs, ctest) Addresses the actionable Fable review findings on the resample-to-reference pipeline (PR InsightSoftwareConsortium#1548): - #2 itkwasm floor: the bindings import `TransformList`, which needs itkwasm >= 1.0b180, but all three Python packages pinned `>= 1.0.b145`. Bump to `>= 1.0.b185` (matching @itk-wasm/transform-io, the sibling that provides TransformList). pixi.lock re-synced. - #3 interpolator validation: the wasi Python binding emitted `interpolator not in ('linear,nearest_neighbor,...')` -- a single comma-joined string, so `in` did substring matching ('sinc', 'near', 'gauss' passed and died deep in the wasm). Root cause is the bindgen generator splitting the CLI11 choice list on ', ' (comma-space) while CLI11 serializes without spaces; the TS generator already splits on ','. Fix bindgen/python/wasi/wasi-function-module.js to split on ',' and regenerate. Verified 'sinc'/'near'/'gauss' now raise ValueError. - #5 SelectInterpolator: replace the bare `else` that silently returned linear for an unknown name with an explicit `linear` branch and an itkGenericExceptionMacro on unknown, so future drift between the CLI option list and the helper fails loudly instead of resampling with the wrong kernel. - #4 reference-image docs: the help text (propagated into every TS/Python docstring) claimed "an empty pixel buffer is acceptable," which over-promised -- the reference must still deserialize as the moving image's type; only its geometry is read. Reword to "Only the geometry (origin, spacing, direction, size) is used; the pixel values are ignored." Regenerated all bindings + demo. - #6 README: the bindgen README is create-once, so it never gained the resampleToReference / resampleToReferenceNode sections. Add both (spliced from a fresh bindgen so the hand-maintained Live-Demo / Documentation links are preserved -- a full regen would have dropped them). - #8 CMake: drop the resample-to-reference smoke ctest, which was argument-for-argument identical to resample-to-reference-linear. Findings #1 (generic transform type), #7 (vector functor duplication), and the remainder of #4/#5 (relax reference type / single interpolator factory table) are design-level and tracked as follow-ups; see the PR comment. Node (4/4) and Python wasi (16/16) suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review (via Fable). I went through all eight findings — disposition below. Fixes are in Addressed in
|
…omposite/multi (finding #1) The resample-to-reference `--transform` option was bound to itk::wasm::InputTransform<itk::AffineTransform<double, N>>. On the in-memory path (the TS/Python bindings) the deserializer validates only precision and dimension, then raw-copies the JSON parameter array into the affine -- so: * a B-spline transform (far more parameters than the affine's N*(N+1)) overran the fixed affine parameter buffer -> wasm heap corruption; * a composite list hit a null CompositeTransform dynamic_cast -> crash; * a Euler/rigid/translation transform was silently mis-parameterized into the affine's first slots -> wrong image, no error; * a multi-entry list silently kept only the last transform. itk::ResampleImageFilter::SetTransform() takes the abstract itk::Transform base, so a resample does not need a concrete affine at all. Read the transform generically instead: resampleReadInputTransform.h reconstructs any single transform parameterization (translation, rigid, affine, B-spline, ...) into itk::Transform<double, N, N> via the ITK object factory -- exactly as itkWasmTransformToTransformFilter does for composite components -- sizing the parameter buffer to the transform's own parameter count. So every single-transform parameterization now resamples correctly. Composite and multi-transform lists are rejected: chained transforms are a planned follow-up, and an explicit rejection is far better than the prior crash / silent last-entry-wins. (In the shipping Release builds the rejection surfaces as a hard pipeline abort rather than a clean message, consistent with how every itk-wasm pipeline error surfaces there; the descriptive message is retained for Debug builds.) The `--transform` option is now bound to a std::string (the memory-store index or filesystem path); `->type_name("INPUT_TRANSFORM")` is unchanged, so the generated TS/Python binding surface is byte-for-byte identical (transform?: TransformList) -- regenerating produced no binding diff. Tests: * Node + Python: a Translation transform resamples identically to the equivalent identity-matrix affine (proves generic parameterization support; the old reader produced a shear here). * Python: a multi-transform list is rejected (raises) rather than silently applying only its last entry. Node 5/5, Python wasi 18/18, and all 11 C++ CTests pass. Full chained/composite support remains a deferred follow-up (see PR InsightSoftwareConsortium#1548). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Update on finding #1 (transform bound to Generic single-transform support. Since
Binding surface unchanged. Tests. Node + Python assert a One caveat worth flagging for the framework: in the shipping Release builds, Full chained/composite ( |
|
Finding #1 is now fully resolved — What changed.
Tests (Node 7/7, Python wasi 21/21, C++ CTests 11/11):
With this, all four hazard cases from the original finding are closed: B-spline (generic reconstruction, |
|
Finding #7 addressed in
No behavior change, verified: the option names, order, |
|
@thewtex I went through the whole pipeline, really nice work. It's clean and well structured: resampling onto a reference image's grid, with an My only real point was that you always need a reference image for the grid, but you Background value: everything outside the moving image takes the filter's default of Nit: addressPrefixLength = 35 in resampleReadInputTransform.h hardcodes the address One last thing, totally out of scope: in medical image synthesis, where images are |
oh great find yes this will be an issue on CT |
|
Background value — addressed in
Design notes:
Tests (C++ CTests 12/12, Node 8/8, Python wasi 22/22):
|
…erpolator + background value) Add a `resample-to-reference` pipeline to @itk-wasm/downsample that resamples a moving image onto a reference image's sampling grid. Features: - Reference image supplies the output grid geometry (origin, spacing, direction, size); only its geometry is read. - Optional transform mapping output-grid points into the moving-image space. Any single parameterization (translation, rigid, affine, B-spline, ...) is reconstructed generically into the abstract itk::Transform base via the ITK object factory; a multi-entry or composite TransformList is composed into an itk::CompositeTransform (last list entry applied to the point first, matching itk::CompositeTransform semantics). Shared with resample-bounding-box through resampleReadInputTransform.h. - Selectable interpolator: linear, nearest_neighbor, label_image, b_spline, windowed_sinc, gaussian. - Configurable background value (`--default-value` / `defaultValue`) threaded into ResampleImageFilter::SetDefaultPixelValue, cast to the pixel type; defaults to 0. - Scalar and itk::VectorImage (multi-component) pixel-type paths, with the shared option surface hoisted into a ResampleOptions helper. Includes TypeScript + Python bindings, Node/Python/C++ tests, a browser demo, and independently generated baselines. Also carries a bindgen fix (wasi Python interpolator-choice validation split on ',' not ', ') and builds @itk-wasm/transform-io in the demo/CI. Rebased onto upstream main alongside the resample-bounding-box pipeline; the two share resampleReadInputTransform.h (this pipeline's version adds composite-list support and retains readInputTransformDimension for bounding-box) and the package test-data tarball (a superset already covering both). Squashed from the prior 15-commit branch to keep the rebase over the sibling pipeline's overlapping generated files tractable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0cac68f to
60bebb2
Compare
Summary
Adds a new
resamplepipeline to@itk-wasm/downsample/itkwasm-downsample. It wrapsitk::ResampleImageFilterto resample a moving image onto a reference image's grid, with an optional transform and a selectable interpolator, across the full ITK-Wasm target matrix (WASI, Node, browser/Emscripten, and Python sync + async / WASI + Emscripten).This was built in five phases: a working C++ prototype,
VectorImagesupport + interpolator ctest coverage, regenerated TypeScript/Python bindings, test data + independent baselines, and the Node/browser/Python test suites.What changed
C++ pipeline —
packages/downsample/resample.cxx(new)input(moving image),reference-image(geometry-only — an empty pixel buffer is accepted, since only origin/spacing/direction/size are read), andoutput.-t,--transformoptional transform mapping output-grid points into moving-image space (defaults to identity).-i,--interpolatorselectable from six methods:linear(default),nearest_neighbor,label_image,b_spline,windowed_sinc,gaussian.itk::VectorImage(multi-component) types.Build —
packages/downsample/CMakeLists.txtresampleexecutable and adds the required ITK modules (ITKImageFunction,ITKTransform) plus native transform IO (ITKIOTransformInsightLegacy,ITKIOTransformHDF5).resamplesmoke test plus one per interpolator (identity resample ofcthead1.png), and alabel_imagetest on2th_cthead1.png— all needing no new test data.Bindings — regenerated TypeScript + Python
resample/resampleNodewithResampleOptions(transform?: TransformList,interpolator?: string) andResampleResult, exported from the package entry points.resampleacross thewasiandemscriptensub-packages.Test data & baselines
test-data-hashinpackage.json/test:data:download.Tests
avatests for the resample bindings.resample.spec.ts.test_resample.py.@itk-wasm/transform-io(TS devDep) anditkwasm-transform-io(pixi) so transform inputs can be exercised in tests.Why
The
downsamplepackage could shrink images but had no general way to map an image onto an arbitrary output grid with a transform and a chosen interpolation method — the standard "resample onto a reference geometry" operation. This adds that as a first-class, fully-bound pipeline.Implementation notes
itk::wasm::InputTransform<itk::AffineTransform<double, N>>rather than the abstractitk::Transform<double, N, N>. The abstract base won't compile — the memory-IO reader callsTTransform::New(), anditk::Transformhas noitkNewMacro. The concrete double-precisionAffineTransformis-aTransform<double, N, N>and still feedsSetTransformpolymorphically.SetReferenceImage(...)+UseReferenceImageOn(), so the reference image contributes only its geometry.ResampleImageFilterhas no native multi-component support, so (mirroringdownsample.cxx) each component is extracted (VectorIndexSelectionCastImageFilter), resampled through shared wiring, then recomposed (ComposeImageFilter). Per-component filters are kept alive until a single final compose update, becauseitk::DataObjectonly holds a weak pointer back to its producer.MakeResampleFilter<TImage>helper reused by both the scalar and per-component vector paths.