ENH: Support elastix version 5.2 with ITK >= 5.4, drop ITK < 5.0 #53

N-Dekker · 2025-01-15T16:07:43Z

elastix 5.2 is the latest elastix release. It has various enhancements,
performance improvements and bug fixes:
https://github.com/SuperElastix/elastix/releases/tag/5.2.0
elastix version 5.2 requires ITK >= 5.4.

The Superbuild at the main branch of Slicer was upgraded to ITK 5.4.0 by
Dženan Zukić, June 3, 2024, pull request Slicer/Slicer#7771
commit Slicer/Slicer@acf3dd4

Dropped support of ITK < 5.0. Slicer now requires at least ITK 5.1.0, at the main branch:
https://github.com/Slicer/Slicer/blob/5971cb1e0d95f8592442f886d0d44edba16719ae/SuperBuild/External_ITK.cmake#L18

N-Dekker · 2025-01-15T16:12:53Z

SuperBuild/External_elastix.cmake

    set(ELASTIX_GIT_REPOSITORY "$https://github.com/SuperElastix/elastix.git")
    set(ELASTIX_GIT_TAG "419313e9cc12727d73c7e6e47fbdf960aa1218b9") # latest commit on "develop" branch as if 2019-10-13
  else()


@lassoan Do you think it would be OK to drop support for ITK 4 now? I mean, would it be OK to remove this CMake code on the "latest commit on "develop" branch as if 2019-10-13"?

The latest master branch of this repository needs to be compatible only with the latest main version of Slicer, which now uses ITK 5.1. Therefore it is OK to drop ITK 4 support.

@lassoan Thanks Adras! I just adjusted the PR to drop ITK < 5.0

elastix 5.2 is the latest elastix release. It has various enhancements, performance improvements and bug fixes: https://github.com/SuperElastix/elastix/releases/tag/5.2.0 elastix version 5.2 requires ITK >= 5.4. The Superbuild at the main branch of Slicer was upgraded to ITK 5.4.0 by Dženan Zukić, June 3, 2024, pull request Slicer/Slicer#7771 commit Slicer/Slicer@acf3dd4 Dropped support of ITK < 5.0. Slicer now requires at least ITK 5.1.0, at the main branch: https://github.com/Slicer/Slicer/blob/5971cb1e0d95f8592442f886d0d44edba16719ae/SuperBuild/External_ITK.cmake#L18

N-Dekker · 2025-01-16T13:26:42Z

SuperBuild/External_elastix.cmake

@@ -26,12 +26,12 @@ if(NOT DEFINED ${proj}_DIR AND NOT ${CMAKE_PROJECT_NAME}_USE_SYSTEM_${proj})
  endif()

  message(STATUS "ITK version: ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR}.${ITK_VERSION_PATCH}.")
-  if(DEFINED ITK_VERSION_MAJOR AND ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_LESS 5.0)
-    set(ELASTIX_GIT_REPOSITORY "$https://github.com/SuperElastix/elastix.git")


The dollar sign here ("$https://.../elastix.git") looks unfamiliar to me. Was it a typo? It appears to be introduced on March 21, 2022, with commit 3e9ef49

If the dollar sign would have caused an error when retrieving elastix.git, I guess ITK < 5.0 was "de facto" already dropped in 2022 🤷

I don't think the change was intentional. Probably it was just a bug that was not detected because we don't have a dashboard with ITKv4.

@lassoan Is there a public dashboard of your SlicerElastix extension?

It is here: https://slicer.cdash.org/index.php?project=SlicerPreview

Thanks @lassoan Do pull request (like this one) also appear at https://slicer.cdash.org/index.php?project=SlicerPreview ?

Extensions need to set up their own CI to test not-yet-merged pull requests. There is a slicer docker image that is updated nightly that could be used for this, but I have not done it for any of my projects yet. It would be nice, but so far I've been just testing risky-looking PRs locally.

Thanks @lassoan So do you consider this PR risky-looking? 😃

jcfr · 2025-01-20T16:44:33Z

SuperBuild/External_elastix.cmake

+  if(DEFINED ITK_VERSION_MAJOR AND ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_LESS 5.4)
    set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")
    set(ELASTIX_GIT_TAG "5.1.0") # 2023-01-12
+  else()
+    set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")
+    set(ELASTIX_GIT_TAG "5.2.0") # 2024-07-18


Suggested change

if(DEFINED ITK_VERSION_MAJOR AND ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_LESS 5.4)

set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")

set(ELASTIX_GIT_TAG "5.1.0") # 2023-01-12

else()

set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")

set(ELASTIX_GIT_TAG "5.2.0") # 2024-07-18

if(NOT DEFINED ITK_VERSION_MAJOR)

message(FATAL_ERROR "Variable ITK_VERSION_MAJOR is expected to be defined.")

endif()

if(${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_EQUAL 5.4 OR ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_GREATER 5.4)

set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")

set(ELASTIX_GIT_TAG "5.2.0") # 2024-07-18

else()

set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")

set(ELASTIX_GIT_TAG "5.1.0") # 2023-01-12

I suggest to have the "preferred / most recent" version be the first branch of the if/else

This could be further simplified by also bumping the minimum required CMake version to at least 3.8 so that VERSION_GREATER_EQUAL is available.

Thanks for your suggestions @jcfr It might be nice to have a separate check on the existence of ITK_VERSION_MAJOR. The if/else branches might also be swapped, if that's preferable. However I feel that that's beyond the scope if this pull request. This PR just aims to propose the minimal change to support elastix version 5.2.

Note that the original code also did something like:

if(older ITK version) use older elastix else use newer elastix

As you can see at the master branch (current revision):

SlicerElastix/SuperBuild/External_elastix.cmake

Lines 34 to 40 in 9ec57ae

if(DEFINED ITK_VERSION_MAJOR AND ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_LESS 5.0)

set(ELASTIX_GIT_REPOSITORY "$https://github.com/SuperElastix/elastix.git")

set(ELASTIX_GIT_TAG "419313e9cc12727d73c7e6e47fbdf960aa1218b9") # latest commit on "develop" branch as if 2019-10-13

else()

set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")

set(ELASTIX_GIT_TAG "5.1.0") # 2023-01-12

endif()

So I feel that this PR is already ready, and your suggestions might be put into a separate PR, afterwards. Do you still think your suggestions might make it easier to get this pull request (#53) merged?

N-Dekker commented Jan 15, 2025

View reviewed changes

N-Dekker force-pushed the Support-elastix-5-2 branch from fa7fee1 to 367e246 Compare January 16, 2025 12:17

N-Dekker force-pushed the Support-elastix-5-2 branch from 367e246 to fffc9f8 Compare January 16, 2025 12:20

N-Dekker changed the title ~~ENH: Support elastix version 5.2, when using ITK >= 5.4~~ ENH: Support elastix version 5.2 with ITK >= 5.4, drop ITK < 5.0 Jan 16, 2025

N-Dekker commented Jan 16, 2025

View reviewed changes

N-Dekker marked this pull request as ready for review January 16, 2025 13:27

jcfr reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Support elastix version 5.2 with ITK >= 5.4, drop ITK < 5.0 #53

ENH: Support elastix version 5.2 with ITK >= 5.4, drop ITK < 5.0 #53

N-Dekker commented Jan 15, 2025 •

edited

Loading

N-Dekker Jan 15, 2025 •

edited

Loading

lassoan Jan 15, 2025

N-Dekker Jan 16, 2025

N-Dekker Jan 16, 2025

lassoan Jan 16, 2025

N-Dekker Jan 17, 2025

lassoan Jan 18, 2025

N-Dekker Jan 20, 2025

lassoan Jan 20, 2025

N-Dekker Jan 20, 2025

jcfr Jan 20, 2025 •

edited

Loading

jcfr Jan 20, 2025

N-Dekker Jan 21, 2025

	if(DEFINED ITK_VERSION_MAJOR AND ${ITK_VERSION_MAJOR}.${ITK_VERSION_MINOR} VERSION_LESS 5.0)
	set(ELASTIX_GIT_REPOSITORY "$https://github.com/SuperElastix/elastix.git")
	set(ELASTIX_GIT_TAG "419313e9cc12727d73c7e6e47fbdf960aa1218b9") # latest commit on "develop" branch as if 2019-10-13
	else()
	set(ELASTIX_GIT_REPOSITORY "https://github.com/SuperElastix/elastix.git")
	set(ELASTIX_GIT_TAG "5.1.0") # 2023-01-12
	endif()

ENH: Support elastix version 5.2 with ITK >= 5.4, drop ITK < 5.0 #53

Are you sure you want to change the base?

ENH: Support elastix version 5.2 with ITK >= 5.4, drop ITK < 5.0 #53

Conversation

N-Dekker commented Jan 15, 2025 • edited Loading

N-Dekker Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfr Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Dekker commented Jan 15, 2025 •

edited

Loading

N-Dekker Jan 15, 2025 •

edited

Loading

jcfr Jan 20, 2025 •

edited

Loading