-
-
Notifications
You must be signed in to change notification settings - Fork 393
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 Interactive Legend to forestplot Bokeh #1591
Conversation
@OriolAbril Can you look into this. I was also facing problem in rebasing so I used the git merge command instead of rebasing. |
Codecov Report
@@ Coverage Diff @@
## main #1591 +/- ##
==========================================
+ Coverage 90.05% 90.07% +0.02%
==========================================
Files 108 108
Lines 11584 11608 +24
==========================================
+ Hits 10432 10456 +24
Misses 1152 1152
Continue to review full report at Codecov.
|
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 had very little time to review, will continue afterwards.
Added the Legend to forest plot in with backend Bokeh. I have also made it interactive but wasn't able to control the different axes with a single legend so had to provide a legend for each axes. See the below image
This is amazing, don't worry about the one legend per axes issue. This is perfectly fine.
What is strange is that the ess/rhat dots should be aligned with the forestplot hdi lines, otherwise they are not very useful. Does this happen always or only with the legend?
One more problem I am facing is that I am unable to hide the y_ticks corresponding to the model when the model plot is hidden. It seems that Bokeh supports interactivity with only glyphs.
Again, this is no problem at all, I recommend users use the NoModel labeller (or mixtures of it) which should be the default already with legend=True
. With everything working well, there is only a label per variable, see the last example in the label guide for instance: https://arviz-devs.github.io/arviz/user_guide/label_guide.html#labeller-mixtures. What should not be happening are the 0 and 12 as ticklabels, I will try to take a look afterwards, but wanted to point it out already.
I was also facing problem in rebasing so I used the git merge command instead of rebasing.
This is to be expected. You are using the same branch you used for the rope PR, which has the changes in the rope PR, but we merged it to master already so you have the same changes in different commits, which makes rebasing tricky.
We recommend keeping the main branch up to date with the main branch in arviz, and never work on it. And every time you need to make a PR, create a new branch for that feature. After merging, we delete the branch and prune the repo from time to time.
legend_it_ess = [] | ||
for (model_name, glyphs) in plotted_ess.items(): | ||
legend_it_ess.append((model_name, glyphs)) | ||
legend_ess = Legend(items=legend_it_ess) | ||
axes[0, idx].add_layout(legend_ess, "right") | ||
axes[0, idx].legend.click_policy = "hide" |
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 think all this should only happen if legend=True
. Same below
yield y, row_label, label, selection_list[idx], values, self.model_color[ | ||
model_name | ||
] | ||
yield y, row_label, model_name, label, selection_list[ |
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.
This is for the code above, not for here, but you need to update the label generation to use the labeller: https://github.com/arviz-devs/arviz/blob/main/arviz/plots/backends/matplotlib/forestplot.py#L555
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.
By this, you just mean that I need to generate the row labels as done In matplotib?
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.
Both row labels and model labels used in legend should be processed by the labeller, you'll see that in matplotlib there are two labeller methods called
@OriolAbril Also the ess/rhat dots are not aligned in the current bokeh forestplot without legend, whereas the matplotlib version is perfect. Also the 0 and 12 problem also appears in the bokeh forestplot without legend. |
And also should I create a new branch or is it fine to work on this branch just for this PR? i.e is there any problem if I use git merge again if I need to sync. Thanks |
Do you want to work on these here or open an issue for you or someone else to work on there at some point later in time?
For now it is fine I think, if you ran into problems or the number of commits increased I's probably suggest a rebase, but let's go to than if we get there. |
I think for now we should create a new issue and when I have finished with the legend thing in |
@OriolAbril in the below plot why are there only orange |
No idea. I am not sure ess and rhat are very useful for multiple models case, but it should not happen, and if it does then we should document that ess/rhat do not work with multiple models. |
@OriolAbril fixed the label generation and also fixed the |
Oh, I see, good catch. The issue is that the code must be checking a dict or set to see if a dot has been plotted, and with the new labeller capabilities, labels don't need to be unique. I should have updated the code to stop using labels as ids but completely missed that. A new PR to fix this would be great. |
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.
Thanks, this is great
@@ -191,8 +202,10 @@ def plot_forest( | |||
] = -all_plotters[ # pylint: disable=protected-access | |||
0 | |||
].group_offset | |||
axes[0, 0].y_range._property_values["end"] = y_max # pylint: disable=protected-access | |||
axes[0, 0].y_range._property_values["end"] = y_max # pylint: disable=protected- |
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.
undo this change to have pyling keep ignoring this warning
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.
Did this, still checks are failing:-C
Update: Got the error, docstring missing in a public method
* Included estimate of probability within ROPE * Positioned ROPE text and added to CHANGELOG * Changed color scheme in plot_posterior in bokeh * Changed size of rope text in posteriorplot bokeh * Added rope_color and ref_val_color arguments to plot_posterior * Minor Changes * Some minor changes * Added Interactive Legend to forest plot * Formatted Code * Updated label generation and fixed some errors * Updated CHANGELOG * Minor change
Description
To fix #1572
Added the Legend to forest plot in with backend Bokeh. I have also made it interactive but wasn't able to control the different axes with a single legend so had to provide a legend for each axes. See the below image
One more problem I am facing is that I am unable to hide the
y_ticks
corresponding to the model when the model plot is hidden. It seems that Bokeh supports interactivity with only glyphs.