ENH: Bump elastix version to 2023-07-19, inc. SetInitialTransform #234

N-Dekker · 2023-07-19T15:13:29Z

Follow-up to #211 commit 1043f10 "ENH: Bump elastix version to 2023-05-15, use C++17"

Including:

SuperElastix/elastix#936
SuperElastix/elastix@115d8e1
ENH: Add `ElastixRegistrationMethod::SetInitialTransform

SuperElastix/elastix#933
SuperElastix/elastix@d33a0d5
BUG: Fix reference count in ConvertItkTransformBaseToSingleItkTransform

SuperElastix/elastix#924
SuperElastix/elastix@71a4a9a
ENH: Add TransformParameterFileName property to TransformixFilter

SuperElastix/elastix#922
SuperElastix/elastix@9d76b86
ENH: Restore ELASTIX_BUILD_EXECUTABLE CMake option

SuperElastix/elastix#909
SuperElastix/elastix@9e64269
ENH: ElastixRegistrationMethod writes initial transform parameter files

SuperElastix/elastix#903
SuperElastix/elastix@2170592
ENH: Remove the letter 's' from "InitialTransformParametersFileName"

SuperElastix/elastix#898
SuperElastix/elastix@691c358
BUG: Add InitialTransformParameterObject maps to registration result

N-Dekker · 2023-07-19T17:35:26Z

@dzenanz @tbirdso @thewtex Do you have any clue about the CI build failures? Other pull requests (for example #230) seem to have the same build failures, specifically on cxx-build-workflow / build-test-cxx (windows-2022)

dzenanz · 2023-07-19T17:51:13Z

Maybe the same thing as fixed here and here?

Did SetInitialTransform work before this PR?

dzenanz · 2023-07-19T17:54:07Z

@tao558 If SetInitialTransform did not work before this PR, it might explain why some of the things I tried in registration notebook didn't work.

N-Dekker · 2023-07-19T18:11:44Z

Did SetInitialTransform work before this PR?

No, this PR adds ElastixRegistrationMethod.SetInitialTransform to ITKElastix for the first time. Although there were similar member functions already: SetInitialTransformParameterFileName, and SetInitialTransformParameterObject. The newly added member function allows passing an ITK transform object directly as argument.

N-Dekker · 2023-07-19T18:18:37Z

Maybe the same thing as fixed here and here?

Interesting... Do you mean ITKElastix should upgrade to a newer revision of ITKRemoteModuleBuildTestPackageAction?

Looking at

ITKElastix/.github/workflows/build-test-package.yml

Line 7 in 4fbdc9c

    
           uses: InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction/.github/workflows/build-test-cxx.yml@85252b549b1e44aa7198fbea470f75732d092c8b

dzenanz · 2023-07-19T18:29:45Z

Yes. Written on cellphone, excuse my brevity.

…

On Wed, Jul 19, 2023, 14:18 Niels Dekker ***@***.***> wrote: Maybe the same thing as fixed here <InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#64> and here <InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#65> ? Interesting... Do you mean ITKElastix should upgrade to a newer revision of ITKRemoteModuleBuildTestPackageAction? — Reply to this email directly, view it on GitHub <#234 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANVQ6OUDF24BG4TNFAPOPTXRAQI5ANCNFSM6AAAAAA2QCU2HA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

tbirdso · 2023-07-19T18:40:19Z

Do you mean ITKElastix should upgrade to a newer revision of ITKRemoteModuleBuildTestPackageAction

I agree with @dzenanz , please first update to the latest commit in ITKRemoteModuleBuildTestPackageAction.

N-Dekker · 2023-07-20T14:18:33Z

"cxx-build-workflow / build-test-cxx (windows-2022)" builds fine now that pull request #235 commit 7bdc103 is merged 👍

However, python-build-workflow-dev / build-linux-py (7, 2014-x64) still fails, saying:

ERROR    InvalidDistribution: Cannot find file (or expand pattern):             
         'dist/itk_*cp37*manylinux2014*x86_64.whl'

Any suggestion?

N-Dekker · 2023-07-20T15:45:44Z

Looks like python-build-workflow-dev / build-macos-py (11) is still using C++14, while elastix really needs C++17 now.


cd /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/Wrapping/Modules/Elastix && env SDKROOT=/Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk MACOSX_DEPLOYMENT_TARGET=10.9 /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/Wrapping/Generators/CastXML/castxml/bin/castxml -o /Users/svc-dashboard/D/P/ITKPythonPackage/scripts/../ITK-3.11-macosx_x86_64/Wrapping/castxml_inputs/itkElastixRegistrationMethod.xml --castxml-gccxml --castxml-start _wrapping_ --castxml-cc-gnu "(" /Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -std=c++14 ")" -w -c @/Users/svc-dashboard/D/P/ITKPythonPackage/scripts/../ITK-3.11-macosx_x86_64/Wrapping/castxml_inputs/.castxml.inc /Users/svc-dashboard/D/P/ITKPythonPackage/scripts/../ITK-3.11-macosx_x86_64/Wrapping/castxml_inputs/itkElastixRegistrationMethod.cxx
In file included from /Users/svc-dashboard/D/P/ITKPythonPackage/scripts/../ITK-3.11-macosx_x86_64/Wrapping/castxml_inputs/itkElastixRegistrationMethod.cxx:16:
In file included from /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Core/Main/itkElastixRegistrationMethod.h:41:
In file included from /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Core/Kernel/elxElastixMain.h:21:
In file included from /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Core/Kernel/elxMainBase.h:23:
In file included from /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Core/Kernel/elxElastixBase.h:33:
In file included from /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Core/Configuration/elxConfiguration.h:25:
In file included from /Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Common/ParameterFileParser/itkParameterMapInterface.h:22:
/Users/runner/work/ITKElastix/ITKElastix/_skbuild/macosx-10.9-universal2-3.11/cmake-build/_deps/elx-src/Core/Install/elxConversion.h:92:24: error: no template named 'is_integral_v' in namespace 'std'; did you mean 'is_integral'?

Specifically, 'is_integral_v' is introduced with C++17, while this particular compiler says:

error: no template named 'is_integral_v' in namespace 'std'; did you mean 'is_integral'?

The compiler appears to be run as usr/bin/c++ -std=c++14, according to the output above here, while that should be -std=c++14!

Any suggestion where this should be specified?

Update: I see now, it also says:

2023-07-21T12:43:33.9935110Z FAILED: /Users/svc-dashboard/D/P/ITKPythonPackage/ITK-3.11-macosx_x86_64/Wrapping/castxml_inputs/itkElastixRegistrationMethod.xml
...
2023-07-21T12:43:35.8310640Z FAILED: /Users/svc-dashboard/D/P/ITKPythonPackage/ITK-3.11-macosx_x86_64/Wrapping/castxml_inputs/itkTransformixFilter.xml

tbirdso · 2023-07-24T13:13:26Z

Looks like python-build-workflow-dev / build-macos-py (11) is still using C++14, while elastix really needs C++17 now.

@N-Dekker Thanks for finding this. As ITK requires a minimum of C++17 it is reasonable to require external modules to do the same. This seems like a bug in build-macos-py. Logged here: InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#66

thewtex · 2023-07-24T14:29:31Z

@N-Dekker thanks for working on the initial transform support 👏

@N-Dekker This should be fixed by the itk-5.4rc01 builds, where C++17 is enabled by the default via ITK. It could be tested by setting versions as in this PR: InsightSoftwareConsortium/ITKSplitComponents#72.

N-Dekker · 2023-07-24T15:27:38Z

Thanks to you both @tbirdso and @thewtex So I guess ITKElastix should upgrade both ITKRemoteModuleBuildTestPackageAction (to include a fix for InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#66 ) and ITK (to itk-5.4rc01 at least), before upgrading to the latest (C++17 based) SuperElastix/elastix, right?

tbirdso · 2023-07-24T15:39:15Z

So I guess ITKElastix should upgrade both ITKRemoteModuleBuildTestPackageAction and ITK before upgrading to the latest (C++17 based) SuperElastix/elastix, right?

Yes, I agree. Here is a potential roadmap for upgrading ITKElastix to the latest Elastix build with C++17:

Merge Itk 5.3rc01 ITKRemoteModuleBuildTestPackageAction#67 into [email protected] feature branch once tests pass (DONE)
Update ITKElastix CI to point to [email protected] feature branch (or fixed commit hash at HEAD). This will include C++17 tooling and an updated ref to ITK v5.4rc01. (See ENH: Bump for ITK v5.4rc01 #236) (DONE)
Update ITKElastix to the latest Elastix build (here)

EDIT: Updated roadmap with progress.

tbirdso · 2023-07-24T21:20:12Z

@N-Dekker FYI, ITKElastix is now updated to target ITK v5.4rc1 with C++17 build tools. Please rebase this PR on main.

Follow-up to InsightSoftwareConsortium#211 commit 1043f10 "ENH: Bump elastix version to 2023-05-15, use C++17" Including: SuperElastix/elastix#936 SuperElastix/elastix@115d8e1 ENH: Add `ElastixRegistrationMethod::SetInitialTransform SuperElastix/elastix#933 SuperElastix/elastix@d33a0d5 BUG: Fix reference count in ConvertItkTransformBaseToSingleItkTransform SuperElastix/elastix#924 SuperElastix/elastix@71a4a9a ENH: Add TransformParameterFileName property to TransformixFilter SuperElastix/elastix#922 SuperElastix/elastix@9d76b86 ENH: Restore ELASTIX_BUILD_EXECUTABLE CMake option SuperElastix/elastix#909 SuperElastix/elastix@9e64269 ENH: ElastixRegistrationMethod writes initial transform parameter files SuperElastix/elastix#903 SuperElastix/elastix@2170592 ENH: Remove the letter 's' from "InitialTransformParametersFileName" SuperElastix/elastix#898 SuperElastix/elastix@691c358 BUG: Add InitialTransformParameterObject maps to registration result

Aims to avoid compile errors from macosx-10.9-universal2-3.11/Xcode_13.2.1 MacOSX12.1.sdk/AppleClang 13.0.0.13000029 saying: > error: no template named 'is_integral_v' in namespace 'std'; did you mean 'is_integral'?

tbirdso

LGTM. CI is now passing with the update to v5.4rc1 CI workflows. 🟢

N-Dekker · 2023-07-25T15:26:56Z

Thanks for your approval Tom! For the sake of time, I'll just merge now!

N-Dekker force-pushed the Bump-elastix-version-2023-07-19-SetInitialTransform branch from 07ca69c to f31041c Compare July 20, 2023 12:26

tbirdso mentioned this pull request Jul 24, 2023

BUG: build-macos-py builds with outdated C++14 InsightSoftwareConsortium/ITKRemoteModuleBuildTestPackageAction#66

Closed

N-Dekker added 2 commits July 25, 2023 13:12

COMP: Ensure that C++ standard version is at least C++17

9327b3c

Aims to avoid compile errors from macosx-10.9-universal2-3.11/Xcode_13.2.1 MacOSX12.1.sdk/AppleClang 13.0.0.13000029 saying: > error: no template named 'is_integral_v' in namespace 'std'; did you mean 'is_integral'?

N-Dekker force-pushed the Bump-elastix-version-2023-07-19-SetInitialTransform branch from 805f89f to 9327b3c Compare July 25, 2023 11:12

tbirdso approved these changes Jul 25, 2023

View reviewed changes

N-Dekker merged commit 0df1ba6 into InsightSoftwareConsortium:main Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Bump elastix version to 2023-07-19, inc. SetInitialTransform #234

ENH: Bump elastix version to 2023-07-19, inc. SetInitialTransform #234

N-Dekker commented Jul 19, 2023

N-Dekker commented Jul 19, 2023

dzenanz commented Jul 19, 2023

dzenanz commented Jul 19, 2023

N-Dekker commented Jul 19, 2023

N-Dekker commented Jul 19, 2023 •

edited

Loading

dzenanz commented Jul 19, 2023 via email

tbirdso commented Jul 19, 2023

N-Dekker commented Jul 20, 2023

N-Dekker commented Jul 20, 2023 •

edited

Loading

tbirdso commented Jul 24, 2023

thewtex commented Jul 24, 2023

N-Dekker commented Jul 24, 2023

tbirdso commented Jul 24, 2023 •

edited

Loading

tbirdso commented Jul 24, 2023

tbirdso left a comment

N-Dekker commented Jul 25, 2023

ENH: Bump elastix version to 2023-07-19, inc. SetInitialTransform #234

ENH: Bump elastix version to 2023-07-19, inc. SetInitialTransform #234

Conversation

N-Dekker commented Jul 19, 2023

N-Dekker commented Jul 19, 2023

dzenanz commented Jul 19, 2023

dzenanz commented Jul 19, 2023

N-Dekker commented Jul 19, 2023

N-Dekker commented Jul 19, 2023 • edited Loading

dzenanz commented Jul 19, 2023 via email

tbirdso commented Jul 19, 2023

N-Dekker commented Jul 20, 2023

N-Dekker commented Jul 20, 2023 • edited Loading

tbirdso commented Jul 24, 2023

thewtex commented Jul 24, 2023

N-Dekker commented Jul 24, 2023

tbirdso commented Jul 24, 2023 • edited Loading

tbirdso commented Jul 24, 2023

tbirdso left a comment

Choose a reason for hiding this comment

N-Dekker commented Jul 25, 2023

N-Dekker commented Jul 19, 2023 •

edited

Loading

N-Dekker commented Jul 20, 2023 •

edited

Loading

tbirdso commented Jul 24, 2023 •

edited

Loading