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

Bokeh colorbars #861

Merged
merged 21 commits into from Sep 14, 2016
Merged

Bokeh colorbars #861

merged 21 commits into from Sep 14, 2016

Conversation

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Sep 13, 2016

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:

  • Test each changed Element in more detail
  • Implement color_mapper range updating
  • Update HeatMapPlot in #849 once this has been merged.
  • Add some unit tests to ensure colorbars get instantiated correctly

Examples:

Different colorbar_positions:

screen shot 2016-09-14 at 12 54 54 am

Log colormapping on points:

screen shot 2016-09-14 at 12 45 16 am

QuadMesh:

screen shot 2016-09-14 at 12 45 48 am

Polygons (more sensible on a Choropleth):

screen shot 2016-09-14 at 12 43 54 am

@jbednar
Copy link
Member

@jbednar jbednar commented Sep 14, 2016

Looks great!

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Sep 14, 2016

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).

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Sep 14, 2016

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')

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

Great! Glad there is an option for this...

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Sep 14, 2016

Ready for review.

@jbednar
Copy link
Member

@jbednar jbednar commented Sep 14, 2016

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!

@jbednar
Copy link
Member

@jbednar jbednar commented Sep 14, 2016

Doesn't moving the toolbar to the top conflict with the title now? I thought that's why the tools moved to the right.

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Sep 14, 2016

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)

I've added black outlines by default in holoviews.

and to plead to have the plot not change shape when a colorbar is added?

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.

Doesn't moving the toolbar to the top conflict with the title now? I thought that's why the tools moved to the right.

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'))

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

Not sure I am happy about hard coding 'viridis' as a default, especially as it is a matplotlib colormap (unless bokeh now has viridis?).

This comment has been minimized.

@jbednar

jbednar Sep 14, 2016
Member

Bokeh does now have viridis.

This comment has been minimized.

@philippjfr

philippjfr Sep 14, 2016
Author Member

Matplotlib has hardcoded defaults, which is viridis now and bokeh does have viridis now as well.

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

Great!

This comment has been minimized.

@jbednar

jbednar Sep 14, 2016
Member

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).

This comment has been minimized.

@jbednar

jbednar Sep 14, 2016
Member

Plus that would be a (very slight) change to the current defaults. Probably few people besides me could tell the difference, though.

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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.

This comment has been minimized.

@jbednar

jbednar Sep 14, 2016
Member

Mpl's hot definitely has regions where it plateaus and conveys little information about changes in value:

image

It looks good to me up until it turns yellow, then it's got a huge yellow stretch with little change as intensity varies. It wouldn't be hard to do a better job, and I'd be happy to do that.

This comment has been minimized.

@jbednar

jbednar Sep 14, 2016
Member

Looks like mpl's hot came from matlab originally? http://ab-initio.mit.edu/wiki/index.php/Color_tables_in_h5topng

This comment has been minimized.

@jbednar

jbednar Sep 14, 2016
Member

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.""")

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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...

This comment has been minimized.

@philippjfr

philippjfr Sep 14, 2016
Author Member

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.

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

I think being able to disable the toolbar without issuing warnings could be convenient so I am happy to leave it as it is.

@jbednar
Copy link
Member

@jbednar jbednar commented Sep 14, 2016

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}

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

When could you request a colorbar but not have a color_mapper available?

This comment has been minimized.

@philippjfr

philippjfr Sep 14, 2016
Author Member

They might have enabled colorbar by default but not set a color_index.

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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...

This comment has been minimized.

@philippjfr

philippjfr Sep 14, 2016
Author Member

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'})

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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')

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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...

This comment has been minimized.

@philippjfr

philippjfr Sep 14, 2016
Author Member

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.

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

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...

This comment has been minimized.

@philippjfr

philippjfr Sep 14, 2016
Author Member

The image glyph accepts a colormapper directly while in other cases it is used as a transform that maps color to a particular column.

This comment has been minimized.

@jlstevens

jlstevens Sep 14, 2016
Contributor

Thanks for the explanation!

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Sep 14, 2016

Tests are passing and I understand this PR is complete. Merging.

@jlstevens jlstevens merged commit 96be6a7 into master Sep 14, 2016
4 checks passed
4 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 71.98%
Details
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@jbednar jbednar deleted the bokeh_colorbars branch Sep 15, 2016
@jbednar
Copy link
Member

@jbednar jbednar commented Sep 19, 2016

The colorbars above are squishing the plots when they are added, which is an issue I just filed for bokeh: bokeh/bokeh#5186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants