-
Notifications
You must be signed in to change notification settings - Fork 75
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 "Random" colormap for visualizing image segmentation maps #2671
Conversation
330a3b4
to
17b65da
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2671 +/- ##
==========================================
- Coverage 90.78% 88.07% -2.72%
==========================================
Files 164 108 -56
Lines 21572 15883 -5689
==========================================
- Hits 19585 13989 -5596
+ Misses 1987 1894 -93 ☔ View full report in Codecov by Sentry. |
@observe('image_colormap_value') | ||
def _random_cmap_limit_update(self, *args, **kwargs): | ||
# if 'Random' colormap is used for visualizing image segmentation, | ||
# ensure the stretch limits are the min and max, so all label colors | ||
# are unique: | ||
if self.image_colormap_value != 'Random': | ||
return | ||
|
||
self.stretch_preset.value = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer this to just toggle an in-plugin warning. I can even see how it would be useful to set non-min/max in order to "exclude" outliers, but I can see how that at least needs some sort of explanatory message and perhaps link to a section in the docs explaining this implementation and the nuances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kecnry For a segmentation image, the default here needs to be min/max. Nothing else really makes sense. Having a non-min/max would clip a lower and upper range of integer labels (which are generally at the bottom and top of the segmentation image) and thus different sources would get assigned the same colors. There is no "outlier values" in a segmentation image. Each source gets assigned a unique integer value.
As an example of why non-min/max is very bad, the background (no detected sources) is assigned a label value of zero. If you were to assign say all values < 10 to the same color, then you couldn't even distinguish between background and sources, defeating the whole point.
This must use min/max. An appropriate message can be shown stating the stretch levels have been changed to min/max.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the stretch function should always be linear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then can we detect that the image is a segmentation image and only show this as an option in that case? I'm just worried about the more common case (at least so far from current use-cases for Imviz) where an image is not a segmentation image, the user selects "random" not knowing the implications, and things are changed automatically and perhaps they loose their carefully set stretch limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segmentation images contain only integer data, but that's not a unique identifier. Not sure of any way to automatically detect a segmentation image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segmentation images often contain consecutive integer values (0 to max label), but that's definitely not a requirement either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this would be better as a separate plugin (or button within plot options that explicitly states all the things that will be set - similar to the RGB defaults button) then? Otherwise, I think we're adding confusion for the default case in order to support a case that isn't relevant for most situations.
Alternatively, if/when colormap is set to "random" and the percentiles are not min/max and/or the stretch function is not linear, there can be an alert directly in line with buttons to press to set both. That would take up to three clicks to achieve the same thing, but would take less UI space than a dedicated button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate plugin
One plugin which controls plot options is Plot Options.
Alternatively, if/when colormap is set to "random" and the percentiles are not min/max and/or the stretch function is not linear, there can be an alert directly in line with buttons to press to set both. That would take up to three clicks to achieve the same thing, but would take less UI space than a dedicated button.
If you're intentionally using this button for its intended purpose, this is adding clicks that you will always have to go through.
Would your concerns be assuaged if we named the cmap "Image Segmentation"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One plugin which controls plot options is Plot Options.
I know, and I agree it makes most sense here, but then it needs to act in a way that makes sense for any input data in which the choice is available. If you're someone who isn't even familiar with segmentation maps and load an image and stumble on this, it will be very jarring and confusing.
If you're intentionally using this button for its intended purpose, this is adding clicks that you will always have to go through.
Yes, that is the downside, but I think it makes sense to add the extra clicks to the specific use-case rather than the general use-case. That said, the default stretch function is already linear, so by default this will just be two clicks, not three. If the parser could detect segmentation maps, then we could even have the percentile default for those layers to min/max from load, and then this only requires extra clicks if the user overrides anything.
Another benefit to this design is if the user changes to "Image Segmentation" (or "Random") and then changes the stretch inputs, the warning will re-appear. The current implementation has no way to catch that case unless it disables those inputs entirely.
Would your concerns be assuaged if we named the cmap "Image Segmentation"?
It is slightly more specific than "Random", but I still don't think it provides the necessary context and explanation for someone who does not have this use-case in their mind at all when looking through the list of options for colormap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See bmorris3#30 for a proposed compromise solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @bmorris3! Should we use a "linear" stretch as default too?
tl;dr -- How much of this becomes irrelevant (i.e., we have to remove/refactor) if we end up with a separate "DQ" plugin? |
DQ and segmentation arrays aren't the same thing (although both do contain integer-only data). |
@pllim We resolved earlier in the series of tickets that image segmentation is not DQ, and we shouldn't conflate them. |
"Random" in the dropdown sounds too cryptic to me. What about "Segmentation"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc here needs to be expanded as well: https://github.com/spacetelescope/jdaviz/blob/main/docs/imviz/displayimages.rst#colormap
17b65da
to
ec2b439
Compare
I still prefer the opposite approach for reasons explained above, but could let it go if there are revert buttons. Is this warning exposed from the API as well? |
fe0d8f5
to
ae83941
Compare
I've just pushed some changes based on feedback from a discussion two weeks ago. They are:
imviz-image-segmentation.mov |
Changing the contrast and bias can also mess up the uniqueness of the color map - do we want to either disable those in Random colormap mode, or handle them the same way as stretch preset and function? Screen.Recording.2024-02-22.at.11.35.27.AM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well now too, thanks.
Feature requested by @larrybradley.
This PR adds a "random" colormap in Plot Options, which is useful for visualizing a segmented image from photutils. The random colormap is based on, and almost identical to, the built-in cmap generator in photutils' segmentation map class.
By default, the colors in the colormap are unique for 10,000 source labels before repeating. Background pixels without sources (
label == 0
) are black.The default stretch preset would render the highest and lowest labels in the segmentation map with the same color, so I observe the image colormap and force the stretch preset to min/max when colormap == 'Random'.
This PR could be updated after glue-viz/glue#2468 to allow for mixed alpha segmentation labels and backgrounds.
Demo
First generate a synthetic image and a segmentation map:
Then visualize with Imviz:
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.