Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions SuperBuild/External_elastix.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🤷

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

set(ELASTIX_GIT_TAG "419313e9cc12727d73c7e6e47fbdf960aa1218b9") # latest commit on "develop" branch as if 2019-10-13
else()
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
Comment on lines +29 to +34
Copy link
Collaborator

@jcfr jcfr Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

endif()

ExternalProject_Add(${proj}
Expand Down