-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fixed automatic color generation in label2rgb function #4531
base: main
Are you sure you want to change the base?
Fixed automatic color generation in label2rgb function #4531
Conversation
Hello @mohitpokharna! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-03-27 10:23:00 UTC |
skimage/color/colorlabel.py
Outdated
colors = DEFAULT_COLORS | ||
colors = [_rgb_vector(c) for c in colors] | ||
else: | ||
colors = np.random.random((nlabel, 3)) |
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 see some potential issues with this approach:
- It is not reproducible
- The contrast between the colors is not guaranteed
- The method is prone to color collisions.
I would rather suggest to consider adopting (we don't have it as a hrad dependency anymore) one of the qualitative matplotlib colormaps (https://matplotlib.org/3.2.0/tutorials/colors/colormaps.html) and sample the colors from it.
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.
(Just to say it's not @mohitpokharna fault, @jni suggested it :) #4507 (comment) )
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.
@soupault Thank you for pointing out the potential issues. I have added a fix. Please review.
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.
@mohitpokharna sorry for not being 100% clear. By adopting I meant literally taking a piece of relevant code for the colormap generation from the matplotlib codebase. As scikit-image we have made a decision to switch matplotlib from a requirement to an optional dependency (#2710). Thus, we should avoid importing it in commonly used functions (such as label2rgb). If you have an alternative idea, please, let us know. 👍
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.
@soupault How about indicating colors = [np.asarray(cm.tab20c(x))[:-1] for x in range(n_colors)]
in the documentation? The user could make its own color series if needed...
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.
Sorry about all the confusion, everyone! 😅 Actually the np.random.random
approach was suggested as an alternative to the user's code, but not something that we would want to do internally. Instead, internally, I think we should use Glasbey + shuffle (+ sampling above some set number of colours, eg 255). But Glasbey is a bit complicated to implement. It might be easier to start from the paper than the code.
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.
@mohitpokharna at any rate, thank you very much for attempting this fix!
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.
@jni Thank you for pointing out. It makes much more sense now. I will try to see if I can carry it on further.
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.
@sciunto not sure if I like that 🙂 . We should rather aim at hiding the complexity.
@@ -1,6 +1,7 @@ | |||
import itertools | |||
|
|||
import numpy as np | |||
from matplotlib import cm |
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 would be happy to not introduce new dependency to matplotlib
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.
Using a qualitative matplotlib colormap is a good idea and would a great addition for the label2rgb
function. But instead of introducing yet an other direct dependency to matplotlib, it would be better to vendor the colormap values (cm.lab20c().colors
).
Moreover, I think that interpolating over the colormap values will be necessary to guaranty that all labels' colors are unique.
""" | ||
# tab20c qualitative colormap to generate the colors | ||
# Transparency is the last item in the tuple object which is removed | ||
colors = [np.asarray(cm.tab20c(x))[:-1] for x in range(n_colors)] |
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.
cm.tab20c
accepts iterables as input, this means that you can directly use
colors = cm.tab20c(range(n_colors))[:, :-1]
but cm.tab20c
is a 20 colors LUT, and colors
obtained as you made are not correct:
>>> from pprint import pprint
>>> import numpy as np
>>> from matplotlib import cm
>>> pprint(cm.tab20c(range(25))[:, :-1])
array([[0.19215686, 0.50980392, 0.74117647],
[0.41960784, 0.68235294, 0.83921569],
[0.61960784, 0.79215686, 0.88235294],
[0.77647059, 0.85882353, 0.9372549 ],
[0.90196078, 0.33333333, 0.05098039],
[0.99215686, 0.55294118, 0.23529412],
[0.99215686, 0.68235294, 0.41960784],
[0.99215686, 0.81568627, 0.63529412],
[0.19215686, 0.63921569, 0.32941176],
[0.45490196, 0.76862745, 0.4627451 ],
[0.63137255, 0.85098039, 0.60784314],
[0.78039216, 0.91372549, 0.75294118],
[0.45882353, 0.41960784, 0.69411765],
[0.61960784, 0.60392157, 0.78431373],
[0.7372549 , 0.74117647, 0.8627451 ],
[0.85490196, 0.85490196, 0.92156863],
[0.38823529, 0.38823529, 0.38823529],
[0.58823529, 0.58823529, 0.58823529],
[0.74117647, 0.74117647, 0.74117647],
[0.85098039, 0.85098039, 0.85098039],
[0.85098039, 0.85098039, 0.85098039],
[0.85098039, 0.85098039, 0.85098039],
[0.85098039, 0.85098039, 0.85098039],
[0.85098039, 0.85098039, 0.85098039],
[0.85098039, 0.85098039, 0.85098039]])
as you see, all the colors with index > 20 are equal.
You need to provide values between 0 and 1:
>>> pprint(cm.tab20c(np.linspace(0, 1, 25))[:, :-1])
array([[0.19215686, 0.50980392, 0.74117647],
[0.19215686, 0.50980392, 0.74117647],
[0.41960784, 0.68235294, 0.83921569],
[0.61960784, 0.79215686, 0.88235294],
[0.77647059, 0.85882353, 0.9372549 ],
[0.90196078, 0.33333333, 0.05098039],
[0.99215686, 0.55294118, 0.23529412],
[0.99215686, 0.55294118, 0.23529412],
[0.99215686, 0.68235294, 0.41960784],
[0.99215686, 0.81568627, 0.63529412],
[0.19215686, 0.63921569, 0.32941176],
[0.45490196, 0.76862745, 0.4627451 ],
[0.63137255, 0.85098039, 0.60784314],
[0.63137255, 0.85098039, 0.60784314],
[0.78039216, 0.91372549, 0.75294118],
[0.45882353, 0.41960784, 0.69411765],
[0.61960784, 0.60392157, 0.78431373],
[0.7372549 , 0.74117647, 0.8627451 ],
[0.85490196, 0.85490196, 0.92156863],
[0.85490196, 0.85490196, 0.92156863],
[0.38823529, 0.38823529, 0.38823529],
[0.58823529, 0.58823529, 0.58823529],
[0.74117647, 0.74117647, 0.74117647],
[0.85098039, 0.85098039, 0.85098039],
[0.85098039, 0.85098039, 0.85098039]])
but again, as you see, some colors are equal in this case again. I think that this is not the correct approach...
rgb = label2rgb(label, image=image) | ||
|
||
# number of unique colors in the rgb image | ||
n_colors = len(np.unique(rgb[0], return_counts=True, axis=0)[1]) |
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.
n_colors = len(np.unique(rgb[0], return_counts=True, axis=0)[1]) | |
n_colors = np.unique(rgb, axis=1).shape[1] |
n_colors = len(np.unique(rgb[0], return_counts=True, axis=0)[1]) | ||
|
||
# test if unique colors generated equal the expected number of colors | ||
assert_array_equal(n_colors, [exp_colors]) |
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.
assert_array_equal(n_colors, [exp_colors]) | |
assert n_colors == exp_colors |
But @jni suggestion #4531 (comment) is even better ;-) |
So, how do we proceed here? My feeling is that there are possible long-term improvements consisting in
In the short term, there is the low-hanging fruit of increasing the number of distinct colors between which we loop. We should take am existing discrete/qualitative colormap and vendor it so that we do not increase dependencies. Several possibilities exist with matplotlib's colormaps or plotly's colormaps (I think the Alphabet one is quite nice with its 25 colors). What do you think? |
This relatively new Colorcet package has some nice Glasbey categorical colormaps. It looks like it is fairly lightweight (but does itself have two pure python dependencies: pyct and param) |
Description
Fixed #4507 to now automatically generate colors if number of unique labels exceed the number of default colors. Also added a test function for the same.
Checklist
Gallery example in./doc/examples
(new features only)Benchmark in./benchmarks
, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.@meeseeksdev backport to v0.14.x