-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Bokeh colorbars #861
Bokeh colorbars #861
Conversation
Looks great! |
I agree it looks great! The only thing that is bothering me slightly is that I would expect a black frame around the colorbars, especially if you consider colormaps ending in white (like the last two examples shown above). |
I agree, it's a design choice the bokeh team decided on but I think enabling a black border makes sense. |
cbar_opts = self.colorbar_specs[self.colorbar_position] | ||
cbar_opts = dict(self.colorbar_specs[self.colorbar_position], | ||
bar_line_color='black', label_standoff=8, | ||
major_tick_line_color='black') |
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.
Great! Glad there is an option for this...
Allows avoiding overlap with legends, colorbars and titles
Ready for review. |
Are there issues on the Bokeh site already where I can go to request that there be a tiny, tiny, black outline (since colorbars starting or ending in white really are not uncommon, e.g. grayscale), and to plead to have the plot not change shape when a colorbar is added? Both of those are quite important for usability! |
Doesn't moving the toolbar to the top conflict with the title now? I thought that's why the tools moved to the right. |
I've added black outlines by default in holoviews.
No way to do this although I could approximate the percentage of the width of the colorbar and offset, not sure I want to go down that route though.
Yes, sorry should have updated my description, the toolbar location is now configurable with the toolbar plot option which accepts ['above', 'below', 'left', 'right', None]. Should I change above and below to top and bottom for consistency? |
|
||
def _get_colormapper(self, dim, element, ranges, style): | ||
low, high = ranges.get(dim.name) | ||
palette = mplcmap_to_palette(style.pop('cmap', 'viridis')) |
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.
Not sure I am happy about hard coding 'viridis' as a default, especially as it is a matplotlib colormap (unless bokeh now has viridis?).
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.
Bokeh does now have viridis.
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.
Matplotlib has hardcoded defaults, which is viridis now and bokeh does have viridis now as well.
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.
Great!
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.
Personally, I like that viridis is perceptually uniform, but I don't actually like it overall, because it has no intuitive ordering. Hot is clearly ordered in a way that people can appreciate at first glance, as are cool colormaps (black->blue->white), but viridis just has to be memorized. In that sense it's as bad as jet (but only that sense).
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.
Plus that would be a (very slight) change to the current defaults. Probably few people besides me could tell the difference, though.
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 we would have to come up with a name for it! :-)
Definitely good suggestions though and something we should consider doing for 1.7.
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.
Looks like mpl's hot came from matlab originally? http://ab-initio.mit.edu/wiki/index.php/Color_tables_in_h5topng
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 we would have to come up with a name for it!
Mpl has hot, afmhot, and gist_heat, so ours could be hhot. :-)
"left", "right", None], | ||
doc=""" | ||
The toolbar location, must be one of 'above', 'below', | ||
'left', 'right', None.""") |
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 assume None
can be a sensible choice even if the user has added tools (e.g to temporarily hide the toolbar). Otherwise, adding tools with toolbar=None could issue a warning...
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.
It's just a convenient way to disable the toolbar altogether. I could either disable it, or warn tools
or warn for tools
and default_tools
.
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 being able to disable the toolbar without issuing warnings could be convenient so I am happy to leave it as it is.
above and below could be top and bottom; sure. |
mapper = self._get_colormapper(cdim, element, ranges, style) | ||
data[cdim.name] = [] if empty else element.dimension_values(cdim) | ||
mapping['color'] = {'field': cdim.name, | ||
'transform': mapper} | ||
|
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.
Looks much cleaner!
@@ -128,6 +119,8 @@ def _init_glyph(self, plot, mapping, properties): | |||
else: | |||
plot_method = self._plot_methods.get('batched' if self.batched else 'single') | |||
renderer = getattr(plot, plot_method)(**dict(properties, **mapping)) | |||
if self.colorbar and 'color_mapper' in self.handles: | |||
self._draw_colorbar(plot, self.handles['color_mapper']) | |||
return renderer, renderer.glyph |
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.
When could you request a colorbar but not have a color_mapper available?
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.
They might have enabled colorbar by default but not set a color_index
.
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, makes sense, thanks.
cmapper = colormapper(palette, low=low, high=high) | ||
if 'color_mapper' not in self.handles: | ||
self.handles['color_mapper'] = cmapper | ||
return cmapper |
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 can see the logic of returning cmapper
but this function could also just be called for the side-effect of adding 'color_mapper' to the handles. I had to check the code here to make sure it is indeed the same thing...
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.
Yes, there's some annoyances I have with this, ideally the handles would be set elsewhere but since it is now called from get_data, which is the main method the user has to implement and is duplicated everywhere I didn't want to move it out. The other thing is only the first colormapper that's created is actually used while the rest are simply used to update the existing colormapper. I'll leave it up to you if you think I should find a cleaner solution otherwise I'll make sure to leave a comment.
'widths': ws.flat, 'heights': hs.flat} | ||
|
||
return (data, {'x': x, 'y': y, 'fill_color': 'color', | ||
return (data, {'x': x, 'y': y, | ||
'fill_color': {'field': z, 'transform': cmapper}, | ||
'height': 'heights', 'width': 'widths'}) |
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.
Here you call it cmapper
but in other places you sometimes call it mapper
. I would prefer to use the former consistently....
return | ||
|
||
opts = dict(cbar_opts['opts'], bar_line_color='black', | ||
label_standoff=8, major_tick_line_color='black') |
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 wouldn't necessarily make these parameters but maybe they could be specified as a dictionary in the class attributes. Alternatively, you could just make them into the default for colorbar_opts
which might make more sense...
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.
When you add them to the colorbar_opts
they all get overridden when the user supplies new options and bar_line_color='black'
just looks silly without major_tick_line_color='black'
, so I wanted them to behave like defaults. Happy to make them into a class attribute though.
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.
A class attribute would be good in that case.
self.handles['color_mapper'] = cmap | ||
val_dim = [d for d in element.vdims][0] | ||
properties['color_mapper'] = self._get_colormapper(val_dim, element, ranges, | ||
properties) | ||
return properties |
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'm just wondering why in this case self._get_colormapper
is used to add the color mapper to the properties (in _glyph_properties
) but is added to the data in get_data
everywhere else...
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 image glyph accepts a colormapper directly while in other cases it is used as a transform that maps color to a particular column.
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 for the explanation!
Tests are passing and I understand this PR is complete. Merging. |
The colorbars above are squishing the plots when they are added, which is an issue I just filed for bokeh: bokeh/bokeh#5186 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Since version 0.12.2 bokeh finally support colorbars and client-side colormapping for all kinds of glyphs. This PR adds a ColorbarPlot mixin class which handles the creation of a Colorbar and a ColorMapper. Additionally it replaces the manual colormapping that was previously done in Python for various plot types. I have also moved the toolbar to the top so it does not overlap with a colorbar by default and because it is more consistent.
To Do:
Examples:
Different
colorbar_position
s:Log colormapping on points:
QuadMesh:
Polygons (more sensible on a Choropleth):