diff --git a/Documentation/docs/migration_guides/itk_6_migration_guide.md b/Documentation/docs/migration_guides/itk_6_migration_guide.md index 7ae8504ab82..e74ce7c20b7 100644 --- a/Documentation/docs/migration_guides/itk_6_migration_guide.md +++ b/Documentation/docs/migration_guides/itk_6_migration_guide.md @@ -734,3 +734,34 @@ removed. The default (non-FFTW) FFT backend is now PocketFFT or upstreamed on the next vnl pin update. - `vnl_fft_prime_factors`, `gpfa`-family symbols, and the vxl `vnl_algo_test_fft*`/`test_convolve` tests are gone from the vendored tree. + +## `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. + +### 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..7fa606091c8 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 { /** @@ -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 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"; +}