Repository navigation
Support multiframe source images in SEG and PM conversion - #558
Conversation
275bd9e to
64026c3
Compare
| help="Series Number of the created series (default: 100)") | ||
| args = ap.parse_args() | ||
|
|
||
| files = sorted(glob.glob(os.path.join(args.input_dir, "*.dcm"))) |
64026c3 to
52455d1
Compare
@michaelonken here did you mean that you ran it over the legacy enhanced content created with |
52455d1 to
832fea3
Compare
|
Yes, exactly - there are no multiframe images in IDC, and the For each case the script downloads a segmentation from IDC together with What identifies the test data are the IDC segmentation series used as the
(The qin_breast segmentation references a PET series, which is what exercised Each case should be reproducible with which needs |
itkimage2segimage and itkimage2paramap so far assumed the source series to consist of classic single-frame instances: the slice mapping read Image Position (Patient) from the top-level dataset, and the created references could only point at whole instances. Enhanced or Legacy Converted Enhanced multiframe sources ended up with a garbage mapping and unusable references. SourceImageIndex now inventories multiframe datasets (recognized by their Per-frame Functional Groups Sequence) per frame, reading the positions from the Plane Position (Patient) FG via FGInterface, so the geometric slice mapping works for any mix of single-frame and multiframe sources. Old-style multiframe images without per-frame functional groups are excluded from the mapping with a warning (they can still be referenced as whole instances with the geometry check disabled). References created from the mapping now restrict Source Image Sequence items of multiframe instances to the frames actually used via Referenced Frame Number (1-based), omitting it when all frames of the instance are referenced, as required for this type 1C attribute. Note that DCMTK's ImageSOPInstanceReferenceMacro::setReferencedFrameNumber() is unusable for this (putUint16 on the IS-VR element always returns EC_IllegalCall), so the values are set via addReferencedFrameNumber(). The Common Instance Reference module stays instance-based and needs no change. Both converters pick this up through the shared code. Two deliberate fixes to the shared derivation image FG written when the geometry check is disabled: it is now also created for a single source dataset (previously only for more than one, leaving e.g. a single multiframe source entirely unreferenced), and its Purpose of Reference Code is now (DCM,121322,"Source image for image processing operation") from CID 7202 instead of the copy-pasted derivation code (DCM,113076,"Segmentation") from CID 7203. Tests: the 24x38x3/23x38x3 phantoms already ship a multiframe (Legacy Converted Enhanced CT) encoding of their source series (58d8fe7), so the existing conversion/round-trip test pairs are duplicated for the multiframe sources, plus dciodvfy runs on their outputs, a PM conversion from the multiframe source, and MultiframeSourceReferencesTest, which asserts that the multiframe-source SEG has the same content as the classic-source SEG and checks the created references frame by frame against the source plane positions. Closes #150. The conversion direction of #217 (PM from multiframe sources) is covered as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The multiframe phantom (mf.dcm) has exactly one frame per plane, which is the easiest case: every segmentation frame references a single source frame. Real Enhanced images routinely use two structures that change how the references have to be written, both of which went untested. Verified against the WG16 Enhanced MR sample set, then reduced to small generated fixtures: - Several frames on the same plane (multi-echo, multi-phase, ...). Every segmentation frame must reference all source frames of its plane, so Referenced Frame Number becomes multi-valued; with interleaved acquisitions the numbers are not contiguous. mf-2frames-per-plane.dcm reproduces this with six frames on the three phantom planes ([1,4], [2,5], [3,6]). - A shared instead of per-frame Plane Position (cine acquisitions). Only the slice on that plane gets references, and since all frames of the instance apply, Referenced Frame Number must be omitted - a reference without frame numbers denotes the whole instance. mf-shared-plane-position.dcm reproduces this with four frames on the first plane. Since the new variant carries its own Series Instance UID, combining it with mf.dcm also covers references spanning two series, i.e. the series grouping in the Common Instance Reference module. Adds MultiframeSourceVariantsTest for the three cases (it resolves every Referenced Frame Number back to the plane position of the source frame instead of hardcoding expected numbers), end-to-end CLI tests with dciodvfy on their output, and a parametric map conversion from the multi-frame-per-plane source. Each new check was confirmed to fail when the corresponding behavior is mutated away. The fixtures are generated from mf.dcm by util/make_multiframe_test_data.py (deterministic UIDs, ~10-15 KB each) so patient, study and frame of reference stay consistent with the phantom and regenerating them is reproducible. They carry the same four dciodvfy findings as the mf.dcm they are derived from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The checked-in multiframe fixtures are small, deterministic and built by us from one converted phantom. They cannot show that dcmqi reads Enhanced images as other tools write them, nor how the conversion behaves on real geometry. This tooling closes that gap for manual runs: for a segmentation from the Imaging Data Commons it downloads the segmentation and the image series it references, converts the segmentation back to ITK, converts the classic series into a Legacy Converted Enhanced image using highdicom, runs itkimage2segimage against both encodings of the same source, and compares the results. The comparison asserts identical segmentation content and, as the actual point, that the frame references of the multiframe run denote the same source images as the instance references of the classic one. It resolves every Referenced Frame Number back to the SOP Instance UID it came from via the Conversion Source Attributes Sequence, so it compares what the references mean rather than assuming a shared slice grid - segmentations do occur that sit on a different grid than the series they reference, and then several source frames legitimately map onto one segmentation frame. Not wired into CTest: it needs network access, the IDC index and highdicom, and downloads tens to hundreds of megabytes per case. The README documents what it adds over the CI fixtures, how to pick a case, and two findings worth knowing before chasing a failure - that many IDC segmentations were themselves written by dcmqi, and that dciodvfy's complaint about a missing Clinical Trial Coordinating Center Name is unrelated to multiframe sources. Validated over seven IDC collections (CT and PET sources, 64 to 126 slices, one to five segments), all of which pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dciodvfy tests for the three multiframe source variants derived both the name of the test they depend on and the file they verify from one loop variable, but the producing tests and their output files used different suffixes: the tests are named after the full variant (_2frames_per_plane, _shared_plane_position) while the files were abbreviated (_2frames, _shared). Two of the three TEST_DEPENDS therefore named tests that do not exist. CTest ignores an unknown DEPENDS silently, so nothing failed - but under ctest -j the dciodvfy test could run before the conversion that writes its input. This stayed invisible because CI had no dciodvfy until #559/#561, which install dicom3tools from PyPI on all platforms; the tests are created and run there now. Name the output files after the variants as well, so a single identifier drives the test name, the file and the dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aaba02a to
d8105e1
Compare
There was a problem hiding this comment.
Reviewed the code against the pre-refactor originals, the DCMTK APIs it relies on (checked against the pinned 5708ba6), and the new tests and fixtures. This is careful, well-documented work and the DICOM reasoning holds up — in particular, omitting Referenced Frame Number when all frames apply is not merely allowed but required: PS3.5 Section 7.4.3 says a Type 1C element shall not be present when its condition is not met.
Now verified by CI. Before the rebase this PR targeted refactor_seg_converter, and the workflows only trigger on PRs against master, so nothing here had ever been built or tested. It is now green on all five platforms. Two things that were previously unverifiable are now confirmed:
dciodvfylanded in CI via #559/#561, so the multiframe conformance tests actually execute. All five pass —_24x38x3_multiframe_source,_23x38x3_multiframe_source, and the three variant tests — with no findings on any multiframe output. The fixtures inherit no conformance problems beyond the phantom's own.- 93/93 tests pass. (The PR description's 92 was counted before the
two_seriestest existed as its own dciodvfy case.)
Two notes on what changed under the PR during the rebase onto master:
- The conflict in
SourceImageIndex.hwas resolved in favour of master's doc block from #560, which rewrote that same comment to describe the cross-study routing. That drops this branch's deletion of the "Factored out of the SEG and PM converters (#192)" note — worth keeping, so no action needed, but commit 2's diff is 3 lines smaller than what you pushed. apps/seg/Testing/CMakeLists.txtneeded a fix (separate commit): thedciodvfyloop derived both the dependency's test name and the file to verify from one variable, but the producers are named_2frames_per_plane/_shared_plane_positionwhile their outputs were abbreviated_2frames/_shared. Two of the threeTEST_DEPENDStherefore named tests that do not exist. CTest ignores an unknownDEPENDSsilently, so it never failed — but underctest -jthe verification could run before the conversion that writes its input. It was invisible until now precisely because CI had no dciodvfy.
Comments below are all non-blocking; the two in SourceImageIndex.cpp are the only ones touching shipped code, and both are edge cases rather than defects on the paths the tests cover.
Two things outside the diff:
- SonarCloud is the only red check, on a path-traversal alert in
util/multiframe-validation/legacy_convert.py:48. It is a manually-run developer tool that takes a directory argument, so the finding is not meaningful, but it does block the quality gate — worth either validating the constructed path or marking it won't-fix in SonarCloud so the gate can go green. ParaMapConverter's slice-mapping log (changed in #557, not here, so I cannot comment inline) prints "Mapping from the ITK image slices to the DICOM instances in the input list" followed by what are now frame ids rather than dataset indices. Harmless, but the label is misleading once the source is multiframe.
🤖 Generated with Claude Code
| for(size_t j=0;j<frameNumbers.size();j++) | ||
| if(frameNumbers[j] > 65535) | ||
| representable = false; | ||
| if(!frameNumbers.empty() && frameNumbers[0] > 0 && frameNumbers.size() < info.numberOfFrames |
There was a problem hiding this comment.
Two edge cases in this condition, both benign on conformant input.
Two sources of truth for the frame count. The inventory is built from FGInterface::getNumberOfFrames(), which returns the number of Per-frame Functional Groups items, while this "are all frames referenced?" test compares against info.numberOfFrames, which comes from (0028,0008) and is set to 0 when the attribute is absent or unparseable. When they disagree the comparison silently goes the wrong way: with info.numberOfFrames == 0, frameNumbers.size() < 0 is false, so a partial reference is written as a whole-instance reference — i.e. it claims frames that were not used. NumberOfFrames is Type 1 for multiframe images so this needs malformed input, but since the frames were inventoried from the per-frame sequence, comparing against the number of inventoried frames for this dataset would remove the discrepancy entirely.
The representable fallback has the same shape. When a frame number exceeds 65535 the code also falls through to a whole-instance reference, which over-references rather than failing. Unreachable in practice, and the comment is honest about why the guard exists — but given the choice between "claim all frames" and "refuse", the latter seems the safer default for a reference that a consumer will act on.
Worth noting the workaround itself is correct and necessary: at the pinned DCMTK (5708ba6), setReferencedFrameNumber() goes through setUint16ValuesOnElement() on a VR IS element; only addReferencedFrameNumber() builds the IS string properly. I confirmed the fix is in current DCMTK master, so the TODO will become actionable on the next bump.
🤖 Generated with Claude Code
|
|
||
| void SourceImageIndex::addMultiframeSourceFrames(size_t datasetIndex, DcmItem& dataset, const InstanceInfo& info) { | ||
| FGInterface fgInterface; | ||
| if(fgInterface.read(dataset).bad()){ |
There was a problem hiding this comment.
A robustness note on the failure mode here: FGInterface::read() returns FG_EC_NoSharedFG when the Shared Functional Groups Sequence is missing or has zero items, and readSingleFG() fails the whole read if any single shared group fails to parse. In either case every frame of that source is dropped from the geometric mapping and the resulting object carries no references to it at all.
That is a fairly wide net for something the converter does not actually need: the mapping only wants Plane Position (Patient), which for the per-frame case is already sitting in the per-frame items. A fallback that reads Plane Position straight out of PerFrameFunctionalGroupsSequence when FGInterface refuses the dataset would keep such files usable — and files with an empty shared sequence do turn up in the wild.
The warning text is good, so at minimum the user can tell what happened.
🤖 Generated with Claude Code
| done | ||
| command -v "$IDC" >/dev/null 2>&1 || { echo "ERROR: $IDC not found (pip install idc-index)"; exit 1; } | ||
|
|
||
| rm -rf "$work" |
There was a problem hiding this comment.
$work comes straight from $1 with no validation, so validate_idc_case.sh / <uid> or validate_idc_case.sh ~ <uid> is unrecoverable. set -u guarantees it is set, and the $# -ne 2 guard guarantees it is non-empty, but neither stops it from being something you very much do not want to rm -rf.
Cheapest fix is to refuse anything that is not a fresh or clearly-owned directory — e.g. require it not to exist yet, or bail if it exists and is non-empty and lacks a marker file this script wrote.
Related, in the same script: it runs under set -u but without set -e -o pipefail, and the two itkimage2segimage invocations are piped into grep, so their exit status is discarded. A failed conversion falls straight through to the comparison, which then reports confusing mismatches against stale or missing files rather than the actual error.
🤖 Generated with Claude Code
| wanted = set(expected) | ||
| resolved = set() | ||
| for frames in actual.values(): | ||
| if frames is None: # whole instance: cannot be narrowed down |
There was a problem hiding this comment.
This branch makes check 4 vacuous in exactly the case most worth checking. When ReferencedFrameNumber is absent, resolved is assigned wanted, so the subsequent resolved != wanted can never fire — the check passes by construction.
That matters because a regression that dropped frame numbers entirely would produce whole-instance references everywhere, and this check would still pass. Check 3 does not catch it either: it only asserts that the referenced instance is the multiframe one, not that the reference is frame-level.
Since the classic-vs-enhanced comparison knows how many source instances the classic run referenced for each frame, asserting that a whole-instance reference only appears where all frames of the instance legitimately apply would close the gap — the same assertion MultiframeSourceVariantsTest already makes in-tree.
🤖 Generated with Claude Code
|
|
||
| for name, ds in variants: | ||
| path = os.path.join(out_dir, name) | ||
| ds.save_as(path, write_like_original=False) |
There was a problem hiding this comment.
write_like_original was removed in pydicom 3.0, so this raises TypeError on any current install. The replacement is enforce_file_format=True, which legacy_convert.py in this same PR already uses — so the two scripts disagree about which pydicom generation they target.
Since the whole point of this script is that the committed fixtures can be regenerated reproducibly, it would be a shame for it to fail on the pydicom most people now have.
🤖 Generated with Claude Code
Review follow-up on the multiframe references: whether all frames of a source are referenced (allowing Referenced Frame Number to be omitted) was decided against NumberOfFrames (0028,0008), while the inventory and the frame numbers come from the per-frame items - two sources of truth. - an absent or unparseable NumberOfFrames was recorded as 0, turning the comparison silently false: a partial reference was written as a whole-instance claim (needs malformed input; the attribute is type 1) - InstanceInfo now records the inventoried frame count per dataset and the completeness check uses it; (0028,0008) remains only to recognize old-style multiframe images without functional groups - new test case strips (0028,0008) from the two-frames-per-plane fixture and asserts the frame numbers are still written; fails with the old comparison - well-formed outputs are byte-identical (all multiframe variants + PM)
Review follow-up, same shape as the NumberOfFrames case: a frame number above 65535 made the code skip Referenced Frame Number and write a whole-instance reference, over-referencing frames that were not used instead of failing. - both DCMTK methods for the attribute take Uint16, while VR IS permits more; the guard stays, but now refuses - returns FG_EC_InvalidData with the offending frame and instance, so the conversion aborts without writing a file - new test case pushes the real frames past 65535 with in-memory filler items and asserts the refusal; it fails with the fallback - well-formed outputs unchanged (multiframe variants + PM) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FGInterface::read() rejects a dataset as a whole - it returns FG_EC_NoSharedFG when the Shared Functional Groups Sequence is missing or empty, and fails if any single shared group does not parse. Every frame of such a source was dropped from the mapping and the result referenced it not at all; files like this do occur. - the mapping only needs Plane Position (Patient), so read it from the per-frame items (shared item as fallback) when FGInterface refuses the dataset, and say so in the warning - verified: with a missing or empty shared sequence the phantom went from 0 of 3 slices mapped and no references to 3 of 3 with frame-level references - new test case covers both variants in memory; it fails without the fallback - well-formed outputs unchanged (multiframe variants + PM) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two robustness defects in validate_idc_case.sh found in review. - $work went unvalidated into rm -rf, so passing / or ~ was unrecoverable; now refuses anything that exists, is non-empty and lacks the marker file the script writes, plus / and empty - the script ran without pipefail and piped both conversions into grep, discarding their exit status: a failed conversion fell through to the comparison, which then reported mismatches against missing files instead of the real error. Conversions now run through a helper that aborts and shows the log tail - verified: /, ~ and "" are refused, a fresh or previously marked directory is accepted, and a failing conversion aborts Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
In compare_references.py, a source image reference without frame numbers set resolved = wanted, so the following inequality could never fire: the check passed by construction in exactly the case worth checking. A regression dropping frame numbers would produce whole-instance references everywhere and still pass, and check 3 only asserts which instance is referenced, not that the reference is frame-level. - a reference without frame numbers now resolves to all frames of the instance, which is what it actually denotes - added a second assertion that the references are frame-level throughout, naming the count when they are not - verified against the phantom: the old check passed a seg with all frame numbers stripped, the new one fails it and exits non-zero Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
make_multiframe_test_data.py passed write_like_original=False while
legacy_convert.py in the same PR already used enforce_file_format,
so the two disagreed on which pydicom generation they target.
- the keyword is chosen by pydicom major version now, so the script
runs clean on both series; on 3.x it no longer trips the
deprecation warning ("removed in v4.0"), which is where this would
have become a hard failure
- checked: the generated fixtures are byte-identical to the
committed ones under pydicom 2.4.2 and 3.0.2 alike, so
regeneration stays reproducible across the split
- README now states that highdicom requires pydicom 3 while the
generator does not
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hi Andrey, Thanks for looking through the PR. All six findings are addressed now. Every fix was
Test count is now 93; |
The two conversions now go through the convert() helper, which forwards its arguments with "$@", so an unquoted $itk2dcm is word-split before it gets there and a DCMQI_BIN containing a space breaks the invocation rather than just the command word. Quote it, which also clears the two SC2086 findings the helper introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Support Enhanced Multiframe in
itkimage2segimageanditkimage2paramapFixes #150. Covers the conversion direction of #217.
Based on #557 — please merge that one first; this PR targets its
branch, so the diff shown here is only the feature. The base switches to
masterautomatically once the other PR is merged.The problem
Both converters assumed one image per instance. The slice mapping read
ImagePositionPatientfrom the top level of each dataset, where a multiframeimage does not carry it, and the created references could only name whole
instances. Multiframe sources therefore produced a meaningless mapping and
unusable references.
What changes
SourceImageIndexnow inventories multiframe datasets frame by frame,recognizing them by their Per-frame Functional Groups Sequence and reading the
positions from the Plane Position (Patient) functional group through
FGInterface. The geometric slice mapping then works for any mix ofsingle-frame and multiframe sources. Old-style multiframe images without
functional groups are excluded from the mapping with a warning; they can still
be referenced as whole instances when the geometry check is disabled.
References created from that mapping restrict Source Image Sequence items of
multiframe instances to the frames actually used, via Referenced Frame Number.
It is omitted when all frames of an instance are referenced, since a reference
without frame numbers denotes the whole instance — required for this type 1C
attribute (PS3.3 Table 10-3 together with PS3.5 Section 7.4.2). The Common
Instance Reference module stays instance-based and needed no change. Both
converters pick this up through the shared component.
Two deliberate fixes to the shared Derivation Image group written when
--referencesGeometryCheck 0is used: it is now also created for a singlesource dataset (previously it required more than one, which left a single
multiframe source entirely unreferenced), and its Purpose of Reference Code is
now
(DCM,121322,"Source image for image processing operation")from CID 7202instead of the copy-pasted derivation code
(DCM,113076,"Segmentation")fromCID 7203.
Tests
The 24x38x3 and 23x38x3 phantoms already shipped a multiframe encoding of
their source series (58d8fe7) that no test used. The existing conversion and
round-trip test pairs are now also run against those, plus dciodvfy on the
results.
Three structures that real Enhanced images use are not covered by that
phantom, and each changes how the references have to be written. Two small
fixtures were generated for them (
util/make_multiframe_test_data.py,deterministic, ~10-15 KB each, derived from the existing
mf.dcmso patient,study and frame of reference stay consistent):
all source frames of its plane, so Referenced Frame Number becomes
multi-valued, and non-contiguous with interleaved acquisitions;
plane gets references, and since all frames apply, Referenced Frame Number
must be absent;
MultiframeSourceVariantsTestchecks all three by resolving every ReferencedFrame Number back to the plane position of the source frame, rather than
hardcoding expected numbers. Each of its checks was confirmed to fail when the
corresponding behavior is mutated away. Test count goes from 76 to 92.
Validation against real data
The last commit adds
util/multiframe-validation/, a manually run tool thattakes a segmentation from the Imaging Data Commons, converts its referenced
series into a Legacy Converted Enhanced image using highdicom — i.e. a
producer independent of dcmqi — and then runs
itkimage2segimageagainst bothencodings of the same source and compares the results. It asserts identical
segmentation content and that the frame references denote the same source
images as the instance references of the classic run, tracing each frame
number back through the Conversion Source Attributes Sequence.
Run over seven IDC collections (
hcc_tace_seg,rider_lung_ct,nsclc_radiomics,c4kc_kits,nlst,lung_pet_ct_dx,qin_breast; CT andPET sources, 64 to 126 slices, one to five segments); they are single-frames but
are converted using highdicom in order to produce Enhnaced MF test data. dcmqi
conversion is then run twice: one time on the classic single-frame input, then on the
converted multi-frame input. Output is then compared. All pass. One of them
produced a multi-valued Referenced Frame Number from real data, because that
segmentation sits on a different slice grid than the series it references and
two source slices fall into one segmentation slice. That is exactly the case
the
mf-2frames-per-planefixture reproduces synthetically.Unrelated finding
On some of those datasets
dciodvfyreports a missing type 2ClinicalTrialCoordinatingCenterName. This predates these changes and isindependent of multiframe sources: dcmqi always writes Clinical Trial Series
ID and Time Point ID, which makes the module present, but writes the
Coordinating Center Name only when the metadata supplies a value. Not
addressed here.