Take the IMPACT backend from ITKIMPACT - #1463
Conversation
b58ebf7 to
de9f456
Compare
The IMPACT losses, ModelConfiguration, online (Jacobian) inference, the vector-image interpolator and the Static feature-map generation come from the ITKIMPACT remote module, a one-way dependency Elastix to ITKIMPACT. The local copies (ImpactLoss.h, ImpactTensorUtils.*, itkBSplineInterpolateVectorImageFunction.*, itkImpactModelConfiguration.h) are removed, and the backend's ModelConfiguration source is compiled into the Impact component so its torch-free public header stays Python-wrappable. With USE_ImpactMetric ON, ITKIMPACT_INCLUDE_DIR must point at the ITKIMPACT include/ directory; both the ImpactMetric component and CommonGTest receive it, so elxImpactMetricGTest.cxx resolves the backend headers. ImpactWriteFeatureMaps becomes ImpactFeatureMapOutputDirectory. The patch geometry follows the backend's. PatchPoint() sends each offset through the fixed image's direction matrix, one column per image axis, and the sampling callbacks receive the patch tensor shape from itk::Impact::PatchTensorShape(). This is the index-to-physical map ITKIMPACT applies in its v4 threader and in ImageToTensorFilter, so both modes describe the same neighbourhood. CI provisions LibTorch with `pip install torch`, the same mechanism the backend uses, on every runner that has a wheel: all three on GitHub Actions, and Windows and Ubuntu on Azure. Azure's hosted macOS agent is x86_64 throughout, whatever the macos-14 image name suggests, CMake there reporting host and target processor x86_64, no NEON, and SSE2 and SSE4; PyTorch publishes no macOS x86_64 wheel past 2.2.2, so no interpreter choice helps and that one job builds LibTorch from source, at the v2.11.0 tag so all three platforms compile against the same headers. On Windows the steps that run elastix set PATH from cmd, whose entry separator is the one the loader expects, so the LibTorch DLLs are found next to the plugin. torch 2.11 is pinned rather than 2.12 because it is the last series still shipping CUDA cu128, the widest GPU coverage for the release: Blackwell plus older cards, driver floor R570, where 2.12 offers only cu126/cu129/cu130. The download_libtorch / build_libtorch actions are removed, nothing referencing them. The three Static baselines are regenerated against this configuration, and the suite is 8/8 on this branch.
de9f456 to
7f73707
Compare
| ) | ||
|
|
||
| if(USE_ImpactMetric) | ||
| find_package(Torch REQUIRED) |
There was a problem hiding this comment.
Maybe for a follow-up: would it be possible to add a minimum version to the find_package(Torch REQUIRED) call?
More specifically, I'm trying out your PR locally now, and I wonder if my LibTorch 2.8 is still good enough. (I have no problem upgrading to LibTorch 2.11, just wondering!)
| "ITKIMPACT_INCLUDE_DIR to its include/ directory.") | ||
| endif() | ||
| target_include_directories(ImpactMetric PRIVATE ${ITKIMPACT_INCLUDE_DIR}) | ||
| target_sources(ImpactMetric PRIVATE ${ITKIMPACT_INCLUDE_DIR}/../src/itkModelConfiguration.cxx) |
There was a problem hiding this comment.
This works as long as ITKIMPACT only has one CXX file (itkModelConfiguration.cxx), of course. OK for now, but it might need to be extended in the future. No problem 👍
itk::ModelConfiguration is now itk::ImpactModelConfiguration in ITKIMPACT, the name being too generic to introduce in the itk namespace. Track the rename here, in the includes, the CMake source reference and the workflow comment. Name the local holding ImpactFeatureMapOutputDirectory after the parameter it reads, rather than writeFeatureMapsStr, which described an older boolean form of the option. No behaviour change.
86f6315 to
b5cc4b5
Compare
| libs: | ||
| - elastix-build/bin/libelx-ANNlib.so | ||
| - os: windows-2022 | ||
| libtorch-cpu-url: "https://download.pytorch.org/libtorch/cpu/libtorch-win-shared-with-deps-2.8.0%2Bcpu.zip" |
There was a problem hiding this comment.
I guess, the previous approach of downloading libtorch from https://download.pytorch.org/libtorch/cpu (rather than doing pip install torch) still remains useful for the Windows Debug version ("libtorch-win-shared-with-deps-debug-2.11.0+cpu.zip"), when building a Debug configuration of elastix with IMPACT, right? Anyway, no problem, just something to keep in mind 😇
| numberOfChannelsVec[i], | ||
| patchSizeVecByModel[i], | ||
| voxelSizeVecByModel[i], | ||
| /*overlap*/ 0u, |
There was a problem hiding this comment.
For other use-cases, the overlap argument would be non-zero, right? I'm just wondering, because when it is always just zero, it doesn't need to be an extra parameter 🤷 No problem, though, just curious.
| #include "itkBSplineInterpolateVectorImageFunction.h" | ||
| // Vector-image interpolator consumed from the ITKIMPACT backend (same per-component | ||
| // B-spline interpolation as the former local itkBSplineInterpolateVectorImageFunction). | ||
| #include "itkInterpolateVectorImageFunction.h" |
There was a problem hiding this comment.
The name "itkInterpolateVectorImageFunction.h" might (also) eventually cause naming conflicts with ITK (which currently just has a file named "itkInterpolateImageFunction.h")
No problem for now, just something I noticed.
|
@vboussot I'm ready to merge your PR now! Is it also ready to you? I guess so, otherwise you may convert to draft 😺 |
itkImpactModelConfigurationDetail.h and itkImageToFeaturesMapInternals.h are implementation details of ITKIMPACT, and no symbol of theirs is used here. ImpactModelConfiguration holds its pimpl in a shared_ptr, whose deleter is captured at construction, so the complete type is never needed on this side.
4e1cd66 to
83b0cde
Compare
The Static path tiles whenever the patch size is non-zero -- the mode a machine short on VRAM has to fall back to -- and assembled its feature maps with no blending at all, the overlap being hardcoded to 0. The comment claiming elastix does not tile was wrong. ImpactOverlap<level> is optional and laid out like PatchSize, per axis and per model. It defaults to a quarter of each patch axis; per axis rather than a single value, because an anisotropic patch given the same overlap on every axis blends its short axis over a far larger fraction of itself than its long one. Nothing shipped changes: every Static parameter map here uses a patch size of 0, which runs the whole image in one pass and never reassembles, and the Jacobian path never reads the overlap.
|
@N-Dekker Thanks for these, all good catches 👍 ModelConfiguration is back to itk::ImpactModelConfiguration, and while I was at it I moved the internal tiling machinery (accumulator, patch grid, blend windows) into itk::Impact. I did leave InterpolateVectorImageFunction and the ImageToTensorFilter / TensorToImageFilter pair alone, though. They're a generic ITK/torch bridge and a generic vector image interpolator rather than IMPACT concepts, so the resemblance to itkInterpolateImageFunction.h is on purpose 🙂 Your nitpick on the internal headers was spot on, and easier to fix than expected. elastix used no symbol from either, and the pimpl is held in a shared_ptr, so the complete type is never needed on this side. Both includes are gone. And overlap turned out to be a real gap rather than a dead parameter, so thanks for pulling on that thread. It drives the blending when patches are reassembled, and elastix does tile in Static mode, which is what a machine short on VRAM has to fall back to. Hardcoding 0 meant those feature maps were stitched with no blending at all. It's now an ImpactOverlap parameter, per axis like PatchSize, defaulting to a quarter of each patch axis. |
| useMixedPrecision); | ||
| // The reassembly reads the per-axis overlap, which the constructor can only broadcast | ||
| // from its scalar; set it explicitly so an anisotropic patch keeps its own per-axis one. | ||
| modelsConfiguration.back().SetOverlaps(overlapVecByModel[i]); |
There was a problem hiding this comment.
This looks somewhat redundant to me: first overlapVecByModel[i].front() is passed to the appended configuration, then overlapVecByModel[i] is also set into the configuration that is just appended. That's the way you mean it to be, right? Just double-checking 🤓
itk::ImpactModelConfiguration now takes the overlap per axis, so the scalar plus SetOverlaps pair is gone.
|
Good catch, it was redundant. The constructor took a scalar and broadcast it, so the SetOverlaps right after overwrote what it had just done. And nothing ever read the scalar back. Fixed in the backend instead of here: the configuration now takes the overlap per axis, like patchSize and voxelSize next to it, and elastix passes it in one call. |
|
Thanks for addressing my overlap comment, @vboussot. Now your pull request is really ready, right? 😃 |
|
Yes, ready 😃 Thanks for the review ! |
3 participants
The IMPACT losses,
ModelConfiguration, online (Jacobian) inference, the vector-image interpolator and the Static feature-map generation come from the ITKIMPACT remote module, a one-way dependency Elastix to ITKIMPACT. The local copies (ImpactLoss.h,ImpactTensorUtils.*,itkBSplineInterpolateVectorImageFunction.*,itkImpactModelConfiguration.h) are removed, and the backend'sModelConfigurationsource is compiled into the Impact component so its torch-free public header stays Python-wrappable.Building. With
USE_ImpactMetricON,ITKIMPACT_INCLUDE_DIRmust point at the ITKIMPACTinclude/directory. Both theImpactMetriccomponent andCommonGTestreceive it, soelxImpactMetricGTest.cxxresolves the backend headers.ImpactWriteFeatureMapsbecomesImpactFeatureMapOutputDirectory.Patch geometry follows the backend's.
PatchPoint()sends each offset through the fixed image's direction matrix, one column per image axis, and the sampling callbacks receive the patch tensor shape fromitk::Impact::PatchTensorShape(). This is the index-to-physical map ITKIMPACT applies in its v4 threader and inImageToTensorFilter, so both modes describe the same neighbourhood.CI provisions LibTorch with
pip install torchon all three OSes, the same mechanism the backend uses. torch 2.11 is pinned rather than 2.12 because it is the last series still shipping CUDA cu128, the widest GPU coverage for the release: Blackwell plus older cards, driver floor R570, where 2.12 offers only cu126/cu129/cu130. Thedownload_libtorch/build_libtorchactions are no longer referenced, andpython-versionmoves to 3.11 for thepip step.