Introduce highdicom library for DICOM Segmentations

[CPBridge]

Addresses #195 (partially)

Introduces the highdicom library to handle the creation of DICOM segmentations. This should lead to more maintainable code by using the well-tested highdicom package for the complex task of segmentation creation, and should make adding new features to the segmentation writer operator considerably easier by leveraging the prior work in that library. Note that the complexity of the ad-hoc code in the dicom segmentation operator file as reduced signficantly.

I am considering this a largely complete "minimum viable" implementation and would appreciate feedback at this stage about whether we should go forward with this and what may need to change. Note that this is a backwards incompatible change to the signature of the DICOMSegmentationWriterOperator's __init__ method. It is now necessary to pass a list of highdicom.seg.SegmentDescription objects to the constructor to describe each segment in the created segmentation. I'd argue that the existing implementation, which hard codes in some default values, is likely to create dicom segs that are semantically incorrect in many cases (e.g. hard coding the segmented category as organ when it may be an abnormality such as a tumor or nodule). I am assuming that this backwards incompatibility is acceptable at this stage since the docstring states that the interface may change. Would appreciate thoughts on this.

I have attempted to update the two examples that use the seg writer: spleen and liver tumor segmetentations. I have managed to test the spleen example and visualize the results, and it's working nicely. I have not managed to test the liver tumor example. I may be being stupid but I can't find where to download the relevant files to actually run it.

Another thing I'm a bit confused about is where and how to add highdicom as an (optional) requirement. There are lots of requirements files and I wasn't totally sure how it all fits together. For now I just added highdicom to requirements_dev.txt. Please let me know whether this is correct.

My apologies that this took much longer than I anticipated to make happen (I largely blame this monster). Now I've figured out how this stuff all fits together, adding further operators (such as SR, GSPS) using the capabilities in highdicom should become much easier if we choose to merge this.

Tagging highdicom author and co-maintainer @hackermd for visibility.

[CPBridge]

Update: apparently I cannot type hint a variable with a highdicom type if highdicom is an optional import, which is why builds are currently failing. We will need to think about either making highdicom a required dependency, or come up with another way to pass all the relevant information to the DICOMSegmentationWriterOperator's constructor

[MMelQin]

Update: apparently I cannot type hint a variable with a highdicom type if highdicom is an optional import, which is why builds are currently failing. We will need to think about either making highdicom a required dependency, or come up with another way to pass all the relevant information to the DICOMSegmentationWriterOperator's constructor

Thanks for the PR.

There had been a architecture decision at the onset of the App SDK that there should be minimal number of dependencies in the App SDK proper, much like the MONAI core, and the same technique of using optional import is carried over to App SDK. In fact, even monai package itself is not a dependency of App SDK itself, although a few built-in operators/classes have to optionally import monai.

There had been an issue with the optional import in the built-in operator, MonaiSegInferenceOperator, which caused runtime error when the operator is not used in the application and rightfully the application does not include the optional depends in its requirements, but the issue was addressed in v0.2.

[gigony]

Hi @CPBridge

I think you can use the following patch to make it pass the format checker(including mypy)

--- a/monai/deploy/operators/dicom_seg_writer_operator.py
+++ b/monai/deploy/operators/dicom_seg_writer_operator.py
@@ -12,7 +12,7 @@
 import os
 from pathlib import Path
 from random import randint
-from typing import List, Union
+from typing import TYPE_CHECKING, List, Union
 
 import numpy as np
 
@@ -28,6 +28,10 @@ Sequence, _ = optional_import("pydicom.sequence", name="Sequence")
 hd, _ = optional_import("highdicom")
 sitk, _ = optional_import("SimpleITK")
 codes, _ = optional_import("pydicom.sr.codedict", name="codes")
+if TYPE_CHECKING:
+    from highdicom.seg import SegmentDescription
+else:
+    SegmentDescription, _ = optional_import("highdicom.seg", name="SegmentDescription")
 
 import monai.deploy.core as md
 from monai.deploy.core import DataPath, ExecutionContext, Image, InputContext, IOType, Operator, OutputContext
@@ -51,7 +55,7 @@ class DICOMSegmentationWriterOperator(Operator):
     # Suffix to add to file name to indicate DICOM Seg dcm file.
     DICOMSEG_SUFFIX = "-DICOMSEG"
 
-    def __init__(self, segment_descriptions: List[hd.seg.SegmentDescription], *args, **kwargs):
+    def __init__(self, segment_descriptions: List[SegmentDescription], *args, **kwargs):
         super().__init__(*args, **kwargs)
         """Instantiates the DICOM Seg Writer instance with optional list of segment label strings.

./run check -f

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[CPBridge]

Thanks a lot @gigony that seems to have fixed it nicely. It seems highdicom can remain an optional dependency

[MMelQin]

@CPBridge Can we add this new implementation of DICOM Seg writer as a new class/operator for v0.4? This way we allow a gradual transition and options for the users, and the example applications will not need to change for v0.4?

[CPBridge]

@MMelQin yes, totally open to whatever you think makes the most sense

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[CPBridge]

Closing in favour of #327