Skip to content

Conversation

@vboussot
Copy link

@vboussot vboussot commented Apr 1, 2025

This pull request introduces the IMPACT similarity metric into Elastix, enabling semantic image registration via deep feature representations extracted from TorchScript models.


📚 Reference

This metric is described in the following preprint, currently under review:

IMPACT: A Generic Semantic Loss for Multimodal Medical Image Registration
V. Boussot, C. Hémon, J.-C. Nunes, J. Dowling, S. Rouzé, C. Lafond, A. Barateau, J.-L. Dillenseger
arXiv:2503.24121 [cs.CV] – https://arxiv.org/abs/2503.24121


⚙️ LibTorch Dependency

IMPACT relies on LibTorch for runtime feature extraction from TorchScript models.

To build Elastix with IMPACT, download and extract LibTorch (e.g., 2.6.0 + CUDA 12.6) and set the Torch_DIR CMake variable when configuring:

 -DTorch_DIR=/path/to/libtorch/share/cmake/Torch

📦 IMPACT Examples & Resources

A full working setup to test the IMPACT similarity metric is available in the following repository:
👉 https://github.com/vboussot/ImpactLoss

This includes:

  • 🧠 Pretrained TorchScript models
    Ready-to-use models exported from TotalSegmentator and other large-scale segmentation networks.

  • ⚙️ Elastix configuration examples
    Parameter files demonstrating how to use Impact

  • 🐳 Docker environment
    A Dockerfile for building Elastix with LibTorch support, preconfigured to run the provided examples.

@N-Dekker
Copy link
Member

N-Dekker commented Apr 7, 2025

@vboussot Thank you very much for this great work. I just discussed with Marius (@mstaring) The use of deep features for image registration looks very interesting to us. There is a practical challenge however, how to maintain this new metric. It's a lot of extra code! And I see, it doesn't yet build on the CI, because it depends on Torch and CUDA.

We are able to build your pull request locally on Windows 11, though! Just one code adjustment was necessary: long should be replaced with int64_t! This is necessary, because on Windows (Visual Studio 2022), a long is only 32-bit. It must be int64_t, or order to be compatible with TorchLib's IntArrayRef (which is ArrayRef<int64_t>.) Shall I send you a patch?

Do you think the use of CUDA is essential, or would it also work on the CPU? (I see, https://pytorch.org/get-started/locally/ also offers LibTorch for the CPU.)

Do you think it would be feasible to write a test that would run on the CI, for this pull request?

@vboussot
Copy link
Author

vboussot commented Apr 8, 2025

Hello @N-Dekker, @mstaring,

Thank you for your interest in the project and for testing the PR on Windows.

I’ve updated the code to use int64_t instead of long to ensure compatibility. However, I can’t confirm it fully resolves all Windows-related issues, feel free to share your patch, as I currently don’t have access to a Windows machine for testing.

The metric now works with the CPU version of LibTorch. However, note that if the Elastix configuration file does not explicitly set GPU = -1, you may encounter the following error: "CUDA is not available. Please check your CUDA installation or run on a compatible device."

As for CI integration, I’ve started setting up a test configuration using LibTorch-CPU. At this stage, the most straightforward and robust approach seems to be configuring Elastix with a user-provided LibTorch version, allowing users to choose between a CPU or CUDA build, depending on their machine and installed CUDA

Valentin

@vboussot
Copy link
Author

vboussot commented Apr 9, 2025

Hello,

Just a quick update:

The build is now working on both Windows and Ubuntu.
On macOS, LibTorch doesn’t provide a precompiled binary for x86_64, only arm64 is available.
To support x86_64, we’ll need to build LibTorch from source targeting the correct architecture.
I’m planning to add this step to the macOS CI, along with caching to keep build times reasonable.

Regarding the functional test for the IMPACT metric:
I’m currently testing four configurations, across 2D and 3D, in both static and Jacobian modes.
Some tests are timing out, as they perform realistic registration on sample data.
To improve CI duration and consistency, I’ll reduce the number of iterations and switch to deterministic sampling.

Let me know if that sounds good, or if you’d prefer a different setup.

Best,
Valentin

@N-Dekker

This comment was marked as resolved.

@vboussot
Copy link
Author

vboussot commented Apr 14, 2025

Thank you @N-Dekker for reporting the bug with such detailed output.

The issue has been resolved in commit 41cebc2. The root cause was a type mismatch in a tensor passed to index_add_(). Specifically, nonZeroJacobianIndices was constructed from a std::vector unsigned long , which led to invalid values (e.g., 4e9, -8e18) on 32-bit platforms when converted to a tensor using torch::from_blob(...) with torch::kLong. An explicit cast to int64_t has been added. This issue only affected index tensors, other types such as double were unaffected. I also ran additional tests in virtualized environments (KVM/QEMU) and didn’t observe any issues.

Finally, the CI has been updated to build LibTorch from source on macOS 13 to support the x86_64 architecture, since official binaries are only available for arm64. The Azure pipeline is now passing successfully. GitHub Actions validation remains to be tested.

@N-Dekker
Copy link
Member

The issue has been resolved in commit 41cebc2. The root cause was a type mismatch in a tensor passed to index_add_(). Specifically, nonZeroJacobianIndices was constructed from a std::vector, which led to invalid values (e.g., 4e9, -8e18) on 32-bit platforms when converted to a tensor using torch::from_blob(...) with torch::kLong. An explicit cast to int64_t has been added. This issue only affected index tensors, other types such as double were unaffected. I also ran additional tests in virtualized environments (KVM/QEMU) and didn’t observe any issues.

Very glad to see you fixed this crash so quickly, thanks! I didn't have a clue! But I see now, elastix defines NonZeroJacobianIndicesType = std::vector<unsigned long>:

using NonZeroJacobianIndicesType = std::vector<unsigned long>;

Maybe we should ban long and unsigned long, both in elastix and ITK, to avoid such problems. 🤷

@vboussot
Copy link
Author

vboussot commented Apr 15, 2025

The previous version, which used torch::from_blob to directly convert a std::vector into a tensor, was indeed more efficient. However, I chose to use an explicit cast to int64_t instead of modifying NonZeroJacobianIndicesType, to avoid altering elastix internals and risking unintended side effects.

That said, I support the idea of replacing long / unsigned long with fixed-width types like int64_t / uint64_t in both elastix and ITK. This would enhance portability between 32 and 64 bit architectures, with minimal risk of breaking existing functionality.

Regarding the CTest error observed on GitHub Actions. Thanks for approving the tests, I believe it stems from two main issues: First, it's generally more robust to use ${TORCH_LIBRARIES} rather than hardcoding Torch in target_link_libraries(), as CMake targets may not always be correctly exported depending on how LibTorch was built. Second, building PyTorch from source requires Python ≥ 3.9

@vboussot vboussot force-pushed the feature/impact-metric branch 3 times, most recently from 00fd9ff to 1227e3d Compare April 15, 2025 19:32
@vboussot vboussot changed the title Add IMPACT Similarity Metric to Elastix ENH: Add IMPACT Similarity Metric to Elastix Apr 16, 2025
N-Dekker added a commit that referenced this pull request Apr 16, 2025
Aims to ease interaction with LibTorch (specifically when passing such indices as `data` parameter to `torch::from_blob`). `unsigned long` appears problematic, as it is typically 32-bit on Windows, while it's typically 64-bit on Linux.

Issue encountered during the development of the IMPACT Similarity Metric, pull request #1311 by Valentin Boussot et al.
Comment on lines 114 to 120
patchSizeVec = GetVectorFromString<std::string>(modelsPathVec.size(), patchSizeStr, "5*5*5");

/** Get and set the voxel size. */
std::string voxelSizeStr;
this->GetConfiguration()->ReadParameter(voxelSizeStr, type + "VoxelSize", this->GetComponentLabel(), level, 0);
std::vector<std::string> voxelSizeVec =
GetVectorFromString<std::string>(modelsPathVec.size(), voxelSizeStr, "1.5*1.5*1.5");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding PatchSize (default "5*5*5") and VoxelSize (default "1.5*1.5*1.5"), the use of the syntax "i*j*k" is a bit uncommon for elastix parameter values. We would rather have an array of three elements. For example in elastix parameter txt file:

(PatchSize 5 5 5)
(VoxelSize 1.5 1.5 1.5)

Internally, elastix would then store the VoxelSize values as a vector<string> of three elements, being { "1.5", "1.5", "1.5" }.

Would it be possible/feasible to you to adjust these parameters that way? Or would you like me to make it a pull request for https://github.com/vboussot/ImpactElastix/pulls ?

Copy link
Author

@vboussot vboussot Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I completely understand the preference for using arrays with space-separated values, as is typically done in elastix.

I had actually considered following the format used for example in FixedImagePyramidRescaleSchedule, but I ended up choosing a string format like "x * y * z" to avoid ambiguity with the multi-resolution syntax, which already uses space-separated values. I also find this format easier to parse consistently for parameters that expect 3D tuples.

That said, I’m of course happy to adapt it if you feel the standard array format would be better for long-term consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your consideration while looking at FixedImagePyramidRescaleSchedule, but I already checked with Marius (@mstaring) and Stefan (@stefanklein), and they also preferred arrays with space-separated values for PatchSize and VoxelSize. Our only concern was that it would break backward compatibility with your experiments, and your paper (the table of hyperparameters). Would that be a problem to you?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all, I understand the importance of staying consistent with the rest of elastix, and I’ll make the changes accordingly. It doesn’t cause any issue with the paper, I believe things are clear enough for readers, so it shouldn’t be a problem. I’ll also update the documentation to reflect the new format.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @N-Dekker, @mstaring, @stefanklein,

I’ve updated the parameter parsing logic.
The revised syntax and usage examples are available here:
https://github.com/vboussot/ImpactLoss/tree/main/ParameterMaps

Please let me know if it aligns with your expectations or if you have any suggestions.

N-Dekker added a commit that referenced this pull request Apr 16, 2025
Aims to ease interaction with LibTorch (specifically when passing such indices as `data` parameter to `torch::from_blob`). `unsigned long` appears problematic, as it is typically 32-bit on Windows, while it's typically 64-bit on Linux.

Issue encountered during the development of the IMPACT Similarity Metric, pull request #1311 by Valentin Boussot et al.
@N-Dekker

This comment was marked as resolved.

@vboussot vboussot force-pushed the feature/impact-metric branch from 7d5c913 to 50a4e4b Compare April 22, 2025 14:28
@vboussot vboussot force-pushed the feature/impact-metric branch from 50a4e4b to aee95b2 Compare April 22, 2025 19:25
{
std::vector<T> values;

if (delimiter == '\0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, this function is only called with delimiter == '\0', right? (Except for the GTest.)

Copy link
Author

@vboussot vboussot Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that’s right. I was actually wondering if I should specialize it here: #1311 (comment). I ended up fixing it by calling it GetBooleanVectorFromString

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up fixing it by calling it GetBooleanVectorFromString

I often find it helpful for different functions to have different names, thanks. (Function overloading is still useful in generic code, of course. But it's easier to figure out which function is being called where, when they have different names.)

Note that historically, std::vector<bool> is a bit troublesome, as it behaves slightly different than std::vector<T> in general. But maybe that's OK in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know that elastix already has quite some functionality to parse, retrieve and convert parameter values, right?

  • elastix::Configuration allows retrieving individual parameter values, and converting them to the requested type
  • The parameters are stored in a parameter map of type ParameterMapType = std::map<std::string, ParameterValuesType>, with ParameterValuesType = std::vector<std::string>
  • ParameterMapInterface wraps the parameter map
  • ParameterFileParser reads a parameter map from file (either ".txt" or ".toml"). It parses lists of values separated by white-space or (in case of TOML) comma-separated.
  • elastix::Conversion offers functions to convert parameter values

I think we should still check if there is overlap between your GetVectorFromString functions and the existing elastix functionality!

- os: windows-2022
c-compiler: "cl.exe"
cxx-compiler: "cl.exe"
libtorch_url: https://download.pytorch.org/libtorch/cpu/libtorch-win-shared-with-deps-2.6.0%2Bcpu.zip
Copy link
Member

@N-Dekker N-Dekker Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: While this zip file ("libtorch-win-shared-with-deps-2.6.0+cpu.zip") works just fine at the CI, there's a bit of a problem here when using Visual Studio on Windows, locally. It does not have the Debug binaries, so it does not allow switching between the Debug and Release configuration easily. (Visual Studio projects usually have the Debug and Release configuration together in one project file.) Fortunately, we are not the only ones with this problem: TorchConfig.cmake and MS Visual debug / release projects at once, Sébastien Maraux, Jun 15, 2023.

I don't have a suggestion right now, it's just something to keep in mind 🤷


For the record, another one, having the same problem: cmake: how to use libtorch in my project, Zhang Qiang, Feb 23, 2022

Valentin Boussot and others added 23 commits November 23, 2025 21:52
…ion` template parameter (#2)

* STYLE: Move helper functions of elxImpactMetricGTest into namespace

Following C++ Core Guidelines, Oct 3, 2024, "Use an unnamed (anonymous) namespace for all internal/non-exported entities",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf22-use-an-unnamed-anonymous-namespace-for-all-internalnon-exported-entities

* STYLE: Place elxImpactMetric functions into elastix namespace

Moved them out of the global namespace, to reduce the chance of name collisions.

* STYLE: Remove `Configuration` template parameters

It appears unnecessary for formatParameterStringByDimensionAndLevel to have `Configuration` as template parameter, because elastix only has one `Configuration` type.
…le (#3)

Moved `ModelConfiguration` from inside `ImpactImageToImageMetric<TFixedImage, TMovingImage>` into its own header file, `itkImpactModelConfiguration.h`, and renamed it to `ImpactModelConfiguration`.

Aims to simplify the design, by expressing that there is just one ModelConfiguration type for the IMPACT metric, independent of the types `TFixedImage` and `TMovingImage`.
Improves memory consumption by 2x and reduces computational time by ~10% on GPU. However, performance is significantly degraded on CPU due to lack of native float16 support. Controlled by parameter ImpactUseMixedPrecision, which should be disabled on CPU. Tested on CPU: Intel i7-12700K (12th Gen) @ 4.9GHz, GPU: NVIDIA GeForce RTX 3080 Lite Hash Rate
…on (#4)

* STYLE: Use range-based `for` loops

Suggested by LLVM 20.1.0 clang-tidy (ran locally), saying:

    warning: use range-based for loop instead [modernize-loop-convert]

See also https://clang.llvm.org/extra/clang-tidy/checks/modernize/loop-convert.html

* STYLE: Simplify initialization `ImpactModelConfiguration::m_patchSize`

Using the `vector(InputIterator first, InputIterator last)`constructor.
…urn a reference (#5)

* STYLE: Avoid copying `ImpactModelConfiguration` objects

Multiple copies of an `ImpactModelConfiguration` would share the same model, which could be confusing, especially when the model is being modified.

It might also be slightly more efficient to pass a `ImpactModelConfiguration` object "by const reference", instead of "by value".

* STYLE: Let GetModel() return a reference, instead of a shared_ptr

Aims to simplify the interface of `ImpactModelConfiguration`.

Aims to express that the model returned by GetModel() is owned and managed by an `ImpactModelConfiguration`. So the lifetime of the model should not be extended beyond the lifetime of the `ImpactModelConfiguration`.
…ssing qualification for dependent base members
…ion> in ImageToTensor and TensorToImage

Rewrote the ImageToTensor and TensorToImage functions in ImpactTensorUtils.hxx:
Removed manual OpenMP parallelization (`#pragma omp parallel`) in favor of `itk::ZeroBasedIndexRange<Dimension>` to handle nested iteration more cleanly.
Performance remains unchanged (~1800 ms for a 256×256×256 image)
@vboussot vboussot force-pushed the feature/impact-metric branch from f9e1a9c to 134b4f2 Compare November 23, 2025 20:52
@N-Dekker
Copy link
Member

Thanks @vboussot This is just a rebase, right?

Maybe we should rethink how to support macos now, as Azure Pipelines and GitHub Action are preparing to drop macos-13: https://github.blog/changelog/2025-09-19-github-actions-macos-13-runner-image-is-closing-down/

Maybe it's OK to just support macos-arm64 by now. Thank would make thinks easier when maintaining the IMPACT metric, right?

@N-Dekker
Copy link
Member

@vboussot Sorry for the delay again! When I would merge your pull request, is it OK to you if I squash all its commits into one? Or do you want to keep the individual commits (24, currently) in the main branch of elastix? Or maybe you want to just squash some of them together?

It's not that important, it's just that I often like to polish the commit history, in general. Eventually the commit history can be helpful when tracing the origin of a design decision or a bug, of course.

@vboussot
Copy link
Author

Hi @N-Dekker !
I'm currently preparing a release of elastix-impact on my fork, because I need binaries compiled for CPU and with LibTorch CUDA 12.6 support for the upcoming SlicerImpactReg. This should work on all machines with CUDA ≥ 12.6, and also on systems without a GPU.

Regarding the commits: feel free to squash them into a single one when merging.

@vboussot
Copy link
Author

"Eventually the commit history can be helpful when tracing the origin of a design decision or a bug, of course."

Actually, I already squashed many of those commits 😅

@vboussot vboussot force-pushed the feature/impact-metric branch 2 times, most recently from 269b57c to 134b4f2 Compare November 27, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants