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 Make HeatMap more general #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 commented Sep 14, 2016

Looks great!

@jlstevens
Copy link
Contributor

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

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')
Copy link
Contributor

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

@philippjfr
Copy link
Member Author

Ready for review.

@jbednar
Copy link
Member

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

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'))
Copy link
Contributor

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

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Copy link
Member

@jbednar jbednar Sep 14, 2016

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

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

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

Copy link
Member

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.""")
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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.

@jbednar
Copy link
Member

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}

Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member Author

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'})
Copy link
Contributor

@jlstevens jlstevens Sep 14, 2016

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')
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

@jlstevens
Copy link
Contributor

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

@jlstevens jlstevens merged commit 96be6a7 into master Sep 14, 2016
@jbednar jbednar deleted the bokeh_colorbars branch September 15, 2016 00:46
@jbednar
Copy link
Member

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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants