Skip to content

ENH: Ingest ITKCuberille into Modules/Filtering#6205

Merged
hjmjohnson merged 115 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:ingest-cuberille
May 5, 2026
Merged

ENH: Ingest ITKCuberille into Modules/Filtering#6205
hjmjohnson merged 115 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:ingest-cuberille

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Ingests Cuberille from the standalone remote module InsightSoftwareConsortium/ITKCuberille into ITK at Modules/Filtering/Cuberille/ using the v4 ingestion pipeline (PR #6204). Stacked on #6204 — that PR must merge first.

Local validation: pre-commit run --all-files clean; CuberilleTestDriver builds; 25/25 Cuberille tests pass locally in 5.3s.

Pipeline metrics
Metric Value
Upstream commits → rewritten 200 → 106
Upstream merges → preserved 59 → 30 (no linearization, per feedback_ingest_merge_topology.md)
Blobs scanned in sanitize 148 (cxx=103, py=1, cmake=28, text=5, skip=11)
Blobs reformatted 95 (clang-format / black / gersemi / trailing-ws / EOF)
Commit subjects auto-prefixed 36
Merges visible in git log -- Modules/Filtering/Cuberille 6
v4 deny-pass bug surfaced and fixed during this run

The first attempt at this ingest leaked test/Docker/{Dockerfile,build.sh,run.sh,test.sh} into ITK history, because the v4 ingest driver had only a whitelist pass — no --invert-paths --path-glob deny-pass for CI/packaging scaffolding that lives inside whitelisted directories. Fixed in PR #6204 by porting v3's deny-glob list (Docker, Jenkinsfile, .github, azure-pipelines, .clang-format inside the module subtree, etc.). Re-running this ingest after the fix produced a clean tree with no scaffolding leakage.

Auto-prefix decisions (sample)
prefix counts across the 36 changed subjects:
  ENH:   28
  STYLE:  4
  COMP:   2
  BUG:    1
  DOC:    1

Every commit on the rewritten history starts with a valid ITK prefix (BUG:/COMP:/DOC:/ENH:/PERF:/STYLE:/WIP:). The Utilities/Maintenance/RemoteModuleIngest/sanitize-history.py log records the (sha, chosen-prefix, original-subject) for each decision so the operator can audit before merge.

Follow-up commits in this PR

After the merge commit (ENH: Ingest ITKCuberille into Modules/Filtering), the standard cleanup commits land on top:

  • STYLE: Apply gersemi 0.19.3 to Cuberille wrapping test CMakeLists. The v4 sanitize pass runs gersemi 0.24.0; ITK's pre-commit pins 0.19.3. The post-merge pre-commit run --all-files gate caught one CMake file that 0.24.0 considered clean but 0.19.3 wanted re-formatted. Working as designed.
  • DOC: Add Modules/Filtering/Cuberille/README.md pointing at the archived upstream.
  • COMP: Remove Modules/Remote/Cuberille.remote.cmake (now in-tree).
  • ENH: Enable Module_Cuberille:BOOL=ON in pyproject.toml configure-ci.
Phase B (upstream archival) — deferred

Per the v4 design (PR #6204), the upstream archival step is intentionally separated. After this PR merges into ITK main, run:

~/src/ITK/Utilities/Maintenance/RemoteModuleIngest/archive-remote-module.sh \
    Cuberille --itk-pr-number <THIS-PR-NUMBER>

That step pushes the deletion + MIGRATION_README.md → README.md promotion to upstream and flips archived=true. This PR makes no upstream changes; the upstream ITKCuberille repository remains live until the operator manually runs Phase B.

root and others added 30 commits August 22, 2011 09:47
QuadricDecimationQuadEdgeMeshFilter.
Refactor as an ITKv4 module.
Add Docker test configuration.
COMP: Remove unused typedefs in the test.
Addresses:

/home/matt/src/ITK/Modules/Remote/Cuberille/include/itkCuberilleImageToMeshFilter.h: In instantiation of ‘class itk::CuberilleImageToMeshFilter<itk::Image<unsigned char, 3u>, itk::Mesh<unsigned char, 3u>, itk::LinearInterpolateImageFunction<itk::Image<unsigned char, 3u> > >’:
/home/matt/src/ITK/Modules/Remote/Cuberille/test/CuberilleTest01.cxx:123:16:   required from here
/home/matt/src/ITK/Modules/Core/Common/include/itkProcessObject.h:522:16: warning: ‘virtual void itk::ProcessObject::SetInput(const DataObjectIdentifierType&, itk::DataObject*)’ was hidden [-Woverloaded-virtual]
   virtual void SetInput(const DataObjectIdentifierType & key, DataObject *input);
                ^
In file included from /home/matt/src/ITK/Modules/Remote/Cuberille/include/itkCuberilleImageToMeshFilter.h:347:0,
                 from /home/matt/src/ITK/Modules/Remote/Cuberille/test/CuberilleTest01.cxx:27:
/home/matt/src/ITK/Modules/Remote/Cuberille/include/itkCuberilleImageToMeshFilter.hxx:53:1: warning:   by ‘void itk::CuberilleImageToMeshFilter<TInputImage, TOutputMesh, TInterpolator>::SetInput(const InputImageType*) [with TInputImage = itk::Image<unsigned char, 3u>; TOutputMesh = itk::Mesh<unsigned char, 3u>; TInterpolator = itk::LinearInterpolateImageFunction<itk::Image<unsigned char, 3u> >; itk::CuberilleImageToMeshFilter<TInputImage, TOutputMesh, TInterpolator>::InputImageType = itk::Image<unsigned char, 3u>]’ [-Woverloaded-virtual]
 CuberilleImageToMeshFilter<TInputImage,TOutputMesh,TInterpolator>
 ^
COMP: Address SetInput override virtual warning.
The defined directory is not correct when built as a Remote module.
BUG: Fix test temporary output dir.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Greptile Summary

This PR ingests ITKCuberille from its standalone remote module into Modules/Filtering/Cuberille/ using the new v4 ingestion pipeline, and adds the pipeline tooling (ingest-module-v4.sh, sanitize-history.py, archive-remote-module.sh) to Utilities/Maintenance/RemoteModuleIngest/. The in-tree module replaces Modules/Remote/Cuberille.remote.cmake and is enabled for CI in pyproject.toml.

  • P1 — global #define macros in public header: DEBUG_PRINT, USE_GRADIENT_RECURSIVE_GAUSSIAN, USE_ADVANCED_PROJECTION, and USE_LINESEARCH_PROJECTION are #defined at file scope in the installed header, polluting the preprocessor namespace for all downstream consumers.
  • P1 — incorrect >= / <= operators in VertexLookupNode: Both operators use >= / <= on the primary key (m_Y) in their first branch, causing them to short-circuit before checking m_X, which inverts results for equal-Y cases.

Confidence Score: 3/5

Two P1 issues in the ingested public header must be resolved before merge.

Two independent P1 findings both touch the same public header: global macro pollution is a present defect for any consumer that includes the header, and the operator bugs are logically wrong class-level API. The rest of the ingestion (CMake wiring, tests, tooling scripts) looks correct.

Modules/Filtering/Cuberille/include/itkCuberilleImageToMeshFilter.h requires fixes before merge.

Important Files Changed

Filename Overview
Modules/Filtering/Cuberille/include/itkCuberilleImageToMeshFilter.h Public header with two issues: four global #define macros that pollute the preprocessor namespace, and logically incorrect >= / <= operators in the private VertexLookupNode class.
Modules/Filtering/Cuberille/include/itkCuberilleImageToMeshFilter.hxx 1052-line header-only implementation; core algorithm looks correct. The four #define guards are consumed here but originate in the .h.
Modules/Filtering/Cuberille/test/CMakeLists.txt 25 CuberilleTest01 variants and CuberilleIssue66 use bare add_test rather than ITK's itk_add_test; the remaining 3 tests use itk_add_test correctly.
Modules/Filtering/Cuberille/itk-module.cmake Module registration looks correct; DEPENDS and TEST_DEPENDS are appropriately specified; EXCLUDE_FROM_DEFAULT and ENABLE_SHARED match other in-tree modules.
Modules/Remote/Cuberille.remote.cmake Correctly deleted as part of ingestion; module is now in-tree.
Utilities/Maintenance/RemoteModuleIngest/sanitize-history.py New ingestion tooling; content-sniffing, formatter wrappers, and commit-callback logic look well-structured and fail-soft. No security concerns.
Utilities/Maintenance/RemoteModuleIngest/ingest-module-v4.sh Phase A driver script; set -euo pipefail throughout, proper preflight checks, clean argument parsing — no obvious issues.
pyproject.toml Adds -DModule_Cuberille:BOOL=ON to the CI configure line, enabling the new in-tree module for Python wheel builds.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[upstream ITKCuberille repo] -->|Phase A: ingest-module-v4.sh| B[git-filter-repo: path whitelist + deny-glob]
    B --> C[sanitize-history.py: clang-format / black / gersemi / prefix]
    C --> D[Mode-A merge into ITK main]
    D --> E[Modules/Filtering/Cuberille/]
    D --> F[Remove Modules/Remote/Cuberille.remote.cmake]
    D --> G[pyproject.toml: enable Module_Cuberille]
    E --> H[itk-module.cmake: DEPENDS / TEST_DEPENDS]
    E --> I[include/itkCuberilleImageToMeshFilter.h+.hxx]
    E --> J[test/ + wrapping/]
    D -->|Phase B: deferred post-merge| K[archive-remote-module.sh]
    K --> L[upstream: delete files + flip archived=true]
Loading

Reviews (1): Last reviewed commit: "ENH: Enable Module_Cuberille in configur..." | Re-trigger Greptile

Comment thread Modules/Filtering/Cuberille/include/itkCuberilleImageToMeshFilter.h Outdated
Comment thread Modules/Filtering/Cuberille/include/itkCuberilleImageToMeshFilter.h
Comment thread Modules/Filtering/Cuberille/test/CMakeLists.txt
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 4, 2026
The four implementation-control macros (DEBUG_PRINT,
USE_GRADIENT_RECURSIVE_GAUSSIAN, USE_ADVANCED_PROJECTION,
USE_LINESEARCH_PROJECTION) were defined unconditionally at the top of
itkCuberilleImageToMeshFilter.h. Any translation unit including the
public header silently inherited DEBUG_PRINT=0, masking a same-named
macro any consumer might want to test with #ifdef.

Move the four #define lines to itkCuberilleImageToMeshFilter.hxx so
they remain visible to the template implementation that depends on
them via #if, but are no longer emitted into consumer translation
units that only include the .h.

Greptile P1 on PR InsightSoftwareConsortium#6205.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 4, 2026
Both operators applied >= / <= on m_Y in their first branch, which
short-circuited to true/false whenever the Y values were equal —
before the X tiebreaker could be evaluated. E.g. {Y=1,X=0} >=
{Y=1,X=5} resolved (1 >= 1) == true and returned true even though
the lhs is strictly less than the rhs.

Replace the first-branch comparison with strict > / < so equal-Y
cases fall through to the X tiebreaker, mirroring the existing
operator> and operator< definitions.

std::map calls only operator<, so existing callers do not observe
the bug, but the broken operators are class-level API and would
silently misbehave under direct use.

Greptile P1 on PR InsightSoftwareConsortium#6205.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 4, 2026
Convert the 20 Cuberille tests registered via plain add_test() to
itk_add_test() so they pick up the standard ITK CTest labels and
properties (matching the three already-itk_add_test'd entries below
them).  Tests with bare add_test are invisible to dashboard-specific
test selectors that filter on those labels.

GTest conversion of these tests is a separate follow-up.

Greptile P2 on PR InsightSoftwareConsortium#6205.
Copy link
Copy Markdown
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.

Mostly looks good.

Comment thread Modules/Filtering/Cuberille/include/itkCuberilleImageToMeshFilter.hxx Outdated
hjmjohnson and others added 5 commits May 4, 2026 17:29
Brings Cuberille from a configure-time remote fetch into the ITK
source tree at Modules/Filtering/Cuberille/ using the v4 ingestion
pipeline (whitelist filter-repo + per-commit clang-format + black +
commit-prefix sanitization).

Upstream repo:  https://github.com/InsightSoftwareConsortium/ITKCuberille.git
Upstream tip:   4ba447f256a93428e56507178f92124bb91e3f3c
Ingest date:    2026-05-04
Whitelist:      Cuberille.list

Per-commit transforms applied across all 105 commits:
  - filter-repo --paths-from-file (whitelist)
  - filter-repo --to-subdirectory-filter Modules/Filtering/Cuberille
  - 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: 59 -> 30 merge(s).

Primary author: Matt McCormick <matt.mccormick@kitware.com>

Co-authored-by: Davis Marc Vigneault <davis.vigneault@gmail.com>
Co-authored-by: Davis Vigneault <davis.vigneault@gmail.com>
Co-authored-by: DVigneault <davis.vigneault@gmail.com>
Co-authored-by: Dženan Zukić <dzenan.zukic@kitware.com>
Co-authored-by: Hans Johnson <hans-johnson@uiowa.edu>
Co-authored-by: Hastings Greer <hastings.greer@kitware.com>
Co-authored-by: Jon Haitz Legarreta <jhlegarreta@vicomtech.org>
Co-authored-by: Jon Haitz Legarreta Gorroño <jon.haitz.legarreta@gmail.com>
Co-authored-by: Mathew Seng <mathewseng@gmail.com>
Co-authored-by: Matt McCormick <matt@mmmccormick.com>
Co-authored-by: root <root@insight-journal.org>
Co-authored-by: Tom Birdsong <tom.birdsong@kitware.com>
Co-authored-by: Zach Williamson <zachary-williamson@uiowa.edu>
@hjmjohnson hjmjohnson changed the title ENH: Ingest ITKCuberille into Modules/Filtering (stacked on #6204) ENH: Ingest ITKCuberille into Modules/Filtering May 4, 2026
hjmjohnson added 5 commits May 4, 2026 17:38
The four implementation-control macros (DEBUG_PRINT,
USE_GRADIENT_RECURSIVE_GAUSSIAN, USE_ADVANCED_PROJECTION,
USE_LINESEARCH_PROJECTION) were defined unconditionally at the top of
itkCuberilleImageToMeshFilter.h. Any translation unit including the
public header silently inherited DEBUG_PRINT=0, masking a same-named
macro any consumer might want to test with #ifdef.

Move the four #define lines to itkCuberilleImageToMeshFilter.hxx so
they remain visible to the template implementation that depends on
them via #if, but are no longer emitted into consumer translation
units that only include the .h.

Greptile P1 on PR InsightSoftwareConsortium#6205.
Both operators applied >= / <= on m_Y in their first branch, which
short-circuited to true/false whenever the Y values were equal —
before the X tiebreaker could be evaluated. E.g. {Y=1,X=0} >=
{Y=1,X=5} resolved (1 >= 1) == true and returned true even though
the lhs is strictly less than the rhs.

Replace the first-branch comparison with strict > / < so equal-Y
cases fall through to the X tiebreaker, mirroring the existing
operator> and operator< definitions.

std::map calls only operator<, so existing callers do not observe
the bug, but the broken operators are class-level API and would
silently misbehave under direct use.

Greptile P1 on PR InsightSoftwareConsortium#6205.
Convert the 20 Cuberille tests registered via plain add_test() to
itk_add_test() so they pick up the standard ITK CTest labels and
properties (matching the three already-itk_add_test'd entries below
them).  Tests with bare add_test are invisible to dashboard-specific
test selectors that filter on those labels.

GTest conversion of these tests is a separate follow-up.

Greptile P2 on PR InsightSoftwareConsortium#6205.
DEBUG_PRINT was a debug-tracing toggle defined to 0 (compile-time disabled)
since the module's first commit; the eight #if DEBUG_PRINT...#endif blocks
they guarded contained std::cout traces from algorithm bring-up that were
never re-enabled.  Removing the macro and the dead code per the reviewer's
preference for cleaner module headers.  Behavior is unchanged because
all guarded blocks were #if 0.
Replaces three preprocessor toggles
  USE_GRADIENT_RECURSIVE_GAUSSIAN
  USE_ADVANCED_PROJECTION
  USE_LINESEARCH_PROJECTION
with class-level `static constexpr bool` members of the same intent.
Preprocessor `#if FLAG` becomes `if constexpr (Flag)` and a
`#if/#else` selecting between two GradientFilterType type aliases
becomes `std::conditional_t<...>`.

Eliminates global-namespace preprocessor pollution flagged in the
Greptile draft review (P1) and matches dzenanz's reviewer preference
for class-level configuration over file-level macros.  Default values
remain false (matching the prior `#define X 0`) so behavior is
unchanged.

The dead linesearch branch had a stray top-level `break;` outside any
loop (a long-standing bit-rot in the disabled-since-2014 alternative
path).  Replaced with `return;` (the void function's natural early-out)
so the branch parses cleanly under `if constexpr` instantiation rules.
@hjmjohnson hjmjohnson requested a review from dzenanz May 4, 2026 23:01
@hjmjohnson hjmjohnson merged commit 9658520 into InsightSoftwareConsortium:main May 5, 2026
18 of 19 checks passed
@hjmjohnson hjmjohnson added this to the ITK 6.0 Release Candidate 1 milestone May 5, 2026
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
The four implementation-control macros (DEBUG_PRINT,
USE_GRADIENT_RECURSIVE_GAUSSIAN, USE_ADVANCED_PROJECTION,
USE_LINESEARCH_PROJECTION) were defined unconditionally at the top of
itkCuberilleImageToMeshFilter.h. Any translation unit including the
public header silently inherited DEBUG_PRINT=0, masking a same-named
macro any consumer might want to test with #ifdef.

Move the four #define lines to itkCuberilleImageToMeshFilter.hxx so
they remain visible to the template implementation that depends on
them via #if, but are no longer emitted into consumer translation
units that only include the .h.

Greptile P1 on PR InsightSoftwareConsortium#6205.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Both operators applied >= / <= on m_Y in their first branch, which
short-circuited to true/false whenever the Y values were equal —
before the X tiebreaker could be evaluated. E.g. {Y=1,X=0} >=
{Y=1,X=5} resolved (1 >= 1) == true and returned true even though
the lhs is strictly less than the rhs.

Replace the first-branch comparison with strict > / < so equal-Y
cases fall through to the X tiebreaker, mirroring the existing
operator> and operator< definitions.

std::map calls only operator<, so existing callers do not observe
the bug, but the broken operators are class-level API and would
silently misbehave under direct use.

Greptile P1 on PR InsightSoftwareConsortium#6205.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
Convert the 20 Cuberille tests registered via plain add_test() to
itk_add_test() so they pick up the standard ITK CTest labels and
properties (matching the three already-itk_add_test'd entries below
them).  Tests with bare add_test are invisible to dashboard-specific
test selectors that filter on those labels.

GTest conversion of these tests is a separate follow-up.

Greptile P2 on PR InsightSoftwareConsortium#6205.
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026
…est-cuberille

ENH: Ingest ITKCuberille into Modules/Filtering
hjmjohnson added a commit to hjmjohnson/ITK that referenced this pull request May 7, 2026
Fixes the four classes of ghostflow errors that surfaced on the first
Cuberille v4 ingest (PR InsightSoftwareConsortium#6205) — the only allowable remaining error
should be the unavoidable upstream root commit.

  - Strip *.orig / *.rej / *.BACKUP.* / *.LOCAL.* / *.REMOTE.* /
    *.BASE.* merge-conflict-artifact files from history (catches
    leftover-conflict-marker errors like the .h.orig on Cuberille).
  - sanitize-history.py: cap every commit subject at 78 chars (ghostflow
    / kw-commit-msg rule); excess words move into the body.
  - sanitize-history.py: insert a blank line between subject and body
    when the original commit message lacked one (two-line subject error).
  - sanitize-history.py: clear the executable bit (mode 100755 -> 100644)
    on text-file extensions so root-commit "executable permissions but
    file does not look executable" stops firing.
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: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.

9 participants