Implement highdicom seg operator #327
Conversation
Signed-off-by: Chris Bridge <chrisbridge44@googlemail.com>
Signed-off-by: Chris Bridge <chrisbridge44@googlemail.com>
Signed-off-by: Chris Bridge <chrisbridge44@googlemail.com>
|
@CPBridge Thank you very much for the updated PR. I was very eager to try it out with the Spleen example app. Happy to say that the new writer works well, after I fixed a couple issues in other relevant sources,
How many segments can the new writer support? The existing one can support upto 8, as it caps the BitsAllocated at 8, as such the UNETR example app could not use the exiting Seg Writer for it can segment upto 13 types of organs. |
|
Thanks for trying it out @MMelQin ! Would it make sense for you to push your fixes for the spleen segmentation app directly to this PR?
From the start, we have keen that highdicom should not contribute to the huge amount of non-compliant DICOM around, so we have implemented quite strict checks on input values to ensure that any file we produce is valid (and deliberately propagate some errors up from pydicom too, as you found). This does occasionally cause issues with existing source data that is invalid, such as this date formatting issue. Similarly the test data used for the unit test has an invalid name (
If this is true then I think that the current implementation might be quite wrong. I believe per the standard, BtisAllocated is capped at 8 for the Segmentation IOD but the BitsAllocated is irrelevant to the number of segments since each segment is recorded as its own frame within the segmentation. (The only use for all 8 bits is when using "fractional" segmentations, where a pixel value represents either the probability that a pixel belongs to a segment, or a the occupancy of that pixel that belongs to the segment. Highdicom supports both but I have not yet implemented fractional segmentations in the seg writer but will try to add this once the overall approach is agreed upon). Therefore there is in principle no limit on the number of segments, and highdicom has no limit in theory. In practice of course a large number of segments can be quite space intensive. Any way long story short this should be no problem at all to use for the 13 classes of the UNETR example, I have personally used the highdicom implementation for more than that in the past. So are you happy with the overall approach and API? If so I will make a couple of further generalizations and fix up the spleen segmentation app before merging? |
|
@CPBridge Thanks for the clarification. The existing Seg Writer supports only Binary Seg type, and has the segment sequence properly assigned. "As a consequence of the enumerated Bits Allocated and Bits Stored Attribute values, single bit pixels shall be packed 8 to a byte as defined by the encoding rules in PS3.5.", so the pixel value binary is quite compact. In any case, I will for sure try out the UNETR app with >8 segments. |
I think I see the possible root of the confusion relating to which 8 pixel values are "packed" into a byte. For segmentations with SegmentationType of "BINARY", the BitAllocated is always 1 regardless of the number of segments, meaning that there are 8 pixel values stored within a single byte. However, these 8 pixels are not drawn from 8 different segments but rather from 8 spatially consecutive pixels within a given segment (you can see this since pixels are encoded as Pixel Sample Values from left to right and top to bottom and for binary segmentations there is only one value per Pixel Sample Value since SamplesPerPixel is 1). Each segment is encoded within a separate frame (or set of frames) of the segmentation, and there can be an arbitrary number of these. As a consequence there are still 8 pixels per byte but no limit on the number of segments. If the old implementation was packing 8 pixels from different segments together into a byte (I haven't checked the implementation to see whether or not it was) then I think it is likely wrong. |
The app is still not compatible with monai v0.9.1 as the app testing revealed that to its Invert transform failed resample the predicted image back to input image spacings. Also, the new Seg Writer impl is strict on DICOM attribute VR conformance, and would throw exception when the input DICOM instances have non-conformant attribute VR values. Signed-off-by: mmelqin <mingmelvinq@nvidia.com>
Signed-off-by: mmelqin <mingmelvinq@nvidia.com>
…version Signed-off-by: mmelqin <mingmelvinq@nvidia.com>
| series_number=random_with_n_digits(4), | ||
| sop_instance_uid=hd.UID(), | ||
| instance_number=1, | ||
| manufacturer="The MONAI Consortium", |
There was a problem hiding this comment.
Nit: For now this set of attributes can be left hard coded. They are mostly used for the contributing equipment module, and have been encapsulated in the App SDK class, EquipmentInfo, which is still mixed in the module of the SR writer. The plan was to have the Model Info and Equipment Info all encapsulated and passed down from the App to all writer types, so that the generated DICOM objects have correct attributes conforming to the IHE AI Result Profile. profile, etc.
as the use of highdicom require pydicom>=2.3.0 Signed-off-by: mmelqin <mingmelvinq@nvidia.com>
Yes, for sure needed a look when we have time. That piece of code was contributed by a guru on the DICOM Standards Committees, quite a number of years back. I looked into the issues with bits packing for more than 8 segments, but put it in the back burner due to other high priority items, and also the multi-seg DICOM Seg could be loaded and viewed in OHIF Viewer. The other issue was that existing impl hardcoded the Recommended Display CIELab Value Attribute to the same value for all segments; on the plan to fix too, but now the new impl does not set this Type 3 attribute, so one less concern. @CPBridge I have updated the app, and pinned the monai to v0.9.0 in the requirements txt as well as in the App SDK operators, so that both local run as well as Docker run succeed. Switching back to >=0.9.0 will be done when the combability issue is addressed. Please review and approve these changes. While I am at it, I have updated the other existing example apps that generate DICOM Seg, namely the LiverTumor and UNETR, in fact both have multiple segments with the latter up to 13. All have been tested working. Please take a closer look at the UNETR App where for a few segments I had to resort to use the generic There are a few minor observations on the headers generated by HighDicom, albeit all Type 2 or Type 3 attributes:
|
Signed-off-by: mmelqin <mingmelvinq@nvidia.com>
Signed-off-by: mmelqin <mingmelvinq@nvidia.com>
|
Kudos, SonarCloud Quality Gate passed!
|
|
@CPBridge I have found all the codes for the 13 organs in the UNETR app, by searching the pydicom dictionary modules and SNOMED CT portals. So I am happy with the apps now. The other nits on filling in more useful but non mandatory attributes, e.g. series description, date and time, contributing equipment module, etc. can be added later in separate lower priority issues. |
|
Hey @MMelQin thanks so much for the review and for bringing the examples in line with the changes! Glad it's working well. Now we have figured out how this can work, I can work on other operators leveraging the things we have developed in the library (various types of SR, GSPS, Parametric Map) |
@CPBridge Thank you too for having a good design and bring this key package into App SDK, without much learning curve for our target user persona. This is definitely the beginning of a good integration, and happy to see the good output will circle back and benefit the production deployment in the clinics. |
| monai == 0.9.0 |
There was a problem hiding this comment.
are there any specific issues working with monai v0.9.1? we can improve the core codebase...
There was a problem hiding this comment.
The v0.9.1 broke the use of invertd with the way it is used in the apps, I have created an issue to update the app and will consult the migration guide. Thanks.
I am recreating this pull request since I completely borked #298 due to the (extremely frustrating) DCO sign off requirement and subsequent attempts to fix it that made it worse 😣
This PR 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 is reduced significantly.
Unlike the original version of #298 I have now removed the highdicom class
highdicom.seg.SegmentDescriptionfrom the public API of the monai deploy class. However I have left thepydicom.sr.coding.Codeclass in the public API as we discussed in the last meeting.I have not yet changed the examples to use the new version of the operator, as this requires merging with some recent changes due to the bundle that I still need to get my head around. This will need to be addressed before merging so that the examples run. But I would like to get agreement on the API of the class before fixing the examples.
Update:
All example apps that make use of the Segmentation Writer have been updated in this PR, and all used the appropriate codes. Also the monai core is pinned at v0.9.0 because the latest version 0.9.1 is not compatible with the apps (not related to the new Seg Writer itself)