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

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

Closed
fedorov opened this issue Oct 7, 2016 · 19 comments

Comments

Projects
None yet
5 participants
@fedorov
Copy link
Member

commented Oct 7, 2016

No description provided.

@cpinter

This comment has been minimized.

Copy link
Collaborator

commented Oct 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2016

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:

  • "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.) " - 3dSlicerLabel if no modifier, 3dSlicerLabel, <modifier> if modifier is present. I don't understand what you refer to by "languages".
  • "How do we make sure the user does not want the name that we identified as default?" - the same way it is done now - we provide user with the means to change the value initialized by default
  • " it would probably be better to make sure some terminology is selected first, as the users should know what they are segmenting" - yes, SlicerGenericAnatomy terminology will be selected first by default, the same way it is done now with the SlicerGenericAnatomy LUT.
@cpinter

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Member

commented Nov 14, 2016

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
#79 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHsfe5ujOylu-WlgapEpG-eH8u6Ykpfks5q-MiwgaJpZM4KRfeD
.

@fedorov

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

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:

  • pick a new color that has not been used yet from the generic anatomy list of colors
  • first time this happens, show popup message to the user notifying about the color override and ask if it is ok or not
  • include a checkbox in the popup whether user wants to override the default color in the future or not
@naucoin

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 15, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 15, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2016

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2016

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

@cpinter

This comment has been minimized.

Copy link
Collaborator

commented Dec 14, 2016

Change committed in
Slicer/Slicer@00f4d75

@fedorov fedorov closed this Dec 29, 2016

@fedorov

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2016

This issue was resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.