Use label id as segment numbers #485

michaelonken · 2023-12-08T09:47:53Z

This pull request contains two parts:

New functionality:

In ITK to DICOM conversion, use Label IDs from input files as Segment Numbers if possible. From the related API documentation:

     * @param sortByLabelID A boolean indicating whether to sort the segments by label ID.
     *        In this case, the label IDs are used as DICOM segment numbers. This only works
     *        if the label IDs start at 1 and are numbered monotonically without gaps. The processing
     *        order of label IDs is not relevant, i.e. they can occur in any order in the input.
     *        If n labels are not assigned uniquely to label IDs 1..n in the input, the
     *        conversion will fail.
     *        If this is set to false (default), the segment numbers are assigned in the order of the
     *        labels that are being converted, i.e. the first label will receive the Segment
     *        Number 1, the second label will receive the Segment Number 2, etc.

Also, a new test group is added that uses some (more than before) sophisticated segment setup with overlapping segments. A first test produces a DICOM file from three NRRD inputs, which are merged together in one DICOM file. A second test splits them again into 3 NRRD files (where 2 NRRD files will contain more than one segment) and compares them to the original inputs from test 1. A third test compares metadata input used in test 1 to metadata output produced by test 2.

Various small enhancements in ITK to DICOM conversion context:

Replaced some deprecated JSON writer code
Enhanced documentation
Made logging output a bit more consistent
Save some memory (don't copy segmentation dataset on return)

Make sure input data has been initialized by user before public methods are called. Made logging message more consistent. Made code more readable.

When converting from ITK to DICOM, allow to use Label IDs as Segment Numbers so that even if reading from various NRRD inputs, the Segments in the DICOM file will still use the Label IDs from the NRRD files. This is particularly helpful for testing since it allows a roundtrip test from DICOM -> multiple NRRD files using potentially multiple segments each -> back to DICOM. If no mapping to the Label IDs takes place, the converter will use ascending Segment Numbers, so the order will depend on the read order, which again usually depends on the order in the JSON meta information. This sorting behavior is disabled by default. It is available as cli option (--sortByLabelID) which is handed to the library through the Itk2DicomConverter::itkimage2dcmSegmentation() call.

Added missing test dependency.

fedorov

I spent quite some time with this PR, and I have a lot of questions. I am not sure it will work.

Also, note that MultiFrameDimensionModule utilizes ReferencedSegmentNumber in the per-frame FGs, see: https://github.com/QIICR/dcmqi/blob/master/libsrc/Itk2DicomConverter.cpp#L58-L62.

I would think that all of the frames would need to be reordered in the general case if the segments are renumbered.

Let me know if it is easier to have a screen share to go over this!

fedorov · 2023-12-13T17:42:20Z

apps/seg/itkimage2segimage.xml

@@ -64,6 +64,15 @@
      <description>Skip empty slices while encoding segmentation image. By default, empty slices will not be encoded, resulting in a smaller output file size.</description>
    </boolean>

+    <boolean>


@michaelonken do we really need this flag? I do not see any downsides in always sorting label IDs and always reusing those if possible in SEG. Since you already implemented this option, why not keep it but set the default to true?

I used this approach for two reasons:

Don't break backward compatibility

Make sure that conversion succeeds also in cases where label IDs cannot be sorted (since then the call will fail). I assume the user does often not know beforehand, whether the labelmaps will allow sorting. So the idea is that you only turn on this functionality if the user knows the prerequisites for this are fulfilled.

If 1. does not matter, one could also enable it by default and fall back to the old behavior if re-sorting does not work. I am not sure whether this can be implemented well, since the "rollback" after half of the re-resorting is probably difficult. And finding this out before actually starting the sorting work costs extra time.

I agree! I made this comment in the beginning of my review, and if I revisited it after the review was done, I would most definitely revert it!

fedorov · 2023-12-13T17:52:46Z

libsrc/Itk2DicomConverter.cpp

@@ -367,16 +372,16 @@ namespace dcmqi {
              CHECK_COND(fgder->addDerivationImageItem(CodeSequenceMacro(code_seg.CodeValue,code_seg.CodingSchemeDesignator,
 				  code_seg.CodeMeaning),"",derimgItem));

-              //cout << "Total of " << siVector.size() << " source image items will be added" << endl;
+              // cout << "Total of " << siVector.size() << " source image items will be added" << endl;


Can you either remove this and the following commented out line, or swap for a logging command? I know this is most likely my code, not yours ....

Yes, sure. There are several places IIRC where I was not sure what to do with old "debug-like" commands. If you say I can do what I find useful, I will over all log messages and try to bring them into good shape.

fedorov · 2023-12-14T21:17:00Z

libsrc/Itk2DicomConverter.cpp

@@ -285,6 +291,7 @@ namespace dcmqi {

        Uint16 segmentNumber;
        CHECK_COND(segdoc->addSegment(segment, segmentNumber /* returns logical segment number */));
+        segNum2Label.insert(make_pair(segmentNumber, label));


Here, different values of segmentNumber can map to the same value of label, which may not be unique across input segmentation files. Is this ok?

Yes, looking further, this will definitely be a problem, since this map is later used for renumbering segments!

Maybe an easy thing to do is to check if label value is already present in the map prior to inserting it, and if it is indeed present, reset sortByLabelID to false, since sorting becomes impossible?

I assumed that labels are unique in the input. If this assumption does not hold, then indeed the sorting does not make sense (since the idea to use the original labels as Segment Numbers cannot work then if two labels use the same number)

fedorov · 2023-12-14T21:26:56Z

libsrc/Itk2DicomConverter.cpp

+
+    if (sortByLabelID)
+    {
+      sortByLabel(segdocDataset.get(), segNum2Label);


I do not see how this could possibly work. I do not see any checks for non-consecutive segment numbering, so if there are gaps in label numbers, the result will violate the conventions for having consecutive values for SegmentNumber. Did I miss something?

I think you are right, this is not addressed. Probably it can easily by done by either checking the labels in in segNum2Label, or iterating over the segments in Segment Sequence once mapping has been performed.

michaelonken · 2023-12-14T21:55:58Z

Thank you for the thorough review @fedorov! I will check soon and provide feedback.

…

-- Sent from mobile 14.12.2023 22:35:48 Andrey Fedorov ***@***.***>:

***@***.**** requested changes on this pull request. I spent quite some time with this PR, and I have a lot of questions. I am not sure it will work. Also, note that *MultiFrameDimensionModule* utilizes *ReferencedSegmentNumber* in the per-frame FGs, see: https://github.com/QIICR/dcmqi/blob/master/libsrc/Itk2DicomConverter.cpp#L58-L62. I would think that all of the frames would need to be reordered in the general case if the segments are renumbered. Let me know if it is easier to have a screen share to go over this! ---------------------------------------- In apps/seg/itkimage2segimage.xml[#485 (comment)]: > @@ -64,6 +64,15 @@ <description>Skip empty slices while encoding segmentation image. By default, empty slices will not be encoded, resulting in a smaller output file size.</description> </boolean> + <boolean> @michaelonken[https://github.com/michaelonken] do we really need this flag? I do not see any downsides in always sorting label IDs and always reusing those if possible in SEG. Since you already implemented this option, why not keep it but set the default to *true*? ---------------------------------------- In libsrc/Itk2DicomConverter.cpp[#485 (comment)]: > @@ -367,16 +372,16 @@ namespace dcmqi { CHECK_COND(fgder->addDerivationImageItem(CodeSequenceMacro(code_seg.CodeValue,code_seg.CodingSchemeDesignator, code_seg.CodeMeaning),"",derimgItem)); - //cout << "Total of " << siVector.size() << " source image items will be added" << endl; + // cout << "Total of " << siVector.size() << " source image items will be added" << endl; Can you either remove this and the following commented out line, or swap for a logging command? I know this is most likely my code, not yours .... ---------------------------------------- In libsrc/Itk2DicomConverter.cpp[#485 (comment)]: > @@ -285,6 +291,7 @@ namespace dcmqi { Uint16 segmentNumber; CHECK_COND(segdoc->addSegment(segment, segmentNumber /* returns logical segment number */)); + segNum2Label.insert(make_pair(segmentNumber, label)); Here, different values of *segmentNumber* can map to the same value of *label*, which may not be unique across input segmentation files. Is this ok? ---------------------------------------- In libsrc/Itk2DicomConverter.cpp[#485 (comment)]: > @@ -483,10 +494,117 @@ namespace dcmqi { segmentsOverlap = "NO"; else segmentsOverlap = "UNDEFINED"; - CHECK_COND(segdocDataset.putAndInsertString(DCM_SegmentsOverlap, segmentsOverlap.c_str())); + CHECK_COND(segdocDataset->putAndInsertString(DCM_SegmentsOverlap, segmentsOverlap.c_str())); + } + + if (sortByLabelID) + { + sortByLabel(segdocDataset.get(), segNum2Label); I do not see how this could possibly work. I do not see any checks for non-consecutive segment numbering, so if there are gaps in label numbers, the result will violate the conventions for having consecutive values for *SegmentNumber*. Did I miss something? ---------------------------------------- In libsrc/Itk2DicomConverter.cpp[#485 (comment)]: > @@ -285,6 +291,7 @@ namespace dcmqi { Uint16 segmentNumber; CHECK_COND(segdoc->addSegment(segment, segmentNumber /* returns logical segment number */)); + segNum2Label.insert(make_pair(segmentNumber, label)); Yes, looking further, this will definitely be a problem, since this map is later used for renumbering segments! — Reply to this email directly, view it on GitHub[#485 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABL56T6F2P24KZ5DNYYRDPLYJNWLHAVCNFSM6AAAAABAMMVHZCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOBQGE3TMMZSGY]. You are receiving this because you were mentioned. [Verfolgungsbild][https://github.com/notifications/beacon/ABL56T7GUQIWXK3DIMLBYG3YJNWLHA5CNFSM6AAAAABAMMVHZCWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTKDNK4M.gif]

michaelonken · 2023-12-15T11:14:34Z

Also, note that MultiFrameDimensionModule utilizes ReferencedSegmentNumber in the per-frame FGs, see: https://github.com/QIICR/dcmqi/blob/master/libsrc/Itk2DicomConverter.cpp#L58-L62.

Good catch, i missed the dimension assignment. The MultiFrameDimensionModule can stay like it is (first dimension Referenced Segment Number, second Image Position Patient) but the dimension assignment in the FrameContentMacro has to be updated as well.

After thinking about it, I assume it would be enough to also update the (Referenced) Segment Number used as 1st dimension value in Dimension Index Value in FrameContentMacro. The selection of frames and their order (denoted by second dimension) for the display do not need to change if we just change the Segment Numbers.

Comment 2023-12-18: I gave it some more thought and we could even keep it as is. The number of frames for display must not change at all if we re-number the segments (only). The display would not start any more with the old (unmapped) Segment 1 but with whatever Segment this has been mapped to, but this is not invalid or wrong. Of course one can adapt the order to start with new (mapped) 1 by applying the mapping to the Dimension Index Values as well in a second step.

fedorov · 2023-12-15T15:37:51Z

I assumed that labels are unique in the input. If this assumption does not hold, then indeed the sorting does not make sense (since the idea to use the original labels as Segment Numbers cannot work then if two labels use the same number)

Aha! That's the key. Yes, I do not think we can assume anything about the label uniqueness. So what do we do with this PR?

fedorov · 2023-12-15T15:50:22Z

apps/seg/itkimage2segimage.xml

@@ -64,6 +64,15 @@
      <description>Skip empty slices while encoding segmentation image. By default, empty slices will not be encoded, resulting in a smaller output file size.</description>
    </boolean>

+    <boolean>
+      <name>sortByLabelID</name>


How about renaming this as useLabelIDasSegmentNumber?

michaelonken · 2023-12-18T08:14:07Z

I assumed that labels are unique in the input. If this assumption does not hold, then indeed the sorting does not make sense (since the idea to use the original labels as Segment Numbers cannot work then if two labels use the same number)

Aha! That's the key. Yes, I do not think we can assume anything about the label uniqueness. So what do we do with this PR?

As discussed (offline), I am pretty sure I know how to fix this. But we will make the flag not the default behavior for now.

Renamed feature from sortByLabel to useLabelIDAsSegmentNumber. Added check for unique, monotonically increasing label IDs before starting reassignment. Added missing update of ReferencedFrameNumber.

michaelonken and others added 8 commits December 7, 2023 11:21

Replace deprecated writing routine.

107c359

Various small enhancements.

cee108c

Make sure input data has been initialized by user before public methods are called. Made logging message more consistent. Made code more readable.

Added documentation.

4d2bbb3

Merge branch 'QIICR:master' into use_label_id_as_segment_numbers

181d3e2

Fixed typos.

3576a86

Try to fix docker test.

4747eed

Added missing test dependency.

add comment to clarify label filter application

4f98007

fedorov requested changes Dec 14, 2023

View reviewed changes

fedorov reviewed Dec 15, 2023

View reviewed changes

michaelonken added 2 commits December 18, 2023 12:50

Fixing/enhancing this PR.

a4b433a

Renamed feature from sortByLabel to useLabelIDAsSegmentNumber. Added check for unique, monotonically increasing label IDs before starting reassignment. Added missing update of ReferencedFrameNumber.

Removed unused debug printouts.

2990dbd

fedorov merged commit a7cb0f7 into QIICR:master Dec 18, 2023
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use label id as segment numbers #485

Use label id as segment numbers #485

michaelonken commented Dec 8, 2023 •

edited

Loading

fedorov left a comment

fedorov Dec 13, 2023

michaelonken Dec 15, 2023

fedorov Dec 18, 2023

fedorov Dec 13, 2023

michaelonken Dec 15, 2023

fedorov Dec 14, 2023

fedorov Dec 14, 2023

fedorov Dec 14, 2023 •

edited

Loading

michaelonken Dec 15, 2023

fedorov Dec 14, 2023

michaelonken Dec 15, 2023

michaelonken commented Dec 14, 2023 via email

michaelonken commented Dec 15, 2023 •

edited

Loading

fedorov commented Dec 15, 2023

fedorov Dec 15, 2023

michaelonken commented Dec 18, 2023

Use label id as segment numbers #485

Use label id as segment numbers #485

Conversation

michaelonken commented Dec 8, 2023 • edited Loading

fedorov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedorov Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelonken commented Dec 14, 2023 via email

michaelonken commented Dec 15, 2023 • edited Loading

fedorov commented Dec 15, 2023

Choose a reason for hiding this comment

michaelonken commented Dec 18, 2023

michaelonken commented Dec 8, 2023 •

edited

Loading

fedorov Dec 14, 2023 •

edited

Loading

michaelonken commented Dec 15, 2023 •

edited

Loading