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

Make the legends interactive in bokeh #1024

Merged
merged 20 commits into from
Feb 13, 2020

Conversation

percygautam
Copy link
Contributor

@percygautam percygautam commented Jan 23, 2020

Fixes part of issue #953 . Make the legends interactive in bokeh.

@percygautam
Copy link
Contributor Author

@ahartikainen @OriolAbril I have started working on the issue and have added the interactive legend feature to densityplot.py. Is this the right way? I'll add legends to other plots in the meantime.

@ahartikainen
Copy link
Contributor

It is basically this way. The problem currently is that we probably don't want to have legends on top of the figure, so we would really need a clever way to position the legend outside the plot. Also for multiple figures should share one legend.

Here should be one working example

https://gist.github.com/ahartikainen/506c23b58db06e162da9c188ceab146c

E.g. location probably can take also tuple, but I'm not sure.

@percygautam
Copy link
Contributor Author

@ahartikainen I looked about it and found this open issue Single Legend for GlyphRenderers on Multiple Plots in bokeh. There's a workaround given in the end which we can look for.
There's also a method specified here, which could work for placing legend outside the plot. But the location of where should we put it needs to be discussed.

@percygautam
Copy link
Contributor Author

@ahartikainen , I have tried to adjust the position of legends and this is what I've come up with.
Screenshot 2020-01-28 at 2 46 37 AM

Also, I've tried to optimize the code for better plotting. We just have to fix the position of title, and the legend position looks good to me.

@percygautam
Copy link
Contributor Author

About the other task of multiple figures sharing one legend, I've tried some methods (like passing glyph renders of one plot to another) but it didn't work. Also, in official bokeh, there is an open issue about the same. I'm thinking of trying to use the grid properties for this. Can you please provide any reference for this?
Also, as described in issue #954 , I think we can use subgroups(using dropdown) to select a school and render only one graph at a time. This will also solve the above problem.

@ahartikainen
Copy link
Contributor

Looks great.

I think that one-legend-thing might be hard to do. Yeah I think dropdown should be quite "easy" (yeah, they never are) to implement.

Maybe something like dropdown + multiple selections (right)

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

LGTM

legend_items[axis_map[label]].append((data_label, plotted))

for ax1, lg in legend_items.items():
legend = Legend(items=lg, location="center_right", orientation="horizontal",)
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 this is a good compromise.

I don't think there is any "universal" way to position legend so it looks good

@percygautam
Copy link
Contributor Author

@ahartikainen So, I'll start working on other plots' legends. Also, I noticed the interactive legends od EnergyPlot are not working. I'll start working on this plot along with other plots having legend( I guess one or two more).

@percygautam
Copy link
Contributor Author

Looks great.

I think that one-legend-thing might be hard to do. Yeah I think dropdown should be quite "easy" (yeah, they never are) to implement.

Maybe something like dropdown + multiple selections (right)

I am thinking of working on dropdowns under the subgroup issue in another PR, as we have to implement those subsections to lot of plots, unlike this legend issue.
The dropdown with single selection will be doable but for multiple selections, I'll try to do but I'm not sure.

@ahartikainen
Copy link
Contributor

Sounds good.

No worries if something doesn't work.

@percygautam
Copy link
Contributor Author

@ahartikainen , I have added interactive legends to all other plots (having legends). The discrepancy of energyplot has also been solved and its working fine now.
I have changed the return value of plot_kde, in order to correct energyplot. This change is not affecting any other plot which uses plot_kde, so I don't think it'll cause a problem. We can always add legend the same way in other plots (when required in future) as we did in energyplot.
There is one problem of the title position, which I can't seem to find solution for. We need some way to write the title in same line as legend or is it fine this way?

@percygautam
Copy link
Contributor Author

@ahartikainen I have changed some font sizes. Interactive legends are working fine. This is how they are looking now:
Screenshot 2020-02-01 at 11 39 40 AM
Screenshot 2020-02-01 at 11 40 25 AM
Screenshot 2020-02-01 at 11 41 14 AM

Comment on lines 238 to 239
if legend and label is not None:
return glyphs
Copy link
Member

Choose a reason for hiding this comment

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

This is what is making the documentation build fail. Maybe bokeh plots can return ax, (extra, elements) or something like this? Maybe a return_glyphs argument only for internal use? @ahartikainen

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe add some argument to bokeh function to return this for internal use. So it would default to false and can not be seen in the main function.

btw did you mean

if legend and label is not None

or

if (legend is not None) and (label is not) None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done as @OriolAbril suggested. I just defined return_glyphs as None and returned it along the axes. If the user doesn't specify the legend, it will plot the graph without legend.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

We have to be careful when returning different values from plotting, the examples involving kde will have to be updated too, a priori they will seem to work but thumb generation will fail.

arviz/plots/backends/bokeh/densityplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/bokeh/densityplot.py Show resolved Hide resolved
arviz/plots/backends/bokeh/kdeplot.py Outdated Show resolved Hide resolved
@percygautam
Copy link
Contributor Author

Still getting sphinx error for bokeh_plot_kde. Any workaround?

@OriolAbril
Copy link
Member

The examples assume the return value is ax, now it is not anymore. You have to update them to ax, _. Docs should also be updated in plot/kdeplot.py

@percygautam
Copy link
Contributor Author

percygautam commented Feb 10, 2020

Okay, thanks. I'll do the changes.

We have to be careful when returning different values from plotting, the examples involving kde will have to be updated too, a priori they will seem to work but thumb generation will fail.

Sorry, for not carefully noticing your comment before.

@ahartikainen
Copy link
Contributor

I still think our plotting code should return array of axes for the end user and glyphs should be for internal use.

This should be the case until we figure out something more advanced.

@percygautam
Copy link
Contributor Author

I have provided an internal argument that enables/disables the return value of plot_kde. Is this what you're looking for? Also what should be added to its docstring?

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

Yes, this looks good.

Maybe add docstring that this is internal use (is there a way for user to return the glyphs?) or that it is for advanced use

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I am not sure about return_glyphs being only in bokeh backend undocumented or allowing advanced users to use it too and therefore making it an arg of general plot_kde and adding it to the docstring. We should probably wait for @ahartikainen to weigh in.

I have commented on how I would handle the docstring if this was eventually chosen, otherwise ignore the comments.

ax.x_range.range_padding = ax.y_range.range_padding = 0

if backend_show(show):
bkp.show(ax, toolbar_location="above")

if legend and label is not None and return_glyph:
Copy link
Member

Choose a reason for hiding this comment

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

the if should be only on return_glyph

Copy link
Contributor Author

@percygautam percygautam Feb 12, 2020

Choose a reason for hiding this comment

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

Then we are getting the problem of unused arguments label and legend. Should I pop them out from kwargs in bokeh backend?


Returns
-------
axes : matplotlib axes or bokeh figures
axes : matplotlib axes or bokeh figures and glyphs
Copy link
Member

Choose a reason for hiding this comment

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

(docstring comment)

Returns
-------
axes : matplotlib.Axes or bokeh.plotting.Figure
    Object containing the kde plot
glyphs : list, optional
    Bokeh glyphs present in plot.  Only provided if ``return_glyph`` is True.

Followed numpy docs like np.unique

legend_items[axis_map[label]].append((data_label, plotted))
else:
legend_items[axis_map[label]] = []
legend_items[axis_map[label]].append((data_label, plotted))
Copy link
Member

Choose a reason for hiding this comment

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

Could legend items be a defaultdict?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will make the code neat. Thanks, for the suggestion.

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@
* Add `skipna` argument to `hpd` and `summary` (#1035)
* Added `transform` argument to `plot_trace`, `plot_forest`, `plot_pair`, `plot_posterior`, `plot_rank`, `plot_parallel`, `plot_violin`,`plot_density`, `plot_joint` (#1036)
* Add `marker` functionality to `bokeh_plot_elpd` (#1040)
* Add the functionality for `interactive legends` for bokeh plots of `densityplot`, `energyplot` and `essplot` (#1024)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe link to bokeh docs on interactive pegends instead of formatting it like code?

@OriolAbril
Copy link
Member

LGTM! And I love this feature, I'll probably merge tomorrow. It would be great to include this in next release.

Note: I have edited the description so it won't close the issue. Reason is I think it would be great to test these interactive legends and afterwards try to get them working in plot_trace (definitely in another PR).

@percygautam
Copy link
Contributor Author

Exactly, my thoughts. I was going to ask you about the legend implementation in plot_trace. These interactive legends will make the visualization clean. (And I have added the description earlier such that issue won't close. I thought I'd leave it up for future, if new plots were added.)

Copy link
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

LGTM

@ahartikainen ahartikainen changed the title [WIP] Make the legends interactive in bokeh Make the legends interactive in bokeh Feb 13, 2020
@ahartikainen ahartikainen merged commit 0001654 into arviz-devs:master Feb 13, 2020
@percygautam percygautam deleted the bokeh_legend branch February 17, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants