ENH: Reimplement GDCMSeriesFileNames on gdcm::IPPSorter/Scanner (drop deprecated SerieHelper)

[hjmjohnson]

Reimplement itk::GDCMSeriesFileNames on the supported modern GDCM API (gdcm::Directory + gdcm::Scanner + gdcm::IPPSorter), dropping the deprecated gdcm::SerieHelper. Part of #6467.

Why

gdcm::SerieHelper is documented by GDCM as deprecated backward-compat code — "DO NOT USE this class, it is only a temporary solution for ITK migration from GDCM 1.x to GDCM 2.x … Instead see ImageHelper or IPPSorter." This PR moves GDCMSeriesFileNames to the supported API.

What changed

• Backend: gdcm::Directory (enumerate, honoring Recursive) → gdcm::Scanner (group by SeriesInstanceUID + detail tags; the series identifier replicates SerieHelper::CreateUniqueSeriesIdentifier) → gdcm::IPPSorter (geometric ImagePositionPatient-on-normal ordering, applied lazily per requested series). Lazy parse with a TimeStamp cache, so repeated GetSeriesUIDs/GetFileNames/GetInputFileNames calls no longer re-scan.
• Non-image objects excluded: Rows (0028,0010) is scanned unconditionally and files lacking it (SR, RTSTRUCT, DICOMDIR, presentation states) are skipped, matching SerieHelper's ImageReader-based image-only acceptance.
• Characterization tests (first commit, added while still on SerieHelper so they pin the pre-existing contract): single-series enumeration/grouping, geometric ordering reconstructs a valid uniformly-spaced volume, GetInputFileNames returns the first series, Recursive descent. Green on both backends. Follow-on commits add contract tests for ambiguous-ordering failure, the legacy fallbacks, and mixed-content directories.

Intentional behavior changes

• IPPSorter is strict; ambiguous ordering now throws by default. Duplicate-IPP and gantry-tilt acquisitions fail geometric sorting; a new FailOnAmbiguousOrdering flag (default true) throws an exception in that case. Setting it false opts into the legacy SerieHelper heuristics — Instance Number (0020,0013) when unique, else lexicographic filename order — retained only for determinism and backward compatibility; the fallback is not DICOM-standards conforming and its output should not be trusted. Enumeration (GetSeriesUIDs) never orders, so it never throws. First-class support for these acquisitions is tracked in Discussion: GDCMSeriesFileNames ordering edge cases (duplicate IPP, gantry tilt) and whether SerieHelper's permissive strategy is correct #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.

Testing

All 92 ITKIOGDCM tests pass locally (series-reader, direction-cosine, compliance suites), including 7 contract GTests (ambiguous-ordering throw, Instance-Number/lexicographic fallbacks, non-image exclusion, mixed-content directory). Downstream GDCMSeriesFileNames consumers (itkAccumulateImageFilterTest, itkGetAverageSliceImageFilterTest, itkImageSeriesWriterTest) pass. pre-commit run --all-files clean. The vendored gdcm::SerieHelper is untouched (still used by GDCM's own code); a follow-up may remove it once upstream GDCM drops it.

[hjmjohnson]

@malaterre I have been looking at removing the "SerieHelper" dependency that is only kept around for ITK (according to the GDCM comments for SerieHelper). Your insights into the implications of the proposed change would be helpful.

[hjmjohnson]

@greptileai re: the P2 "LoadSequences/LoadPrivateTags are silently dead" (outside-diff comment) — addressed in 50dfb13. The two flags are intentionally inert with the gdcm::Scanner backend (the scan reads only the grouping/ordering tags). Rather than restore unsupported behavior, they're now documented as no-ops on both setters (Doxygen) and in the ITK 6 migration guide. Retained for source compatibility.

[greptile-apps]

Thanks for the pointer to 50dfb13. The resolution looks correct:

• The Doxygen on both SetLoadSequences and SetLoadPrivateTags now explicitly states "No effect with the gdcm::Scanner backend (retained for source compatibility)" — clear, accurate, visible at the call site.
• The migration guide entry covers it too: "SetLoadSequences/SetLoadPrivateTags have no effect… retained for source compatibility but no longer alter enumeration."

Documenting them as intentional no-ops rather than either silently dropping behavior or fabricating emulation is the right call. The gdcm::Scanner API simply doesn't expose sequence/private-tag parsing at scan time, so there's nothing meaningful to delegate to. P2 addressed — marking resolved.

[thewtex]

LGTM but it would be good to get @malaterre 's review.

There may be additional SerieHelper tests in GDCM to port, check for behavior changes, and augment our interface to reduce changes in behavior.

[hjmjohnson]

Thanks @thewtex! I checked upstream GDCM for portable SerieHelper tests, and additionally ran this PR through the advanced "Fable 5" code review, which suggested making the behavior more conformant with the DICOM standard while still providing a backward-compatible path. That extended the scope of the change a bit, but I think for the better: 3 new commits, head 04c142f40.

Short answer on tests: GDCM's TestSerieHelper.cxx is instantiation-only (gdcm::SerieHelper sh; return 0;) — there are no behavior tests to port. The related TestIPPSorter{,2,3} tests exercise gdcm::IPPSorter directly and remain in GDCM's own suite.

Behavior deltas found by diffing SerieHelper against the new backend, now closed:

Ordering fallback chain (7cb3bbe)

SerieHelper::OrderFileList fell back from IPP ordering to Instance Number, then lexicographic filename order. The initial reimplementation returned unsorted readdir order on IPPSorter failure — non-deterministic across filesystems and contradicting the class docs. Now: standards-conformant geometric ordering is the only trusted path, and a new FailOnAmbiguousOrdering flag (default true) throws when it fails; setting it false opts into the legacy heuristics (Instance Number (0020,0013), read by the existing scan, when unique; else lexicographic) — retained for determinism/backward compatibility but explicitly documented as a non-standards-conforming hack whose output should not be trusted. Ordering is now lazy per requested series, so GetSeriesUIDs() on a directory containing an unsortable series never throws.

Non-image DICOM objects (debb566)

SerieHelper::AddFileName gated files through gdcm::ImageReader::Read(), so SR/RTSTRUCT/DICOMDIR objects never entered the grouping; gdcm::Scanner accepts any parseable DICOM object. Rows (0028,0010) is now scanned unconditionally and files lacking it are skipped, restoring the image-only acceptance.

New contract tests (7cb3bbe, debb566, 04c142f)

Four new GTests pin the behaviors above: duplicate-IPP series throws by default and enumeration doesn't; FailOnAmbiguousOrderingOff() orders by crafted Instance Numbers (chosen to reverse lexicographic order) then falls to filename order; a non-image object sharing the series' SeriesInstanceUID is excluded; and a mixed-content directory (image series + SR-like + DICOMDIR-like + non-DICOM file) enumerates exactly one series that still reconstructs a valid volume. All 92 ITKIOGDCM tests and the downstream GDCMSeriesFileNames consumers pass locally.

Migration guide and class docs updated to match.

Update: the interface-ordering hazard (SetUseSeriesDetails(true) resetting prior AddSeriesRestriction calls — a regression introduced by this PR's reimplementation, not present in the SerieHelper backend) is now also fixed, in e5265f7, with a call-order contract test.

Update 2: UseSeriesDetails now defaults to false (72a6747), matching the documented opt-in ("you may want to try calling SetUseSeriesDetails(true)") and DICOM series identity: default-constructed instances group by raw SeriesInstanceUID (0020,000e) and GetFileNames(<raw UID>) matches out of the box. This is grouping-behavior-identical to the SerieHelper backend, which never applied the detail tags until SetUseSeriesDetails(true) was called even though the getter misreported true; the getter is now truthful. Pinned by a contract test that verifies the enumerated identifier equals the fixture's actual SeriesInstanceUID.

[hjmjohnson]

@thewtex requesting a re-review: your review prompted follow-up commits that go a bit beyond the original scope — the deltas are summarized in the comment above. In short: ambiguous slice ordering now throws by default (FailOnAmbiguousOrdering, with the legacy SerieHelper heuristics as a documented opt-out), non-image objects are excluded from enumeration, AddSeriesRestriction/SetUseSeriesDetails are call-order independent, and UseSeriesDetails defaults to false so series group by raw SeriesInstanceUID per the DICOM information model. A little scope-creep from the original reimplementation, but it tightens testing (9 contract GTests, 96 ITKIOGDCM tests green) and DICOM conformance.

[thewtex]

Thanks, Hans. These details are tricky. I am glad the further analysis / updates were made.

[seanm]

@hjmjohnson If I set FailOnAmbiguousOrdering=false (in order for my app to continue opening files I've thus far supported) is there any way to know if the fallback was actually used? That would allow me to maintain backwards compatibility, but show a message to the user telling them the DICOM files are suspect.

[seanm]

@hjmjohnson If I set FailOnAmbiguousOrdering=false (in order for my app to continue opening files I've thus far supported) is there any way to know if the fallback was actually used? That would allow me to maintain backwards compatibility, but show a message to the user telling them the DICOM files are suspect.

Here's my patch for that: #6766