ENH: Update vendored Eigen3 to 5.0.1+master; verify USE_SYSTEM 3.3/3.4/5.x#6176
Conversation
github-actions
Bot
added
the
type:Enhancement
|
@greptile review |
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
hjmjohnson
force-pushed
the
eigen-5.0.1-update
branch
from
a3699e0 to
6fbd1d6
Compare
hjmjohnson
changed the title
hjmjohnson
force-pushed
the
eigen-5.0.1-update
branch
from
6fbd1d6 to
016f9ac
Compare
This comment was marked as resolved.
This comment was marked as resolved.
hjmjohnson
force-pushed
the
eigen-5.0.1-update
branch
2 times, most recently
from
56a7240 to
eda5a20
Compare
|
/azp run ITK.Windows |
This comment was marked as resolved.
This comment was marked as resolved.
|
/azp run ITK.Windows |
|
Too many files changed for review. ( |
|
/azp run ITK.Windows |
blowekamp
left a comment
blowekamp
left a comment
There was a problem hiding this comment.
Looking good.
Thank you for updating the library. After I refactored from an off fetch_content in the root of the project to an itk standard third party library I ran out of steam to upgrade.
There were some rather odd usages of Eigen in external modules. The intention moving forward is just have it behave just like a normal ITK interface library without special cmake code.
hjmjohnson
force-pushed
the
eigen-5.0.1-update
branch
from
e7a197c to
be77e85
Compare
|
/azp run ITK.Windows |
Phase 2 prep for Eigen 5.x update. Points UpdateFromUpstream.sh at the
refreshed InsightSoftwareConsortium/eigen tag derived from gitlab
libeigen/eigen master tip 879885e (2026-05-01), which adds the
"Modernize internal utilities for C++14" patch (libeigen/eigen!2490)
on top of the 5.0.1 release tip plus three small follow-on fixes
(RealQZ pushDownZero counting, small-determinant LU fastpath, and
IncompleteLUT static row matching).
Adds new top-level support headers introduced since the previous import:
AccelerateSupport, KLUSupport, ThreadPool, and Version. The Version
header is required because Eigen 5 moved EIGEN_{MAJOR,MINOR,PATCH}_VERSION
out of src/Core/util/Macros.h.
Code extracted from:
https://github.com/InsightSoftwareConsortium/eigen
at commit 505023a2a0fc8eeff5f92d07683ab5ff0c03ba0f (for/itk-20260501-879885e1).
* upstream-Eigen3: Eigen3 2026-05-01 (505023a2) # Conflicts: # Modules/ThirdParty/Eigen3/src/itkeigen/Eigen/src/Core/products/SelfadjointMatrixVector.h
The warning is purely from the Eigen master post-5.0.1 update (EIGEN_VERSION_STRING="5.0.1-dev+master") changing instantiation/ inlining. Eigen's own CMakeLists.txt already adds -Wno-maybe-uninitialized for its own builds (line 444, with a comment that GCC 12+ emits false positives), and DisableStupidWarnings.h doesn't cover this one. The fix is to suppress at the ITK consumer site, mirroring Eigen's stance.
- Restore @EIGEN3_TARGETS_FILE@ substitution in Eigen3Config.cmake.in. - Remove duplicate option(ITK_USE_EIGEN_MPL2_ONLY) declaration. - Remove commented-out binary-dir generator expression. USE_SYSTEM_EIGEN for 3.3+ unaffected (all under itkeigen, only configured when USE_SYSTEM_EIGEN=OFF).
Bump line 2 from 3.10.2 to 3.16.3 to match ITK proper, and replace the if(FALSE) ... endif() wrapper around the upstream Eigen logic with #[[ ... #]] CMake multiline-comment markers so IDEs render the disabled region as dimmed dead code.
Tridiagonalization.h emits C4750 ('function with _alloca() inlined into
a loop') under MSVC whenever an ITK consumer instantiates the symmetric
eigen-decomposition path. The warning is benign (Eigen's intentional
small-allocation alloca path) but cannot be addressed inside ITK and
fails CDash because the dashboard treats any warning as fatal.
The previous attempt to set CMAKE_CXX_FLAGS in the vendored Eigen
CMakeLists was a no-op (Eigen is header-only, so no .cxx file picks up
the flag) and the alternative -- attaching /wd4750 to the eigen_internal
INTERFACE library -- would propagate to ITKCommon (which DEPENDS on
ITKEigen3) and from there to most of the toolkit, suppressing C4750
for non-Eigen ITK code too. Both rejected per @blowekamp.
Instead, extend the existing 'Modules/ThirdParty/Eigen3/.*warning:.*'
CTestCustom regex with a sibling that matches MSVC's
'warning C####:' form. Compiler still emits the warning so reviewers
see it in build logs, but the dashboard ignores it for warning-count
purposes -- exactly the same scope the GCC/Clang side has had since
this exception was added.
hjmjohnson
force-pushed
the
eigen-5.0.1-update
branch
from
be77e85 to
76d09b8
Compare
# Conflicts: # CMake/CTestCustom.cmake.in
|
/azp run ITK.Linux.Python |
|
@phcerdan Do you have any suggestions for how this PR can be better instrumented? |
|
It's looking good to me @hjmjohnson, you performed all the needed steps for updating a remote. @blowekamp might be more up-to-date after all the work he did on standardizing eigen. |
@blowekamp I am uncertain if your previous comment is requesting changes to this PR, or indicating work that was already done. If this PR is OK as is, please approve and merge. If there are additions, please indicate if they should be on this PR, or a followup and what is needed. Thank you, Hans |
|
The TVProx was one remote module I spent some time on refactoring to work with the refactored Eigen and the updated ITK CMake. There is still an outstanding PR regarding these changes. The CI could not be made green due to the changes with Eigan being after the most recent tag: Eigan is use more by remote modules than internal, which should be an important part of testing. There may be other remote modules that may still have issues with the previous updates. This is all to say that Eigan usage is rough around the edges, and needs some extra checks. There were options related to what code was included and compile time defines that I could not make sense of too. Also, the ITK on conda-forge is using the latest eigen with out problems, but that usage is minimum, |
|
Deep-dive verified PR #6176 builds cleanly end-to-end (full ITK 6156/6156 targets) and exports a correctly-configured vendored Eigen 5; smoke-tested via The deep dive did surface one pre-existing ITK architectural detail that affects InsightSoftwareConsortium/ITKTotalVariation#57's modern-target-interfaces refactor (proxTV's What was tested
Architectural finding — pre-existing in ITK, exposed by ITKTotalVariation#57ITK ships its vendored Eigen3 headers at: ITK's own internal code uses a special macro form
set(ITKEigen3_INCLUDE_DIRS ${ITKEigen3_SOURCE_DIR}/src)This single path then flows into # /tmp/itk-pr6176-build/ITKTargets.cmake:99-107
set_target_properties(ITK::eigen_internal PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES ".../Modules/ThirdParty/Eigen3/src/itkeigen/..") # → .../Eigen3/src
set_target_properties(ITK::ITKEigen3Module PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES ".../Eigen3/src;.../Modules/ThirdParty/Eigen3/src"
INTERFACE_LINK_LIBRARIES "ITK::eigen_internal")External consumers that legitimately use the upstream-Eigen convention The This is not introduced by PR #6176 — Proposed resolution — separate follow-up PRSingle-file, one-block change to - set(ITKEigen3_INCLUDE_DIRS ${ITKEigen3_SOURCE_DIR}/src)
+ set(
+ ITKEigen3_INCLUDE_DIRS
+ ${ITKEigen3_SOURCE_DIR}/src
+ ${ITKEigen3_SOURCE_DIR}/src/itkeigen
+ )Both paths flow into After this lands:
Branch: What this PR (#6176) does not need to changeNothing. The findings above are independent of the Eigen 5 vendored update; the include-dir omission predates this PR by years and would surface against ITKTotalVariation#57 regardless of which Eigen version is vendored. PR #6176 is verified ready on its own merits — vendored Eigen 5 builds clean, exports the correct |
hjmjohnson
merged commit f11b2cb
into
InsightSoftwareConsortium:main
…onsumers
ITK ships its vendored Eigen3 headers at
Modules/ThirdParty/Eigen3/src/itkeigen/Eigen/<header>
ITK code accesses these via the ITK_EIGEN(<header>) macro
defined in itk_eigen.h, which expands to
<itkeigen/Eigen/<header>>
When ITK_USE_SYSTEM_EIGEN=OFF, the prior ITKEigen3_INCLUDE_DIRS
contained only ${ITKEigen3_SOURCE_DIR}/src — sufficient for the
macro form (the compiler then appends /itkeigen/Eigen/<header>) but
not for the upstream Eigen convention <Eigen/<header>>, which needs
${ITKEigen3_SOURCE_DIR}/src/itkeigen on the include path.
External consumers that legitimately use the upstream pattern hit
'fatal error: Eigen/Dense: No such file or directory' as soon as
they link against ITK::ITKEigen3Module or ITK::eigen_internal.
This blocks the modern-target-interfaces refactor in
InsightSoftwareConsortium/ITKTotalVariation#57: proxTV's
lapackFunctionsWrap.cpp uses #include <Eigen/Dense> and so cannot
build against the vendored Eigen via either of ITK's exported
imported targets.
Add the itkeigen subdirectory to ITKEigen3_INCLUDE_DIRS so both
include conventions resolve. ITK's own consumers continue to find
<itkeigen/Eigen/<header>> via the first entry; external consumers
now find <Eigen/<header>> via the second. Both entries flow into
INTERFACE_INCLUDE_DIRECTORIES of the IMPORTED ITK::ITKEigen3Module
target, so the fix applies in-tree and to find_package(ITK)
consumers alike.
The Eigen3::Eigen IMPORTED target produced by Eigen's own export
already exposes the correct path, but it is in-source-scope only
(declared as add_library(Eigen3::Eigen ALIAS eigen)) and therefore
invisible to FetchContent sub-builds; relying on it is not an
option for downstream remote modules that pull proxTV via
FetchContent.
This change is independent of the Eigen 5 vendored update in
PR #6176; the architectural mismatch predates that PR and applies
to upstream/main as-is.
|
This MR broke my build: Bisect results: There are only 'skip'ped commits left to test. |
6 participants
Vendors Eigen master post-5.0.1 (gitlab
libeigen/eigentipfef1460e, late April 2026;EIGEN_VERSION_STRING="5.0.1-dev+master") via the canonicalUpdateFromUpstream.shflow, and demonstratesITK_USE_SYSTEM_EIGEN=ONagainst system Eigen 3.3.9, 3.4.0, and 5.0.1. The 3.3 minimum-version gate is preserved.Acceptance matrix (build + Eigen-related tests)
5a7cc61d6(Eigen 5.0.1+master)eigen=3.3.9(proves min-version gate)eigen=3.4.0eigen=5.0.1The 11 tests:
itkSymmetricEigenAnalysisTest,itkEigenAnalysis2DImageFilterTest,itkSymmetricEigenAnalysisImageFilterTest{OrderByValue,OrderByMagnitude,DoNotOrder},vnl_algo_test_{complex,generalized,real,symmetric}_eigensystem,ITKEigenKWStyleTest,ITKEigenInDoxygenGroup.What changed in the vendored tree
UpdateFromUpstream.shtag bumped to the new ISC fork tag (suffix-fef1460ek). Whitelist gainedAccelerateSupport,KLUSupport,ThreadPool,Version(theEigen/Versionheader is required because Eigen 5 movedEIGEN_{MAJOR,MINOR,PATCH}_VERSIONout ofsrc/Core/util/Macros.h).src/itkeigen/CMakeLists.txtversion parser was rewritten on the fork (be4bdb732) to readEigen/Version(semverMAJOR.MINOR.PATCH) for Eigen 5+, falling back toMacros.h(WORLD.MAJOR.MINOR) for legacy 3.4.x.Eigen3ConfigVersion.cmakenow reportsPACKAGE_VERSION "5.0.1".Eigen/src/Core/products/SelfadjointMatrixVector.h: Eigen 5 fully rewrote that kernel into 4-col / 2-col / 1-col phases, already initializing accumulators withpzero(Packet{}). Resolved by taking upstream entirely — the older in-tree COMP patch91f6232("pzero(Packet{}) for SelfadjointMatrixVector accumulators") is subsumed and obsolete.ISC fork (InsightSoftwareConsortium/eigen) state
5a7cc61d6.for/itk-masteradvanced to5a7cc61d6.fef1460e: CMake override (be4bdb732),.gitattributeslapacke relax (fb7121abd), andREADME.kitware.md(5a7cc61d6).Outer-module CMake
Modules/ThirdParty/Eigen3/CMakeLists.txtkeeps_Eigen3_min_version=3.3(no upper bound).find_package(Eigen3 REQUIRED CONFIG)plus the explicitVERSION_LESScheck accepts both 3.x and 5.x system installs without modification.