Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for instance segmentation with LABELED segmentation type #184

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hackermd
Copy link
Collaborator

@hackermd hackermd commented Jul 15, 2022

This PR introduces changes to support storing multiple connected components for the same segment (labeled mask) in a DICOM Segmentation instance. This is currently not supported by the DICOM standard, but is necessary for many image analysis use cases in computational pathology (e.g., instance segmentation of single cells in whole slide microscopy images).

To support these use cases, all that would be necessary is a new segmentation type, which I called "LABELED".

Since we need to encode a potentially large number of instances (e.g., a whole slide image may easily contain > 100, 000 cells), support for larger bit depths (16-bit and 32-bit) is also required. This PR therefore also introduces changes to allow for encoding of segments with 32-bit unsigned integer pixels (currently restricted to "LABELED" segmentation type to not break existing behavior).

cc @fedorov @dclunie

@hackermd hackermd added the enhancement New feature or request label Jul 15, 2022
@hackermd hackermd requested a review from CPBridge July 15, 2022 22:53
@hackermd hackermd self-assigned this Jul 15, 2022
@hackermd hackermd marked this pull request as draft July 23, 2022 02:23
Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not had a chance to follow through all the logic yet. I believe there may be some specific situations where this implementation will not behave as expected. But I'm posting the comments that I have so far

@@ -121,7 +121,7 @@ Derive a Segmentation image from a multi-frame Slide Microscopy (SM) image:
)

# Create the Segmentation instance
seg_dataset = Segmentation(
seg_dataset = hd.seg.Segmentation(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the speculative nature of this PR, we should just fix this on master

src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
Comment on lines 504 to 505
'The number of allocated bits must be a multiple of 8 and '
'less than or equal to 32.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24 is a multiple of 8 and less than or equal to 32 ;)

Comment on lines +812 to +820
if segmentation_type == SegmentationTypeValues.BINARY:
# Note that integer arrays with segments stacked down
# the last dimension will already have been converted
# to bool, leaving only "label maps" here, which must
# be converted to binary masks.
planes = np.zeros(pixel_array.shape, dtype=np.uint8)
planes[pixel_array == segment_number] = 1
else:
planes = pixel_array
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong: the top branch (if branch) should be used if segmentation type is either BINARY or FRACTIONAL. We allow users to pass "label map" style pixel arrays regardless of the BINARY/FRACTIONAL segmentation type.

# The pixel values in the pixel array must all belong to
# a described segment
if not np.all(
if segmentation_type == SegmentationTypeValues.BINARY:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be perfectly permissible for a user to pass an unsigned integer array and request that it be stored as a FRACTIONAL seg, e.g. in order to use an efficient lossless codec or to ensure individual frames may be efficiently read.

Therefore this should include BINARY and FRACTIONAL, or in fact you might want to swap the order and make this case the else branch (i.e. if not LABELED)

'pixel type, the pixel array must be binary.'
)
pixel_array = pixel_array.astype(np.bool_)
if segmentation_type == SegmentationTypeValues.BINARY:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this needs to include FRACTIONAL


# Need to check whether or not segments overlap
if pixel_array.shape[-1] == 1:
# A single segment does not overlap
segments_overlap = SegmentsOverlapValues.NO
elif pixel_array.sum(axis=-1).max() > 1:
elif (pixel_array > 0).sum(axis=-1).max() > 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this change is required. To get here, the pixel array must have an unsigned integer type, so the new and old version of this line are logical equivalent as far as I can tell

segments_overlap = SegmentsOverlapValues.YES
else:
segments_overlap = SegmentsOverlapValues.NO

elif (pixel_array.dtype in (np.float_, np.float32, np.float64)):
elif pixel_array.dtype.kind == 'f':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll probably need some extra logic in this section for the LABELED case. You may want to either disallow floating point data types for LABELED, or check that the values are actually integer values (like the BINARY case where we allow users to pass a float array that contains only the vales 0.0 and 1.0 exactly)

@hackermd
Copy link
Collaborator Author

We may want to define a separate Instance Segmentation IOD and implement it as a separate highdicom.seg.InstanceSegmentation class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants