search

qurit / rt-utils

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

27 roigeneration algorithm #28

Closed ThomasBudd closed 3 years ago

ThomasBudd commented 3 years ago

Fixes issue #27. The value of the attribute ROIGenerationAlgorithm can now be set by the optional input argument ROIGenerationAlgorithm of add_roi. I'm not sure if the upper case argument matches the conventions of your library, would you prefer something like 'roi_generation_algorithm'? IMO the default value should be 'AUTOMATIC' as I guess that most of the users will use the library for exporting of some segmentations created by ML algorithms. In practice I found the previous default 'MANUAL' rare, our Radiologist work with 'SEMIAUTOMATIC' algorithms. In our lab we have three different dcm viewers and I found that one of them raises a complaint when the ROIGenerationAlgorithm is not one of 'AUTOMATIC', 'SEMIAUTOMATIC', or 'MANUAL', do you think it is worth raising a warning when a user picks a different option? Sorry for the very long message about this very simple change. Thanks for letting me participate in this!

asim-shrestha commented 3 years ago

Thanks for doing this! And no worries about the long message, I think its all worth considering.

I might be wrong but I think the options you mentioned might be the only ones possible, at least according to this link.

With that in mind, I'm thinking we add an ROI_GENERATION ENUM for those 3 values so that users can import them for use. This way they won't have to worry about spelling / capitalization / other user errors. What do you think?

ThomasBudd commented 3 years ago

I checked the dcm rt files yesterday with the Microsoft Radiomics app that is being used in our dept. That one accepts also files with arbitrarily set arguments for ROIGenerationAlgorithm. So I would suggest both options, allowing 0=\'AUTOMATIC\', 1=\'SEMIAUTOMATIC\', 2=\'MANUAL\' as an input argument and strs for people who really want to use a different option, but printing a notice in that case. What do you think?

asim-shrestha commented 3 years ago

@ThomasBudd That sounds good, lets use an enum for the values and we can do the string validation in roi_data I believe (I think colours are validated there as well)

ThomasBudd commented 3 years ago

Sorry for the late reply. I moved the valiadation of roi_generation_algorithm now to ROIData as you suggested.

asim-shrestha commented 3 years ago

Hey @ThomasBudd, just in the middle of exams at the moment but ill have a look at this on Monday and hopefully get a new version released with it all :)

ThomasBudd commented 3 years ago

Hey @asim-shrestha, I'm just on my way into my vacation, I will come back in two weeks to work further on this if required :)