Skip to content

Refactor third-party Eigen3 CMake code#5831

Merged
blowekamp merged 3 commits intoInsightSoftwareConsortium:mainfrom
blowekamp:eigen_update
Mar 13, 2026
Merged

Refactor third-party Eigen3 CMake code#5831
blowekamp merged 3 commits intoInsightSoftwareConsortium:mainfrom
blowekamp:eigen_update

Conversation

@blowekamp
Copy link
Member

@blowekamp blowekamp commented Feb 24, 2026

Make Eigen3 a follow standard ITK 3rd party library conventions.

Removes Eigen3 as an external project. Converted to use ITK module macros to install and export the eigen_internal target. Removes the CMake code to install auxiliary cmake and configuration file for target. The target is now imported through ITKTargets, and not a separate export and configuration file.

Preserve Eigen3::Eigen project exports.

Updated ITK/eigen repo then used sub-tree merge script to update.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:ThirdParty Issues affecting the ThirdParty module labels Feb 24, 2026
@dzenanz
Copy link
Member

dzenanz commented Feb 24, 2026

Request review from Pablo once this is ready.

@hjmjohnson hjmjohnson requested a review from phcerdan February 25, 2026 14:27
@blowekamp
Copy link
Member Author

@seanm @hjmjohnson @phcerdan I know Eigen3 is used in ITKTotalVariations and itkSymmetricEigenAnalysis.h. I thought there was more interest in using Eigen in the ITK community. Are there other existing use cases?

@dzenanz
Copy link
Member

dzenanz commented Feb 25, 2026

There was a desire to replace VNL vectors, matrices, and other similar functions by the ones from Eigen. But we never had the resources to do that.

@phcerdan
Copy link
Contributor

If you are touching third-party code (eigen code), better to make those changes in our third-party clone (https://github.com/InsightSoftwareConsortium/eigen) rather than ITK proper. This approach is really forced in VTK (ghostflow-check-main checks that). In ITK we are more flexible, but still recommended.

You are destroying with this all the efforts to make ITK modules to be able to reuse Eigen as-is, but honestly, I think is the right call. Nobody is doing it, and the extra complexity doesn't pay off.

Given that we are not allowing this anymore, I would strongly suggest to mangle symbols (and header guards) for our internal eigen to avoid conflicts with external ones (other libraries that use Eigen externally). VTK does this in a clean way.

@phcerdan
Copy link
Contributor

phcerdan commented Feb 25, 2026

And yes, the idea was to take advantage of Eigen to replace VNL. I honestly think that codex/claude can nail this nowadays. It's more verbose than complex. A bit tricky, but our regressions tests should really help with this.

@blowekamp blowekamp changed the title WIP: Refactor third-party Eigen3 CMake code Refactor third-party Eigen3 CMake code Feb 25, 2026
@blowekamp
Copy link
Member Author

You are destroying with this all the efforts to make ITK modules to be able to reuse Eigen as-is, but honestly, I think is the right call. Nobody is doing it, and the extra complexity doesn't pay off.

With the current updates here and in TotalVariation and proxTV, I was able to build ITK with TotalVariation enabled. Then I build SimpleITK with a ProxTVImageFilter wrapping enable against ITK.

These changes work with both ITK's integrated Eigen, and system Eigen. In the cases of build SimpleITK against ITK build directory and ITK installed directory.

An ITK module can be built against a system Eigen through ITK by specifying ITK::ITKEigen3Module as the Eigen library to use/link against. This follows the pattern of all the other ITK third party libraries.

Are there other cases to consider?

@phcerdan
Copy link
Contributor

Good job modifying all the moving parts.
My only question here is in proxTV, the headers of Eigen there are not "mangled" with itk headers (itkeigen/core.h), they are just eigen/core.h

Can you still use the internal eigen of ITK (itkeigen/,..) there? Or do you need to force calling again a find_package(Eigen3)?

@github-actions github-actions bot added the area:Core Issues affecting the Core module label Mar 11, 2026
@blowekamp blowekamp mentioned this pull request Mar 12, 2026
7 tasks
@blowekamp
Copy link
Member Author

Are there other cases to consider?

The other case I tried was building the updated TotalVariation with wrapping enabled against an ITK build directory. There were a various target linking properties for the TotalVariation libraries that were not being propagated to wrapping. So the python wrapping was refactored to use target properties.

#5842

There is a new remote module being added which used Eigen3, so I'd like to get they PR in soon.

I also update this PR to use the standard "itk_Eigenvalues.h" headers. This is the way other vendored third party libraries are done. It works when a third party is a system or from the itk build system. It has the advantage that it prevents a system include ( or some other vendors packaged library). So I think this is a good approach.

My only question here is in proxTV, the headers of Eigen there are not "mangled" with itk headers (itkeigen/core.h), they are just eigen/core.h

Can you still use the internal eigen of ITK (itkeigen/,..) there? Or do you need to force calling again a find_package(Eigen3)?

Yes, proxTV is compiling with the changes. I had to enable passing an explicit CMake library for Eigen with the "ITK::ITKEigen3Module" target.

See phcerdan/proxTV@0270347

@blowekamp
Copy link
Member Author

Wrapping TotalVariation works with InsightSoftwareConsortium/ITKTotalVariation#57.

I enabled #5925 in an ITK build and that work too.

I pushed the CMake changes for eigen to https://github.com/InsightSoftwareConsortium/eigen/tree/itk_module_install and did ran the update script.

This PR should be about ready to go!

blowekamp and others added 3 commits March 12, 2026 20:35
Remove Eigen3 external project, use itk module macros to install and
export eigen_internal target. Remove the CMake code to install
auxiliary cmake and configuration file for target. This target is now
imported through ITKTargets, and not a separate export and
configuration file.

Preserve Eigen3::Eigen project exports.
Code extracted from:

    https://github.com/InsightSoftwareConsortium/eigen

at commit b581628ee0bf0af5bcf070c107b0ce1e25c50843 (itk_module_install).
# By Eigen Upstream
* upstream-Eigen3:
  Eigen3 2026-03-12 (b581628e)
@github-actions github-actions bot removed the area:Core Issues affecting the Core module label Mar 12, 2026
@blowekamp blowekamp marked this pull request as ready for review March 13, 2026 12:37
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

LGTM

@blowekamp
Copy link
Member Author

It would be good to get a couple more eyes on the change @phcerdan @thewtex

But I will like merge at the end of the day if there are no objections, as I don't want these changes to get stale or out of sync.

@blowekamp blowekamp dismissed phcerdan’s stale review March 13, 2026 20:22

Upstream has been updated and the sub-tree merge done

@blowekamp blowekamp merged commit 7ed3dab into InsightSoftwareConsortium:main Mar 13, 2026
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ThirdParty Issues affecting the ThirdParty module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants