-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Conversation
f233af1
to
7977945
Compare
Ready to merge when tests pass. |
holoviews/plotting/bokeh/element.py
Outdated
opts = dict(factors=factors) | ||
if 'NaN' in colors: | ||
opts['nan_color'] = colors['NaN'] | ||
|
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 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.
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, some of that can definitely be made into a utility.
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.
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): |
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 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?
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.
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.
Once my two comments are addressed, I'm happy to merge. |
61e9a57
to
2f4b51c
Compare
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. |
2f4b51c
to
a8f0e1d
Compare
Ended up factoring it out into a separate method. Ready to merge now. |
A separate method also helps break up the block of code. Looks good, merging. |
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. |
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.