qurit / rt-utils

A minimal Python library to facilitate the creation and manipulation of DICOM RTStructs.
MIT License
181 stars 56 forks source link

Add shorthand function to merge two pre-existing RTStruct structure sets #70

Closed rheg49 closed 1 year ago

rheg49 commented 1 year ago
rheg49 commented 1 year ago

Had to merge two RTStructs (one for OARs, one for targets) and stumbled across your repository. First solution with the already existing functions was to create masks from one existing dataset and then re-add the mask to the second dataset. But with that solution the other metadata such as the color of the ROI got lost, so I coded my own version keeping all metadata and just working on the level of the data resulting from dcmread. Maybe this could be interesting for your toolbox?

asim-shrestha commented 1 year ago

Hey Robin! Thanks for looking into this :)

I think this is a great addition to the library but will need some modifications before we're able to bring it in. A few notes:

awtkns commented 1 year ago

It would probably be also good to add some documentation in the readme so people know that this exists.

rheg49 commented 1 year ago

Hey, thanks to both of you for the fast detailed and constructive feedback! That sounds very reasonable to me :)

Asim, I think I fulfilled your second and third point in my recent commit. I also switched to the usage of the RTStruct class and its built-in functions for the structure set reading instead of using pydicom functionalities.

I also understand the request to use add_roi for merging the structure sets, but this would require disassembly of the metadata from the RTStruct to be integrated in one of the existing RTStruct. As far as I can see, add_roi produces kind of artificial datasets for the three ROIContourSequence, StructureSetROISequence and RTROIObservationsSequence. That would cause to extract e.g. the color from the dataset to re-set it as parameter for add_roi. In addition (and from my POV a more powerful, because clinically motivated, argument), the type of the contour stored in RTROIObservationsSequence in the RTROIInterpretedType DICOM tag would get lost, if one uses the artificial dataset created by ds_helper.create_rtroi_observation(roi_data). So I think it would be easier to just keep the full dataset as it comes from the RTStruct to be merged and just append it to the other RTStruct. But if you prefer using add_roi instead of the hard-coded proposal in the recent commit, I can do that anyway.

On your fourth point: Are you speaking of these automatic GitHub routines that are checked on each commit? I would be happy to get some help or advice on this, because I am not experienced at all with those workflows :)

At the end of this, I can of course add some documentation and examples to the readme, thank you for the proposal, Adam!

asim-shrestha commented 1 year ago

Thanks for making those changes @rheg49! I think this is a fair middle ground solution. If you can, would you mind submitting a ticket to this repo with all of the information that would be missing from a call to add_roi? We should support adding those values within the tool.

In terms of tests, I was referring to the unit tests we have for the project found https://github.com/qurit/rt-utils/tree/main/tests. Maybe a few tests demonstrating the following merge cases:

rheg49 commented 1 year ago

Thanks for merging my code into your project! I recently submitted an issue on the DICOM tag missing in the add_roi call. I also added a MWE for the RTStructMerger to the README.md in another PR. For the tests I currently have no idea how to implement so I think I will need more time to get used to the coding structure to adapt the tests to the use case of RTStructMerger.