Skip to content

ENH: set a flag when DICOM ambiguous ordering fallback is used - #6766

Open
seanm wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
seanm:dicom-ambiguous
Open

ENH: set a flag when DICOM ambiguous ordering fallback is used#6766
seanm wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
seanm:dicom-ambiguous

Conversation

@seanm

@seanm seanm commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

This flag can be used by callers to know the fallback happened, and warn end users.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

This flag can be used by callers to know the fallback happened, and warn end users.
@github-actions github-actions Bot added type:Enhancement Improvement of existing methods or implementation area:IO Issues affecting the IO module labels Aug 17, 2026
@seanm
seanm requested a review from hjmjohnson August 17, 2026 21:45
@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change exposes whether GDCM series ordering used the legacy ambiguous-ordering fallback. Two correctness issues need attention before merge: the status can remain set after the same object subsequently orders a geometrically sortable series, and public mutators let application code overwrite a result that should describe the ordering operation.

T-Rex validation blocked

  • Missing tool: The fallback-then-geometric-ordering C++ harness could not run because cmake is unavailable and the workspace has no configured ITK build tree or generated itkConfigure.h.
  • Artifact metadata failure: The focused probe for the public mutators executed, but its uploaded artifact references omitted required labels. The execution evidence therefore cannot be published as a complete proof.

Confidence Score: 3/5

Not safe to merge until the per-ordering status is reset and the diagnostic result is made read-only to callers.

There are two independent non-security correctness issues: a stale status can misreport a later reliable ordering, and public setters can falsify the reported result while changing cache invalidation behavior.

Files Needing Attention: Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx needs the status lifecycle corrected; Modules/IO/GDCM/include/itkGDCMSeriesFileNames.h needs the result API restricted to read-only access.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex attempted to configure the focused C++ harness but was blocked by missing cmake and a preconfigured ITK build, so the configuration could not start.
  • A focused probe against the public mutator and MTime cache ran, but the uploaded evidence lacked required labels, so the execution record cannot be published as a complete proof.
  • T-Rex produced proofs for two posted P1 findings, as referenced in the review comments.
  • Validation identified a public API gap for DidUseAmbiguousOrdering and recommended a fix that uses only a getter and a private assignment path when fallback occurs.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (2)

  1. General comment

    P1 Ambiguous-ordering status can remain true after later successful geometric ordering

    • Bug
      • On a reused itk::GDCMSeriesFileNames object configured with FailOnAmbiguousOrdering=false, an initial duplicate-IPP series takes the legacy fallback and sets DidUseAmbiguousOrdering true. After SetInputDirectory selects a later geometrically sortable series, OrderSeries succeeds through gdcm::IPPSorter::Sort but does not clear the status, so callers observe a stale warning flag despite the most recent requested series having reliable geometric ordering.
    • Cause
      • m_DidUseAmbiguousOrdering is initialized false in itkGDCMSeriesFileNames.h:268 and assigned true only in itkGDCMSeriesFileNames.cxx:234-237. The success branch at itkGDCMSeriesFileNames.cxx:221-226 returns without assigning false. Cache invalidation by SetInputDirectory (:91-109) rebuilds the second directory but does not reset this member.
    • Fix
      • Reset m_DidUseAmbiguousOrdering for each new ordering request/cache rebuild, or explicitly set it to false before returning from a successful geometric sort. Add the captured focused regression harness as a test using duplicate-IPP files followed by a valid multi-slice geometric series on the same object.

    T-Rex Ran code and verified through T-Rex

  2. General comment

    P1 Public mutators let callers overwrite the ambiguous-ordering result and spuriously invalidate the cache

    • Bug
      • itkSetMacro(DidUseAmbiguousOrdering, bool) and itkBooleanMacro(DidUseAmbiguousOrdering) publish SetDidUseAmbiguousOrdering, DidUseAmbiguousOrderingOn, and DidUseAmbiguousOrderingOff. A caller can clear a true fallback result or set a false result to true. The generated setter calls Modified() when the value changes; BuildSeriesMap() reuses its cache only when its build time is newer than object MTime, so this result-only mutation unnecessarily invalidates the cache.
    • Cause
      • The PR treated an implementation-owned output/result flag as a configurable boolean property by applying ITK setter and boolean macros.
    • Fix
      • Expose only itkGetConstMacro(DidUseAmbiguousOrdering, bool). Keep the member private and update it internally when ambiguous fallback occurs; do not expose a public setter or On/Off methods.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "ENH: set a flag when DICOM ambiguous ord..." | Re-trigger Greptile

Comment on lines +234 to +237
{
// Set a flag so that callers know this fallback occurred, and can show a warning.
m_DidUseAmbiguousOrdering = true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Fallback status remains stale

When the same instance first orders an ambiguous series and then orders a geometrically sortable series, this branch latches m_DidUseAmbiguousOrdering to true, but the successful IPPSorter::Sort path returns without clearing it. SetInputDirectory rebuilds the series map without resetting the member, so callers can warn about or reject the later reliably ordered result. Reset the result for each ordering operation, or explicitly clear it before returning from a successful geometric sort.

Comment on lines +190 to +192
/** @ITKStartGrouping */
itkSetMacro(DidUseAmbiguousOrdering, bool);
itkGetConstMacro(DidUseAmbiguousOrdering, bool);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Diagnostic result is publicly mutable

itkSetMacro(DidUseAmbiguousOrdering, bool) and itkBooleanMacro(DidUseAmbiguousOrdering) publish setters that allow callers to clear a real fallback result or fabricate one. Because the generated setter calls Modified(), changing this reporting-only value also invalidates the MTime-based series-map cache. Expose only a getter and keep updates to this result internal to the ordering implementation.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Enhancement Improvement of existing methods or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant