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 "labelmap" segmentations #234

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

Add "labelmap" segmentations #234

wants to merge 16 commits into from

Conversation

CPBridge
Copy link
Collaborator

This is an experimental feature for NA-MIC project week 2023

@CPBridge
Copy link
Collaborator Author

Update on the status of this. I successfully implemented "LABELMAP" style segmentations using the existing highdicom.seg.Segmentation class __init__ method at project week. You can trigger this by using segmentation_type="LABELMAP". This should work with all the input array formats currently supported by BINARY segmentations (except overlapping segments stacked down the final dimension).

Currently, the get_pixels... methods will not work correctly with "LABELMAP" style segmentations, I will try to add this at some point for prototyping purposes.

David Clunie is currently drafting up a formal proposal of what this would look like. I will try to keep this branch up to date with the formal proposal. Notably, there will likely be a new IOD created, rather than using the existing IOD with a new SegmentationType, and this will involve some refactoring of the codebase.

@CPBridge
Copy link
Collaborator Author

I have rebased this branch on the current release (0.22.0) so that it is now possible to use features from that release in combination with labelmaps, e.g. TILED_FULL segmentations, autotiling, multi-threaded encoding.

The plan is still to keep this as a draft pull request until the labelmap segmentation becomes part of the standard (assuming it does). However it remains a fully functional reference implementation for creating (but not yet parsing) labelmap segmentations in line with the current proposal.

@fedorov
Copy link
Member

fedorov commented Jan 30, 2024

@CPBridge related to the discussion in QIICR/dcmqi#491 (comment), are you restricting the dimensionality of LABELMAP in any way?

If I understand correctly, there is nothing that would prevent it from being n-dimensional, which would arguably allow to completely supersede binary SEG! Am I missing something?

@CPBridge
Copy link
Collaborator Author

CPBridge commented Jan 30, 2024

@fedorov the labelmap implementation on this branch is a generalisation of the BINARY/FRACTIONAL implementation in the latest release. Consequently it inherits all options and constraints from that implementation, except where those options/constraints explicitly differ between the BINARY and LABELMAP (e.g. segments may not overlap).

However this implementation is not fully general it what it allows users to create. Currently there are three options:

  • Frames indexed by image position (and segment number for BINARY). Note this does not imply/require regular spacing. This basically covers most 2D/3D radiology cases
  • Frames indexed by X and y offset in total pixel matrix (and segment number for BINARY). This covers most pathology use cases
  • Frames indexed by frame segment number (for BINARY) or frame label (just to have something to index off for LABELMAP). This covers 2D images that do not define a frame of reference coordinate system (e.g. chest x-rays)

Support for other (or custom) indexing is something I have considered adding and probably will at some point, but it is pretty complex.

In our conversation today I mentioned the 2D+T cine case merely theoretically, because it could in principle be observed.

The point of some disagreement boiled down to whether we should allow people to define two different labelmaps at the same image position. One reason for this night be to allow for multiple overlapping groups of segments.

  • Would a second dimension index be required to do this? Or is it actually allowed for two different frames in a multi frame image to have the same dimension index value across all dimensions? It seems we don't know the answer to this and it may be allowed. I feel that this definitely should not be allowed and I think there was general agreement here.
  • If a second index is required, should we create one for this purpose (e.g. a "labelmap index" attribute)? I feel that we should not encourage this but note that we can't really stop people from indexing down some junk dimension (e.g. frame label) to achieve this, so perhaps if you can't beat them, join them? It was in this context that I mentioned that I thought it would be perfectly valid and appropriate to do this using a time dimension in the cine case.

@pieper @dclunie @spalte @michaelonken

@michaelonken
Copy link

@CPBridge Hey Chris, do you have 8 and 16 bit PALETTE COLOR examples available as produced by highdicom? I am returning to the labelmap work in DCMTK/dcmqi. Thanks!

@CPBridge
Copy link
Collaborator Author

CPBridge commented Mar 4, 2024

@michaelonken there are several different versions in this shared folder. Note that the color palettes were improvised quickly and not very sensible, but they are valid.

@CPBridge
Copy link
Collaborator Author

Pasting the latest version of the DICOM supplement here: https://dicom.nema.org/medical/dicom/Supps/LB/sup243_lb_LabelMapSeg.pdf

@CPBridge
Copy link
Collaborator Author

CPBridge commented Jun 2, 2024

Some to-dos upon re-reading the latest supplement:

  • Ensure that the checks on the segment numbers are in line with the supplement (constraints on consecutive values are relaxed in the LABELMAP case. Also need to relax the constraints on the ordering of items within the segment sequence, especially for decoding.
  • Forbid segmented LUTs
  • Checks on combination of photometric interpretation and CIELab

Further thoughts:

  • Add conversion from BINARY to LABELMAP, following the convention mentioned in point 10 of the "closed issues", i.e. using the Source Instance Sequence?

@fedorov
Copy link
Member

fedorov commented Sep 26, 2024

@CPBridge - @dclunie is reminding me that this is now in the final text, and now this PR can be completed, whenever you have cycles!

@CPBridge
Copy link
Collaborator Author

Indeed, it's on the radar. Unfortunately this may take a while. In the meantime, on the volumetric branch (#277) I am significantly refactoring the segmentation code to have a shared underlying MultiframeImage class so that we don't have to have a load of shared code between segs, labelmaps, parametric maps etc and don't have to solve the same problems (volumes, tiling, efficiency) over and over again. Then once that's in place, it should be relatively straightforward to tweak this implementation to just be a special case of the general implmentation

@michaelonken
Copy link

I also have it on radar for DCMTK but the feature needs to wait a bit. It requires some extra code e.g. for handling of padding value but also major merging and testing and the time I can spend on this right now is limited as well.

@fedorov
Copy link
Member

fedorov commented Sep 27, 2024

Thank you both! 👍

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

Successfully merging this pull request may close these issues.

3 participants