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 Interactive legend to ppc_plot bokeh #1602

Merged
merged 4 commits into from
Mar 10, 2021

Conversation

Rishabh261998
Copy link
Contributor

@Rishabh261998 Rishabh261998 commented Mar 5, 2021

Description

To fix #1563
I have added an interactive legend to ppc_plot in bokeh.
Is the position of the legend is good enough?
Screenshot from 2021-03-06 01-10-25

Screenshot from 2021-03-06 01-10-35

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@Rishabh261998
Copy link
Contributor Author

@OriolAbril could you look into this? Thanks

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #1602 (a77709b) into main (c4005b1) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1602      +/-   ##
==========================================
+ Coverage   90.86%   90.88%   +0.02%     
==========================================
  Files         108      108              
  Lines       11620    11652      +32     
==========================================
+ Hits        10558    10590      +32     
  Misses       1062     1062              
Impacted Files Coverage Δ
arviz/plots/backends/bokeh/ppcplot.py 98.11% <100.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4005b1...f19b925. Read the comment docs.

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.

Thanks!

if legend:
legend = Legend(
items=legend_it,
location="center_right",
Copy link
Member

Choose a reason for hiding this comment

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

given that the legend is fixed, I would place it on the top left, which will generally be en empty corner for both kde and cdf plots. https://arviz-devs.github.io/arviz/examples/plot_ppc_cumulative.html https://arviz-devs.github.io/arviz/examples/plot_ppc.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!
Screenshot from 2021-03-06 15-18-12

@Rishabh261998
Copy link
Contributor Author

@OriolAbril could you please review this? Thanks

@canyon289
Copy link
Member

@Rishabh261998 Thank you for this contribution.

Can a small test be added or amended to cover this line?
https://app.codecov.io/gh/arviz-devs/arviz/compare/1602/diff#D1L313
image

@Rishabh261998
Copy link
Contributor Author

Rishabh261998 commented Mar 7, 2021

@canyon289 for line 313?
Also I'm new to pytest so could you provide me some existing tests which I can refer to? Thanks
Update: sth like this?

def test_plot_legend(models):

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.

Regarding tests, I would make a copy of https://github.com/arviz-devs/arviz/blob/main/arviz/tests/base_tests/test_plots_bokeh.py#L880 without the parametrizations, name it test_plot_ppc_textsize and call plot_ppc using textsize

)
ax_i.add_layout(legend)
if textsize is not None:
ax_i.legend.label_text_font_size = str(textsize) + "pt"
Copy link
Member

Choose a reason for hiding this comment

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

I would use an f-string here instead of adding strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OriolAbril Done!

@OriolAbril OriolAbril merged commit 5c0581f into arviz-devs:main Mar 10, 2021
utkarsh-maheshwari pushed a commit to utkarsh-maheshwari/arviz that referenced this pull request May 27, 2021
* Added interactive legend to ppc_plot bokeh

* Minor Change

* Updated CHANGELOG and positioned legend

* Added test for textsize in ppc_plot bokeh
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.

Show legend in ppc plot (backend bokeh)
3 participants