Bump elastix version to 2023-05-15, use C++17, support any scalar pixel type

[N-Dekker]

Bump elastix version to 2023-05-15, including:

pull request SuperElastix/elastix#891
commit SuperElastix/elastix@ed15547
"PERF: Fill jsj (JacobianOfSpatialJacobian) in-place and remove jsj1"

pull request SuperElastix/elastix#890
commit SuperElastix/elastix@b9ea3a8
"PERF: Fill jsh (JacobianOfSpatialJacobian) in-place and remove jsh1"

pull request SuperElastix/elastix#887
commit SuperElastix/elastix@8298485
PERF: Make EvaluateParzenValues calls faster, using raw buffer of values

pull request SuperElastix/elastix#882
commit SuperElastix/elastix@58e0a7b
"ENH: Convert the input images to the user-specified internal pixel type"

pull request SuperElastix/elastix#864
commit SuperElastix/elastix@c3d478e
"ENH: Upgrade elastix from C++14 to C++17"

pull request SuperElastix/elastix#856
commit SuperElastix/elastix@48c6458
"ENH: Add SetInitialTransformParameterObject to ElastixRegistrationMethod"

pull request SuperElastix/elastix#832
commit SuperElastix/elastix@05d2b40
ENH: Support "ShowProgressPercentage" parameter (false by default)

pull request SuperElastix/elastix#815
commit SuperElastix/elastix@c4ef707
"ENH: Add ElastixLogLevel to the ITK interface"

Explicitly specified C++17 as standard for the compilation of ITKElastix.

Supported any scalar type (not just real) as input pixel type.

Because of the included elastix PERF commits, this pull request appears to make the itkElastixRegistrationMethodTest around 10% faster.

[N-Dekker]

@thewtex Can you please have a look at the CI failures from "Test notebooks / nbmake (ubuntu-22.04)"?

It says:

2023-05-03T15:38:55.7704100Z RuntimeError: module compiled against API version 0x10 but this version of numpy is 0xe . Check the section C-API incompatibility at the Troubleshooting ImportError section at https://numpy.org/devdocs/user/troubleshooting-importerror.html#c-api-incompatibility for indications on how to solve this problem .

[thewtex]

@N-Dekker I rebased #187 -- it could be related.

[N-Dekker]

Thanks @thewtex, I hope #187 will help.

Actually there are some more build problems with this PR of mine. The main commit, 2189fa8 has some other failures. I wonder if it's because of the added 3rd party library dependency: elastix now uses the spdlog library, with commit SuperElastix/elastix@085a393 The dependency is "header only", spdlog is retrieved by git submodule. Do you have a suggestion?

[thewtex]

@N-Dekker is it possible to use CMake FetchContent instead of a git submodule? This is easier for developers and library consumers.

Otherwise, maybe GIT_SUBMODULES needs to be set here:

ITKElastix/CMakeLists.txt

Lines 55 to 58 in c36279f

	FetchContent_Declare(
	elx
	GIT_REPOSITORY ${elastix_GIT_REPOSITORY}
	GIT_TAG ${elastix_GIT_TAG})

per: https://gitlab.kitware.com/cmake/cmake/-/issues/20579

[thewtex]

@N-Dekker with #187, a rebase on main should help with notebook CI 🍏

[N-Dekker]

@N-Dekker is it possible to use CMake FetchContent instead of a git submodule? This is easier for developers and library consumers.

Thanks Matt, but I would like to give "git submodule" a fair chance. One advantage is that GitHub clearly shows which revision of the third party library is being used, for example looking at https://github.com/SuperElastix/elastix/tree/main/ThirdParty

Otherwise, maybe GIT_SUBMODULES needs to be set here:

ITKElastix/CMakeLists.txt

Lines 55 to 58 in c36279f

	FetchContent_Declare(
	elx
	GIT_REPOSITORY ${elastix_GIT_REPOSITORY}
	GIT_TAG ${elastix_GIT_TAG})

per: https://gitlab.kitware.com/cmake/cmake/-/issues/20579

Interesting... https://gitlab.kitware.com/cmake/cmake/-/issues/20579 says:

When FetchContent_Declare is called with a GIT_SUBMODULES argument, it is not honored. Instead, FetchContent_MakeAvailable() will clone the declared git repository's submodules recursively, even if the named content was declared with GIT_SUBMODULES "".

For elastix, it would be just fine if its submodules are cloned recursively. It actually seems to be the default, when the option GIT_SUBMODULES is not specified. As I understand from https://gitlab.kitware.com/cmake/cmake/-/issues/20579#expected-result

However, this CMake issue seems to be specifically about using FetchContent_MakeAvailable(). As far as I can see, ITKElastix/CMakeLists.txt does not use FetchContent_MakeAvailable. Did you try FetchContent_MakeAvailable before?

The documentation of FetchContent_Populate says:

Note: Where possible, prefer to use FetchContent_MakeAvailable() instead of implementing population manually with this command

So would it be OK to you if we replace the existing FetchContent_Populate(elx) call from ITKElastix/CMakeLists.txt by the corresponding FetchContent_MakeAvailable() call?

[thewtex]

@N-Dekker yes, let's use FetchContent_MakeAvailable if it works! 👷‍♂️

[N-Dekker]

As we just discussed: the effect of replacing FetchContent_Populate with FetchContent_MakeAvailable is not yet entirely clear to me (especially when using OpenCL), so it may need some more research, and I prefer to postpone this change (this specific commit) to a possible later pull request.

[thewtex]

@N-Dekker thanks!!

Minor issue noted inline.

[N-Dekker]

@thewtex Do I understand correctly that the CI is still failing because of "the time status check"? https://open.cdash.org/viewTest.php?onlytimestatus&buildid=8677302 Apparently itkElastixRegistrationMethodTest takes too long:

https://open.cdash.org/test/1064322215 says:

This test took longer to complete (1m 370ms) than the threshold allows (8s).

Has this threshold been changed recently? Or does itkElastixRegistrationMethodTest really take more time than before? I'm preparing a performance improvement now: SuperElastix/elastix#887 However, I don't know if it is necessary in order to make the CI happy, and neither do I know if it would be sufficient (just ~5% performance gain).

[thewtex]

@N-Dekker agreed, the timeout could be longer.

We could add a longer time:

https://github.com/InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction

with the ctest --timeout flag.

Perhaps also make it configurable.

@tbirdso

[tbirdso]

We could add a longer time ... with the ctest --timeout flag.

You can try passing CTest options to the cxx-build-workflow in build-test-package.yml:

  with:
    ctest-options: '--timeout'

See more details in ITK reusable workflow documentation.

[N-Dekker]

Thanks for the hints, @thewtex and @tbirdso Honestly I just don't understand it. Here https://open.cdash.org/test/1066752286 it says:

This test took longer to complete (1m 1s 580ms) than the threshold allows (1m 53s 150ms)

So 1m 1s 580ms is longer than 1m 53s 150ms ?!? If you can tweak the threshold to allow passing this test, please do! (Possibly on a separate PR.)

In the mean time, I think I still made elastix somewhat faster, SuperElastix/elastix#890 but I guess it won't be much more than ~5%.

[N-Dekker]

All checks have passed
4 successful checks
I'm afraid that's not enough, 4 checks only! I'll try to retrigger the CI by a "dummy" force-push

---

Looks like GitHub has some "incidents", which might be related: https://www.githubstatus.com/

[N-Dekker]

FYI, on my PC, the registration of itkElastixRegistrationMethodTest appears slightly faster when using the latest revision from the main branch of SuperElastix/elastix (~33 seconds), compared to the elastix 5.1.0 release that is used by the main branch of ITKElastix (~34 seconds).

[thewtex]

@N-Dekker here is a start

[N-Dekker]

@N-Dekker here is a start

Thanks @thewtex but now it looks like the "acceptable range" has been adjusted already! The threshold value is much higher now. 😃 Looking at https://open.cdash.org/test/1068413323?graph=time today I saw:

The CI is going well so far (17 successful, 2 in progress, and 9 queued checks) so I'm still hopeful 🙏 that this PR may be merged as it is right now.

[dzenanz]

I restarted the builds. At least one of them timed out (6 hours and 6 minutes).

[N-Dekker]

I restarted the builds. At least one of them timed out (6 hours and 6 minutes).

Thanks @dzenanz So I guess we'd have to wait another 6 hours from now...?

[tbirdso]

Do you have administrative privileges in this repo? If so, you could merge right now.

OK, thanks, but then, what are we missing? It should actually produce python packages for all supported platforms, right? Will they be there?

Echoing @dzenanz , itk-elastix packages are not published on PyPI until a release is tagged. So, merging now will not affect PyPI package availability.

Another CI run will typically be triggered on merge into master with a merge commit, meaning experimental packages will be available as GitHub Actions artifacts approximately 6 hours from now.

If building for Linux ARM platforms (aarch64) is prohibitive for PR reviews then that platform target could be excluded from the build (see ITK reusable workflow documentation), or only built when CI runs on the master branch. In the future we aim to speed up the build process with better cross-compilation for ARM and/or with unified Python packages.

[N-Dekker]

Thanks @tbirdso

Another CI run will typically be triggered on merge into master with a merge commit, meaning experimental packages will be available as GitHub Actions artifacts approximately 6 hours from now.

Are there no artifacts (Python wheels) available from a successful PR before the merge?

[tbirdso]

Are there no artifacts (Python wheels) available from a successful PR before the merge?

Yes, artifacts are expected to be made available from any successful CI run, whether that run occurs in a PR or in the master branch. If you wait for CI to complete successfully here, experimental itk-elastix packages will be available in the latest run before merge. If you move forward with the merge before CI completes, I think remaining runs will be cancelled and artifacts will not be available from the PR. But, CI will run again in master and on completion those artifacts will be available.

EDIT: For instance, this appears to be the most recent successful run, scroll down to see artifacts: https://github.com/InsightSoftwareConsortium/ITKElastix/actions/runs/4907878277

[N-Dekker]

In the future we aim to speed up the build process with better cross-compilation for ARM and/or with unified Python packages.

I see it tries to build 11 wheels with each PR, while it is not even being merged. Wouldn't 1 wheel per platform (linux, macos, windows), with one Python version, be OK already?

Also I wonder, does the CI build both ITK and elastix from scratch with each PR trigger? Maybe those binaries could be cached somehow, if either of the two libraries are unmodified?

Unfortunately I'm not a yml/CI expert. So I'm just asking.

[dzenanz]

If I remember correctly, ITK build is cached, but elastix gets rebuilt each time. That allows us the flexibility of just changing the SHA of elastix.

[N-Dekker]

If I remember correctly, ITK build is cached, but elastix gets rebuilt each time.

OK. FYI, it takes 40 to 60 minutes to build elastix at https://github.com/SuperElastix/elastix (GitHub Actions CI).

[N-Dekker]

@dzenanz @tbirdso @thewtex By the way, I'm curious, did any of you actually change the timeout threshold for CDash/CTest already? Because just today those timeout failures from itkElastixRegistrationMethodTest disappeared and the "Acceptable Range" appeared to have become more "acceptable", as I mentioned at #211 (comment) (Even though I did not yet process Matt's PR N-Dekker#1).

[dzenanz]

change the timeout threshold

Not me.

[N-Dekker]

The CI still fails, including cancellations of build-linux-py (311, 2014-x64) and build-linux-py (311, 2014-x64)

But I'm hopeful that a rebase main might help, due to PR #214 , and I also have another C++ PERF improvement from elastix to be included. Please wait, I'll do a force-push.

[N-Dekker]

🎉 Approved by the CI 🥇

[NHPatterson]

Hi all, I'm not really a part of this conversation but I did notice that the new feature did not include any specific tests to ensure images with images with dtypes besides float32 and double work. I took the liberty of downloading the artifacts from the most recent CI and tried them out. Good news a lot more dtypes are supported for registration!

For reference:

import itk
import numpy as np

parameter_object = itk.ParameterObject.New()
default_rigid_parameter_map = parameter_object.GetDefaultParameterMap('rigid')
parameter_object.AddParameterMap(default_rigid_parameter_map)

int_dtypes = (np.uint8, np.int16, np.uint16)
for dtype in int_dtypes:
    image = np.random.randint(0, 100, (1024,1024), dtype=dtype)
    itk_image = itk.GetImageFromArray(image)
    
    result_image, result_transform_parameters = itk.elastix_registration_method(
    itk_image, itk_image,
    parameter_object=parameter_object,
    log_to_console=False)
    
    assert itk.template(result_image)[1][0] == itk.template(itk_image)[1][0]

    
floating_dtypes = (np.float32, np.double)
for dtype in floating_dtypes:
    image = np.random.rand(1024*1024).astype(dtype).reshape(1024,1024)
    itk_image = itk.GetImageFromArray(image)
    
    result_image, result_transform_parameters = itk.elastix_registration_method(
    itk_image, itk_image,
    parameter_object=parameter_object,
    log_to_console=False)
    
    assert itk.template(result_image)[1][0] == itk.template(itk_image)[1][0]

Thanks for this.

[N-Dekker]

@NHPatterson You're welcome, I'm very glad to hear so! I did test the extended pixel type support quite extensively at the C++ side (as part of the GoogleTest unit tests at SuperElastix/elastix/Core/Main/GTesting), but I only did a few local checks on a drafty Jupyter notebook (also using a wheel from the CI ) to see if it works. So thanks for checking it as well!