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

Added support for categorical colormapping in bokeh backend #1137

Merged
merged 4 commits into from
Feb 23, 2017

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Feb 15, 2017

Adds support for categorical colormapping on bokeh ColorbarPlot and implements it specifically for Points. We already have this support in matplotlib and bokeh now makes it easy with a CategoricalColorMapper. This is also already achievable with an NdOverlay of Points but this allows selections to work on a single datastructure which is a lot easier to deal with for streams.

%%opts Scatter [color_index=2 toolbar='above' legend_position='right' width=600] (cmap='Set1' size=10)
hv.Scatter([('Week %d' % (i%10), np.random.rand(), chr(65+np.random.randint(5)))
           for i in range(100)], kdims=['Date'], vdims=['r2', 'Group'])

screen shot 2017-02-15 at 12 33 09 am

@philippjfr
Copy link
Member Author

Ready to merge when tests pass.

opts = dict(factors=factors)
if 'NaN' in colors:
opts['nan_color'] = colors['NaN']

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if a chunk of this should be a utility, e.g these lines. Both self.clipping_colors and self.logz seem to be sensible argument for a general color mapping utility.

I'll leave it up to you but to me it seems like a fair chunk of code that doesn't have to be tied up in a plotting class.

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, some of that can definitely be made into a utility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't really worth it factoring much of this out.

legend_specs = {'right': dict(pos='right', loc=(5, -40)),
'left': dict(pos='left', loc=(0, -40)),
'top': dict(pos='above', loc=(120, 5)),
'bottom': dict(pos='below', loc=(60, 0))}

def _process_legend(self, plot=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 see this was moved from PointPlot and I expect it is more general here. That said, does this have anything to do with colormapping or is it just an unrelated improvement?

Copy link
Member Author

@philippjfr philippjfr Feb 20, 2017

Choose a reason for hiding this comment

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

No, it was moved from BarPlot. Basically it handles providing control over automatically created legends, which is a general concept. Since categorical colormappers can automatically create a legend it's useful to move this method up to LegendPlot and let PointPlot make use of it.

@jlstevens
Copy link
Contributor

Once my two comments are addressed, I'm happy to merge.

@philippjfr
Copy link
Member Author

Factored out a tiny little function but the rest of it is just very closely tied to the signatures of the bokeh colormappers and won't benefit much from being separate utilities. This should be ready to merge now.

@philippjfr
Copy link
Member Author

Ended up factoring it out into a separate method. Ready to merge now.

@jlstevens
Copy link
Contributor

A separate method also helps break up the block of code.

Looks good, merging.

@jlstevens jlstevens merged commit ed01c3c into master Feb 23, 2017
@philippjfr philippjfr deleted the categorical_cmapper branch February 27, 2017 23:29
@philippjfr philippjfr added this to the v1.7.0 milestone Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants