ENH: Ingest ITKMGHIO into Modules/IO#6137
Merged
hjmjohnson merged 89 commits intoInsightSoftwareConsortium:mainfrom Apr 28, 2026
Merged
ENH: Ingest ITKMGHIO into Modules/IO#6137hjmjohnson merged 89 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson merged 89 commits intoInsightSoftwareConsortium:mainfrom
Conversation
A copy of the Slicer itkMGHImageIO mechanism for ITK. The intent of this repository is to extract the itkMGHImageIO from it's embedded Slicer only implementation and make it accessible to any ITK compliant program. This will be accomplished using the ITKv4 'Remote Module' mechanism as defined at: http://www.itk.org/Wiki/ITK/Policy_and_Procedures_for_Adding_Remote_Modules
The ITK_EXPORT define was set to nothing and had no known remaining purpose. It was removed to make the over all code easier to understand. There was, understandably, a bit of confusion about the need for this being pervasive in the code. It is currently backwards compatible to have this in code, but at some future point it will be removed.
Following the new naming rules for ITK remote modules, the remote module name should not contain the "ITK" prefix.
Add "EXCLUDE_FROM_ALL" tag for this remote module.
The headers in the test dir can be found without explicitly including the directory.
Clean up the module dependency. MGHIO does not depend on ITKTransform.
Remove "ITK" from the module name.
EXCLUDE_FROM_ALL is replaced by EXCLUDE_FROM_DEFAULT in ITK commit:0a98a978509a9536e07c22cde254559cc3c7d417.
FIX: ITK warnings of deprecated EXCLUDE_FROM_ALL.
BUG: missing steps converting origin back to freesurfer space
existing logic couldn't handle filenames with a dot before the filename extension.
existing logic couldn't handle filenames with a dot before the filename extension.
The dynamically allocated variable must be deleted in the destructor.
Remove usage of reserved "__" prefix.
BUG: Fix KWStyle error for header guarder
To be consistent with ITK, ITK_OVERRIDE and ITK_NULLPTR are added.
ENH: Add ITK_OVERRIDE and ITK_NULLPTR designations
Remove usage of reserved "__" prefix. The test header needed to have the __prefix removed.
BUG: Fix KWStyle error for header guarderd in test
Convert MGHIO to ITKIOMGH to conform to the ITK naming conventions for IO modules. This places the MGH IO for images with other IO modules in the cmake system, and automatically identifies it as needing to be registered.
After discussion with ITK team, it was determined that the convention was mis-interpreted in the previous patch. Internal and Remote IO modules have different naming conventions. Since MGHIO is still an external IO library, it should not use the internal IO naming conventions. This reverts commit 7616415.
Make the MGHIO module a shared library like the other IO modules. ctest -R MGH 1/14 Test InsightSoftwareConsortium#810: ITKIOMGHInDoxygenGroup ........... Passed 0.05 sec 2/14 Test InsightSoftwareConsortium#811: MGHFactoryCreationTest ........... Passed 0.09 sec 3/14 Test InsightSoftwareConsortium#812: MGHReadImagesTest_mgz ............ Passed 4.23 sec 4/14 Test InsightSoftwareConsortium#813: MGHReadImagesTest_mgh ............ Passed 0.68 sec 5/14 Test InsightSoftwareConsortium#814: MGHReadImagesTest_mgh.gz ......... Passed 4.11 sec 6/14 Test InsightSoftwareConsortium#815: itkITKIOMGHInternalTests ......... Passed 0.08 sec 7/14 Test InsightSoftwareConsortium#816: itkITKIOMGHOriginTest ............ Passed 4.16 sec 8/14 Test InsightSoftwareConsortium#2356: MGHIOInDoxygenGroup .............. Passed 0.04 sec 9/14 Test InsightSoftwareConsortium#2357: MGHFactoryCreationTest ........... Passed 0.07 sec 10/14 Test InsightSoftwareConsortium#2358: MGHReadImagesTest_mgz ............ Passed 4.23 sec 11/14 Test InsightSoftwareConsortium#2359: MGHReadImagesTest_mgh ............ Passed 0.68 sec 12/14 Test InsightSoftwareConsortium#2360: MGHReadImagesTest_mgh.gz ......... Passed 4.13 sec 13/14 Test InsightSoftwareConsortium#2361: itkMGHIOInternalTests ............ Passed 0.07 sec 14/14 Test InsightSoftwareConsortium#2362: itkMGHIOOriginTest ............... Passed 4.13 sec 100% tests passed, 0 tests failed out of 14
The test cases used determined that all pixel values were the same, but did not check that the image meta-data was preserved during writing! The tests have been improved to demonstrate that a bug has existed in the MGHIO writer for a very long time. If the DirectionCosine = Transpose(DirectionCosine) then there was not problem. A future patch will address the bug.
Previously the replay used ``git rev-list base..HEAD`` (which includes merge commits) and then invoked ``git cherry-pick`` without ``-m N``. ``cherry-pick`` rejects merge commits without an explicit ``-m``, so the very first merge in the upstream history aborted the entire replay. This blocked applying the v3 ingestion strategy's per-commit pre-commit replay step to any module whose upstream has merges in its surviving history (which is most of them). Fix: * ``git rev-list --no-merges`` now drops merge commits from the replay list. Merge commits typically carry no content of their own; the non-merge commits we keep collectively reproduce the final tree, and ``git blame`` after the merge boundary still walks back to original authors because authorship is preserved on the non-merge commits. * The dropped-merge count is reported on stderr so reviewers see what was elided. * Defense-in-depth: if a merge ever reaches the cherry-pick loop anyway (e.g. someone calls the script with a custom rev range), detect it via the parent count from ``%P`` and pass ``-m 1`` so cherry-pick succeeds rather than crashing the run. Surfaced while ingesting ITKMGHIO; would similarly bite any module whose surviving history contains a merge commit.
Drop the standalone-build conditional inherited from the remote-module days; once in-tree, the only path that matters is itk_module_impl(). Also drop the redundant cmake_minimum_required (ITK's root sets it) and the project() VERSION/LANGUAGES (the ITK module system supplies both via DEPENDS). Brings the file in line with siblings such as Modules/Filtering/GenericLabelInterpolator/CMakeLists.txt (post-ingest) and the established core IO modules (e.g. Modules/IO/HDF5/CMakeLists.txt). Addresses @dzenanz review comment on PR InsightSoftwareConsortium#6137.
hjmjohnson
force-pushed
the
ingest-MGHIO
branch
from
bedf142 to
a180130
Compare
Member
Author
|
@dzenanz Addressed in 674e55d: the ingest merge commit ( |
* end-of-file-fixer stripped a trailing blank line * gersemi reformatted the file to match ITK's house CMake style (block-arg layout, lowercase CMake command names) Both hooks fired on the inherited upstream test driver.
dzenanz
approved these changes
Apr 27, 2026
gersemi reformatted Modules/IO/MGHIO/itk-module.cmake and Modules/IO/MGHIO/src/CMakeLists.txt to match ITK's house CMake style; the prior pass at this had only covered the test/ tree.
hjmjohnson
merged commit 2e61183
into
InsightSoftwareConsortium:main
14 of 16 checks passed
hjmjohnson
added a commit
to InsightSoftwareConsortium/ITKMGHImageIO
that referenced
this pull request
Apr 28, 2026
The MGHIO module was ingested into the main ITK source tree on 2026-04-28 via InsightSoftwareConsortium/ITK#6137 (merge commit 2e61183c46). Per the ITK ingestion strategy (Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md), the one-definition rule requires that any file now living at Modules/IO/MGHIO/ in ITK no longer live at this upstream repo's tree tip. This commit: - Removes the whitelisted files that transferred during ingest: include/, src/, test/, wrapping/, CMakeLists.txt, itk-module.cmake, CTestConfig.cmake. - Adds MIGRATION_README.md at the repo root pointing users at the authoritative in-tree ITK location and linking the ingest PR. - Leaves the pre-ingest history intact so blame, paper material, and historical references remain reachable. After this PR merges, this repository will be marked ARCHIVED via GitHub's repository settings (Danger Zone -> Archive this repository), making it read-only. Cloning ITK main gets the authoritative MGHIO module under Modules/IO/MGHIO/. Cloning this repo after archival gets only the MIGRATION_README pointer plus everything that was deliberately not migrated.
This was referenced Apr 28, 2026
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The upstream remote module's top-level CMakeLists.txt carried a dual-mode preamble that supports both standalone (find_package(ITK) + ITKModuleExternal) and in-tree (itk_module_impl) builds. Once the module lives under Modules/Filtering/, only the in-tree branch is reachable -- the standalone branch is dead code, the cmake_minimum_required and project() lines duplicate what ITK's root CMake already owns. Collapse the file to a single itk_module_impl() call, matching every other in-tree Modules/<Group>/<Name>/ module. Per the standalone-build cleanup convention documented in Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md under "Standalone-build boilerplate cleanup (modes A/B)" and applied previously to GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The module now lives at Modules/Filtering/FastBilateral/. The configure-time fetch declaration in Modules/Remote/ would otherwise clone the (soon-to-be-archived) upstream and double-define the module. Mirrors the in-tree transition completed for GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The upstream remote module's top-level CMakeLists.txt carried a dual-mode preamble that supports both standalone (find_package(ITK) + ITKModuleExternal) and in-tree (itk_module_impl) builds. Once the module lives under Modules/Filtering/, only the in-tree branch is reachable -- the standalone branch is dead code, the cmake_minimum_required and project() lines duplicate what ITK's root CMake already owns. Collapse the file to a single itk_module_impl() call, matching every other in-tree Modules/<Group>/<Name>/ module. Per the standalone-build cleanup convention documented in Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md under "Standalone-build boilerplate cleanup (modes A/B)" and applied previously to GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The module now lives at Modules/Filtering/FastBilateral/. The configure-time fetch declaration in Modules/Remote/ would otherwise clone the (soon-to-be-archived) upstream and double-define the module. Mirrors the in-tree transition completed for GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
This was referenced Apr 28, 2026
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The upstream remote module's top-level CMakeLists.txt carried a dual-mode preamble that supports both standalone (find_package(ITK) + ITKModuleExternal) and in-tree (itk_module_impl) builds. Once the module lives under Modules/Filtering/, only the in-tree branch is reachable -- the standalone branch is dead code, the cmake_minimum_required and project() lines duplicate what ITK's root CMake already owns. Collapse the file to a single itk_module_impl() call, matching every other in-tree Modules/<Group>/<Name>/ module. Per the standalone-build cleanup convention documented in Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md under "Standalone-build boilerplate cleanup (modes A/B)" and applied previously to GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The module now lives at Modules/Filtering/FastBilateral/. The configure-time fetch declaration in Modules/Remote/ would otherwise clone the (soon-to-be-archived) upstream and double-define the module. Mirrors the in-tree transition completed for GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
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.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
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.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
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.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
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.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ingests
ITKMGHIO(FreeSurfer.mgh/.mgzImageIO) from the standalone remote module intoModules/IO/MGHIO/, freshly redone on top ofupstream/mainusing the v3 ingestion tooling atUtilities/Maintenance/RemoteModuleIngest/. Includes the per-commit pre-commit replay step (now working — fixed in this PR) and removes the previousModules/Remote/MGHIO.remote.cmakefetch declaration; opts the module into CI.What this PR is
v3-tooling) ingest. Completely redone on top of latestupstream/mainusing the currentingest-remote-module.shdriver and the published whitelist + scaffolding-deny passes.af74507c1ddc82722637c37d1f5d169e3000553a.MODULE_COMPLIANCE_LEVEL 2declaration).Modules/IO/MGHIO/.Ingest mode & metrics
include/,src/,test/,wrapping/,CMakeLists.txt,itk-module.cmake).md5/.shaNNN/.cidverify-cid-access.sh Modules/IO/MGHIOAfter the per-commit replay (see next section) the linearized history retains 60 non-merge upstream commits with original
author/author-datepreserved, plus 4 follow-up commits in this PR.Per-commit pre-commit replay (now working — bug fixed in this PR)
Utilities/Maintenance/RemoteModuleIngest/normalize-ingest-commits.pypreviously crashed on the very first merge commit because it invokedgit cherry-pickwithout-m N. This PR includes the fix as1f70efe288:git rev-list --no-mergesfilters merge commits out of the replay list. Merge commits typically carry no content of their own; the non-merge commits we keep collectively reproduce the final tree, andgit blameafter the merge boundary still walks back to original authors because authorship is preserved on every commit.%Pis checked and-m 1is passed automatically.Replay result for this branch:
Two commits had subjects without an ITK prefix (
BUG:/COMP:/DOC:/ENH:/PERF:/STYLE:) and were normalized; the original subject is preserved asOriginal subject: <text>in the body. One historical style-only commit became empty under today's hooks and was dropped (today's hooks would have prevented it from existing in the first place).Already-ingested modules (
AnisotropicDiffusionLBR,GenericLabelInterpolator) can now be re-normalized in a follow-up sweep using the same fixed tooling.Commit chain on this branch
1f70efe288COMP: Fix normalize-ingest-commits.py merge-commit handling0e87992cf7ENH: Enable MGHIO in CI via configure-ci86a1a89951COMP: Remove MGHIO.remote.cmake; now in-tree7be615e423DOC: Add README.md pointing at archived upstream for MGHIO…(60 commits)author/author-datepreservedb1bd26497eENH: Adding Remote Module for ITKv4(oldest replayed upstream commit, 2013-05-16)git blameon any line inModules/IO/MGHIO/walks back to the original upstream author.Verification done locally
ingest-remote-module.sh MGHIO IO --dry-run --keep-tempdir— clean, whitelist-verification pass green.ingest-remote-module.sh MGHIO IO— merge created, 4 follow-up commits added.verify-cid-access.sh Modules/IO/MGHIO— all 3 CIDs resolved from cache + IPFS.normalize-ingest-commits.py --base upstream/main— completed cleanly with the fix; kept=62, subject-rewritten=2, dropped-empty=1.gh pr viewreports 19 files changed (15 ingested + 1 README +MGHIO.remote.cmakedeletion +pyproject.tomlenable +normalize-ingest-commits.pyfix).