fix(adapters): do not drop segmentation slices when merging sparse segment data - #2847
Open
Joeycho wants to merge 1 commit into
Open
fix(adapters): do not drop segmentation slices when merging sparse segment data#2847Joeycho wants to merge 1 commit into
Joeycho wants to merge 1 commit into
Conversation
…gment data
`getSegmentData()` builds its result as a SPARSE array -- `segmentData[i]` is
assigned only for images that actually hold voxels:
if (hasWrittenSegmentationData) {
segmentData[currentLabelMapImageIndex] = segmentationDataForImageId;
}
`compactMergeSegmentDataWithoutInformationLoss` then merged two of those arrays
with `largerArray.forEach(...)`. `Array.prototype.forEach` SKIPS HOLES, so the
merge only visited indices where `largerArray` had an element. When the existing
layer is the longer of the two, `largerArray` IS that layer -- and every image
where only the INCOMING segment had data was never visited, and was silently
discarded.
This surfaces when two segments do not collide in-plane (so they merge into one
layer rather than being pushed as separate layers) and the incoming segment
extends beyond the existing layer's populated index range.
Observed on a 694-slice CT: segment A spanned images [375..525] (length 526),
segment B spanned [241..453] (length 454). B merged into A's layer, `largerArray`
became A, and B came back as only [375..453] -- 213 slices reduced to 79.
Reloading a saved DICOM-SEG returned a truncated mask, and re-exporting wrote the
truncation back, so the loss compounded on every save/reload cycle. Nothing
raised an error. Note the survivor is always A.start..B.end regardless of how far
B extends below it, so masks of different sizes collapse to the same size.
Fixed by iterating the index RANGE instead of one array's populated entries.
`checkHasOverlapping` iterates the same way but is CORRECT and is left unchanged:
an overlap requires both arrays to be populated at the same index, so a hole can
never be an overlap.
The existing tests all use DENSE arrays -- `[undefined, [0, 1]]` holds an
`undefined` ELEMENT, which `forEach` does visit -- so none of them exercised a
real hole. Added a regression test that builds sparse arrays the way
`getSegmentData()` does; it fails before this change and passes after, with the
eight existing cases unaffected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe segment-data merge now traverses all indices up to the maximum input length. It copies incoming data into empty slots, merges overlapping image data, and skips indices without incoming data. A regression test covers sparse arrays with non-overlapping and overlapping indices. ChangesSegmentation merge
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Context
Fixes #2846.
getSegmentData()inlabelmapImagesFromBuffer.tsbuilds its result as a sparse array —segmentData[i]is assigned only for images that actually hold voxels:compactMergeSegmentDataWithoutInformationLossthen merged two of those arrays withlargerArray.forEach(...).Array.prototype.forEachskips holes, so the merge only visited indices wherelargerArrayhad an element. When the existing layer is the longer of the two,largerArrayis that layer — and every image where only the incoming segment had data was never visited, and was silently discarded.It surfaces when two segments do not collide in-plane (so they merge into one layer instead of being pushed as separate layers) and the incoming segment extends beyond the existing layer's populated index range.
Observed on a 694-slice CT: segment A spanned images
[375..525](length 526), segment B spanned[241..453](length 454). B merged into A's layer,largerArraybecame A, and B came back as only[375..453]— 213 slices reduced to 79. Reloading a saved DICOM-SEG returned a truncated mask, and re-exporting wrote the truncation back, so the loss compounded on every save/reload cycle. Nothing raised an error.A detail that makes this recognisable: the survivor is always
A.start..B.end, regardless of how far B extends belowA.start. Masks of 134 and 213 slices both collapsed to exactly 79, which is what made the symptom look so strange from the outside.Changes & Results
compactMergeSegData.ts: iterate the index range (Math.maxof both lengths) instead of one array's populated entries.checkHasOverlappingiterates the same way but is correct and is deliberately left unchanged: an overlap requires both arrays to be populated at the same index, so a hole can never be an overlap.Before / after (the new test case):
Downstream verification in our application (OHIF-based): reloading the affected DICOM-SEG and re-exporting is now byte-identical to the original — 1428/1428 frames, 13/13 segments, zero differing pixels, compared per (segment label + referenced source SOP instance). Before the fix the same round trip returned 79 of 213 slices for the affected segment.
Testing
Nine cases: the eight existing ones plus the new sparse-array case. The new case fails on
mainand passes with this change; the existing eight are unaffected either way.Worth noting why this was not already covered: every existing case uses dense arrays.
[undefined, [0, 1]]holds anundefinedelement, whichforEachdoes visit — that is materially different from a hole, which it skips. The new test builds sparse arrays the waygetSegmentData()actually does.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit