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 shorthand function to merge two pre-existing RTStruct structure sets #70

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

rheg49
Copy link
Contributor

@rheg49 rheg49 commented Jan 30, 2023

  • Merge two structure sets belonging to same image series to one resulting structure set
  • Check DICOM metadata for compatibility with one another and with image series
  • Append ROIContourSequence, StructureSetROISequence and RTROIObservationsSequence of first dataset to array of second dataset and save merged dataset as new RTStruct file afterwards
  • Edit ROI reference number to avoid ambiguity, check datasets for name duplication

Merge two structure sets belonging to same image series to one resulting structure set
Check DICOM metadata for compatibility with one another and with image series
Append ROIContourSequence, StructureSetROISequence and RTROIObservationsSequence of first dataset to array of second dataset and save merged dataset as new RTStruct file afterwards
Edit ROI reference number to avoid ambiguity, check datasets for name duplication
@rheg49
Copy link
Contributor Author

rheg49 commented Jan 30, 2023

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?

…rent ROI name

previously, accidently 'iterated' over only the name of the StructureSetROISequence object which is not defined and led to an AttributeError
@asim-shrestha
Copy link
Contributor

asim-shrestha commented Jan 31, 2023

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:

  • We should use the underlying RTStruct type within this package itself for all of the merging operations. This ensures we can avoid a hard dependency on pydicom in multiple places. I understand that this results in some metadata loss. Is there any other metadata you're looking for besides color? We should work to include that in add_roi itself first. And small note, color is already an optional parameter for add_roi, did it not work for your use case?
  • The merge function shouldn't care about persistence of the rt struct so we can remove the code to write it to the filesystem
  • We should place this within a separate merging class
  • For functionality like this, we should add some tests

@awtkns
Copy link
Contributor

awtkns commented Jan 31, 2023

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

- read existing structure set files as RTStruct class object with the RTStructBuilder
- remove saving and export of resulting file, just return merged RTStruct object instead
@rheg49
Copy link
Contributor Author

rheg49 commented Jan 31, 2023

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
Copy link
Contributor

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:

  • Merging two rt structs with no ROIs
  • Merging two rt structs where rt struct 1 has an ROI but the other doesn't and vice versa
  • Merging two rt structs, each having multiple ROIs with no conflicts
  • Merging two rt structs, each having multiple ROIs with some conflicts
    I've created a separate ticket for tests: Add unit tests for merging RT Structs #72
    Feel free to tackle it if you'd like :)

@rheg49
Copy link
Contributor Author

rheg49 commented Feb 7, 2023

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.

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