Update dicom_seg_writer_operator.py

[kavmar]

Hi @CPBridge

Proposing these changes with respect to #512 (comment)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[CPBridge]

Hi @kavmar,

Sorry for slow response as I am travelling over these few weeks. Thanks for stepping up and creating this PR. However, there are a few problems that I see here.

Firstly, I feel strongly that a segment description should not be omitted purely because it does not happen to be present in a particular image. It conveys important information to the receiver of the file when a segment description is included when the corresponding segment is not present: it indicates that the creator of the file checked for the presence of that segment and determined that it was not present. This information is lost if you just omit all segments that are not present. Therefore, the proposed changes should be limited to correcting the mapping of input values to segment numbers stored in the file, and should not attempt to remove or change the segment descriptions themselves.

Secondly, the proposed method for specifying this behaviour could be streamlined in my opinion. The force_continguous_labels parameter does not seem necessary. Why not just enable the behaviour when label_mapping_dict is present, and disable it when label_mapping_dict is None? This would save a parameter. Alternatively, in my personal opinion, I think this could be streamlined even further by removing both the force_contiguous_labels and label_mapping_dict parameters, and adding a further optional parameter to the SegmentDescription class (the one defined in monai app sdk not the class of the same name in highdicom) that specifies the pixel value that segment will take in the input segmentation masks. The writer operator class can then use this information to determine the mapping of input pixel values to segment numbers: if the input pixel value is specified for a given segment, it is used as provided, if it was not specified in the segment description it s assumed to the position of the segment description in the list (the current behaviour). To me this feels neater because it groups all the related information about each segment into one place (the SegmentDescription) rather than splitting it up into different places.

Also, the logic to relabel the segmentation mask is currently a little overcomplicated and will be slow for large arrays. There is a simple numpy trick to do this efficiently in a single operation, see here.

Another minor comment is that I would not make class properties "public" unless there is a good reason to do so. Therefore I would suggest renaming self.force_contiguous_labels to self._force_contiguous_labels and self.label_mapping_dict to self._label_mapping_dict (if these are even kept of course). This gives us more flexibility to change how things work under the hood in the future without breaking the public API.

[kavmar]

Hi @CPBridge and thanks for response.

Hi @kavmar,

Sorry for slow response as I am travelling over these few weeks. Thanks for stepping up and creating this PR. However, there are a few problems that I see here.

Firstly, I feel strongly that a segment description should not be omitted purely because it does not happen to be present in a particular image. It conveys important information to the receiver of the file when a segment description is included when the corresponding segment is not present: it indicates that the creator of the file checked for the presence of that segment and determined that it was not present. This information is lost if you just omit all segments that are not present. Therefore, the proposed changes should be limited to correcting the mapping of input values to segment numbers stored in the file, and should not attempt to remove or change the segment descriptions themselves.

I get your point. However, based on my experience, I am concerned with downstream usability. Imagine that the SEG is a result of a model, which is used as an initial segmentation of a multilabel segmentation, such as TotalSegmentator or Vista3D, and the consumer would use it to further finetune some of the classes. I case the input volume would be only a subregion (head) of all possible model outputs (wholebody), the resulting SEG file would have in your described situation all the classes and the user would need to manually check if there aren't any voxels miss classified. This would be very cumbersome, to go through all the classes and check them.
I could imagine to leave the empty segments in if they would be somehow clearly marked. In a head scan, something like "Aorta - undetected" or "Liver - empty", so that the user would know right away that he/she doesn't need to check false positives.

Secondly, the proposed method for specifying this behaviour could be streamlined in my opinion. The force_continguous_labels parameter does not seem necessary. Why not just enable the behaviour when label_mapping_dict is present, and disable it when label_mapping_dict is None? This would save a parameter. Alternatively, in my personal opinion, I think this could be streamlined even further by removing both the force_contiguous_labels and label_mapping_dict parameters, and adding a further optional parameter to the SegmentDescription class (the one defined in monai app sdk not the class of the same name in highdicom) that specifies the pixel value that segment will take in the input segmentation masks. The writer operator class can then use this information to determine the mapping of input pixel values to segment numbers: if the input pixel value is specified for a given segment, it is used as provided, if it was not specified in the segment description it s assumed to the position of the segment description in the list (the current behaviour). To me this feels neater because it groups all the related information about each segment into one place (the SegmentDescription) rather than splitting it up into different places.

My proposal would leave the option to the user to have the current behavior and leave undetected classes in the SEG file. I am not trying to push my solution. Just to give a space to have the option for those who need or want it. That is why I introduced the parameter 'force_contiguous_labels = False'.

Also, the logic to relabel the segmentation mask is currently a little overcomplicated and will be slow for large arrays. There is a simple numpy trick to do this efficiently in a single operation, see here.

Another minor comment is that I would not make class properties "public" unless there is a good reason to do so. Therefore I would suggest renaming self.force_contiguous_labels to self._force_contiguous_labels and self.label_mapping_dict to self._label_mapping_dict (if these are even kept of course). This gives us more flexibility to change how things work under the hood in the future without breaking the public API.

Cleaning up the code, as you propose is of course OK, including more clear names, ...

How do we continue?

[kavmar]

Hi @kavmar,

Sorry for slow response as I am travelling over these few weeks. Thanks for stepping up and creating this PR. However, there are a few problems that I see here.

Firstly, I feel strongly that a segment description should not be omitted purely because it does not happen to be present in a particular image. It conveys important information to the receiver of the file when a segment description is included when the corresponding segment is not present: it indicates that the creator of the file checked for the presence of that segment and determined that it was not present. This information is lost if you just omit all segments that are not present. Therefore, the proposed changes should be limited to correcting the mapping of input values to segment numbers stored in the file, and should not attempt to remove or change the segment descriptions themselves.

A quick note: TotalSegmentator also exports only non-empty masks: https://github.com/wasserth/TotalSegmentator/blob/0fe651c7680d76f64dad9ae4a5be69290c184617/totalsegmentator/dicom_io.py#L150