ENH: Ingest ITKIOTransformDCMTK into Modules/IO#6310
Merged
dzenanz merged 83 commits intoMay 25, 2026
Conversation
This commit updates CMakeLists.txt to ensure built library is shared. This commit introduces explicit instantiation where templated factories are explicitly instantiated for "double" and "float" types. This will allow templated factories to be registered in one translation unit and instantiated in an other. See http://stackoverflow.com/questions/8024010/why-do-template-class-functions-have-to-be-declared-in-the-same-translation-unit See ITK issue InsightSoftwareConsortium#3393 https://issues.itk.org/jira/browse/ITK-3393
BUG: Ensure factory is registered once.
This commit updates explicit instantiation to use approach similar to what was done in InsightSoftwareConsortium/ITK@b72726c (BUG: Explicitly instantiate common MetaDataObjects). While "extern" is only part of the c++ standard starting with c++11 (see section 14.7.2 of the standard), it is supported by Visual Studio 2008, GCC and Clang using compiler specific extension in earlier version: * Visual Studio 2008: https://msdn.microsoft.com/en-us/library/by56e477(v=vs.90).aspx * Gcc: https://gcc.gnu.org/onlinedocs/gcc-4.8.4/gcc/Template-Instantiation.html#Template-Instantiation * Clang (since v2.9): http://clang.llvm.org/cxx_status.html For completeness, initial draft of the "extern" keyword were first introduced in 2003 [1] and then revised in 2006 [2], and ultimately standardized in c++11 [3]: [1] N1448: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2003/n1448.pdf [2] N1987: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n1987.htm [3] N3242: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/ See ITK issue InsightSoftwareConsortium#3393 https://issues.itk.org/jira/browse/ITK-3393 Suggested-by: Bradley Lowekamp <blowekamp@mail.nih.gov>
3393 transform image io as shared
This commit fixes warnings introduced in InsightSoftwareConsortium/ITK@eb2316a (BUG: TransformIO: Implement explicit template instantiation using "extern".) by handling version of GCC not supporting "pragma diagnostic" preprocessor instruction. It does so by using the new ITK macros introduced in ITK commit (COMP: TransformIO: Handle compiler not supporting "pragma diagnostic".)
COMP: Handle compiler not supporting "pragma diagnostic".
This commit fixes the approach introduced in InsightSoftwareConsortium/ITK@eb2316a (BUG: TransformIO: Implement explicit template instantiation using "extern".). By moving the use of custom EXPORT_EXPLICIT macro *OUTSIDE* the header guards, it ensures the defined and undefined states of ITK_TEMPLATE_EXPLICIT_<classname> macro are considered within the same compilation unit.
Must also update / improve the library link specification to avoid linker errors.
…on-approa ch 3393 update explicit instantiation approach
Correction of bug ITK-3459 [1]. EXERCISE_BASIC_OBJECT_METHODS now requires 3 arguments instead of 2. The new argument is the Superclass name. [1] https://issues.itk.org/jira/browse/ITK-3459
Brings IOTransformDCMTK from a configure-time remote fetch into the ITK source tree at Modules/IO/IOTransformDCMTK/ using the v4 ingestion pipeline (whitelist filter-repo + per-commit clang-format + black + commit-prefix sanitization). Upstream repo: https://github.com/InsightSoftwareConsortium/ITKIOTransformDCMTK.git Upstream tip: acc3378f1d5207ae5bbc8d590616ceb70fe3c1c9 Ingest date: 2026-05-20 Whitelist: default.list Per-commit transforms applied across all 61 commits: - filter-repo --paths-from-file (whitelist) - filter-repo --to-subdirectory-filter Modules/IO/IOTransformDCMTK - 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: 17 -> 12 merge(s). Primary author: Matt McCormick <matt.mccormick@kitware.com> Co-authored-by: Dženan Zukić <dzenan.zukic@kitware.com> Co-authored-by: Francois Budin <francois.budin@gmail.com> 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: Jean-Christophe Fillion-Robin <jchris.fillionr@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: Tom Birdsong <tom.birdsong@kitware.com> Co-authored-by: Zach Williamson <zachary-williamson@uiowa.edu>
Modules/IO/IOTransformDCMTK is EXCLUDE_FROM_DEFAULT and requires Module_ITKDCMTK. README documents the gating and ingest provenance. Also remove Modules/Remote/IOTransformDCMTK.remote.cmake; the module is now in-tree and the configure-time fetch is obsolete.
Upstream IOTransformDCMTK CMakeLists used the dual-mode external-build boilerplate (find_package(ITK) fallback) and add_library + manual itk_module_target. As an in-tree module, the standard itk_module_add_library macro propagates include dirs and link deps automatically (matches MGHIO and other in-tree IO modules).
itk-module.cmake DESCRIPTION was a placeholder string; replace with a one-line summary of the module's purpose (DICOM Spatial-Registration reading via DCMTK). DCMTKTransformIO class doxygen had a bare \todo; replace with a brief description of which DICOM SOP class is recognized, how the matrix type drives transform-class selection, and the role of the FrameOfReferenceUID filter.
The ingested upstream description was a placeholder string. Replace with a one-line summary of what the module reads (DICOM Spatial-Registration Objects).
- Guard fileFormat.getDataset() and three getItem() return values against nullptr before dereferencing; DCMTK returns nullptr on malformed sequences. - Remove dead 'break' after itkExceptionMacro (which throws); the break is unreachable. - ReadDicomTransformAndResampleExample required 5 positional args plus argv[0] but checked argc < 5; reading argv[5] with the minimal 6-arg invocation would have read past the array bound. Tighten to argc < 6.
DCMTKTransformIOFactory::PrintSelf was empty and did not chain to Superclass; replace with a Superclass-only forward (the factory holds no extra state). Also normalize the stray 'Factory ::' spacing. DCMTKTransformIO declared no PrintSelf override despite carrying m_FrameOfReferenceUID state; add a declaration + implementation that prints the desired Frame-of-Reference UID after the base class print.
The upstream .wrap used itk_wrap_simple_class, which targets a non-templated class. DCMTKTransformIO is templated on the internal computation value type; mirror the HDF5TransformIO wrapping pattern (itk_wrap_class + itk_wrap_template for D and F) so ITK_WRAP_PYTHON builds resolve the symbol.
DICOM Decimal-String values always use '.' as decimal separator; on a comma-locale host (e.g. de_DE), std::stod would truncate matrix entries at the first '.'. Wrap the Read() body in an itk::NumericLocale RAII scope guard (thread-safe via uselocale/_configthreadlocale per platform) so the parse is locale-independent. Mirrors the NumericLocale usage in itkNrrdImageIO.
…QUAL ITK convention across Modules/IO/*/test/ puts the observed value first and the expected value second in ITK_TEST_EXPECT_EQUAL. Two size() comparisons in itkDCMTKTransformIOTest had the literal expected count first; swap. Also add a brief comment naming the fixture's MatrixRegistration structure so the literal counts read self-explanatorily.
OFString provides an operator==(const char *) overload; replace the explicit .compare(...) == 0 form with the natural equality comparison.
The orphan comment sat between the closing namespace and the header guard #endif, separated from the actual extern template declarations it described by the include guard boundary. The instantiations below are self-evident.
IOTransformDCMTK is EXCLUDE_FROM_DEFAULT and gated on Module_ITKDCMTK, so without an explicit opt-in no CDash dashboard would compile or test it post-ingest, leaving the in-tree module without coverage. Enable both modules in pyproject.toml's configure-ci task so the test driver runs on at least one nightly configuration.
Addresses ITK#6308 (migrated from upstream ITKIOTransformDCMTK#5): - Replace early-return ITK_TEST_EXPECT_TRUE/EQUAL with the _STATUS_VALUE variants so all checks run on every invocation. - Add ITK_TEST_SET_GET_VALUE coverage for FrameOfReferenceUID before and after Set, exercising the macro pair. - Use 'auto' for SmartPointer locals to remove explicit ::Pointer noise. - Replace IsNull() with explicit nullptr comparison (raw pointer from dynamic_cast). - Print 'Test finished.' before returning testStatus, matching the ITK testing convention.
Replace per-step raw try/catch + return EXIT_FAILURE blocks with ITK_TRY_EXPECT_NO_EXCEPTION; replace if(!ExposeMetaData) early returns with ITK_TEST_EXPECT_TRUE_STATUS_VALUE accumulating into testStatus. Drop the commented-out DCMTKImageIO and DCMTKSeriesFileNames type aliases (GDCM is the only path; the MetaDataDictionary-population gap for DCMTKImageIO is a separate concern). Use 'auto' uniformly for SmartPointer locals.
Rename ReadDicomTransformAndResampleExample.cxx -> itkDCMTKTransformIOResampleTest.cxx and rename the entry-point function and ctest name to match. Aligns with ITK's itk<TypeUnderTest>Test.cxx convention.
DCMTK is a transitive dependency of IOTransformDCMTK; users only need to set Module_IOTransformDCMTK=ON. Mentioning Module_ITKDCMTK explicitly in the enable example confuses readers about which knob is doing the work.
The override only called Superclass::PrintSelf(os, indent), which is exactly what happens by default when no override is declared. Removing it eliminates dead-code noise without changing print behavior.
Drop the explicit WRAPPER_AUTO_INCLUDE_HEADERS=OFF guard and the
hand-written itk_wrap_include("itkDCMTKTransformIO.h"). The class
name itk::DCMTKTransformIO maps to the canonical itkDCMTKTransformIO.h
header via the default name-derivation rule, so no override is
needed here. Precedents like HDF5TransformIO keep the override only
because they wrap two classes that share a header.
Two functional defects in DCMTKTransformIO::Read() flagged in greptile review of InsightSoftwareConsortium#6310: 1. RIGID_SCALE matrices were stored in a ScaleTransform via SetMatrix. ScaleTransform models axis-aligned scaling only; its inherited MatrixOffsetTransformBase::SetMatrix path writes the matrix without updating m_Scale, so GetParameters() returns stale defaults that do not describe the rotation+isotropic-scale matrix in the file. Spatial mapping was correct via raw-matrix application but parameter inspection and serialization were wrong. DICOM RIGID_SCALE is rotation + isotropic scale + translation (7 DOF). Use Similarity3DTransform, whose SetMatrix(m, tol) recovers the isotropic scale as cbrt(det(m)) and the rotation via polar decomposition. 2. When DCM_FrameOfReferenceTransformationMatrixType was absent the fully-parsed matrix was silently discarded with no exception and no warning, returning a CompositeTransform with no children. Throw instead, naming the offending matrix-sequence index. The unused OFString seriesNumber local in CanReadFile and the unused itkVersorTransform.h include are removed in the same commit; the new RIGID_SCALE path needs itkSimilarity3DTransform.h instead. A new GoogleTest-style unit test itkDCMTKTransformIORigidScaleRoundTripTest verifies the polar- decomposition path: given a known versor + isotropic scale + translation, build the equivalent rotation*scale matrix as an AffineTransform (mimicking what Read() assembles from the DICOM matrix entries), apply the same Similarity3DTransform::SetMatrix + SetOffset calls the IO now uses, and confirm versor, scale, translation, and mapped point coordinates all round-trip within 1e-10.
hjmjohnson
force-pushed
the
ingest-IOTransformDCMTK
branch
from
596964a to
1ab5384
Compare
Member
Author
|
Windows Pixi failure is a DCMTK/MSVC Symptom
The Root causeDCMTK is built with This is the well-known MSVC Recommended fixEnsure # Modules/IO/DCMTK/src/CMakeLists.txt
target_compile_options(ITKIODCMTK PUBLIC
$<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus>)
|
DCMTK's generated osconfig.h guards C++11/14/17 on the __cplusplus macro. MSVC reports 199711L unless /Zc:__cplusplus is passed, so every DCMTK consumer fails with C1189 even when built as C++17. DCMTK sets the flag for its own build but does not export it to consumers. Attach it to the ITKDCMTKModule INTERFACE so all consumers inherit it.
github-actions
Bot
added
the
area:ThirdParty
ITKIODCMTK public headers include DCMTK headers, so ITKDCMTK is an interface-level dependency. Declaring it public (not PRIVATE_DEPENDS) propagates ITKDCMTKModule usage requirements -- notably the MSVC /Zc:__cplusplus flag -- to the module's test and header-test targets, which otherwise fail with C1189 from osconfig.h.
dzenanz
approved these changes
May 25, 2026
dzenanz
merged commit f117adb
into
InsightSoftwareConsortium:main
19 of 20 checks passed
97 tasks
hjmjohnson
added a commit
to InsightSoftwareConsortium/ITKIOTransformDCMTK
that referenced
this pull request
May 27, 2026
The IOTransformDCMTK module has been ingested into ITK main under Modules/IO/IOTransformDCMTK via InsightSoftwareConsortium/ITK#6310. Delete the whitelisted module sources, preserve the original README, and promote the migration notice to README.md so the GitHub landing page reflects archived status.
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
ITKIOTransformDCMTK(DICOM Spatial-Registration reader, DCMTK-based) from the standalone remote module intoModules/IO/IOTransformDCMTK/using the v4 merge-topology-preserving pipeline.EXCLUDE_FROM_DEFAULT, hardDEPENDS ITKDCMTK. Closes the migrated test-improvement work in #6308 and surfaces a follow-up audit in #6309.Ingest provenance & topology
9d7f4a56(2026-04-23).Utilities/Maintenance/RemoteModuleIngest/ingest-module-v4.sh IOTransformDCMTK IO(filter-repo whitelist + scaffolding deny-pass + per-commit sanitize-history.py + Mode-A--no-ff --allow-unrelated-historiesmerge).git rev-list --count --merges upstream/main..HEAD).Modules/Remote/IOTransformDCMTK.remote.cmakeconfigure-time fetch is removed in the same PR.Why EXCLUDE_FROM_DEFAULT + DEPENDS ITKDCMTK
IOTransformDCMTKrequires the DCMTK third-party library (Module_ITKDCMTK). The module'sitk-module.cmakedeclaresDEPENDS ITKDCMTKandEXCLUDE_FROM_DEFAULT; the readonly test-only deps (ITKIODCMTK,ITKIOGDCM,ITKImageGrid,ITKTestKernel) cover both the DCMTK transform-read path and the GDCM-based example driver. Enabling the module is opt-in:pyproject.toml'sconfigure-citask is updated to set both, so at least one CDash nightly configuration exercises the module post-merge.Review-driven fixes on top of the raw ingest
The raw upstream module was a 2-star compliance level. A pre-PR greptile-style review surfaced ten concrete issues; the following are addressed before this PR opens (commit hashes are on the branch):
6e1b61db46find_package(ITK)CMakeLists to in-treeitk_module_impl()+itk_module_add_library2dab47c240README.mddocumenting in-tree provenance54f09020a6\todo Detailed description.in class doxygen with SRO + matrix-type behaviorb7f470d596itk-module.cmake DESCRIPTIONb2d99d5e3eDCMTK getDataset()/getItem()returns; drop deadbreakafteritkExceptionMacro(which throws); fixargc < 5off-by-one (argv[5]accessed)b55645c8e9PrintSelfon the templated IO + the factory (was empty, missingSuperclass::PrintSelf)cc5ac212a4DCMTKTransformIOas templated class forDandFinstead ofitk_wrap_simple_class(was wrong forITK_WRAP_PYTHON)c56af3a5b8LC_NUMERIC=Cviaitk::NumericLocaleRAII forstd::stodmatrix parsing4b5dd2707dModule_IOTransformDCMTK/Module_ITKDCMTKinpyproject.tomlconfigure-ci1ff3c0a035ITK_TEST_EXPECT_EQUAL+ name fixture in commenta92e2aec52OFString::compare(...) == 0withoperator==2ce6713f18ITK#6308 — test improvements (migrated from ITKIOTransformDCMTK#5)
Migrated the upstream test-improvement issue
ITKIOTransformDCMTK#5(originally opened by @jhlegarreta) to ITK as #6308 and addressed three of four bullets in this PR:f77c0a9187—itkDCMTKTransformIOTestuses_STATUS_VALUEmacros +testStatusaccumulator; addsITK_TEST_SET_GET_VALUEforFrameOfReferenceUID.7944060ee0—ReadDicomTransformAndResampleExample.cxxrefactored toITK_TRY_EXPECT_NO_EXCEPTION+ITK_TEST_EXPECT_TRUE_STATUS_VALUE; commented-outDCMTKImageIO/DCMTKSeriesFileNamesblocks dropped. (Surfaced a SmartPointer-lifetime bug caught only via the image-baseline--comparestep.)94f421a776— RenameReadDicomTransformAndResampleExample.cxx→itkDCMTKTransformIOResampleTest.cxx(file + function + ctest name) to match ITK convention.The remaining bullet (status-accumulating exception macros) requires changes to
ITKTestKernel— out of scope here; left open in #6308.Related followup — ITK#6309
Filed ITK#6309 to audit the GDCM→DCMTK fallback in the example test. The original "DCMTKImageIO does not populate the MetaDataDictionary yet" comment from upstream is now stale (resolved by
54887f7e9b+ follow-ups), but the example still usesGDCMImageIO/GDCMSeriesFileNamesto read the FoR UID. Swapping toDCMTK*requires API-parity verification onDCMTKSeriesFileNamesand a baseline-compare check.Local verification (build-standard, macOS arm64)
Post-merge follow-up actions
After this PR merges, run Phase B:
Phase B publishes the upstream removal commit + flips
archived=trueonInsightSoftwareConsortium/ITKIOTransformDCMTK. Per theingest-archive-readmerule, the archived repo's GitHub-rendered README must be promoted fromMIGRATION_README.mdtoREADME.md(with the originalREADME.rst→info.rst).