ENH: Ingest ITKVariationalRegistration into Modules/Registration/VariationalRegistration#6276
Conversation
Updated CMakeLists.txt for tests and newer ITK style. Added input images and Baseline images for the tests.
…pacing in VariationalRegistrationNCCFunction and VariationalRegistrationSSDFunction. Remove option to return unscaled time step.
…ages of the 2D tests. Dont use image spacing for Gaussian regularization (unintended results).
Conflicts: src/VariationalRegistrationMain.cxx
Gaussian and Consider image spacing as default
github-actions
Bot
added
type:Infrastructure
|
@greptileai review this draft before I make it official |
1 similar comment
|
@greptileai review this draft before I make it official |
This comment was marked as resolved.
This comment was marked as resolved.
|
| Filename | Overview |
|---|---|
| Modules/Registration/VariationalRegistration/wrapping/itkVariationalDiffeomophicRegistrationFilter.wrap | Wrap file registers itk::VariationalDiffeomophicRegistrationFilter (typo — missing 'r') instead of the real class itk::VariationalDiffeomorphicRegistrationFilter; Python wrapping build will fail. |
| Modules/Registration/VariationalRegistration/src/CMakeLists.txt | Uses deprecated COMPILE_FLAGS property and directory-wide include_directories instead of modern target_compile_definitions / target_include_directories; otherwise correctly builds the 2D/3D executable pair. |
| Modules/Registration/VariationalRegistration/src/win32_compatibility/getopt.c | BSD 4-clause ("advertising materials") license; adds an obligation beyond Apache 2.0 that downstream consumers may not be aware of. |
| Modules/Registration/VariationalRegistration/src/itkVariationalRegistrationIncludeRequiredIOFactories.h | Defines RegisterRequiredFactories() in a header; safe only while included by a single TU, but an ODR hazard if the include list grows. |
| Modules/Registration/VariationalRegistration/itk-module.cmake | Correctly declares dependencies; IO modules in DEPENDS (not PRIVATE_DEPENDS) are required since there is no compiled library — only executables. |
| Modules/Registration/VariationalRegistration/wrapping/CMakeLists.txt | Lists all wrap submodules in the correct order; the two Diffeomophic filenames match the (misspelled) physical files, so this file is internally consistent but will fail once the wrap file is corrected. |
| Modules/Remote/VariationalRegistration.remote.cmake | Remote module stub correctly removed as part of the ingest into Modules/Registration/. |
| pyproject.toml | Adds -DModule_VariationalRegistration:BOOL=ON to the CI CMake configure command to enable the newly ingested module in CI builds. |
| Modules/Registration/VariationalRegistration/test/CMakeLists.txt | Comprehensive test suite covering 2D and 3D registration variants; contains dead commented-out code referencing a defunct HTTP URL. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class VariationalRegistrationFilter {
+SetFixedImage()
+SetMovingImage()
+Update()
}
class VariationalDiffeomorphicRegistrationFilter {
+SetNumberOfExponentiatorIterations()
}
class VariationalSymmetricDiffeomorphicRegistrationFilter
class VariationalRegistrationMultiResolutionFilter {
+SetNumberOfLevels()
}
class VariationalRegistrationFunction {
<<abstract>>
+ComputeUpdate()
}
class VariationalRegistrationDemonsFunction
class VariationalRegistrationSSDFunction
class VariationalRegistrationNCCFunction
class VariationalRegistrationFastNCCFunction
class VariationalRegistrationRegularizer {
<<abstract>>
+RegularizeField()
}
class VariationalRegistrationGaussianRegularizer
class VariationalRegistrationDiffusionRegularizer
class VariationalRegistrationElasticRegularizer
class VariationalRegistrationCurvatureRegularizer
class VariationalRegistrationStopCriterion
class VariationalRegistrationLogger
VariationalRegistrationFilter <|-- VariationalDiffeomorphicRegistrationFilter
VariationalDiffeomorphicRegistrationFilter <|-- VariationalSymmetricDiffeomorphicRegistrationFilter
VariationalRegistrationMultiResolutionFilter --> VariationalRegistrationFilter : uses
VariationalRegistrationFilter --> VariationalRegistrationFunction : uses
VariationalRegistrationFilter --> VariationalRegistrationRegularizer : uses
VariationalRegistrationFilter --> VariationalRegistrationStopCriterion : uses
VariationalRegistrationFilter --> VariationalRegistrationLogger : uses
VariationalRegistrationFunction <|-- VariationalRegistrationDemonsFunction
VariationalRegistrationFunction <|-- VariationalRegistrationSSDFunction
VariationalRegistrationFunction <|-- VariationalRegistrationNCCFunction
VariationalRegistrationNCCFunction <|-- VariationalRegistrationFastNCCFunction
VariationalRegistrationRegularizer <|-- VariationalRegistrationGaussianRegularizer
VariationalRegistrationRegularizer <|-- VariationalRegistrationDiffusionRegularizer
VariationalRegistrationRegularizer <|-- VariationalRegistrationElasticRegularizer
VariationalRegistrationRegularizer <|-- VariationalRegistrationCurvatureRegularizer
Comments Outside Diff (6)
-
Modules/Registration/VariationalRegistration/wrapping/itkVariationalDiffeomophicRegistrationFilter.wrap, line 1 (link)Wrong class name will break Python wrapping
The class name passed to
itk_wrap_classisitk::VariationalDiffeomophicRegistrationFilter(note "Diffeomophic" — missing the 'r'), but the actual C++ class defined initkVariationalDiffeomorphicRegistrationFilter.hisitk::VariationalDiffeomorphicRegistrationFilter. When Python wrapping is enabled, SWIG will attempt to instantiate a class that does not exist, producing a hard build error. -
Modules/Registration/VariationalRegistration/src/CMakeLists.txt, line 40-45 (link)Using the deprecated
COMPILE_FLAGStarget property prevents per-config handling and causes a CMake deprecation warning. The modern equivalent istarget_compile_definitions. -
Modules/Registration/VariationalRegistration/src/CMakeLists.txt, line 1 (link)include_directoriesis a directory-wide command that leaks into every target defined afterward in the same scope.target_include_directoriesscoped to the two executables is the idiomatic modern-CMake approach. -
Modules/Registration/VariationalRegistration/src/win32_compatibility/getopt.c, line 1-22 (link)BSD 4-clause "advertising" license may create an obligation beyond Apache 2.0
getopt.ccarries the original BSD 4-clause license (University of California, Berkeley), which requires advertising materials to display an acknowledgement.getopt.hcarries the NetBSD Foundation's BSD 4-clause with the same clause. ITK is Apache 2.0 licensed; the advertising requirement adds a disclosure obligation that end-consumers of ITK may not be aware of. Consider replacing these files with a BSD-2-clause–licensed implementation or using a CMakecheck_function_exists(getopt)guard so the bundled copy is only compiled when truly unavailable. -
Modules/Registration/VariationalRegistration/src/itkVariationalRegistrationIncludeRequiredIOFactories.h, line 34-47 (link)Function definition in a header will cause ODR violations if included in multiple TUs
RegisterRequiredFactories()is a non-inline, non-template function defined in a.hfile. If this header is ever included in more than one translation unit in the same link step, the linker will error with a multiple-definition violation. Currently only included byVariationalRegistrationMain.cxx, so not a current build failure, but it is a maintenance hazard. The definition should be moved to a.cxxfile or the function markedinline. -
Modules/Registration/VariationalRegistration/test/CMakeLists.txt, line 9-11 (link)Dead commented-out code referencing a broken external URL
These two lines are commented out with the note "both approaches do not work" and reference an HTTP endpoint that appears to no longer be maintained. Removing these lines would make the intent clearer.
Reviews (1): Last reviewed commit: "COMP: Enable VariationalRegistration mod..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The wrap files referenced itk::VariationalDiffeomophicRegistrationFilter (missing 'r') which would fail Python wrapping compilation. Rename files and class reference to match the actual C++ class name VariationalDiffeomorphicRegistrationFilter.
The function was defined in a header without inline, which would trigger a multiple-definition linker error if the header is ever included in more than one translation unit.
Replace directory-scoped include_directories with target-scoped target_include_directories, add the missing module include/ path so the example executable finds module headers, and replace the deprecated COMPILE_FLAGS target property with target_compile_definitions for USE_2D_IMPL.
The ten ITKIO* modules are required only by the standalone example executable's RegisterRequiredFactories(), not by the header-only library. Moving them to PRIVATE_DEPENDS prevents every downstream consumer from being forced to link all ten IO backends transitively.
|
/azp run |
The IPFS CIDs for VariationalRegistrationElastic2DTest.tif and VariationalRegistrationCurvature2DTest.tif (bafkrei...la and bafkrei...s4) are not retrievable through any IPFS gateway (dweb.link, ipfs.io, w3s.link all return 504) and the Populate ExternalData cache job rejects partial caches. Replace the .cid content links with .sha512 content links matching the upstream ITKVariationalRegistration test baselines; the blobs are present on data.kitware.com and resolve through ITK's existing sha512 ExternalData URL template.
The example executables (VariationalRegistration and
VariationalRegistration2D) previously linked against
${VariationalRegistration_LIBRARIES}, which is never populated
because the module exposes no compiled library sources of its own.
This caused undefined references to itk::DataObjectError and
itk::InvalidRequestedRegionError at link time on macOS / Linux /
Windows Pixi-Cxx builds.
Enumerate the module DEPENDS / PRIVATE_DEPENDS targets directly so
the example resolves itk::Exception and image IO symbols
deterministically from inside the ITK source tree.
|
/azp run |
hjmjohnson
force-pushed
the
ingest-VariationalRegistration
branch
from
1d98ff1 to
5745085
Compare
|
Rewrote history with |
|
/azp run |
- itk-module.cmake: replace placeholder DESCRIPTION with substantive one-liner describing the demons / diffeomorphic / curvature-based variational registration filters with multi-resolution support. - src/CMakeLists.txt: use CMAKE_CURRENT_LIST_DIR instead of CMAKE_CURRENT_SOURCE_DIR for example-executable target_include_directories (and the WIN32 win32_compatibility branch). CMAKE_CURRENT_LIST_DIR is the directory of the currently-processed CMakeLists file, which is the more robust idiom inside an included subdirectory.
|
/azp run |
…ink list ITKImageFilterBase, ITKFiniteDifference, ITKDisplacementField, ITKRegistrationCommon, and ITKBinaryMathematicalMorphology are header-only ITK modules (no src/ directory, no compiled library). Passing them to target_link_libraries caused CMake to emit bare `-lITKImageFilterBase` etc. on the link line, which the linker could not resolve: ld: library not found for -lITKImageFilterBase The headers from these modules remain available via the module DEPENDS include-path propagation, so dropping them from the example link list is sufficient to fix macOS / Linux Pixi-Cxx builds without losing any compile-time access.
|
/azp run |
|
@greptileai review |
11 participants
Ingest the ITKVariationalRegistration remote module into
Modules/Registration/VariationalRegistration(group: Registration). Source upstream:InsightSoftwareConsortium/ITKVariationalRegistration. Tracking issue: #6160.Ingest stats
Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md)Modules/Registration/VariationalRegistration/: 824f2adde59d738dd57f6ea32b9ee3eef9a40b31f5External-data fixtures
CID / .sha512 content-links present in this ingest:
If any fixtures resolve via ITKTestingData, that repo must contain the matching content-link before this PR can merge cleanly.
Follow-on commits (on top of the unrelated-histories merge)