ENH: Ingest ITKMeshNoise into Modules/Filtering (closes #5174)#6161
ENH: Ingest ITKMeshNoise into Modules/Filtering (closes #5174)#6161hjmjohnson merged 40 commits intoInsightSoftwareConsortium:mainfrom
Conversation
98cf59c to
4769b1d
Compare
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
|
| Filename | Overview |
|---|---|
| Modules/Filtering/MeshNoise/include/itkAdditiveGaussianNoiseMeshFilter.hxx | GenerateData uses MaxTopologicalDimension for point-coordinate loop (should be PointDimension) and PrintSelf omits m_Seed — both raised by prior reviewers and still unresolved. |
| Modules/Filtering/MeshNoise/include/itkAdditiveGaussianNoiseQuadEdgeMeshFilter.hxx | Uses correct PointDimension for coordinate loop; PrintSelf omits m_Seed (same gap as the companion filter, raised by prior reviewer). |
| Modules/Filtering/MeshNoise/test/itkAdditiveGaussianNoiseMeshFilterTest.cxx | Compile error from #5174 fixed via VectorType::Filled(static_cast(…)); test exercises Set/Get for all three parameters. |
| Modules/Filtering/MeshNoise/test/itkAdditiveGaussianNoiseQuadEdgeMeshFilterTest.cxx | Same VectorType::Filled fix applied; structurally mirrors the Mesh test and should compile cleanly. |
| Modules/Filtering/MeshNoise/wrapping/itkAdditiveGaussianNoiseQuadEdgeMeshFilter.wrap | Uses POINTER (consistent with other QuadEdge filters in ITK) but only wraps double, creating an asymmetry with the companion Mesh filter that wraps WRAP_ITK_REAL. |
| Modules/Remote/MeshNoise.remote.cmake | Remote module descriptor cleanly removed; now superseded by the in-tree module. |
| pyproject.toml | -DModule_MeshNoise:BOOL=ON added to configure-ci task, enabling the module in Pixi-based CI builds. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class MeshToMeshFilter {
<<ITKMesh>>
}
class AdditiveGaussianNoiseMeshFilter {
+CoordinateType m_Mean
+CoordinateType m_Sigma
+int m_Seed
+GenerateData()
+PrintSelf()
}
class QuadEdgeMeshToQuadEdgeMeshFilter {
<<ITKQuadEdgeMesh>>
}
class AdditiveGaussianNoiseQuadEdgeMeshFilter {
+CoordinateType m_Mean
+CoordinateType m_Sigma
+int m_Seed
+GenerateData()
+PrintSelf()
}
class NormalVariateGenerator {
<<ITKStatistics>>
+Initialize(seed)
+GetVariate()
}
MeshToMeshFilter <|-- AdditiveGaussianNoiseMeshFilter
QuadEdgeMeshToQuadEdgeMeshFilter <|-- AdditiveGaussianNoiseQuadEdgeMeshFilter
AdditiveGaussianNoiseMeshFilter ..> NormalVariateGenerator : uses
AdditiveGaussianNoiseQuadEdgeMeshFilter ..> NormalVariateGenerator : uses
Reviews (2): Last reviewed commit: "COMP: Normalize exec bit on ingested tes..." | Re-trigger Greptile
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
Require CMake minimum version to be 3.9.5 following ITKv5 requiring C++11: https://discourse.itk.org/t/minimum-cmake-version-update/585
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"
== 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.
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.
As agreed in: https://discourse.itk.org/t/cmake-update/870/ Set the cmake_minimum_required to version **3.10.2**.
When compiling ITK with Module_MeshNoise=ON and ITK_WRAP_PYTHON=ON, ITK fails to build with a number of compilation errors due to the fact that we are attempting to instantiate the filters over DynamicMeshTraits, but PointSet, Mesh, MeshSource, and MeshToMeshFilter have been instantiated for only StaticMeshTraits. This results in a number of errors of the form "Nothing known about base class XXX." This patch ensures that the two superclasses (MeshToMeshFilter and MeshSource) are wrapped by passing the appropriate option (POINTER_WITH_2_SUPERCLASSES), and explicitly wraps Mesh and PointSet, allowing the module to build successfully.
Fixes changes made in InsightSoftwareConsortium#2053. ITK_DISALLOW_COPY_AND_ASSIGN will be used if ITK_FUTURE_LEGACY_REMOVE=OFF.
The ability to include either .h or .hxx files as header files required recursively reading the .h files twice. The added complexity is unnecessary, costly, and can confuse static analysis tools that monitor header guardes (due to reaching the maximum depth of recursion limits for nested #ifdefs in checking).
…acro Added two new macro's, intended to replace the old 'itkTypeMacro' and 'itkTypeMacroNoParent'. The main aim is to be clearer about what those macro's do: add a virtual 'GetNameOfClass()' member function and override it. Unlike 'itkTypeMacro', 'itkOverrideGetNameOfClassMacro' does not have a 'superclass' parameter, as it was not used anyway. Note that originally 'itkTypeMacro' did not use its 'superclass' parameter either, looking at commit 699b66c, Will Schroeder, June 27, 2001: https://github.com/InsightSoftwareConsortium/ITK/blob/699b66cb04d410e555656828e8892107add38ccb/Code/Common/itkMacro.h#L331-L337
For the sake of code readability, a new 'CoordinateType' alias is added for each nested 'CoordRepType' alias. The old 'CoordRepType' aliases will still be available with ITK 6.0, but it is recommended to use 'CoordinateType' instead. The 'CoordRepType' aliases will be removed when 'ITK_FUTURE_LEGACY_REMOVE' is enabled. Similarly, 'InputCoordinateType', 'OutputCoordinateType', and 'ImagePointCoordinateType' replace 'InputCoordRepType', 'OutputCoordRepType', and 'ImagePointCoordRepType', respectively.
MeshNoise/test/itkAdditiveGaussianNoiseMeshFilterTest.cxx:56:20:
error: no viable conversion from 'const double' to 'VectorType' (aka 'Vector<float, 3U>')
56 | sphere->SetScale(SPHERE_SCALE);
| ^~~~~~~~~~~~
Match the ITK version minimum requirements for cmake.
Match version for ITK v5.4.2
This change excludes the MeshNoise module from the default ITK build, making it an optional module that must be explicitly enabled.
Collapses the dual-mode CMakeLists.txt to a single itk_module_impl() call, matching every other in-tree Modules/<Group>/<Name>/ module and the FastBilateral / GenericLabelInterpolator / MGHIO / Montage / AnisotropicDiffusionLBR ingest pattern.
Inline the module DESCRIPTION as a literal string and add a Modules/Filtering/MeshNoise/README.md pointer to the archived upstream repository, matching the pattern used for FastBilateral, GenericLabelInterpolator, and MGHIO.
…m#5174) itk::Vector<float, N> dropped its single-value implicit-conversion constructor; the test was calling TSphere::VectorType(SPHERE_SCALE) where SPHERE_SCALE is a double literal, which no longer compiles ("no viable conversion from 'const double' to 'VectorType'"). Use the canonical Filled() factory with an explicit float cast. Closes InsightSoftwareConsortium#5174.
The module now lives at Modules/Filtering/MeshNoise/. The configure-time fetch declaration would otherwise clone the (soon-to-be-archived) upstream and double-define the module.
Add -DModule_MeshNoise:BOOL=ON to the Pixi-Cxx configure-ci task so the in-tree module is exercised by the default CI matrix.
Apply ITK's current gersemi configuration to the test/CMakeLists.txt and wrapping/CMakeLists.txt files inherited from upstream. The upstream module pre-dates ITK's gersemi adoption and was authored before this formatting pass existed; running it now lets the in-tree pre-commit gate pass cleanly. Mode A step 6 of the ingestion strategy (Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md).
The upstream remote module shipped test/CMakeLists.txt with mode 100755. Normalize to 100644 in-tree; CMakeLists files are never executed directly and ghostflow flags exec bits on text files.
4769b1d to
dfb04eb
Compare
|
Addressed greptile in dfb04eb: P1 — QuadEdge test compile error: applied the same P2 — exec bit on @greptileai re-review. Note: |
5d95c9b
into
InsightSoftwareConsortium:main
|
This got merged with linearized history. I guess some force push changed it? Topology is obvious when using graphical log viewers, such as TortoiseGit on Windows, and |
|
Still, let's leave it as-is. Enough fuss about it. |
The MeshNoise module has been ingested into ITK main via InsightSoftwareConsortium/ITK#6161 (merge commit 5d95c9bda7), which also closes the user-blocking compile error reported in InsightSoftwareConsortium/ITK#5174. Remove the now-duplicated content from this repo's tree tip and add MIGRATION_README.md pointing future visitors at the authoritative in-tree location. Pre-ingest git history is preserved for blame and archaeology; this repo will be marked ARCHIVED in GitHub Settings after merge.
…est-MeshNoise ENH: Ingest ITKMeshNoise into Modules/Filtering (closes InsightSoftwareConsortium#5174)
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
Ingests the ITKMeshNoise remote module into
Modules/Filtering/MeshNoise/per the v3 whitelist-based ingestion strategy, and fixes the long-standing compile error reported in #5174. Top-of-Wave-1 priority per the master tracking issue #6160.What's in this PR
Same shape as the GenericLabelInterpolator (#6135), MGHIO (#6137), and FastBilateral (#6159) ingests:
ENH: Ingest ITKMeshNoise into Modules/Filtering—git filter-repoMode A merge of the whitelisted upstream history (35 normalized commits, 12 files), preserving original-authorgit blame.COMP: Drop standalone-build boilerplate from MeshNoise— collapseCMakeLists.txttoitk_module_impl().COMP: Drop missing README.rst read in ingested MeshNoise— inlineDESCRIPTIONand addREADME.mdpointing at the soon-to-be-archived upstream.BUG: Fix MeshNoise test compile error (issue #5174)—itk::Vector<float, N>no longer accepts a single-doubleconstructor argument; replaceTSphere::VectorType(SPHERE_SCALE)with the explicitFill()pattern. Verified locally:itkAdditiveGaussianNoiseMeshFilterTest.cxxnow compiles cleanly under Apple Clang-Wshadow -Wall -Wextra.COMP: Remove MeshNoise.remote.cmake; now in-tree.ENH: Enable MeshNoise in CI via configure-ci— adds-DModule_MeshNoise:BOOL=ONto the Pixi-Cxx configure-ci task.Verification
pre-commit run --all-filesexit 0 against the rewritten tree (per~/.claude/rules/pre-commit-mandatory.md).cmake -DModule_MeshNoise=ON ..configures cleanly.MeshNoiseTestDriver.dir/itkAdditiveGaussianNoiseMeshFilterTest.cxx.ocompiles cleanly with the fix in place — direct verification that Compile error inMeshNoise/test/itkAdditiveGaussianNoiseMeshFilterTest.cxx:56#5174 is resolved.ghostflow-check-mainclean (executable bits on the testCMakeLists.txtwere stripped viagit filter-repoacross the whole ingest history).--no-ff --allow-unrelated-historiesmerge; per-commit pre-commit replay vianormalize-ingest-commits.py(38 → 35 commits after dropping empty style-only commits and re-prefixing two subjects).Post-merge follow-ups
InsightSoftwareConsortium/ITKMeshNoise(delete whitelisted files, addMIGRATION_README.md); same pattern as ITKMGHImageIO#50, ITKGenericLabelInterpolator#43.MeshNoise/test/itkAdditiveGaussianNoiseMeshFilterTest.cxx:56#5174 once this lands.