Repository navigation
Factor source image handling out of the SEG converter and restructure it - #557
Merged
Merged
Conversation
Introduce SourceImageIndex, an inventory of the source images a derived object is created from. It owns the three concerns that were previously spread over (and partly copy-pasted 3x within) itkimage2dcmSegmentation: - normalizing the source datasets into a list of referenceable frames (prepares for multiframe sources, #150; addresses #192) - mapping slices of the converted ITK image to those frames by geometry (replaces ConverterBase::getSliceMapForSegmentation2DerivationImage, still used by the PM converter until it is ported) - creating Derivation Image FG items and accumulating the referenced instances for the Common Instance Reference module Decompose the ~840-line itkimage2dcmSegmentation into focused helpers (document creation, dimension module, geometry shared FGs, segment creation from JSON attributes, binary frame extraction, labelmap frame composition, attribute patching) and remove the dead cropSegmentsBBox block. Manage segdoc and the per-frame FGs with unique_ptr; copy code sequences into segments instead of sharing pointers with the metadata handler, which would double-free now that segdoc is released. Behavior-preserving for valid inputs; verified by the test suite and by dcmdump-diffing outputs (binary, labelmap, multi-file, geometry check off) against pre-refactoring builds. Deliberate edge-case fixes: - frames whose slice has no mapped source no longer get an empty (invalid) DerivationImageSequence when other slices of the same file have references - all instances referenced by a frame now land in ReferencedSeriesSequence (previously only the first per frame) - labelmap frame bit depth now always matches the document bit depth (was inconsistent for gapped label IDs > 255) - referenced instances are grouped by their actual series instead of all being attributed to the series of the first dataset Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the fourth copy of the slice-mapping/reference-building code in itkimage2paramap with the shared SourceImageIndex introduced for the SEG converter. This also removes the CastImageFilter workaround (the mapping now operates on itk::ImageBase geometry directly, no short-typed copy of the parametric map needed) and the now-unused ConverterBase::getSliceMapForSegmentation2DerivationImage (#192). Behavior-preserving, verified by the paramap test suite and dcmdump diffs, with one deliberate fix: frames whose slice has no mapped source image no longer carry an empty (invalid) DerivationImageSequence; the derivation image FG is now only added to frames that actually reference source images. Also drops the unused ImageProcessing derivation code local and the stale ADD_DERIMG preprocessor remnant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three functional groups that are re-filled for every output frame were handled as four loose local variables (the groups plus the OFVector handed to addFrame()), and the protocol they require was spelled out twice - once in the binary frame loop, once in the labelmap one: clear the vector, push plane position and frame content, conditionally push the derivation image group, remember in a bool that it was pushed, and clear its data again after the frame was added. Forgetting that last step makes a frame inherit the source image references of its predecessor, so it is the kind of rule that should not live in caller discipline. PerFrameFunctionalGroups now owns the groups and exposes that protocol as beginFrame() / accessors / get() / endFrame(), with the derivation image group added to the frame on first use so that it is never present but empty. Both loops lost their duplicated bookkeeping. Behavior-preserving: verified by the test suite and by dcmdump-diffing 19 conversion variants (binary, labelmap, multi-file, label IDs as segment numbers, gapped label IDs, overlapping segments, geometry check off, skipped empty slices, all multiframe source variants, parametric maps, and the labelmap overlap rejection) against pre-refactoring output - all byte-identical including the console logs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Which Segment Number a label gets, and what follows from that number, was spread over the converter: two loose variables plus a per-file map held the state, the rules for using input label IDs as segment numbers lived inline in the segment loop, and three consumers restated parts of them - the labelmap bit depth was derived from the same rules a second time inside createSegmentationDocument(), the labelmap frame composition looked up pixel values in the raw map, and the post-write re-mapping to label IDs read the segment-to-label map directly. SegmentNumberAssigner now owns the scheme (sequential or label IDs) together with the mappings that follow from it, and offers them as determineLabelmapBitDepth(), proposeSegmentNumber() / recordSegment(), pixelValueFor() and segmentToLabel(). Proposing and recording are separate because DcmSegmentation::addSegment() may hand back a different number than the one requested; the recorded number is the one actually used, as before. createSegmentationDocument() no longer needs the metadata handler at all and shrank from eight parameters to six. Behavior-preserving. Verified with the test suite, by dcmdump-diffing the same 19 conversion variants as the previous commit (all byte-identical, including console output), and by exercising the error paths that those variants do not reach - colliding label IDs across input files in both output flavors, a segment number beyond the 16 bit pixel range, and a label without metadata - against a build of the pre-refactoring code: identical exit codes and identical messages. Note that the "label IDs must be positive" rejection is not reachable through the command line tools, since a negative labelID is already rejected when the metadata JSON is parsed; it is kept for callers using the library API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
itkimage2dcmSegmentation() still ran the whole conversion in one body: create the document, add the shared functional groups, map the segmentation slices to source frames, walk files and labels to create the segments, write the frames (binary per segment, labelmap per slice), then finish and write the dataset. The steps were readable individually but shared so much state - the document, the source image inventory and its slice mapping, the segment numbering, the reused functional groups, the frame counter - that pulling any of them into a function meant handing over a dozen parameters, which is why the earlier extractions stopped where they did. SegmentationEncoder makes that state members instead, so each step becomes a method of its own with no or few parameters: createDocument(), addSharedFunctionalGroups(), mapSourceFrames(), encodeSegments() with addBinaryFrames(), encodeLabelmapFrames() with addLabelmapFrameReferences(), and writeDocument(). None exceeds 70 lines, most are far shorter. The five conversion flags travel as one Options struct; the public signature is unchanged, and so are the explicit template instantiations. itkimage2dcmSegmentation() is now a 35-line wrapper that builds the options, runs the encoder and applies the label-ID re-mapping to the written dataset - that last step stays with the caller because it uses the protected static helpers of Itk2DicomConverter. Together with the two preceding commits the function went from 840 lines to 35. Behavior-preserving, verified as before: the test suite, the same 19 conversion variants dcmdump-diffed byte-for-byte against pre-refactoring output including console logs, and the error paths (colliding label IDs in both flavors, segment number beyond the 16 bit range, label without metadata) compared against a build of the original code - identical exit codes and messages. Compiles clean under -Wall -Wextra -Wreorder. Also carries along the working-tree comment edits in the public header (rewrapped parameter documentation, dropped issue and supplement references). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
This PR also fixes #554 . |
Locks in two reference-bookkeeping fixes of this refactoring, following up on review feedback: - all source instances mapped to the same segmentation frame must appear in the top-level Referenced Instance Sequence, not only the first one per frame (issue #554); - referenced instances must be grouped by the series they actually belong to, not attributed to the series of the first input dataset. The additional source instances (a same-plane duplicate and one on a second series of the same study) are derived in memory from the classic test series, so no fixture files are needed. Both assertions fail when the corresponding fix is mutated away. Cross-study references remain out of scope.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactoring of
itkimage2segimageanditkimage2paramap, with no intendedchange in behavior. It stands on its own, but is also the groundwork for
multiframe source support, which follows in a separate PR.
Fixes #192.
Why
The slice-to-instance mapping and the code building the source image
references lived inline in
Itk2DicomConverter.cpp, copy-pasted three timeswithin
itkimage2dcmSegmentation()and a fourth time inParaMapConverter.That function had grown to ~840 lines, holding the whole conversion in one
body, and the duplication had started to drift apart between the copies.
What changes
A new
SourceImageIndexcomponent (include/dcmqi/SourceImageIndex.h,libsrc/SourceImageIndex.cpp) owns everything about the source images:normalizing the datasets into a list of referenceable frames, mapping the
slices of the converted image onto them by geometry, and creating the
Derivation Image items plus the Common Instance Reference module from that
mapping. Both converters use it, which removes the duplication and lets the
PM converter drop its
CastImageFilterworkaround (the mapping now works onitk::ImageBasegeometry directly).Three components carved out of the SEG converter, each with a concrete
reason rather than just moving lines:
PerFrameFunctionalGroupsowns the functional groups reused for every frameand enforces the protocol they need. The rule "clear the derivation image
group after the frame was added, or the next frame inherits its references"
was previously written out twice and left to caller discipline.
SegmentNumberAssignerowns the segment numbering scheme and the mappingsderived from it. The rules were restated in four places, including a second
implementation of the labelmap bit depth inside the document creation.
SegmentationEncoderturns the conversion into named steps(
createDocument(),addSharedFunctionalGroups(),mapSourceFrames(),encodeSegments(),encodeLabelmapFrames(),writeDocument()). These shareso much state that as free functions they would have needed a dozen
parameters each; as members of one class they take none or few.
itkimage2dcmSegmentation()is now a 35-line wrapper. The public signaturesand the explicit template instantiations are unchanged.
Behavior
Intended to be behavior-preserving, with four deliberate edge-case fixes in
the first commit, all in situations that previously produced questionable
output:
therefore invalid)
DerivationImageSequencewhen other slices of the sameinput file do have references.
ReferencedSeriesSequence;previously only the first one per frame did.
could differ for gapped label IDs above 255.
being attributed to the series of the first input dataset.
Verification
conversion variants against a build of the preceding state: binary,
labelmap, multiple input files, label IDs as segment numbers, gapped label
IDs, overlapping segments, geometry check disabled, empty slices skipped and
not skipped, and parametric maps. All byte-identical apart from UIDs and
timestamps, including the console output.
reach, the error paths were compared separately against a build of the
original code: colliding label IDs across input files in both output
flavors, a segment number beyond the 16 bit pixel range, and a label without
metadata. Identical exit codes and identical messages.
-Wall -Wextra -Wreorder.Review order
The commits are meant to be read in order; each is self-contained and
individually verified. The first is the largest and carries the edge-case
fixes listed above.