[Sunderlandkyl]

The "Referenced Segment" content item of TID 1411 (Volumetric ROI Measurements) only accepted Segmentation Storage and Surface Segmentation Storage objects. Now, Label Map Segmentation Storage objects are accepted as well, since they are also instances of the Segmentation IOD and, thus, "Segmentation Image" objects in the sense of TID 1411 Row 7.

Extended the TID 1500 test accordingly: a label map segmentation is accepted, both as a reference value and as a dataset, while an image is rejected with CMR_EC_InvalidSegmentationObject.

[jriesmeier]

@Sunderlandkyl Thank you for the PR, which looks quite good to me. About two years ago, I already added the Label Map Segmentation Storage SOP Class to DSRImageReferenceValue::isSegmentationObject() with commit efe0f6f (see also commit ef6aea0). Perhaps, we could use this method for the check in TID1411_VolumetricROIMeasurements<>::setReferencedSegment() to avoid code duplication.

However, I first need to check what DICOM PS3.16 means by "Segmentation Image", since this term is not clearly defined. For example, does this term refer only to the Segmentation IOD or also the Height Map Segmentation IOD?

[Sunderlandkyl]

Thanks @jriesmeier! Happy to update the PR either way!

[fedorov]

However, I first need to check what DICOM PS3.16 means by "Segmentation Image", since this term is not clearly defined. For example, does this term refer only to the Segmentation IOD or also the Height Map Segmentation IOD?

@dclunie @CPBridge what are your thoughts on this one? As it always happens, putting new features of the standard to work uncovers unexpected questions!

[jriesmeier]

@fedorov I've already sent David an email asking him about it. Maybe, we need another CP for that.

[fedorov]

Jörg, thank you!

What does it mean for this PR? Would it make sense to allow LABELMAP now and add additional classes later - whenever it is clarified?

[CPBridge]

Pragmatically it would appear to make sense to allow labelmaps here though I honestly don't know if whether that can be inferred from the current standard text.

On a tangentially related note, I am more concerned about its cousin TID1410 actually, which has a ReferencedSegmentationFrame item on the assumption inherited from the BINARY seg that a single segmentation frame contains a single segment. It would appear problematic to use that with LABELMAPs, since a single frame can contain many segments but there is no mechanism currently to specify which. This is something I noticed a while back and never did anything about...

[jriesmeier]

Andrey,

What does it mean for this PR? Would it make sense to allow LABELMAP now and add additional classes later - whenever it is clarified?

Of course, that would be possible. However, we've already had "feature freeze" for DCMTK 3.7.1, so we'll have to wait for the release of this version, which is expected to happen later this month.

[fedorov]

TID1410 actually, which has a ReferencedSegmentationFrame item on the assumption inherited from the BINARY seg that a single segmentation frame contains a single segment

Chris, I do not see the problem. The segment number will be specified in the reference - isn't it sufficient?

[CPBridge]

I'm sorry, you are right, there's a place for it the Image Content Item

[CPBridge]

For what it's worth, highdicom has allowed references to labelmap segmentations in the referenced segment / referenced segmentation frame items in TID1410 and TID1411 since we supported LABELMAP segs. Rightly or wrongly...

[dclunie]

@jriesmeier we do need a CP with definitions for completeness, but I see no reason not to allow labelmaps (and heightmaps) as the target of the ReferencedSegmentationFrame related content items since the text does not forbid it and the macro supports the reference and all of those IODs use SegmentNumber.

[jriesmeier]

@dclunie OK, I will come up with a proposal and submit a CP. However, that will take some time.

@Sunderlandkyl We should then be able to use DSRImageReferenceValue::isSegmentationObject(), as described above.

[fedorov]

I agree with @dclunie - I do not see how the standard forbids the use of labelmap, and I do not quite see why a CP would be needed before this PR can be merged (I do understand the feature freeze constraint, although, arguably, this is a bug fix and not a feature).

[jriesmeier]

@fedorov The CP is needed to define terms such as "Segmentation" and "Segmentation Image", which are currently used in the standard text without definition. This is not directly related to the implementation of the DICOM standard, e.g., the proposed changes to the DCMTK. I just happened to come across it while I was looking up some information in the standard for this PR.

However, if an implementation deviates from the possible features of the DICOM standard - since the standard has evolved in the meantime - I wouldn't call that a "bug fix". It's more of a missing feature. Or a matter of the completeness of an implementation (in this case, when creating an SR document based on a specific template). Anyway, I'll talk to the other team members to see if they have any issues with me checking in the necessary changes before the release.

[Sunderlandkyl]

Thanks all! @jriesmeier updated to use isSegmentation().

[fedorov]

@jriesmeier I agree with you, and to be clear - I am not pushing to deviate from your procedures! Thank you for your support, as always!

[jriesmeier]

@Sunderlandkyl The PR has now been merged to the "testing" branch (with minor changes to the commit message, e.g., regarding the line length). Commit ea1f2e8 should be visible in the public git repository (i.e., "master"), after the nightly builds have been completed successfully and additional integration tests have run without errors.