-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
Conversation
…ghdicom into feature/instance-segmentation
There was a problem hiding this 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( |
There was a problem hiding this comment.
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
'The number of allocated bits must be a multiple of 8 and ' | ||
'less than or equal to 32.' |
There was a problem hiding this comment.
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 ;)
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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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': |
There was a problem hiding this comment.
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)
Co-authored-by: Chris Bridge <[email protected]>
We may want to define a separate Instance Segmentation IOD and implement it as a separate |
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