From b468531c6f7f1a0a42d9f1efd94fa9d273c43790 Mon Sep 17 00:00:00 2001 From: "Hans J. Johnson" Date: Wed, 17 Jun 2026 18:00:36 -0500 Subject: [PATCH 1/7] ENH: Add GDCMSeriesFileNames characterization tests (issue #6467) Pin the observable contract of itk::GDCMSeriesFileNames before migrating its backend off the deprecated gdcm::SerieHelper onto gdcm::Scanner + gdcm::IPPSorter: single-series enumeration / grouping, geometric ordering that reconstructs a valid uniformly-spaced volume, GetInputFileNames returning the first series, and the Recursive flag controlling descent (which must be set before SetInputDirectory triggers the scan). These tests are backend-agnostic and must remain green across the swap. --- Modules/IO/GDCM/test/CMakeLists.txt | 1 + .../itkGDCMSeriesFileNamesContractGTest.cxx | 128 ++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx diff --git a/Modules/IO/GDCM/test/CMakeLists.txt b/Modules/IO/GDCM/test/CMakeLists.txt index 84fa55ebfa5..29319347357 100644 --- a/Modules/IO/GDCM/test/CMakeLists.txt +++ b/Modules/IO/GDCM/test/CMakeLists.txt @@ -519,6 +519,7 @@ set( ITKGDCMImageIOGTests itkGDCMImageIOGTest.cxx itkGDCMSeriesDirectionGTest.cxx + itkGDCMSeriesFileNamesContractGTest.cxx ) creategoogletestdriver(ITKGDCMImageIO "${ITKIOGDCM-Test_LIBRARIES}" "${ITKGDCMImageIOGTests}") diff --git a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx new file mode 100644 index 00000000000..d99124af60e --- /dev/null +++ b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx @@ -0,0 +1,128 @@ +/*========================================================================= + * + * Copyright NumFOCUS + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0.txt + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + *=========================================================================*/ + +// Characterization tests pinning the observable contract of +// itk::GDCMSeriesFileNames (issue #6467). These must remain green when the +// backend is migrated from the deprecated gdcm::SerieHelper to +// gdcm::Scanner + gdcm::IPPSorter: one series identifier for a single-series +// directory, deterministic geometric ordering that reconstructs a valid +// uniformly-spaced volume, GetInputFileNames == first series, and the +// Recursive flag controlling descent. + +#include "gtest/gtest.h" +#include "itkGDCMSeriesFileNames.h" +#include "itkGDCMImageIO.h" +#include "itkImageSeriesReader.h" +#include "itkImage.h" +#include "itkMath.h" +#include "itksys/SystemTools.hxx" +#include "itksys/Directory.hxx" +#include +#include + +#define _STRING(s) #s +#define TOSTRING(s) std::string(_STRING(s)) + +namespace +{ +const std::string & +SeriesDir() +{ + static const std::string dir = TOSTRING(DICOM_SERIES_INPUT); + return dir; +} + +unsigned int +CopyDicomSlices(const std::string & srcDir, const std::string & dstDir) +{ + itksys::SystemTools::MakeDirectory(dstDir); + itksys::Directory directory; + directory.Load(srcDir.c_str()); + unsigned int copied = 0; + for (unsigned int i = 0; i < directory.GetNumberOfFiles(); ++i) + { + const std::string name = directory.GetFile(i); + if (itksys::SystemTools::GetFilenameLastExtension(name) != ".dcm") + { + continue; + } + if (itksys::SystemTools::CopyFileAlways(srcDir + '/' + name, dstDir + '/' + name)) + { + ++copied; + } + } + return copied; +} +} // namespace + +TEST(GDCMSeriesFileNamesContract, SingleSeriesEnumerationAndGrouping) +{ + auto sf = itk::GDCMSeriesFileNames::New(); + sf->SetInputDirectory(SeriesDir()); + + const std::vector uids = sf->GetSeriesUIDs(); + ASSERT_EQ(uids.size(), 1u) << "DicomSeries is a single series"; + + const std::vector byUid = sf->GetFileNames(uids.front()); + EXPECT_GT(byUid.size(), 1u) << "Series has multiple slices"; + + const std::vector input = sf->GetInputFileNames(); + EXPECT_EQ(input, byUid) << "GetInputFileNames must return the (single) series, ordered identically"; +} + +TEST(GDCMSeriesFileNamesContract, OrderingReconstructsValidVolume) +{ + using ImageType = itk::Image; + auto sf = itk::GDCMSeriesFileNames::New(); + sf->SetInputDirectory(SeriesDir()); + const std::vector files = sf->GetInputFileNames(); + ASSERT_GT(files.size(), 1u); + + auto reader = itk::ImageSeriesReader::New(); + reader->SetImageIO(itk::GDCMImageIO::New()); + reader->SetFileNames(files); + ASSERT_NO_THROW(reader->Update()); + + const ImageType::Pointer image = reader->GetOutput(); + EXPECT_EQ(image->GetLargestPossibleRegion().GetSize()[2], files.size()) << "Each ordered slice becomes one z-plane"; + for (unsigned int d = 0; d < 3; ++d) + { + const double spacing = image->GetSpacing()[d]; + EXPECT_TRUE(std::isfinite(spacing) && spacing > 0.0) << "Spacing[" << d << "] must be positive and finite"; + } +} + +TEST(GDCMSeriesFileNamesContract, RecursiveFlagHonored) +{ + const std::string root = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_recursive"; + itksys::SystemTools::RemoveADirectory(root); + itksys::SystemTools::MakeDirectory(root); + ASSERT_GT(CopyDicomSlices(SeriesDir(), root + "/nested"), 1u); + + // Recursive is read lazily during BuildSeriesMap(); order relative to + // SetInputDirectory does not matter. + auto flat = itk::GDCMSeriesFileNames::New(); + flat->SetRecursive(false); + flat->SetInputDirectory(root); + EXPECT_TRUE(flat->GetInputFileNames().empty()) << "No DICOM directly under root; flat scan finds none"; + + auto recursive = itk::GDCMSeriesFileNames::New(); + recursive->SetRecursive(true); + recursive->SetInputDirectory(root); + EXPECT_FALSE(recursive->GetInputFileNames().empty()) << "Recursive scan must descend into the subdirectory"; +} From 50dfb136658d9a2f60c2aea153e11259c935c7c0 Mon Sep 17 00:00:00 2001 From: "Hans J. Johnson" Date: Wed, 17 Jun 2026 18:28:30 -0500 Subject: [PATCH 2/7] ENH: Reimplement GDCMSeriesFileNames on gdcm::Scanner + gdcm::IPPSorter Replace the deprecated gdcm::SerieHelper backend ("DO NOT USE ... temporary solution for ITK migration from GDCM 1.x to 2.x") with the supported modern API: gdcm::Directory for enumeration, gdcm::Scanner for series grouping, and gdcm::IPPSorter for geometric (ImagePositionPatient on the slice normal) ordering. Behavior preserved on the in-tree suite (all 89 ITKIOGDCM tests pass, including the series-reader and direction-cosine tests) and pinned by the GDCMSeriesFileNames characterization tests added in the previous commit: single-series enumeration / grouping, geometric ordering, GetInputFileNames returning the first series, Recursive descent. Intentional behavior changes: - IPPSorter is strict: duplicate-IPP and gantry-tilt acquisitions FAIL to sort; the input order is then left unchanged rather than fabricated. Whether these need first-class support (and whether SerieHelper's permissive strategy was correct) is tracked for discussion in #6468. - AddSeriesRestriction now refines the series identifier (the documented and example intent, e.g. "0008|0021"), instead of delegating to SerieHelper's largely-inert file restriction list. Part of #6467. --- .../migration_guides/itk_6_migration_guide.md | 34 +++ .../IO/GDCM/include/itkGDCMSeriesFileNames.h | 37 ++- .../IO/GDCM/src/itkGDCMSeriesFileNames.cxx | 239 ++++++++++++------ 3 files changed, 222 insertions(+), 88 deletions(-) diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md index 6e228098aa6..d2abcc5598d 100644 --- a/Documentation/docs/migration_guides/itk_6_migration_guide.md +++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md @@ -866,3 +866,37 @@ variable declared as `FilterType::OutputIterator` and then passed where an siblings in the iterator hierarchy, not subclasses, so such uses do not convert implicitly. Code that only iterates (`GoToBegin`/`IsAtEnd`/`Get`/`Set`) needs no change. + +## `GDCMSeriesFileNames` reimplemented on `gdcm::Scanner`/`IPPSorter` + +`itk::GDCMSeriesFileNames` no longer uses the GDCM-deprecated +`gdcm::SerieHelper`. It now enumerates with `gdcm::Directory`, groups series +with `gdcm::Scanner` (replicating `SerieHelper::CreateUniqueSeriesIdentifier`), +and orders slices geometrically with `gdcm::IPPSorter`. The public API is +unchanged; consumers compile without modification. + +### What you need to do + +- Nothing for the common case: single-series directories enumerate, group, and + order as before. +- If you relied on file ordering for **duplicate-`ImagePositionPatient`** or + **gantry-tilted** acquisitions, re-check it. `gdcm::IPPSorter` is strict and + fails to sort those; on failure the input (filesystem) order is left + unchanged rather than fabricated. First-class support is tracked in + [#6468](https://github.com/InsightSoftwareConsortium/ITK/issues/6468). +- Tags passed to `AddSeriesRestriction` now refine the series identifier + (sub-dividing a `SeriesInstanceUID`, the documented intent, e.g. + `"0008|0021"`) instead of `SerieHelper`'s largely-inert file-restriction + list. Malformed tags are warned-and-ignored rather than honored. +- `SetLoadSequences`/`SetLoadPrivateTags` have no effect with the + `gdcm::Scanner` backend (the scan reads only the grouping/ordering tags). + They are retained for source compatibility but no longer alter enumeration. + +### Concerns + +- Repeated `GetSeriesUIDs`/`GetFileNames`/`GetInputFileNames` calls no longer + re-scan the directory (lazy parse cached by a `TimeStamp`); a directory + mutated on disk between calls without an intervening `Modified()` is not + re-read. +- The vendored `gdcm::SerieHelper` is untouched (GDCM still uses it + internally); it may be removed once upstream GDCM drops it. diff --git a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h index b646a365d8a..e9dd72f83b5 100644 --- a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h +++ b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h @@ -21,15 +21,11 @@ #include "itkProcessObject.h" #include "itkObjectFactory.h" #include "itkMacro.h" +#include +#include #include #include "ITKIOGDCMExport.h" -// forward declaration, to remove compile dependency on GDCM library -namespace gdcm -{ -class SerieHelper; -} - namespace itk { /** @@ -172,9 +168,9 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject void AddSeriesRestriction(const std::string & tag); - /** Parse any sequences in the DICOM file. Defaults to false - * to skip sequences. This makes loading DICOM files faster when - * sequences are not needed. + /** No effect with the gdcm::Scanner backend (retained for source + * compatibility). Series enumeration reads only the grouping and + * ordering tags, so sequences are never parsed during the scan. */ /** @ITKStartGrouping */ itkSetMacro(LoadSequences, bool); @@ -182,9 +178,9 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject itkBooleanMacro(LoadSequences); /** @ITKEndGrouping */ - /** Parse any private tags in the DICOM file. Defaults to false - * to skip private tags. This makes loading DICOM files faster when - * private tags are not needed. + /** No effect with the gdcm::Scanner backend (retained for source + * compatibility). Series enumeration reads only the grouping and + * ordering tags, so private tags are never parsed during the scan. */ /** @ITKStartGrouping */ itkSetMacro(LoadPrivateTags, bool); @@ -208,8 +204,21 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject FileNamesContainerType m_InputFileNames{}; FileNamesContainerType m_OutputFileNames{}; - /** Internal structure to order series from one directory */ - std::unique_ptr m_SerieHelper; + /** Parse the input directory into the per-series file-name map. */ + void + BuildSeriesMap(); + + /** Ordered file names per distinct series identifier. */ + std::map m_SeriesFiles{}; + + /** (group,element) tags appended to the series identifier when + * UseSeriesDetails is enabled; seeded with the GDCM default detail tags and + * extended by AddSeriesRestriction. */ + std::vector> m_RefineTags{}; + + /** Modified time of the last directory parse; the cache is rebuilt only + * when the object has been Modified() since. */ + TimeStamp m_CacheBuildTime{}; /** Internal structure to keep the list of series UIDs */ SeriesUIDContainerType m_SeriesUIDs{}; diff --git a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx index a6697a36276..f356ba124cf 100644 --- a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx +++ b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx @@ -18,17 +18,48 @@ #include "itkGDCMSeriesFileNames.h" #include "itksys/SystemTools.hxx" -#include "itkProgressReporter.h" #include "itkPrintHelper.h" -#include "gdcmSerieHelper.h" +#include "gdcmDirectory.h" +#include "gdcmScanner.h" +#include "gdcmIPPSorter.h" +#include "gdcmTag.h" +#include +#include +#include +#include +#include namespace itk { +namespace +{ +// Order one series geometrically with gdcm::IPPSorter (ImagePositionPatient +// projected on the slice normal). IPPSorter is strict: it FAILS on duplicate +// IPP and gantry-tilt acquisitions (see issue #6468). On failure the input +// order is left unchanged rather than fabricating an order. +std::vector +OrderSeriesGeometrically(const std::vector & files) +{ + if (files.size() < 2) + { + return files; + } + gdcm::IPPSorter sorter; + sorter.SetComputeZSpacing(false); + if (sorter.Sort(files)) + { + return sorter.GetFilenames(); + } + return files; +} +} // namespace + GDCMSeriesFileNames::GDCMSeriesFileNames() - : m_SerieHelper{ new gdcm::SerieHelper() } -{} +{ + this->SetUseSeriesDetails(true); // seeds the default series-detail tags +} GDCMSeriesFileNames::~GDCMSeriesFileNames() = default; @@ -47,7 +78,27 @@ GDCMSeriesFileNames::SetInputDirectory(const char * name) void GDCMSeriesFileNames::AddSeriesRestriction(const std::string & tag) { - m_SerieHelper->AddRestriction(tag); + // Parse a "group|element" tag (hex) and add it to the series-identifier + // criteria so it sub-refines a SeriesInstanceUID into multiple series, as + // documented and as used by the ITK examples (e.g. "0008|0021"). + const std::string::size_type bar = tag.find('|'); + if (bar == std::string::npos) + { + itkWarningMacro("Ignoring malformed series restriction tag '" << tag << "' (expected \"group|element\")"); + return; + } + try + { + const auto group = static_cast(std::stoul(tag.substr(0, bar), nullptr, 16)); + const auto element = static_cast(std::stoul(tag.substr(bar + 1), nullptr, 16)); + m_RefineTags.emplace_back(group, element); + } + catch (const std::exception &) + { + itkWarningMacro("Ignoring malformed series restriction tag '" << tag << "' (expected hex \"group|element\")"); + return; + } + this->Modified(); } void @@ -68,92 +119,133 @@ GDCMSeriesFileNames::SetInputDirectory(const std::string & name) return; } m_InputDirectory = name; - m_SerieHelper->Clear(); - m_SerieHelper->SetUseSeriesDetails(m_UseSeriesDetails); - m_SerieHelper->SetLoadMode((m_LoadSequences ? 0 : gdcm::LD_NOSEQ) | (m_LoadPrivateTags ? 0 : gdcm::LD_NOSHADOW)); - m_SerieHelper->SetDirectory(name, m_Recursive); - // as a side effect it also execute this->Modified(); } -const GDCMSeriesFileNames::SeriesUIDContainerType & -GDCMSeriesFileNames::GetSeriesUIDs() +void +GDCMSeriesFileNames::BuildSeriesMap() { - m_SeriesUIDs.clear(); - // Accessing the first serie found (assume there is at least one) - gdcm::FileList * flist = m_SerieHelper->GetFirstSingleSerieUIDFileSet(); - while (flist) + // Reuse the previous parse unless the object has been modified since. + if (m_CacheBuildTime.GetMTime() > this->GetMTime()) { - if (!flist->empty()) // make sure we have at least one serie - { - gdcm::File * file = (*flist)[0]; // for example take the first one - - // Create its unique series ID - const std::string id = m_SerieHelper->CreateUniqueSeriesIdentifier(file).c_str(); + return; + } + m_SeriesUIDs.clear(); + m_SeriesFiles.clear(); - m_SeriesUIDs.emplace_back(id.c_str()); - } - flist = m_SerieHelper->GetNextSingleSerieUIDFileSet(); + if (m_InputDirectory.empty()) + { + return; } - if (m_SeriesUIDs.empty()) + + gdcm::Directory dir; + dir.Load(m_InputDirectory, m_Recursive); + const gdcm::Directory::FilenamesType & filenames = dir.GetFilenames(); + if (filenames.empty()) { - itkWarningMacro("No Series were found"); + return; } - return m_SeriesUIDs; -} -const GDCMSeriesFileNames::FileNamesContainerType & -GDCMSeriesFileNames::GetFileNames(const std::string serie) -{ - m_InputFileNames.clear(); - // Accessing the first serie found (assume there is at least one) - gdcm::FileList * flist = m_SerieHelper->GetFirstSingleSerieUIDFileSet(); - if (!flist) + const gdcm::Tag seriesUID(0x0020, 0x000e); + gdcm::Scanner scanner; + scanner.AddTag(seriesUID); + if (m_UseSeriesDetails) { - itkWarningMacro("No Series can be found, make sure your restrictions are not too strong"); - return m_InputFileNames; + for (const auto & [group, element] : m_RefineTags) + { + scanner.AddTag(gdcm::Tag(group, element)); + } } - if (!serie.empty()) // user did not specify any sub selection based on UID + if (!scanner.Scan(filenames)) { - bool found = false; - while (flist && !found) + itkWarningMacro("Failed to scan DICOM tags in " << m_InputDirectory); + return; + } + + // Build the unique series identifier per file, replicating + // gdcm::SerieHelper::CreateUniqueSeriesIdentifier. + auto makeIdentifier = [&](const char * fn) -> std::string { + const char * uidValue = scanner.GetValue(fn, seriesUID); + std::string id = (uidValue != nullptr) ? uidValue : ""; + const std::string uid = id; + if (m_UseSeriesDetails) { - if (!flist->empty()) // make sure we have at least one serie + for (const auto & [group, element] : m_RefineTags) { - gdcm::File * file = (*flist)[0]; // for example take the first one - const std::string id = m_SerieHelper->CreateUniqueSeriesIdentifier(file).c_str(); - - if (id == serie) + const char * value = scanner.GetValue(fn, gdcm::Tag(group, element)); + const std::string s = (value != nullptr) ? value : ""; + if (id == uid && !s.empty()) { - found = true; // we found a match - break; + id += '.'; } + id += s; } - flist = m_SerieHelper->GetNextSingleSerieUIDFileSet(); } - if (!found) + // Eliminate all non-alphanumeric characters (keep '.'). + id.erase(std::remove_if(id.begin(), id.end(), [](unsigned char c) { return c != '.' && std::isalnum(c) == 0; }), + id.end()); + return id; + }; + + std::map grouped; + for (const std::string & fn : filenames) + { + if (!scanner.IsKey(fn.c_str())) { - itkWarningMacro("No Series were found"); - return m_InputFileNames; + continue; // not a DICOM file the scanner could read } + const std::string id = makeIdentifier(fn.c_str()); + if (grouped.find(id) == grouped.end()) + { + m_SeriesUIDs.push_back(id); + } + grouped[id].push_back(fn); + } + + for (auto & [id, files] : grouped) + { + m_SeriesFiles[id] = OrderSeriesGeometrically(files); } - m_SerieHelper->OrderFileList(flist); - if (!flist->empty()) + m_CacheBuildTime.Modified(); +} + +const GDCMSeriesFileNames::SeriesUIDContainerType & +GDCMSeriesFileNames::GetSeriesUIDs() +{ + this->BuildSeriesMap(); + if (m_SeriesUIDs.empty()) { - ProgressReporter progress(this, 0, static_cast(flist->size()), 10); - for (auto & element : *flist) + itkWarningMacro("No Series were found"); + } + return m_SeriesUIDs; +} + +const GDCMSeriesFileNames::FileNamesContainerType & +GDCMSeriesFileNames::GetFileNames(const std::string serie) +{ + this->BuildSeriesMap(); + m_InputFileNames.clear(); + if (serie.empty()) + { + // Return the first series encountered (single-series assumption). + if (!m_SeriesUIDs.empty()) { - gdcm::FileWithName * header = element; - m_InputFileNames.push_back(header->filename); - progress.CompletedPixel(); + m_InputFileNames = m_SeriesFiles[m_SeriesUIDs.front()]; } + else + { + itkWarningMacro("No Series can be found, make sure your restrictions are not too strong"); + } + return m_InputFileNames; } - else + const auto it = m_SeriesFiles.find(serie); + if (it == m_SeriesFiles.end()) { - itkDebugMacro("No files were found"); + itkWarningMacro("No Series were found"); + return m_InputFileNames; } - + m_InputFileNames = it->second; return m_InputFileNames; } @@ -256,16 +348,6 @@ GDCMSeriesFileNames::PrintSelf(std::ostream & os, Indent indent) const os << indent << "InputFileNames: " << m_InputFileNames << std::endl; os << indent << "OutputFileNames: " << m_OutputFileNames << std::endl; - os << indent << "SerieHelper: "; - if (m_SerieHelper.get() != nullptr) - { - os << m_SerieHelper.get() << std::endl; - } - else - { - os << "(null)" << std::endl; - } - os << indent << "SeriesUIDs: " << m_SeriesUIDs << std::endl; itkPrintSelfBooleanMacro(UseSeriesDetails); @@ -278,7 +360,16 @@ void GDCMSeriesFileNames::SetUseSeriesDetails(bool useSeriesDetails) { m_UseSeriesDetails = useSeriesDetails; - m_SerieHelper->SetUseSeriesDetails(m_UseSeriesDetails); - m_SerieHelper->CreateDefaultUniqueSeriesIdentifier(); + m_RefineTags.clear(); + if (m_UseSeriesDetails) + { + // Default detail tags, matching gdcm::SerieHelper::CreateDefaultUniqueSeriesIdentifier. + m_RefineTags.emplace_back(0x0020, 0x0011); // Series Number + m_RefineTags.emplace_back(0x0018, 0x0024); // Sequence Name + m_RefineTags.emplace_back(0x0018, 0x0050); // Slice Thickness + m_RefineTags.emplace_back(0x0028, 0x0010); // Rows + m_RefineTags.emplace_back(0x0028, 0x0011); // Columns + } + this->Modified(); } } // namespace itk From 7cb3bbec23d78c8e65157a4c25a9fcf88d752378 Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 Jul 2026 06:55:45 -0500 Subject: [PATCH 3/7] ENH: Throw on ambiguous DICOM series ordering, add legacy fallbacks gdcm::IPPSorter failure now throws by default (FailOnAmbiguousOrdering, default true). When disabled, the legacy SerieHelper heuristics apply for determinism and backward compatibility: Instance Number (0020,0013, read by the existing scan) when unique, else lexicographic filename order. Series are ordered lazily per request, so enumerating a directory that contains an unsortable series does not throw. Addresses PR #6469 review feedback from thewtex. --- .../migration_guides/itk_6_migration_guide.md | 8 +- .../IO/GDCM/include/itkGDCMSeriesFileNames.h | 41 ++++++- .../IO/GDCM/src/itkGDCMSeriesFileNames.cxx | 110 ++++++++++++------ .../itkGDCMSeriesFileNamesContractGTest.cxx | 97 +++++++++++++++ 4 files changed, 215 insertions(+), 41 deletions(-) diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md index d2abcc5598d..04a73d16a39 100644 --- a/Documentation/docs/migration_guides/itk_6_migration_guide.md +++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md @@ -881,8 +881,12 @@ unchanged; consumers compile without modification. order as before. - If you relied on file ordering for **duplicate-`ImagePositionPatient`** or **gantry-tilted** acquisitions, re-check it. `gdcm::IPPSorter` is strict and - fails to sort those; on failure the input (filesystem) order is left - unchanged rather than fabricated. First-class support is tracked in + fails to sort those; on failure an exception is thrown by default. Call + `SetFailOnAmbiguousOrdering(false)` to instead accept the legacy + `SerieHelper` heuristics (Instance Number when unique, else lexicographic + filename order) — a non-standards-conforming fallback retained only for + determinism and backward compatibility; do not trust its output. + First-class support is tracked in [#6468](https://github.com/InsightSoftwareConsortium/ITK/issues/6468). - Tags passed to `AddSeriesRestriction` now refine the series identifier (sub-dividing a `SeriesInstanceUID`, the documented intent, e.g. diff --git a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h index e9dd72f83b5..a4cbb95d6c4 100644 --- a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h +++ b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h @@ -38,10 +38,11 @@ namespace itk * * 1. Extract Image Orientation & Image Position from DICOM images, and then * calculate the ordering based on the 3D coordinate of the slice. - * 2. If for some reason this information is not found or failed, another - * strategy is used: the ordering is based on 'Instance Number'. - * 3. If this strategy also failed, then the filenames are ordered by - * lexicographical order. + * 2. If the geometric ordering fails (duplicate positions, inconsistent + * orientation), an exception is thrown by default; see + * FailOnAmbiguousOrdering. When FailOnAmbiguousOrdering is false, the + * ordering falls back to 'Instance Number' when unique, else to + * lexicographic filename order. * * If multiple volumes are being grouped as a single series for your * DICOM objects, you may want to try calling SetUseSeriesDetails(true) @@ -168,6 +169,18 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject void AddSeriesRestriction(const std::string & tag); + /** Throw an exception when a series cannot be ordered geometrically by + * gdcm::IPPSorter (duplicate ImagePositionPatient, inconsistent + * orientation). When false, fall back to the legacy SerieHelper + * heuristics: Instance Number when unique, else lexicographic filename + * order. The fallback is not DICOM-standards conforming and its output + * should not be trusted; it exists only for backward compatibility. */ + /** @ITKStartGrouping */ + itkSetMacro(FailOnAmbiguousOrdering, bool); + itkGetConstMacro(FailOnAmbiguousOrdering, bool); + itkBooleanMacro(FailOnAmbiguousOrdering); + /** @ITKEndGrouping */ + /** No effect with the gdcm::Scanner backend (retained for source * compatibility). Series enumeration reads only the grouping and * ordering tags, so sequences are never parsed during the scan. @@ -208,8 +221,23 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject void BuildSeriesMap(); - /** Ordered file names per distinct series identifier. */ - std::map m_SeriesFiles{}; + /** Files of one series; ordered lazily on the first GetFileNames call. */ + struct SeriesEntry + { + FileNamesContainerType Files{}; + bool Ordered{ false }; + }; + + /** Order one series geometrically, or apply the legacy fallback / throw + * per FailOnAmbiguousOrdering. */ + void + OrderSeries(SeriesEntry & entry); + + /** File names per distinct series identifier. */ + std::map m_SeriesFiles{}; + + /** Instance Number (0020,0013) per scanned file, for the legacy fallback. */ + std::map m_InstanceNumbers{}; /** (group,element) tags appended to the series identifier when * UseSeriesDetails is enabled; seeded with the GDCM default detail tags and @@ -224,6 +252,7 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject SeriesUIDContainerType m_SeriesUIDs{}; bool m_UseSeriesDetails = true; + bool m_FailOnAmbiguousOrdering = true; bool m_Recursive = false; bool m_LoadSequences = false; bool m_LoadPrivateTags = false; diff --git a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx index f356ba124cf..9d341d4696d 100644 --- a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx +++ b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx @@ -32,30 +32,6 @@ namespace itk { -namespace -{ -// Order one series geometrically with gdcm::IPPSorter (ImagePositionPatient -// projected on the slice normal). IPPSorter is strict: it FAILS on duplicate -// IPP and gantry-tilt acquisitions (see issue #6468). On failure the input -// order is left unchanged rather than fabricating an order. -std::vector -OrderSeriesGeometrically(const std::vector & files) -{ - if (files.size() < 2) - { - return files; - } - gdcm::IPPSorter sorter; - sorter.SetComputeZSpacing(false); - if (sorter.Sort(files)) - { - return sorter.GetFilenames(); - } - return files; -} -} // namespace - - GDCMSeriesFileNames::GDCMSeriesFileNames() { this->SetUseSeriesDetails(true); // seeds the default series-detail tags @@ -132,6 +108,7 @@ GDCMSeriesFileNames::BuildSeriesMap() } m_SeriesUIDs.clear(); m_SeriesFiles.clear(); + m_InstanceNumbers.clear(); if (m_InputDirectory.empty()) { @@ -147,8 +124,10 @@ GDCMSeriesFileNames::BuildSeriesMap() } const gdcm::Tag seriesUID(0x0020, 0x000e); + const gdcm::Tag instanceNumber(0x0020, 0x0013); gdcm::Scanner scanner; scanner.AddTag(seriesUID); + scanner.AddTag(instanceNumber); if (m_UseSeriesDetails) { for (const auto & [group, element] : m_RefineTags) @@ -187,7 +166,6 @@ GDCMSeriesFileNames::BuildSeriesMap() return id; }; - std::map grouped; for (const std::string & fn : filenames) { if (!scanner.IsKey(fn.c_str())) @@ -195,19 +173,81 @@ GDCMSeriesFileNames::BuildSeriesMap() continue; // not a DICOM file the scanner could read } const std::string id = makeIdentifier(fn.c_str()); - if (grouped.find(id) == grouped.end()) + SeriesEntry & entry = m_SeriesFiles[id]; + if (entry.Files.empty()) { m_SeriesUIDs.push_back(id); } - grouped[id].push_back(fn); + entry.Files.push_back(fn); + if (const char * number = scanner.GetValue(fn.c_str(), instanceNumber)) + { + m_InstanceNumbers[fn] = number; + } } - for (auto & [id, files] : grouped) + m_CacheBuildTime.Modified(); +} + +void +GDCMSeriesFileNames::OrderSeries(SeriesEntry & entry) +{ + if (entry.Ordered || entry.Files.size() < 2) { - m_SeriesFiles[id] = OrderSeriesGeometrically(files); + entry.Ordered = true; + return; } - - m_CacheBuildTime.Modified(); + // Geometric ordering: ImagePositionPatient projected on the slice normal. + // gdcm::IPPSorter is strict: it FAILS on duplicate IPP and gantry-tilt + // acquisitions (see issue #6468). + gdcm::IPPSorter sorter; + sorter.SetComputeZSpacing(false); + if (sorter.Sort(entry.Files)) + { + entry.Files = sorter.GetFilenames(); + entry.Ordered = true; + return; + } + if (m_FailOnAmbiguousOrdering) + { + itkExceptionMacro("Series cannot be ordered geometrically (duplicate ImagePositionPatient or inconsistent " + "orientation, see issue #6468). Set FailOnAmbiguousOrdering to false to accept the legacy " + "non-standard ordering heuristics."); + } + // Legacy SerieHelper heuristics (Instance Number, then lexicographic), + // kept only for determinism and backward compatibility: an untrustworthy, + // non-standard hack whose output should not be trusted. + std::map byInstanceNumber; + bool instanceNumbersUsable = true; + for (const std::string & fn : entry.Files) + { + const auto found = m_InstanceNumbers.find(fn); + try + { + const long number = std::stol(found != m_InstanceNumbers.end() ? found->second : std::string()); + instanceNumbersUsable = byInstanceNumber.emplace(number, fn).second; + } + catch (const std::exception &) + { + instanceNumbersUsable = false; + } + if (!instanceNumbersUsable) + { + break; + } + } + if (instanceNumbersUsable) + { + entry.Files.clear(); + for (const auto & [number, fn] : byInstanceNumber) + { + entry.Files.push_back(fn); + } + } + else + { + std::sort(entry.Files.begin(), entry.Files.end()); + } + entry.Ordered = true; } const GDCMSeriesFileNames::SeriesUIDContainerType & @@ -231,7 +271,9 @@ GDCMSeriesFileNames::GetFileNames(const std::string serie) // Return the first series encountered (single-series assumption). if (!m_SeriesUIDs.empty()) { - m_InputFileNames = m_SeriesFiles[m_SeriesUIDs.front()]; + SeriesEntry & entry = m_SeriesFiles[m_SeriesUIDs.front()]; + this->OrderSeries(entry); + m_InputFileNames = entry.Files; } else { @@ -245,7 +287,8 @@ GDCMSeriesFileNames::GetFileNames(const std::string serie) itkWarningMacro("No Series were found"); return m_InputFileNames; } - m_InputFileNames = it->second; + this->OrderSeries(it->second); + m_InputFileNames = it->second.Files; return m_InputFileNames; } @@ -351,6 +394,7 @@ GDCMSeriesFileNames::PrintSelf(std::ostream & os, Indent indent) const os << indent << "SeriesUIDs: " << m_SeriesUIDs << std::endl; itkPrintSelfBooleanMacro(UseSeriesDetails); + itkPrintSelfBooleanMacro(FailOnAmbiguousOrdering); itkPrintSelfBooleanMacro(Recursive); itkPrintSelfBooleanMacro(LoadSequences); itkPrintSelfBooleanMacro(LoadPrivateTags); diff --git a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx index d99124af60e..e3eac23bb2b 100644 --- a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx +++ b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx @@ -32,6 +32,11 @@ #include "itkMath.h" #include "itksys/SystemTools.hxx" #include "itksys/Directory.hxx" +#include "gdcmReader.h" +#include "gdcmWriter.h" +#include "gdcmDataElement.h" +#include "gdcmTag.h" +#include #include #include @@ -68,6 +73,43 @@ CopyDicomSlices(const std::string & srcDir, const std::string & dstDir) } return copied; } + +std::string +FirstDicomSlice(const std::string & srcDir) +{ + itksys::Directory directory; + directory.Load(srcDir.c_str()); + for (unsigned int i = 0; i < directory.GetNumberOfFiles(); ++i) + { + const std::string name = directory.GetFile(i); + if (itksys::SystemTools::GetFilenameLastExtension(name) == ".dcm") + { + return srcDir + '/' + name; + } + } + return {}; +} + +// Copy one slice, overwriting Instance Number (0020,0013) so duplicate-IPP +// fixtures exercise the legacy ordering fallbacks. +bool +WriteWithInstanceNumber(const std::string & src, const std::string & dst, const char * instanceNumber) +{ + gdcm::Reader reader; + reader.SetFileName(src.c_str()); + if (!reader.Read()) + { + return false; + } + gdcm::DataElement de(gdcm::Tag(0x0020, 0x0013)); + de.SetVR(gdcm::VR::IS); + de.SetByteValue(instanceNumber, static_cast(std::strlen(instanceNumber))); + reader.GetFile().GetDataSet().Replace(de); + gdcm::Writer writer; + writer.SetFileName(dst.c_str()); + writer.SetFile(reader.GetFile()); + return writer.Write(); +} } // namespace TEST(GDCMSeriesFileNamesContract, SingleSeriesEnumerationAndGrouping) @@ -126,3 +168,58 @@ TEST(GDCMSeriesFileNamesContract, RecursiveFlagHonored) recursive->SetInputDirectory(root); EXPECT_FALSE(recursive->GetInputFileNames().empty()) << "Recursive scan must descend into the subdirectory"; } + +TEST(GDCMSeriesFileNamesContract, AmbiguousOrderingThrowsByDefault) +{ + const std::string root = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_ambiguous_throw"; + itksys::SystemTools::RemoveADirectory(root); + itksys::SystemTools::MakeDirectory(root); + const std::string slice = FirstDicomSlice(SeriesDir()); + ASSERT_FALSE(slice.empty()); + // Two copies of the same slice: duplicate ImagePositionPatient. + ASSERT_TRUE(itksys::SystemTools::CopyFileAlways(slice, root + "/copy_a.dcm")); + ASSERT_TRUE(itksys::SystemTools::CopyFileAlways(slice, root + "/copy_b.dcm")); + + auto sf = itk::GDCMSeriesFileNames::New(); + EXPECT_TRUE(sf->GetFailOnAmbiguousOrdering()); + sf->SetInputDirectory(root); + EXPECT_NO_THROW(sf->GetSeriesUIDs()) << "Enumeration does not order and must not throw"; + EXPECT_THROW(sf->GetInputFileNames(), itk::ExceptionObject); +} + +TEST(GDCMSeriesFileNamesContract, LegacyFallbackUsesInstanceNumberThenFilename) +{ + const std::string slice = FirstDicomSlice(SeriesDir()); + ASSERT_FALSE(slice.empty()); + + // Duplicate IPP with unique Instance Numbers: fallback must order by + // Instance Number, chosen here to be the reverse of lexicographic order. + const std::string byInstance = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_fallback_instance"; + itksys::SystemTools::RemoveADirectory(byInstance); + itksys::SystemTools::MakeDirectory(byInstance); + ASSERT_TRUE(WriteWithInstanceNumber(slice, byInstance + "/z_first.dcm", "1 ")); + ASSERT_TRUE(WriteWithInstanceNumber(slice, byInstance + "/a_second.dcm", "2 ")); + + auto sf = itk::GDCMSeriesFileNames::New(); + sf->FailOnAmbiguousOrderingOff(); + sf->SetInputDirectory(byInstance); + const std::vector instanceOrdered = sf->GetInputFileNames(); + ASSERT_EQ(instanceOrdered.size(), 2u); + EXPECT_NE(instanceOrdered[0].find("z_first"), std::string::npos); + EXPECT_NE(instanceOrdered[1].find("a_second"), std::string::npos); + + // Duplicate IPP and duplicate Instance Numbers: final fallback is + // lexicographic filename order. + const std::string byName = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_fallback_filename"; + itksys::SystemTools::RemoveADirectory(byName); + itksys::SystemTools::MakeDirectory(byName); + ASSERT_TRUE(itksys::SystemTools::CopyFileAlways(slice, byName + "/copy_b.dcm")); + ASSERT_TRUE(itksys::SystemTools::CopyFileAlways(slice, byName + "/copy_a.dcm")); + + auto lex = itk::GDCMSeriesFileNames::New(); + lex->FailOnAmbiguousOrderingOff(); + lex->SetInputDirectory(byName); + const std::vector nameOrdered = lex->GetInputFileNames(); + ASSERT_EQ(nameOrdered.size(), 2u); + EXPECT_LT(nameOrdered[0], nameOrdered[1]); +} From debb56666fc279f797dc801f2b4085369cc880ee Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 Jul 2026 07:24:57 -0500 Subject: [PATCH 4/7] ENH: Exclude non-image DICOM objects from series enumeration Scan Rows (0028,0010) unconditionally and skip files lacking it (SR, RTSTRUCT, DICOMDIR, presentation states), restoring SerieHelper's ImageReader-based image-only acceptance. Addresses PR #6469 review feedback from thewtex. --- .../migration_guides/itk_6_migration_guide.md | 3 ++ .../IO/GDCM/src/itkGDCMSeriesFileNames.cxx | 6 +++ .../itkGDCMSeriesFileNamesContractGTest.cxx | 45 +++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md index 04a73d16a39..15fe5108cd7 100644 --- a/Documentation/docs/migration_guides/itk_6_migration_guide.md +++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md @@ -895,6 +895,9 @@ unchanged; consumers compile without modification. - `SetLoadSequences`/`SetLoadPrivateTags` have no effect with the `gdcm::Scanner` backend (the scan reads only the grouping/ordering tags). They are retained for source compatibility but no longer alter enumeration. +- Non-image DICOM objects (structured reports, RTSTRUCT, DICOMDIR, + presentation states — anything without Rows `(0028,0010)`) are excluded + from enumeration, matching `SerieHelper`'s `ImageReader`-based acceptance. ### Concerns diff --git a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx index 9d341d4696d..53c973ecd64 100644 --- a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx +++ b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx @@ -125,9 +125,11 @@ GDCMSeriesFileNames::BuildSeriesMap() const gdcm::Tag seriesUID(0x0020, 0x000e); const gdcm::Tag instanceNumber(0x0020, 0x0013); + const gdcm::Tag rows(0x0028, 0x0010); gdcm::Scanner scanner; scanner.AddTag(seriesUID); scanner.AddTag(instanceNumber); + scanner.AddTag(rows); if (m_UseSeriesDetails) { for (const auto & [group, element] : m_RefineTags) @@ -172,6 +174,10 @@ GDCMSeriesFileNames::BuildSeriesMap() { continue; // not a DICOM file the scanner could read } + if (scanner.GetValue(fn.c_str(), rows) == nullptr) + { + continue; // no Rows: not an image object (SR, RTSTRUCT, DICOMDIR, ...) + } const std::string id = makeIdentifier(fn.c_str()); SeriesEntry & entry = m_SeriesFiles[id]; if (entry.Files.empty()) diff --git a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx index e3eac23bb2b..0cca5969051 100644 --- a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx +++ b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx @@ -110,6 +110,28 @@ WriteWithInstanceNumber(const std::string & src, const std::string & dst, const writer.SetFile(reader.GetFile()); return writer.Write(); } + +// Copy one slice, stripping Rows/Columns/PixelData so it mimics a non-image +// DICOM object (SR, RTSTRUCT, ...) sharing the series' SeriesInstanceUID. +bool +WriteNonImageObject(const std::string & src, const std::string & dst) +{ + gdcm::Reader reader; + reader.SetFileName(src.c_str()); + if (!reader.Read()) + { + return false; + } + gdcm::DataSet & ds = reader.GetFile().GetDataSet(); + ds.Remove(gdcm::Tag(0x0028, 0x0010)); + ds.Remove(gdcm::Tag(0x0028, 0x0011)); + ds.Remove(gdcm::Tag(0x7fe0, 0x0010)); + gdcm::Writer writer; + writer.SetCheckFileMetaInformation(false); + writer.SetFileName(dst.c_str()); + writer.SetFile(reader.GetFile()); + return writer.Write(); +} } // namespace TEST(GDCMSeriesFileNamesContract, SingleSeriesEnumerationAndGrouping) @@ -223,3 +245,26 @@ TEST(GDCMSeriesFileNamesContract, LegacyFallbackUsesInstanceNumberThenFilename) ASSERT_EQ(nameOrdered.size(), 2u); EXPECT_LT(nameOrdered[0], nameOrdered[1]); } + +TEST(GDCMSeriesFileNamesContract, NonImageObjectsExcluded) +{ + const std::string root = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_nonimage"; + itksys::SystemTools::RemoveADirectory(root); + itksys::SystemTools::MakeDirectory(root); + const unsigned int copied = CopyDicomSlices(SeriesDir(), root); + ASSERT_GT(copied, 1u); + const std::string slice = FirstDicomSlice(SeriesDir()); + ASSERT_FALSE(slice.empty()); + ASSERT_TRUE(WriteNonImageObject(slice, root + "/structured_report.dcm")); + + auto sf = itk::GDCMSeriesFileNames::New(); + sf->SetInputDirectory(root); + const std::vector uids = sf->GetSeriesUIDs(); + ASSERT_EQ(uids.size(), 1u) << "Non-image object must not surface as an extra series"; + const std::vector files = sf->GetFileNames(uids.front()); + EXPECT_EQ(files.size(), copied) << "Only image slices are enumerated"; + for (const std::string & fn : files) + { + EXPECT_EQ(fn.find("structured_report"), std::string::npos); + } +} From 04c142f4025b448b2a7546ec89bd0de9011698a8 Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 Jul 2026 11:33:10 -0500 Subject: [PATCH 5/7] ENH: Add mixed-content directory contract test A directory holding an image series plus an SR-like object (same SeriesInstanceUID, no Rows/PixelData), a DICOMDIR-like object (no SeriesInstanceUID), and a non-DICOM text file must enumerate exactly one series with only the image slices, and still reconstruct a valid volume. Addresses PR #6469 review feedback from thewtex. --- .../itkGDCMSeriesFileNamesContractGTest.cxx | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx index 0cca5969051..52352dc167a 100644 --- a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx +++ b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx @@ -37,6 +37,7 @@ #include "gdcmDataElement.h" #include "gdcmTag.h" #include +#include #include #include @@ -112,9 +113,10 @@ WriteWithInstanceNumber(const std::string & src, const std::string & dst, const } // Copy one slice, stripping Rows/Columns/PixelData so it mimics a non-image -// DICOM object (SR, RTSTRUCT, ...) sharing the series' SeriesInstanceUID. +// DICOM object (SR, RTSTRUCT, ...) sharing the series' SeriesInstanceUID; +// stripSeriesUID additionally drops SeriesInstanceUID (DICOMDIR-like). bool -WriteNonImageObject(const std::string & src, const std::string & dst) +WriteNonImageObject(const std::string & src, const std::string & dst, bool stripSeriesUID = false) { gdcm::Reader reader; reader.SetFileName(src.c_str()); @@ -126,6 +128,10 @@ WriteNonImageObject(const std::string & src, const std::string & dst) ds.Remove(gdcm::Tag(0x0028, 0x0010)); ds.Remove(gdcm::Tag(0x0028, 0x0011)); ds.Remove(gdcm::Tag(0x7fe0, 0x0010)); + if (stripSeriesUID) + { + ds.Remove(gdcm::Tag(0x0020, 0x000e)); + } gdcm::Writer writer; writer.SetCheckFileMetaInformation(false); writer.SetFileName(dst.c_str()); @@ -268,3 +274,44 @@ TEST(GDCMSeriesFileNamesContract, NonImageObjectsExcluded) EXPECT_EQ(fn.find("structured_report"), std::string::npos); } } + +TEST(GDCMSeriesFileNamesContract, MixedContentDirectoryEnumeratesOnlyImageSeries) +{ + const std::string root = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_mixed"; + itksys::SystemTools::RemoveADirectory(root); + itksys::SystemTools::MakeDirectory(root); + const unsigned int copied = CopyDicomSlices(SeriesDir(), root); + ASSERT_GT(copied, 1u); + const std::string slice = FirstDicomSlice(SeriesDir()); + ASSERT_FALSE(slice.empty()); + // SR-like object sharing the series' SeriesInstanceUID. + ASSERT_TRUE(WriteNonImageObject(slice, root + "/report_in_series.dcm")); + // DICOMDIR-like object with no SeriesInstanceUID at all. + ASSERT_TRUE(WriteNonImageObject(slice, root + "/dicomdir_like.dcm", true)); + // Plain non-DICOM file. + { + std::ofstream readme(root + "/README.txt"); + readme << "not a DICOM file\n"; + } + + auto sf = itk::GDCMSeriesFileNames::New(); + sf->SetInputDirectory(root); + const std::vector uids = sf->GetSeriesUIDs(); + ASSERT_EQ(uids.size(), 1u) << "Only the image series is enumerated"; + const std::vector ordered = sf->GetInputFileNames(); + ASSERT_EQ(ordered.size(), copied); + for (const std::string & fn : ordered) + { + EXPECT_EQ(fn.find("report_in_series"), std::string::npos); + EXPECT_EQ(fn.find("dicomdir_like"), std::string::npos); + EXPECT_EQ(fn.find("README"), std::string::npos); + } + + // The mixed content must not disturb geometric ordering of the series. + using ImageType = itk::Image; + auto reader = itk::ImageSeriesReader::New(); + reader->SetImageIO(itk::GDCMImageIO::New()); + reader->SetFileNames(ordered); + ASSERT_NO_THROW(reader->Update()); + EXPECT_EQ(reader->GetOutput()->GetLargestPossibleRegion().GetSize()[2], ordered.size()); +} From e5265f7282e0a5885d7e14a8fcd20693f248ee05 Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 Jul 2026 11:46:26 -0500 Subject: [PATCH 6/7] BUG: Make AddSeriesRestriction independent of SetUseSeriesDetails order SetUseSeriesDetails(true) cleared previously added restriction tags. Keep user tags (m_UserRefineTags) separate from the constant default detail tags; SetUseSeriesDetails now only toggles whether both are applied, matching SerieHelper's append-only Refine list. Addresses PR #6469 review feedback from thewtex. --- .../IO/GDCM/include/itkGDCMSeriesFileNames.h | 11 ++-- .../IO/GDCM/src/itkGDCMSeriesFileNames.cxx | 60 ++++++++++--------- .../itkGDCMSeriesFileNamesContractGTest.cxx | 18 ++++++ 3 files changed, 56 insertions(+), 33 deletions(-) diff --git a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h index a4cbb95d6c4..8362b1e479a 100644 --- a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h +++ b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h @@ -163,7 +163,8 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject /** Add more restriction on the selection of a Series. This follows the same * approach as SetUseSeriesDetails, but allows a user to add even more DICOM * tags to take into account for subrefining a set of DICOM files into multiple - * series. The tag format is "group|element" of a DICOM tag. + * series. The tag format is "group|element" of a DICOM tag. Call order + * relative to SetUseSeriesDetails does not matter. * \warning UseSeriesDetails needs to be set to true. */ void @@ -239,10 +240,10 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject /** Instance Number (0020,0013) per scanned file, for the legacy fallback. */ std::map m_InstanceNumbers{}; - /** (group,element) tags appended to the series identifier when - * UseSeriesDetails is enabled; seeded with the GDCM default detail tags and - * extended by AddSeriesRestriction. */ - std::vector> m_RefineTags{}; + /** User (group,element) tags from AddSeriesRestriction, appended to the + * series identifier (after the default detail tags) when UseSeriesDetails + * is enabled. */ + std::vector> m_UserRefineTags{}; /** Modified time of the last directory parse; the cache is rebuilt only * when the object has been Modified() since. */ diff --git a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx index 53c973ecd64..1aff70081ca 100644 --- a/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx +++ b/Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx @@ -32,10 +32,20 @@ namespace itk { -GDCMSeriesFileNames::GDCMSeriesFileNames() +namespace { - this->SetUseSeriesDetails(true); // seeds the default series-detail tags -} +// Default series-detail tags, matching +// gdcm::SerieHelper::CreateDefaultUniqueSeriesIdentifier. +constexpr std::pair DefaultDetailTags[] = { + { 0x0020, 0x0011 }, // Series Number + { 0x0018, 0x0024 }, // Sequence Name + { 0x0018, 0x0050 }, // Slice Thickness + { 0x0028, 0x0010 }, // Rows + { 0x0028, 0x0011 }, // Columns +}; +} // namespace + +GDCMSeriesFileNames::GDCMSeriesFileNames() = default; GDCMSeriesFileNames::~GDCMSeriesFileNames() = default; @@ -67,7 +77,7 @@ GDCMSeriesFileNames::AddSeriesRestriction(const std::string & tag) { const auto group = static_cast(std::stoul(tag.substr(0, bar), nullptr, 16)); const auto element = static_cast(std::stoul(tag.substr(bar + 1), nullptr, 16)); - m_RefineTags.emplace_back(group, element); + m_UserRefineTags.emplace_back(group, element); } catch (const std::exception &) { @@ -123,6 +133,13 @@ GDCMSeriesFileNames::BuildSeriesMap() return; } + std::vector> refineTags; + if (m_UseSeriesDetails) + { + refineTags.assign(std::begin(DefaultDetailTags), std::end(DefaultDetailTags)); + refineTags.insert(refineTags.end(), m_UserRefineTags.begin(), m_UserRefineTags.end()); + } + const gdcm::Tag seriesUID(0x0020, 0x000e); const gdcm::Tag instanceNumber(0x0020, 0x0013); const gdcm::Tag rows(0x0028, 0x0010); @@ -130,12 +147,9 @@ GDCMSeriesFileNames::BuildSeriesMap() scanner.AddTag(seriesUID); scanner.AddTag(instanceNumber); scanner.AddTag(rows); - if (m_UseSeriesDetails) + for (const auto & [group, element] : refineTags) { - for (const auto & [group, element] : m_RefineTags) - { - scanner.AddTag(gdcm::Tag(group, element)); - } + scanner.AddTag(gdcm::Tag(group, element)); } if (!scanner.Scan(filenames)) { @@ -149,18 +163,15 @@ GDCMSeriesFileNames::BuildSeriesMap() const char * uidValue = scanner.GetValue(fn, seriesUID); std::string id = (uidValue != nullptr) ? uidValue : ""; const std::string uid = id; - if (m_UseSeriesDetails) + for (const auto & [group, element] : refineTags) { - for (const auto & [group, element] : m_RefineTags) + const char * value = scanner.GetValue(fn, gdcm::Tag(group, element)); + const std::string s = (value != nullptr) ? value : ""; + if (id == uid && !s.empty()) { - const char * value = scanner.GetValue(fn, gdcm::Tag(group, element)); - const std::string s = (value != nullptr) ? value : ""; - if (id == uid && !s.empty()) - { - id += '.'; - } - id += s; + id += '.'; } + id += s; } // Eliminate all non-alphanumeric characters (keep '.'). id.erase(std::remove_if(id.begin(), id.end(), [](unsigned char c) { return c != '.' && std::isalnum(c) == 0; }), @@ -409,17 +420,10 @@ GDCMSeriesFileNames::PrintSelf(std::ostream & os, Indent indent) const void GDCMSeriesFileNames::SetUseSeriesDetails(bool useSeriesDetails) { - m_UseSeriesDetails = useSeriesDetails; - m_RefineTags.clear(); - if (m_UseSeriesDetails) + if (m_UseSeriesDetails != useSeriesDetails) { - // Default detail tags, matching gdcm::SerieHelper::CreateDefaultUniqueSeriesIdentifier. - m_RefineTags.emplace_back(0x0020, 0x0011); // Series Number - m_RefineTags.emplace_back(0x0018, 0x0024); // Sequence Name - m_RefineTags.emplace_back(0x0018, 0x0050); // Slice Thickness - m_RefineTags.emplace_back(0x0028, 0x0010); // Rows - m_RefineTags.emplace_back(0x0028, 0x0011); // Columns + m_UseSeriesDetails = useSeriesDetails; + this->Modified(); } - this->Modified(); } } // namespace itk diff --git a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx index 52352dc167a..406805d1e9f 100644 --- a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx +++ b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx @@ -275,6 +275,24 @@ TEST(GDCMSeriesFileNamesContract, NonImageObjectsExcluded) } } +TEST(GDCMSeriesFileNamesContract, SeriesRestrictionSurvivesSetUseSeriesDetails) +{ + // Instance Number is unique per slice, so restricting on it splits the + // single series into one series per file — easy to detect. + auto restrictionFirst = itk::GDCMSeriesFileNames::New(); + restrictionFirst->AddSeriesRestriction("0020|0013"); + restrictionFirst->SetUseSeriesDetails(true); + restrictionFirst->SetInputDirectory(SeriesDir()); + const std::vector uids = restrictionFirst->GetSeriesUIDs(); + EXPECT_GT(uids.size(), 1u) << "Restriction must take effect regardless of call order"; + + auto detailsFirst = itk::GDCMSeriesFileNames::New(); + detailsFirst->SetUseSeriesDetails(true); + detailsFirst->AddSeriesRestriction("0020|0013"); + detailsFirst->SetInputDirectory(SeriesDir()); + EXPECT_EQ(detailsFirst->GetSeriesUIDs(), uids) << "Call order must not change the series grouping"; +} + TEST(GDCMSeriesFileNamesContract, MixedContentDirectoryEnumeratesOnlyImageSeries) { const std::string root = TOSTRING(ITK_TEST_OUTPUT_DIR) + "/gdcmcontract_mixed"; From 72a6747586f821cc5bcdbe9915b0e33ff8ec7c66 Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 Jul 2026 11:56:22 -0500 Subject: [PATCH 7/7] BUG: Default UseSeriesDetails to false, matching documented opt-in The class documentation has always presented SetUseSeriesDetails(true) as an explicit opt-in for splitting multi-volume series. The previous backend behaved accordingly (detail tags were seeded only by that call) while GetUseSeriesDetails() misreported true. Default the flag to false so behavior and reporting match the documentation and DICOM series identity: series group by raw SeriesInstanceUID (0020,000e) and GetFileNames() matches out of the box. Addresses PR #6469 review feedback from thewtex. --- .../migration_guides/itk_6_migration_guide.md | 8 +++++ .../IO/GDCM/include/itkGDCMSeriesFileNames.h | 6 ++-- .../itkGDCMSeriesFileNamesContractGTest.cxx | 36 +++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md index 15fe5108cd7..51e4d4cd117 100644 --- a/Documentation/docs/migration_guides/itk_6_migration_guide.md +++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md @@ -898,6 +898,14 @@ unchanged; consumers compile without modification. - Non-image DICOM objects (structured reports, RTSTRUCT, DICOMDIR, presentation states — anything without Rows `(0028,0010)`) are excluded from enumeration, matching `SerieHelper`'s `ImageReader`-based acceptance. +- `GetUseSeriesDetails()` now defaults to `false`, consistent with the + documented opt-in ("you may want to try calling `SetUseSeriesDetails(true)`") + and with DICOM series identity: by default series are grouped by their raw + `SeriesInstanceUID` `(0020,000e)`, and `GetFileNames()` matches. + Grouping behavior is unchanged — the previous backend never applied the + detail tags until `SetUseSeriesDetails(true)` was called, even though the + getter misreported `true`. Only code that read `GetUseSeriesDetails()` + before calling the setter sees a different (now truthful) value. ### Concerns diff --git a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h index 8362b1e479a..b1c0659d6f4 100644 --- a/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h +++ b/Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h @@ -145,7 +145,9 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject /** Use additional series information such as ProtocolName * and SeriesName to identify when a single SeriesUID contains - * multiple 3D volumes - as can occur with perfusion and DTI imaging + * multiple 3D volumes - as can occur with perfusion and DTI imaging. + * Off by default: series are identified by their SeriesInstanceUID + * (0020,000e) alone, per the DICOM information model. */ void SetUseSeriesDetails(bool useSeriesDetails); @@ -252,7 +254,7 @@ class ITKIOGDCM_EXPORT GDCMSeriesFileNames : public ProcessObject /** Internal structure to keep the list of series UIDs */ SeriesUIDContainerType m_SeriesUIDs{}; - bool m_UseSeriesDetails = true; + bool m_UseSeriesDetails = false; bool m_FailOnAmbiguousOrdering = true; bool m_Recursive = false; bool m_LoadSequences = false; diff --git a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx index 406805d1e9f..e3e9343dddb 100644 --- a/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx +++ b/Modules/IO/GDCM/test/itkGDCMSeriesFileNamesContractGTest.cxx @@ -35,6 +35,7 @@ #include "gdcmReader.h" #include "gdcmWriter.h" #include "gdcmDataElement.h" +#include "gdcmStringFilter.h" #include "gdcmTag.h" #include #include @@ -91,6 +92,25 @@ FirstDicomSlice(const std::string & srcDir) return {}; } +std::string +ReadSeriesInstanceUID(const std::string & file) +{ + gdcm::Reader reader; + reader.SetFileName(file.c_str()); + if (!reader.Read()) + { + return {}; + } + gdcm::StringFilter sf; + sf.SetFile(reader.GetFile()); + std::string uid = sf.ToString(gdcm::Tag(0x0020, 0x000e)); + while (!uid.empty() && (uid.back() == '\0' || uid.back() == ' ')) + { + uid.pop_back(); + } + return uid; +} + // Copy one slice, overwriting Instance Number (0020,0013) so duplicate-IPP // fixtures exercise the legacy ordering fallbacks. bool @@ -275,6 +295,22 @@ TEST(GDCMSeriesFileNamesContract, NonImageObjectsExcluded) } } +TEST(GDCMSeriesFileNamesContract, DefaultGroupingUsesRawSeriesInstanceUID) +{ + const std::string slice = FirstDicomSlice(SeriesDir()); + ASSERT_FALSE(slice.empty()); + const std::string uid = ReadSeriesInstanceUID(slice); + ASSERT_FALSE(uid.empty()); + + auto sf = itk::GDCMSeriesFileNames::New(); + EXPECT_FALSE(sf->GetUseSeriesDetails()) << "Series details are a documented opt-in"; + sf->SetInputDirectory(SeriesDir()); + const std::vector uids = sf->GetSeriesUIDs(); + ASSERT_EQ(uids.size(), 1u); + EXPECT_EQ(uids.front(), uid) << "Default identifiers are plain SeriesInstanceUIDs (0020,000e)"; + EXPECT_FALSE(sf->GetFileNames(uid).empty()) << "A raw SeriesInstanceUID from other DICOM tooling must match"; +} + TEST(GDCMSeriesFileNamesContract, SeriesRestrictionSurvivesSetUseSeriesDetails) { // Instance Number is unique per slice, so restricting on it splits the