Skip to content

ENH: Ingest ITKSimpleITKFilters into Modules/Filtering/SimpleITKFilters#6277

Closed
hjmjohnson wants to merge 84 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:ingest-SimpleITKFilters
Closed

ENH: Ingest ITKSimpleITKFilters into Modules/Filtering/SimpleITKFilters#6277
hjmjohnson wants to merge 84 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:ingest-SimpleITKFilters

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Ingest the ITKSimpleITKFilters remote module into Modules/Filtering/SimpleITKFilters (group: Filtering). Source upstream: SimpleITK/ITKSimpleITKFilters. Tracking issue: #6160.

Ingest stats
  • Commits: 79 (including 13 merge commits -- Mode A topology preserved per Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md)
  • Files installed under Modules/Filtering/SimpleITKFilters/: 33
  • Approx. content size: 128 KiB
  • Tip SHA: c1f18fdde73398a4e038f2b2035c0f9738f964bf
External-data fixtures

CID / .sha512 content-links present in this ingest:

  • Modules/Filtering/SimpleITKFilters/test/Baseline/ObjectnessMeasureImageFilterTest1.nii.cid
  • Modules/Filtering/SimpleITKFilters/test/Baseline/ObjectnessMeasureImageFilterTest2.nii.cid
  • Modules/Filtering/SimpleITKFilters/test/Baseline/itkHessianToObjectnessMeasureImageFilterTest2.mha.cid
  • Modules/Filtering/SimpleITKFilters/test/Input/DSA.png.cid
  • Modules/Filtering/SimpleITKFilters/test/Input/itkHessianToObjectnessMeasureImageFilterTest.mha.cid

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)
  • c1f18fd ENH: Enable Module_SimpleITKFilters and remove remote stub
  • f4118d7 ENH: Convert from md5 to .cid tags.
  • 410f9c3 ENH: ITKv5_CONST macro for VerifyPreconditions() and VerifyInputInformation()
  • f3d3d1f COMP: Use modern macro for name of class
  • 33fadfb ENH: Wrap DICOMOrientation

blowekamp and others added 30 commits October 20, 2017 16:16
This repo is based on the ITKModuleTemplate
92c68b187dd1664961102dc02c9f116a64b9df2a
These test reuse the test and baseline images from HessianToObjectness
filter. The over all image looks very similar but there are a number
of pixels (~7000) which are different. This is likely due to the
difference between the discrete derivative and utilizing the recursive
Gaussian derivatives.
This addresses the following configuration error:
Target "SimpleITKFiltersGTestDriver" links to target "GTest::GTest"
but the target was not found.

This occurs when the default modules are explicitly not enabled.
This enables this module to be built alone as opposed to as part of
ITK.
Provide initial conversion of features to ITKv5.
Provide remove virtual and override

Use clang-tidy to add ITK_OVERRIDE, and to remove
redundant virtual on functions.

cd ../ITK;
clang-tidy -p ITK-clangtidy $find Modules/[A-J]*  -name *.cxx |fgrep -v ThirdParty) -checks=-*,modernize-use-override  -header-filter=.* -fix
clang-tidy -p ITK-clangtidy $(find Modules/[K-Z]*  -name *.cxx |fgrep -v ThirdParty) -checks=-*,modernize-use-override  -header-filter=.* -fix

https://stackoverflow.com/questions/39932391/virtual-override-or-both-c

When you override a function you don't technically need to write either virtual
or override.

The original base class declaration needs the keyword virtual to mark it as virtual.

In the derived class the function is virtual by way of having the ¹same type as
the base class function.

However, an override can help avoid bugs by producing a compilation error when
the intended override isn't technically an override. E.g. that the function
type isn't exactly like the base class function. Or that a maintenance of the
base class changes that function's type, e.g. adding a defaulted argument.

In the same way, a virtual keyword in the derived class can make such a bug
more subtle, by ensuring that the function is still is virtual in further
derived classes.

So the general advice is,

virtual for the base class function declaration.
This is technically necessary.

Use override (only) for a derived class' override.
This helps with maintenance.
git grep -l "ITK_OVERRIDE" |   fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake |   xargs sed -i '' -e "s/ITK_OVERRIDE/override/g"
Provide initial conversion of features to ITKv5.
Provide remove virtual and override

Use clang-tidy to add ITK_OVERRIDE, and to remove
redundant virtual on functions.

cd ../ITK;
clang-tidy -p ITK-clangtidy $find Modules/[A-J]*  -name *.cxx |fgrep -v ThirdParty) -checks=-*,modernize-use-override  -header-filter=.* -fix
clang-tidy -p ITK-clangtidy $(find Modules/[K-Z]*  -name *.cxx |fgrep -v ThirdParty) -checks=-*,modernize-use-override  -header-filter=.* -fix

https://stackoverflow.com/questions/39932391/virtual-override-or-both-c

When you override a function you don't technically need to write either virtual
or override.

The original base class declaration needs the keyword virtual to mark it as virtual.

In the derived class the function is virtual by way of having the ¹same type as
the base class function.

However, an override can help avoid bugs by producing a compilation error when
the intended override isn't technically an override. E.g. that the function
type isn't exactly like the base class function. Or that a maintenance of the
base class changes that function's type, e.g. adding a defaulted argument.

In the same way, a virtual keyword in the derived class can make such a bug
more subtle, by ensuring that the function is still is virtual in further
derived classes.

So the general advice is,

virtual for the base class function declaration.
This is technically necessary.

Use override (only) for a derived class' override.
This helps with maintenance.
git grep -l "ITK_OVERRIDE" |   fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake |   xargs sed -i '' -e "s/ITK_OVERRIDE/override/g"
ENH: Providing ITKv5 updates for C++11
Use static constexpr directly now that C++11 conformance
is required by all compilers.

:%s/itkStaticConstMacro *( *\([^,]*\),[ \_s]*\([^,]*\),\_s*\([^)]*\)) */static constexpr \2 \1 = \3/ge
== http://en.cppreference.com/w/cpp/language/type_alias ==

Type alias is a name that refers to a previously defined type (similar
to typedef).

A type alias declaration introduces a name which can be used as a
synonym for the type denoted by type-id. It does not introduce a new
type and it cannot change the meaning of an existing type name. There is
no difference between a type alias declaration and typedef declaration.
This declaration may appear in block scope, class scope, or namespace
scope.

== https://www.quora.com/Is-using-typedef-in-C++-considered-a-bad-practice ==

While typedef is still available for backward compatibility, the new
Type Alias syntax 'using Alias = ExistingLongName;' is more consistent
with the flow of C++ than the old typedef syntax 'typedef
ExistingLongName Alias;', and it also works for templates (Type alias,
alias template (since C++11)), so leftover 'typedef' aliases will differ
in style from any alias templates.
Use constexpr for constant numeric literals.
ENH: Providing ITKv5 updates for C++11
Move `ITK_DISALLOW_COPY_AND_ASSIGN` calls to public section following
the discussion in
https://discourse.itk.org/t/noncopyable

If legacy (pre-macro) copy and assing methods existed, subsitute them
for the `ITK_DISALLOW_COPY_AND_ASSIGN` macro.
…ToPublicS

ection

COMP: Move ITK_DISALLOW_COPY_AND_ASSIGN calls to public section.
Use itk::SimpleFilterWatcher instead of itk::FilterWatcher.

The itk::FilterWatcher class was removed in favour of
itk::SimpleFilterWatcher (and thus, its header file named
itkFilterWatcher.h was deleted) in this gerrit topic:
http://review.source.kitware.com/#/c/23415/

This bug was identified thanks to the following gerrit topic:
http://review.source.kitware.com/#/c/23428/
Fix errors steming from the use of the new `itk::PoolMultiThreader`
class for multi-threading.

The errors stemmed when this gerrit topic got merged:
http://review.source.kitware.com/#/c/23175/

And were later related to the following gerrit-topic:
http://review.source.kitware.com/#/c/23434/

The solution adopted in this patch set was based on the proposal in:
http://review.source.kitware.com/#/c/23439/
The source implementation and the Insight Journal paper can be found:
https://github.com/blowekamp/itkSuperPixel

The code has been update to work with C++03
blowekamp and others added 6 commits December 12, 2022 08:26
…rap_dicom_orientation

Wrap DICOMOrientation
When preparing for the future with ITK by setting
ITK_FUTURE_LEGACY_REMOVE:BOOL=ON
ITK_LEGACY_REMOVEBOOL=ON

The future preferred macro should be used
│ -  itkTypeMacro
│ +  itkOverrideGetNameOfClassMacro
…mation()

ITKv5_CONST enables backwards compatible behavior when ITKV4_COMPATIBILITY
is turned ON for methods which have acquired 'const' qualifier in ITKv5.
Breaking changes were originally introduced by ITK  commits
3e6b6f5 (on 2018-10-18) and
16eae15 (on 2018-10-23).
Brings SimpleITKFilters from a configure-time remote fetch into the ITK
source tree at Modules/Filtering/SimpleITKFilters/ using the v4 ingestion
pipeline (whitelist filter-repo + per-commit clang-format + black +
commit-prefix sanitization).

Upstream repo:  https://github.com/InsightSoftwareConsortium/ITKSimpleITKFilters.git
Upstream tip:   1e65004cebc51ed96d18bc05271f5c2ccd40dde1
Ingest date:    2026-05-14
Whitelist:      default.list

Per-commit transforms applied across all 77 commits:
  - filter-repo --paths-from-file (whitelist)
  - filter-repo --to-subdirectory-filter Modules/Filtering/SimpleITKFilters
  - clang-format -style=file (ITK main's .clang-format) for *.cxx/.h/.hxx/...
  - black for *.py
  - heuristic ITK prefix added to commit subjects without one

Merge topology preserved: 20 -> 12 merge(s).

Primary author: Bradley Lowekamp <blowekamp@mail.nih.gov>

Co-authored-by: Hans J. Johnson <hans-johnson@uiowa.edu>
Co-authored-by: Hans Johnson <hans-johnson@uiowa.edu>
Co-authored-by: Hans Johnson <hans.j.johnson@gmail.com>
Co-authored-by: Jon Haitz Legarreta <jhlegarreta@vicomtech.org>
Co-authored-by: Jon Haitz Legarreta Gorroño <jhlegarreta@vicomtech.org>
Co-authored-by: Mathew Seng <mathewseng@gmail.com>
Co-authored-by: Tom Birdsong <tom.birdsong@kitware.com>
Remove Modules/Remote/SimpleITKFilters.remote.cmake now that the
module is ingested at Modules/Filtering/SimpleITKFilters, and enable
Module_SimpleITKFilters in the configure-ci task.

Includes post-ingest formatting fixes (gersemi, clang-format) caught
by pre-commit against the rewritten history's tip tree.
@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:Remotes Issues affecting the Remote module type:Data Changes to testing data labels May 14, 2026
@hjmjohnson
Copy link
Copy Markdown
Member Author

@greptileai review this draft before I make it official

1 similar comment
@hjmjohnson
Copy link
Copy Markdown
Member Author

@greptileai review this draft before I make it official

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR ingests the ITKSimpleITKFilters remote module into Modules/Filtering/SimpleITKFilters, replacing the remote stub (Modules/Remote/SimpleITKFilters.remote.cmake) with the full in-tree source. Five filters are brought in: DICOMOrientImageFilter, DICOMOrientation, HessianImageFilter, MaskedAssignImageFilter, NPasteImageFilter, and ObjectnessMeasureImageFilter, along with tests, Python wrapping stubs, and CI enablement in pyproject.toml.

  • The remote cmake stub is correctly deleted and Module_SimpleITKFilters is enabled in CI via pyproject.toml.
  • itkNPasteImageFilterGTest.cxx is added to the repo but omitted from the SimpleITKFiltersGTest CMake source list, so its tests are never compiled or run.
  • The #ifndef ITK_MANUAL_INSTANTIATION / #include \".hxx\" block in itkMaskedAssignImageFilter.h is placed after the closing #endif of the header include guard, contrary to the pattern used in all other filters in this module.

Confidence Score: 3/5

The module ingest is broadly correct, but the NPaste GTests are silently excluded from the build, meaning a whole filter's test suite goes unexercised on CI.

The NPasteImageFilter GTest file exists in the repo but is absent from the CMake driver list — every test in that file is dead on CI. The include-guard misplacement in itkMaskedAssignImageFilter.h is a style deviation with no runtime impact. Together these reduce confidence that the module is fully exercised.

Modules/Filtering/SimpleITKFilters/test/CMakeLists.txt (missing NPaste GTest source) and Modules/Filtering/SimpleITKFilters/include/itkMaskedAssignImageFilter.h (include guard ordering).

Important Files Changed

Filename Overview
Modules/Filtering/SimpleITKFilters/test/CMakeLists.txt itkNPasteImageFilterGTest.cxx is present in the repo but missing from the SimpleITKFiltersGTest list, so the NPaste GTests are never compiled or run.
Modules/Filtering/SimpleITKFilters/itk-module.cmake Module declared with ITKCommon and ITKImageFeature dependencies; ITKImageGrid (used by DICOMOrientImageFilter) is missing from DEPENDS.
Modules/Filtering/SimpleITKFilters/include/itkMaskedAssignImageFilter.h Header include guard is correct but the #ifndef ITK_MANUAL_INSTANTIATION block is placed after the guard's closing #endif, contrary to the pattern in every other filter in this module.
Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientImageFilter.h Well-structured 3-D orientation filter wrapping PermuteAxes + Flip; static_assert enforces 3-D restriction.
Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientImageFilter.hxx Mini-pipeline implementation using PermuteAxes + Flip + Cast; orientation determination logic looks correct.
Modules/Filtering/SimpleITKFilters/include/itkNPasteImageFilter.h Multi-dimensional paste filter; static_assert guards dimension constraint; include guard pattern is correct.
Modules/Filtering/SimpleITKFilters/include/itkObjectnessMeasureImageFilter.h Composite filter wrapping HessianImageFilter + HessianToObjectnessMeasureImageFilter; parameters and destructor look correct.
Modules/Filtering/SimpleITKFilters/src/itkDICOMOrientation.cxx Static map initialization for orientation code ↔ string conversion is complete and correct; orientation math looks sound.
Modules/Remote/SimpleITKFilters.remote.cmake Remote stub correctly removed as part of the ingest; no residual references expected.
pyproject.toml CI configuration correctly enables Module_SimpleITKFilters for the configure-ci step.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Input Image] --> B[DICOMOrientImageFilter]
    B --> C{NeedToPermute?}
    C -- Yes --> D[PermuteAxesImageFilter]
    C -- No --> E[skip permute]
    D --> F{NeedToFlip?}
    E --> F
    F -- Yes --> G[FlipImageFilter]
    F -- No --> H[skip flip]
    G --> I[CastImageFilter]
    H --> I
    I --> J[Output Image]

    K[Input Image] --> L[ObjectnessMeasureImageFilter]
    L --> M[HessianImageFilter]
    M --> N[HessianToObjectnessMeasureImageFilter]
    N --> O[Objectness Output]

    P[DestinationImage] --> Q[NPasteImageFilter]
    R[SourceImage / Constant] --> Q
    Q --> S[Pasted Output]

    T[InputImage] --> U[MaskedAssignImageFilter]
    V[MaskImage] --> U
    W[AssignImage / Constant] --> U
    U --> X[Masked Output]
Loading

Reviews (1): Last reviewed commit: "ENH: Enable Module_SimpleITKFilters and ..." | Re-trigger Greptile

Comment thread Modules/Filtering/SimpleITKFilters/test/CMakeLists.txt
Comment thread Modules/Filtering/SimpleITKFilters/itk-module.cmake
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Greptile Summary

Ingests the ITKSimpleITKFilters remote module into Modules/Filtering/SimpleITKFilters, adding five filters (DICOMOrientImageFilter, DICOMOrientation, HessianImageFilter, MaskedAssignImageFilter, NPasteImageFilter, ObjectnessMeasureImageFilter) along with wrapping, tests, and CID-based test data. The remote stub is removed and pyproject.toml is updated to enable the module in CI.

  • itkNPasteImageFilterGTest.cxx is added to the repo but omitted from SimpleITKFiltersGTest in test/CMakeLists.txt, so its tests are never compiled or run.
  • itk-module.cmake leaves COMPILE_DEPENDS empty despite itkDICOMOrientImageFilter.h including headers from ITKImageGrid (PermuteAxesImageFilter, FlipImageFilter), which could break standalone/external builds.
  • Minor quality issues remain: a dead #if 0 block in itkDICOMOrientation.h and two small typos in doc strings and a static assertion message.

Confidence Score: 3/5

Safe to merge after fixing the NPaste test registration; the filter implementations themselves appear correct.

The NPaste Google Test file is shipped but never compiled — the test driver omits it — so the entire NPaste test suite is silently skipped. Merging as-is means NPasteImageFilter goes into the tree with no executed tests, which is the kind of gap that lets regressions slip through undetected. The remaining issues (missing ITKImageGrid in COMPILE_DEPENDS, dead #if 0 block, two small typos) are minor and do not affect runtime behavior.

test/CMakeLists.txt (missing test registration) and itk-module.cmake (empty COMPILE_DEPENDS despite headers from ITKImageGrid)

Important Files Changed

Filename Overview
Modules/Filtering/SimpleITKFilters/test/CMakeLists.txt Missing itkNPasteImageFilterGTest.cxx from SimpleITKFiltersGTest — NPaste Google Tests are never compiled or run
Modules/Filtering/SimpleITKFilters/itk-module.cmake COMPILE_DEPENDS is empty but itkDICOMOrientImageFilter.h includes headers from ITKImageGrid (PermuteAxesImageFilter, FlipImageFilter); external builds may miss include directories
Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientation.h Introduces DICOMOrientation orientation enum/utility class; contains a dead #if 0 branch and a minor doc typo
Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientImageFilter.h Adds 3-D-only DICOM orient filter; static_assert message has typo ("are support" → "are supported")
Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientImageFilter.hxx Template implementation of DICOMOrientImageFilter; logic for permutation/flip determination and mini-pipeline looks correct
Modules/Filtering/SimpleITKFilters/src/itkDICOMOrientation.cxx Implements orientation enum ↔ string and direction-cosine conversions; operator<< uses assert (no-op in release) but all valid enum values are covered in the map
Modules/Filtering/SimpleITKFilters/include/itkObjectnessMeasureImageFilter.h Composite wrapper combining HessianImageFilter and HessianToObjectnessMeasureImageFilter; API looks complete and correct
Modules/Filtering/SimpleITKFilters/include/itkNPasteImageFilter.h Adds N-dimensional paste filter with in-place support and lower-dimensional source images; API looks clean
Modules/Remote/SimpleITKFilters.remote.cmake Remote module stub correctly removed as part of the ingestion
pyproject.toml Correctly enables Module_SimpleITKFilters in the CI configure step

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Input Image] --> B[DICOMOrientImageFilter]
    B --> B1[DICOMOrientation\nDirectionCosinesToOrientation]
    B1 --> B2[DeterminePermutationsAndFlips]
    B2 --> B3{NeedToPermute?}
    B3 -- Yes --> C[PermuteAxesImageFilter]
    B3 -- No --> D
    C --> D{NeedToFlip?}
    D -- Yes --> E[FlipImageFilter]
    D -- No --> F
    E --> F[CastImageFilter]
    F --> G[Output Image]

    H[Input Image] --> I[ObjectnessMeasureImageFilter]
    I --> J[HessianImageFilter]
    J --> K[HessianToObjectnessMeasureImageFilter]
    K --> L[Output Objectness Image]

    M[Destination Image] --> N[NPasteImageFilter]
    O[Source Image / Constant] --> N
    N --> P[Output Image with pasted region]
Loading

Reviews (2): Last reviewed commit: "ENH: Enable Module_SimpleITKFilters and ..." | Re-trigger Greptile

Comment thread Modules/Filtering/SimpleITKFilters/test/CMakeLists.txt
Comment thread Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientImageFilter.h Outdated
Comment thread Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientation.h Outdated
Comment thread Modules/Filtering/SimpleITKFilters/include/itkDICOMOrientation.h Outdated
Comment thread Modules/Filtering/SimpleITKFilters/itk-module.cmake
@blowekamp
Copy link
Copy Markdown
Member

I thought we agree to not ingest the SimpleITKFilters remote module?

I have not been looking closely at the criteria for the other modules being ingested. There is too many GH messages to follow.

Add itkNPasteImageFilterGTest.cxx to SimpleITKFiltersGTest source list so
the NPaste tests compile and run.
Move the .hxx include block before the closing #endif of the header
guard to match the pattern used in other ITK filter headers.
itkDICOMOrientImageFilter.h directly includes headers from ITKImageGrid
(itkPermuteAxesImageFilter.h, itkFlipImageFilter.h). Declare the
dependency explicitly so standalone/external builds resolve include
paths correctly.
…aders

- itkDICOMOrientImageFilter.h: static_assert message 'are support' -> 'are supported'.
- itkDICOMOrientation.h: Doxygen 'closes axes' -> 'closest axes',
  'is approximated' -> 'are approximated'. Remove unused #if 0/#else
  scaffolding around ITK_ORIENTATIONENUM macro.
@hjmjohnson
Copy link
Copy Markdown
Member Author

/azp run

Copy link
Copy Markdown
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

STOP WORK PLEASE.

The if(NOT ITK_SOURCE_DIR) branch (find_package(ITK) +
include(ITKModuleExternal)) only executes when building the module
standalone outside ITK. In-tree builds always take the
itk_module_impl() else branch. Drop the dead branch following the
cleanup pattern from InsightSoftwareConsortium#6272/InsightSoftwareConsortium#6279.
@hjmjohnson
Copy link
Copy Markdown
Member Author

I removed SimpleITKFilters from the list of Phase3 import and moved it to the "remain remote" phase 4 in issue #6160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Filtering Issues affecting the Filtering module area:Python wrapping Python bindings for a class area:Remotes Issues affecting the Remote module type:Data Changes to testing data type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants