Skip to content

Add consistent legend support across backends#115

Merged
aloctavodia merged 3 commits intoarviz-devs:mainfrom
Advaitgaur004:support-legend
May 17, 2025
Merged

Add consistent legend support across backends#115
aloctavodia merged 3 commits intoarviz-devs:mainfrom
Advaitgaur004:support-legend

Conversation

@Advaitgaur004
Copy link
Copy Markdown
Contributor

@Advaitgaur004 Advaitgaur004 commented Jan 22, 2025

Description

This PR implements consistent legend support across all plotting backends (matplotlib, bokeh, and plotly) and adds automatic legend generation for plot_psense_dist.

Key Changes

  • Added full legend support for Plotly backend
  • Enhanced Bokeh legend with automatic placement logic
  • Added automatic legend generation for:
    • plot_psense_dist (for alpha parameter)

Implementation Details

  • Implemented legend() function for Plotly backend
  • Enhanced Bokeh's legend placement with auto-positioning
  • Added automatic legend generation in plot_psense_dist for power scaling factor
  • Ensured consistent behavior of pc.add_legend("dim_name") across all backends

Checklist

  • Added/updated tests
  • Updated documentation
  • Added example notebooks (if applicable)
  • Passes all CI checks

Issue Targeted

#109


📚 Documentation preview 📚: https://arviz-plots--115.org.readthedocs.build/en/115/

Comment thread src/arviz_plots/backend/bokeh/legend.py
Comment thread src/arviz_plots/backend/plotly/legend.py
Comment thread src/arviz_plots/plots/psensedistplot.py
@aloctavodia aloctavodia requested a review from amaloney January 23, 2025 09:48
Copy link
Copy Markdown
Member

@amaloney amaloney left a comment

Choose a reason for hiding this comment

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

Add more context in the linking issue, and why these features are needed. Also add visuals that explicitly show how this new code is adding to the user experience.

Comment thread src/arviz_plots/backend/plotly/legend.py
Comment thread src/arviz_plots/backend/bokeh/legend.py Outdated
Comment thread src/arviz_plots/backend/plotly/legend.py
Comment thread src/arviz_plots/backend/plotly/legend.py
Comment thread src/arviz_plots/backend/plotly/legend.py Outdated
Comment thread src/arviz_plots/plots/psensedistplot.py
@Advaitgaur004
Copy link
Copy Markdown
Contributor Author

Advaitgaur004 commented Jan 24, 2025

I have added a legend placement threshold in the Plotly backend legend, which can now be altered, eliminating the need for hard coding.
And for MRE that links to bokeh.backend.legend

if side == "auto":
        plot_width = target_plot.width
        if plot_width >= legend_placement_threshold:
            side = "right"
        else:
            side = "center"

can be reproduced by following the code.

import numpy as np
from bokeh.plotting import figure, show
from bokeh.layouts import gridplot

def create_example_plot(width):
    p = figure(width=width, height=300)
    x = np.linspace(0, 10, 100)
    grid = gridplot([[p]], merge_tools=False)
    p.line(x, np.sin(x), line_color="red")
    p.line(x, np.cos(x), line_color="blue")
    legend(grid, 
           [{'color': 'red'}, {'color': 'blue'}], 
           ['sin(x)', 'cos(x)'], 
           side='auto')
    
    return grid
narrow_plot = create_example_plot(300)
wide_plot = create_example_plot(800)
show(gridplot([[narrow_plot, wide_plot]]))

300x300 dim 300x300

800x300 dim(Enough space is there so legend can be placed outside) 800x300

@Advaitgaur004
Copy link
Copy Markdown
Contributor Author

Any Thoughts on aliasing in various backend plotting modules, one has to make thing easier for developer too @aloctavodia

@Advaitgaur004
Copy link
Copy Markdown
Contributor Author

gentle reminder @amaloney

@amaloney
Copy link
Copy Markdown
Member

amaloney commented Feb 2, 2025

As a user I would expect the following.

  • If I request an image size of 300x300 pixels then I should get an image of 300x300 pixels.

I get this with the first example

image

but I do not get this with the second example.

image

The second plot has a total dimension of 300x800 pixels where a portion of that image is where the plot exists plus the size of the legend. This is not what I would have expected the output to be, since I requested an plot of 300x800 pixels in size.

There unfortunately is no easy solution to this.

We could instead have an argument where the user can request the legend be outside the plot area. Then in the docstring of the method we indicate that if this argument is selected, the resulting plot area will be smaller in size to accomodate the legend, given the requested figure size. How about you try implementing this idea.

@Advaitgaur004
Copy link
Copy Markdown
Contributor Author

Advaitgaur004 commented Feb 2, 2025

First of all, there is a slight error; the dimensions should be 800x300, as the height is predefined to be 300 in my test case. Therefore, it is not 300x800 but rather 800x300.

If the legend_in_place argument is set to False (indicating that the user wants the legend to be outside the plot), I will reduce the original plot width to 80% and adjust the legend to occupy the remaining 20%. Should I go this way or should we automate this thing, if the width is too large then place the legend inside the plot(reasonable) and if small then legend should be outside

I have changed the magic number to 500 for the time being and these are the test case

def create_example_plot(width):
    p = figure(width=width, height=300)
    x = np.linspace(0, 10, 100)
    grid = gridplot([[p]], merge_tools=False)
    p.line(x, np.sin(x), line_color="red")
    p.line(x, np.cos(x), line_color="blue")
    legend(grid, 
           [{'color': 'red'}, {'color': 'blue'}], 
           ['sin(x)', 'cos(x)'], 
           side='auto')
    
    return grid
narrow_plot = create_example_plot(450)
middle_plot = create_example_plot(550)
wide_plot = create_example_plot(800)
show(gridplot([[narrow_plot, middle_plot, wide_plot]]))

for 450x300
image

for 550x300
image

for 800x300
image

For Comparison
image

If seems correct I'll commit right away

@Advaitgaur004
Copy link
Copy Markdown
Contributor Author

@amaloney Review

Copy link
Copy Markdown
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.

This is a great starting point, thanks!

I have rebased it and taken care of tests and docs so it can be merged, sorry we haven't been able to do proper continued reviews at a reasonable pace so far.

@Advaitgaur004
Copy link
Copy Markdown
Contributor Author

This is a great starting point, thanks!

I have rebased it and taken care of tests and docs so it can be merged, sorry we haven't been able to do proper continued reviews at a reasonable pace so far.

No worries sénior 👍

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 16, 2025

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.06%. Comparing base (827cece) to head (09ca520).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/arviz_plots/backend/bokeh/legend.py 80.00% 1 Missing ⚠️
src/arviz_plots/backend/plotly/legend.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   72.44%   73.06%   +0.61%     
==========================================
  Files          42       43       +1     
  Lines        4921     4941      +20     
==========================================
+ Hits         3565     3610      +45     
+ Misses       1356     1331      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aloctavodia aloctavodia merged commit 3192596 into arviz-devs:main May 17, 2025
3 checks passed
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.

5 participants