QIICR / QuantitativeReporting

Segmentation-based measurements with DICOM import and export of the results.
https://qiicr.gitbooks.io/quantitativereporting-guide
Other
23 stars 23 forks source link

Segment name should be initialized from 3dSlicerLabel attribute in the terminology context #79

Closed fedorov closed 7 years ago

cpinter commented 8 years ago

I can implement this feature easily, but I didn't do that on purpose. As I explained earlier: "We also agreed that if a segment has a default name, then it is changed from the chosen terminology. I didn’t implement this feature, because some things are not clear: How do we assemble the name? (if the type has a modifier, or if we want to use 3dSlicerLabel then it may not be found, languages etc.) How do we make sure the user does not want the name that we identified as default? Again, it would probably be better to make sure some terminology is selected first, as the users should know what they are segmenting."

Using the 3dSlicerLabel field makes sense, but questions still remain. Should I just ignore those and add this default name replacement feature?

fedorov commented 8 years ago

Right, but also as I responded earlier.

· We also agreed that if a segment has a default name, then it is changed from the chosen terminology. I didn’t implement this feature, because some things are not clear: How do we assemble the name? (if the type has a modifier, or if we want to use 3dSlicerLabel then it may not be found, languages etc.) How do we make sure the user does not want the name that we identified as default? Again, it would probably be better to make sure some terminology is selected first, as the users should know what they are segmenting.

I don't have a solution about how to assemble a name. I think for now it makes sense to use "3dSlicerLabel" as that name. Where "3dSlicerLabel" is not populated, we can add a meaningful value. Where we have a modifier, we can augment the name.

If the user doesn't like the default, it will be editable, right? Changing the name of the segment doesn't change the codes for the terminology. It is just an uncontrolled free text.

Definitely the user should first select the terminology before doing any segmentation, and GeneralAnatomy should be that default selected automatically, I think.

I agree with you on almost all points, except I don't think default should be assigned at the time of storage.

I think user should be forced to choose the default, and be aware of what default was chosen. I also think that from the perspective of a user who doesn't want to deal with terminology, the impact should be minimal.

I think it is possible to achieve this rather easily. I believe in most cases GeneralAnatomy LUT is used as a default LUT. We can keep it the same way. The only thing that changes is that now each of the items in the LUT has a code assigned. The first item in that LUT is "Tissue", and I think it makes sense to keep that as default.

About the colors, GenericAnatomy has the colors assigned. Of course, with the new design, we can have multiple segment that correspond to the same tissue type (e.g., multiple tumors), and will have identical colors. I think in this situation, it will be up to the user to change those colors, if they don't like the defaults. I think specialized applications that are more aware of the specific use case can handle this better (e.g., assign consecutive colors from the GeneralAnatomy color set to the consecutively segmented tumors).

To go over specific items you listed and I didn't address explicitly perhaps:

cpinter commented 8 years ago

Thanks Andrey! OK I think I see where the misunderstanding is.

The very last point ("it would probably be better to make sure some terminology is selected first, as the users should know what they are segmenting") in my interpretation meant that when the user creates a segment, the exact terminology entry could be populated before allowing it to be edited (this is what the "users should know what they are segmenting" part refers to).

This is the main reason I haven't added this feature yet, as if we decide do go the initial terminology selection way, then it will be a major change in workflow. It is not clear that selecting the terminology (or assigning suitable defaults) is the best way, and also not clear yet how it could be done best, but I think this should be decided. In the meantime I can add this feature, I really don't want to hold it up as it's an easy one.

fedorov commented 8 years ago

Csaba, this doesn't hold anyone, I just think that it makes things more clear to the user. I still (sorry!!!) don't understand how populating the name with the default will make things worse or more confusing than they are now, with the default LUT selection. This can wait until we have a chance to discuss later, no rush at all!

fedorov commented 8 years ago

Csaba, I propose to do the following:

1) assign "3D Slicer General Anatomy list" to be the default terminology for all newly created segments 2) when a new segment is created, automatically assign Category=Tissue, PropertyType=Tissue 3) automatically set segment Name to be the same as PropertyType

If you see issues with this approach, we can probably just implement it in Reporting module, and decide how to proceed with the SegmentEditor later. What do you suggest?

As is right now, terminology of a newly created segment is not initialized, UI is confusing, since "Color" name does not communicate to the user that the color swatch also has a tooltip with the terminology details, and "Name" column content is not particularly helpful.

If I again missed your point, let's have a quick chat!

cc: @che85

lassoan commented 8 years ago

What would you recommend to do with colors? By default we should have different colors, but by choosing tissue/tissue everything would be just all green (and even if we implement automatic changing of shades when there are similar colors in the segmentation, it would be all just too green).

Maybe we could have a shortlist with some basic tissue types (fat, muscle, bone, skin, tumor, foreign object, etc) that we would use to generate new segments? The shortlist could be either hardcoded (specified in application settings) or generated automatically (it could contain the list of most recently used segments).

How terminology selection is done in other software?

pieper commented 8 years ago

The generic anatomy table was designed to have a short list of common ones in the low numbers. And it has generic 'regions' starting at 292.

https://www.slicer.org/wiki/Slicer3:2010_GenericAnatomyColors

You could start with region 0 (292) and then just autoincrement for each new segment if it hasn't already been selected. Seems unlikely anyone would pick more than 16 new segments manually (if they do that much work they can also define a custom table).

I'm not sure how other software handles terminology but we could investigate at RSNA.

On Mon, Nov 14, 2016 at 3:46 PM, Andras Lasso notifications@github.com wrote:

What would you recommend to do with colors? By default we should have different colors, but by choosing tissue/tissue everything would be just all green (and even if we implement automatic changing of shades when there are similar colors in the segmentation, it would be all just too green).

Maybe we could have a shortlist with some basic tissue types (fat, muscle, bone, skin, tumor, foreign object, etc) that we would use to generate new segments? The shortlist could be either hardcoded (specified in application settings) or generated automatically (it could contain the list of most recently used segments).

How terminology selection is done in other software?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/QIICR/Reporting/issues/79#issuecomment-260457467, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHsfe5ujOylu-WlgapEpG-eH8u6Ykpfks5q-MiwgaJpZM4KRfeD .

fedorov commented 8 years ago

What would you recommend to do with colors? By default we should have different colors, but by choosing tissue/tissue everything would be just all green (and even if we implement automatic changing of shades when there are similar colors in the segmentation, it would be all just too green).

That is a valid concern, I agree. How about taking consecutive colors from the Slicer Generic Anatomy LUT (as it is defined in the original LUT) when a duplicate color is encountered? I think it should also be accompanied by a popup notifying the user that a default color for the selected tissue has been changed, and an option to disable this override.

How terminology selection is done in other software?

Figuring this out is one of the goals of our RSNA exhibit! https://fedorov.gitbooks.io/rsna2016-qirr-dicom4qi/content/ (very few responses so far!)

But reality is other software usually don't handle this selection.

fedorov commented 8 years ago

Sorry, I forgot to write this. The reason color override should be optional is that in some situations I can see how the user might want to segment multiple instances of the same structure and keep the same color. It should be allowed to do that.

naucoin commented 8 years ago

There's been quit a bit of work on the atlas colour tables in Slicer to define colours that work together for segmenting anatomical structures, my vote for this issue goes to @pieper 's suggestion of sticking with values in the Generic Anatomy table and using the associated terminolgy values rather than trying to redo a color set from scratch.

fedorov commented 8 years ago

We are using those colors, that is not the question.

The difficulty arises due to the flexibility of the new terminology support. In the Slicer color LUTs, there was fixed assignment of color to the label name, and only one instance of the given label color/name was allowed in a segmented image.

In the new approach, color can be changed independently from the segment semantics AND it is possible to have multiple segments of the same type. The question is how to assign colors to the segments of a type that is already present in the list of segments.

There is no question that we should use colors from generic anatomy LUT. The question is how to implement this. What I suggest is that when a segment is introduced to the list with the color that matches color already in the list, do the following:

naucoin commented 8 years ago

Thanks for the clarification! For picking the new colour, skipping up to index 292 (label "region 0") as @pieper suggested and reassigning it with the same segment type makes sense, you're not over riding any specific anatomical names.

fedorov commented 8 years ago

you're not over riding any specific anatomical names

We are, because each term in the terminology may have a pre-assigned color. By assigning a different color we modify that default. What I was saying is that in some situations user may want to have the same color assigned to the items of the same type.

cpinter commented 8 years ago

Very useful discussion!

Using generic anatomy colors makes a lot of sense. Segment Editor already assigns consecutive colors from a different color table (Labels, based on an arbitrary decision earlier). I'm also not against the popup window (but I know Ron is in general against popups). Your suggestion sounds exactly like what Subject hierarchy is doing now, asking whether an operation is desired (No, Yes, Always).

Automatic Tissue/Tissue selection is something we also discussed here. One question about that was the color, but with this consecutive generic color assignment is seems to be solvable. When the user chooses terminology explicitly, then setting the PropertyType name to segment name is fine. Propagating the automatic Tissue/Tissue selection to the segment name, however, does not seem to be any better than having the default Segment_N name. I like the short list approach that Andras and Steve mentioned. I think a good implementation of this would be to assign the terminology equivalent of the consecutive structures from generic anatomy along with its color. If the structure is not what the user wants, then he/she would need to select a terminology for that structure manually (which is good in the sense that they are forced to select a correct terminology, and bad because it is more user interaction - the question is what is more important, fastness or correctness).

lassoan commented 8 years ago

I agree with Ron that popups are really bad. We don't need to use them, just go with the most common choice (choose different colors for tissue/tissue). We could add a "default" checkbox next to the color swatch and next to the segment name editbox (we would add an editbox to the terminology window where the user can see/edit the segment name). If the default checkbox is checked then the color/segment name is automatically set to the default (and updated automatically when user selects a different terminology item). If the default checkbox is unchecked then it means that the user set a custom color/segment name, which will be kept, even when a different terminology item is selected.

fedorov commented 8 years ago

I agree with Ron that popups are really bad.

I tend to not agree with the sweeping statements like this. It all depends on the context and the choice between evils. Alternative is to add various choices to the front UI and make it look like a fighter jet control panel, or hide it in the advanced options where users are unlikely to look at all. There is always a balance to seek between what should be exposed and how. I don't see a problem with careful use of popups.

Anyway, I think this choice should be configurable via Segments/Segmentations API, so that module developer does not have to resort to manually hiding UI elements that are not relevant in a given application, or override hard-coded behavior by intercepting events.

We could add a "default" checkbox

Should we come up with mockups of the UI to make sure we understand each other well?

I also think this is not a critical issue, it might be more efficient to customize this behavior in Reporting, and refine after RSNA. We don't even have a working beta for the end product, so I would not distract the development in this direction.

I think a more urgent issue where @cpinter help would be most welcomed is the update of the DICOMSegmentationPlugin to account for the new Terminology infrastructure (issue #85).

lassoan commented 8 years ago

OK, get back to this later when more urgent issues are addressed.

cpinter commented 7 years ago

Change committed in https://github.com/Slicer/Slicer/commit/00f4d75d9695a97d5926fe073c48077449130829

fedorov commented 7 years ago

This issue was resolved