COMP: Use ITK 5.1.0 strongly typed enums in elxTransformBase #245

thewtex · 2020-04-30T01:09:45Z

To address:

In file included from _deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.h:384:0,
from _deps/elastix_fetch-src/Core/Kernel/elxElastixTemplate.h:39,
from _deps/elastix_fetch-src/Core/Install/elxIncludes.h:39,
from _deps/elastix_fetch-src/Components/FixedImagePyramids/FixedRecursivePyramid/elxFixedRecursivePyramid.h:21,
from _deps/elastix_fetch-src/Components/FixedImagePyramids/FixedRecursivePyramid/elxFixedRecursivePyramid.cxx:19:
_deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx: In member function ‘void itk::PixelTypeChangeCommand::Execute(itk::Object*, const itk::EventObject&)’:
_deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx:75:55: error: ‘VECTOR’ is not a member of ‘itk::ImageIOBase’
castcaller->GetModifiableImageIO()->SetPixelType( ImageIOBase::VECTOR );
^
_deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx: In member function ‘void itk::PixelTypeChangeCommand::Execute(const itk::Object*, const itk::EventObject&)’:
_deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx:83:55: error: ‘VECTOR’ is not a member of ‘itk::ImageIOBase’
castcaller->GetModifiableImageIO()->SetPixelType( ImageIOBase::VECTOR );

N-Dekker

Thank you, Matt. I'm sorry to say this pull request cannot be merged right now, as we we would still like to remain compatible with ITK 5.0.1 for now.

N-Dekker · 2020-04-30T17:05:57Z

@thewtex Did you build ITK with ITK_LEGACY_REMOVE ON or OFF? For the time being, I think it should be OFF for ITK 5.1, in order to preserve ITK 5.0.1 compatibility for elastix. Right?

thewtex · 2020-05-11T22:03:40Z

@N-Dekker ok, this could wait until elastix requires ITK 5.1.0.

Did you build ITK with ITK_LEGACY_REMOVE ON or OFF?

Yes, this is for the ITKElastix remote module / Python package, which requires the lastest and greatest ITK :-).

N-Dekker · 2020-05-12T09:15:41Z

Yes, this is for the ITKElastix remote module / Python package, which requires the lastest and greatest ITK :-).

@thewtex Thanks for explaining, Matt. Unfortunately we do not have enough resources for elastix to actively support two different versions of ITK. As I just discussed with Marius @mstaring and Stefan @stefanklein However, once ITK 5.1 (final) is officially released, we intend to quickly upgrade the develop branch of elastix from ITK 5.0.1 to ITK 5.1.

Can you please give us an estimation of when ITK 5.1 is officially released?

thewtex · 2020-05-13T20:06:53Z

@N-Dekker makes sense.

The Git tag for ITK v5.1.0 is out. I anticipate the final release announcement sometime in the next week.

N-Dekker · 2020-05-15T15:23:46Z

@thewtex Could you possibly still amend your commit (and force-push) just to adjust the following line of code in Testing/CI/Azure/ci.yml to the latest ITK version tag?

ITKv5_VERSION: v5.0.1

elastix/Testing/CI/Azure/ci.yml

Line 2 in 56c2ebe

ITKv5_VERSION: v5.0.1

It would be great to see your pull request going from red to green 😃

thewtex · 2020-05-15T15:46:04Z

@N-Dekker yes, done :-) 💚

N-Dekker · 2020-05-15T17:51:32Z

Thanks for adjusting the ITK tag to v5.1.0 in the Elastix yml file, Matt! Unfortunately it appears that there are still some more compilation errors left now, at the Azure CI, including:

itkAdvancedBSplineDeformableTransformTest.cxx
d:\a\1\s\common\transforms\itkAdvancedBSplineDeformableTransformBase.h(78): error C2039: 'TransformCategoryType': is not a member of 'itk::AdvancedTransform<TScalarType,3,3>' [D:\a\1\Elastix-build\Testing\itkAdvancedBSplineDeformableTransformTest.vcxproj]

...

d:\a\1\s\common\transforms\itkAdvancedBSplineDeformableTransformBase.h(270): error C2555: 'itk::AdvancedBSplineDeformableTransformBase<TScalarType,3>::GetTransformCategory': overriding virtual function return type differs and is not covariant from 'itk::Transform<TScalarType,3,3>::GetTransformCategory' [D:\a\1\Elastix-build\Testing\itkBSplineJacobianGradientPerformanceTest.vcxproj]

thewtex · 2020-07-20T16:14:47Z

@N-Dekker do you possibly have time to follow-up on this?

N-Dekker · 2020-08-05T13:43:30Z

@thewtex I think I finally have some time today! Unfortunately your PR still fails on the CI. I'll have a look.

Do you happen to have a fix for the remaining errors at the CI? Like this one:

elxTransformParametersCompare.cxx
d:\a\1\s\common\transforms\itkAdvancedMatrixOffsetTransformBase.h(138): error C2039: 'TransformCategoryType': is not a member of 'itk::AdvancedTransform<TScalarType,3,3>' [D:\a\1\Elastix-build\Testing\elxInvertTransform.vcxproj]

At https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=825&view=logs&j=34a65711-f108-577e-6707-abb292f048dd&t=b76e394f-4286-5ca5-0a60-2e5c8021c924&l=74

thewtex · 2020-08-06T16:27:06Z

@N-Dekker awesome, thanks!

error C2039: 'TransformCategoryType'

This is now itk::TransformBaseTemplateEnums::TransformCategory

https://github.com/InsightSoftwareConsortium/ITK/blob/8edb8659da2c21fd9dc4c6b2037f7311943a7b04/Modules/Core/Transform/include/itkTransformBase.h#L39-L54

N-Dekker · 2020-08-20T14:10:17Z

@thewtex Sorry for letting you wait, I'm just back from vacation (heatwave)! Thank you very much for moving the commit from PR #247 to this one, PR #245. Are these two commits together sufficient for you to build elastix + ITK 5.1 + ITK_LEGACY_REMOVE? I just tried your PR branch (elxTransformBase-strongly-typed-enum), and I still get many compilation errors, like:

elxAdaptiveStochasticGradientDescent.cxx
[elastix-source]\Common\Transforms\itkAdvancedBSplineDeformableTransformBase.h(78,32): error C2039: 'TransformCategoryType': is not a member of 'itk::AdvancedTransform<double,2,2>'

See also https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=829&view=logs&j=34a65711-f108-577e-6707-abb292f048dd&t=b76e394f-4286-5ca5-0a60-2e5c8021c924&l=143

So it seems to me that there are still some errors to be fixed before this PR can be merged. Right? If so, do you still have time to make this PR ready? Or what is your suggestion?

N-Dekker · 2020-08-26T19:17:49Z

@thewtex I guess we see many more legacy-remove errors on the CI than the ones you already fixed, because the CI includes Elastix CMake options -DBUILD_TESTING=ON -DUSE_ALL_COMPONENTS=ON

ITK's `TransformBaseTemplate::TransformCategoryType` was removed with commit InsightSoftwareConsortium/ITK@33daf94 "ENH: Consistent use of typed enums, naming", by Matt McCormick, Dec 6, 2019. It was then defined "legacy" with commit InsightSoftwareConsortium/ITK@0775d4a "COMP: expose TransformCategoryType under LEGACY", by Dženan Zukić, Jan 7, 2020. This commit aims to pave the way for `ITK_LEGACY_REMOVE`, as in pull request #245 by Matt McCormick.

N-Dekker · 2020-08-28T19:34:44Z

FYI, I just did a rebase develop to get this PR back in sync with our develop branch. My intention is to do another rebase develop once pull request #269 is merged successfully.

Uses LoggerBaseEnums::TimeStampFormat, to address: > elastix/Common/OpenCL/ITKimprovements/itkOpenCLLogger.cxx:114:15: > error: no type named 'TimeStampFormatType' in 'itk::LoggerBase'; > did you mean 'TimeStampFormatEnum'? Uses ITK 5.1.0 strongly typed enums in elxTransformBase, to address: In file included from _deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.h:384:0, from _deps/elastix_fetch-src/Core/Kernel/elxElastixTemplate.h:39, from _deps/elastix_fetch-src/Core/Install/elxIncludes.h:39, from _deps/elastix_fetch-src/Components/FixedImagePyramids/FixedRecursivePyramid/elxFixedRecursivePyramid.h:21, from _deps/elastix_fetch-src/Components/FixedImagePyramids/FixedRecursivePyramid/elxFixedRecursivePyramid.cxx:19: _deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx: In member function ‘void itk::PixelTypeChangeCommand<T>::Execute(itk::Object*, const itk::EventObject&)’: _deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx:75:55: error: ‘VECTOR’ is not a member of ‘itk::ImageIOBase’ castcaller->GetModifiableImageIO()->SetPixelType( ImageIOBase::VECTOR ); ^ _deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx: In member function ‘void itk::PixelTypeChangeCommand<T>::Execute(const itk::Object*, const itk::EventObject&)’: _deps/elastix_fetch-src/Core/ComponentBaseClasses/elxTransformBase.hxx:83:55: error: ‘VECTOR’ is not a member of ‘itk::ImageIOBase’ castcaller->GetModifiableImageIO()->SetPixelType( ImageIOBase::VECTOR ); Replaced legacy TransformCategoryType, fixed use of StopConditionType, to address (from VS2017): [elastix-source]\Common\Transforms\itkAdvancedBSplineDeformableTransformBase.h(78,32): error C2039: 'TransformCategoryType': is not a member of 'itk::AdvancedTransform<double,2,2>' At https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=829&view=logs&j=34a65711-f108-577e-6707-abb292f048dd&t=b76e394f-4286-5ca5-0a60-2e5c8021c924&l=143 and: > elxSimultaneousPerturbation.h(113): error C3646: 'StopConditionType': unknown override specifier At https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=875&view=logs&j=34a65711-f108-577e-6707-abb292f048dd&t=b76e394f-4286-5ca5-0a60-2e5c8021c924&l=397

N-Dekker · 2020-08-31T12:44:02Z

@thewtex Hi Matt, I just combined your two commits and mine into a single commit, that I would like to merge to the develop branch. Can you please check if it's still OK to you? I would like to merge tomorrow morning 😃

N-Dekker · 2020-09-01T08:09:00Z

Thank you very much for your contribution @thewtex I'll merge now, I hope it will allow you to get ITKElastix back in sync with SuperElastix/elastix 😃

thewtex requested review from N-Dekker, mstaring and stefanklein April 30, 2020 01:09

N-Dekker requested changes Apr 30, 2020

View reviewed changes

thewtex force-pushed the elxTransformBase-strongly-typed-enum branch from 56c2ebe to 0cc710e Compare May 15, 2020 15:46

thewtex mentioned this pull request Jul 20, 2020

Getting the result image in the object oriented approach fails InsightSoftwareConsortium/ITKElastix#51

Closed

N-Dekker mentioned this pull request Aug 6, 2020

COMP: itkOpenCLLogger ITK 5.1 TimeStampFormat typed enum #247

Closed

thewtex force-pushed the elxTransformBase-strongly-typed-enum branch from 0cc710e to e7d6ccf Compare August 10, 2020 18:27

N-Dekker mentioned this pull request Aug 28, 2020

STYLE: Remove Superclass::TransformCategoryType (ITK legacy) #269

Closed

N-Dekker force-pushed the elxTransformBase-strongly-typed-enum branch from e7d6ccf to 1c8e3ca Compare August 28, 2020 19:29

N-Dekker force-pushed the elxTransformBase-strongly-typed-enum branch from bc23908 to 4f3a089 Compare August 29, 2020 09:36

N-Dekker force-pushed the elxTransformBase-strongly-typed-enum branch from 4f3a089 to 5e0c541 Compare August 31, 2020 12:40

N-Dekker merged commit 092d289 into SuperElastix:develop Sep 1, 2020

N-Dekker mentioned this pull request Sep 11, 2020

ENH: Switch to latest version of SuperElastix/elastix (using ITK 5.1.1) InsightSoftwareConsortium/ITKElastix#60

Merged

COMP: Use ITK 5.1.0 strongly typed enums in elxTransformBase #245

COMP: Use ITK 5.1.0 strongly typed enums in elxTransformBase #245

Uh oh!

Conversation

thewtex commented Apr 30, 2020

Uh oh!

N-Dekker left a comment

Choose a reason for hiding this comment

Uh oh!

N-Dekker commented Apr 30, 2020

Uh oh!

thewtex commented May 11, 2020

Uh oh!

N-Dekker commented May 12, 2020

Uh oh!

thewtex commented May 13, 2020

Uh oh!

N-Dekker commented May 15, 2020

Uh oh!

thewtex commented May 15, 2020

Uh oh!

N-Dekker commented May 15, 2020

Uh oh!

thewtex commented Jul 20, 2020

Uh oh!

N-Dekker commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewtex commented Aug 6, 2020

Uh oh!

N-Dekker commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

N-Dekker commented Aug 26, 2020

Uh oh!

N-Dekker commented Aug 28, 2020

Uh oh!

N-Dekker commented Aug 31, 2020

Uh oh!

N-Dekker commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

N-Dekker commented Aug 5, 2020 •

edited

Loading

N-Dekker commented Aug 20, 2020 •

edited

Loading