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

Add a zoom tool per subcoordinate_y group #6122

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Add a zoom tool per subcoordinate_y group #6122

merged 4 commits into from
Apr 17, 2024

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Feb 20, 2024

closes #5901

When users assign a group to an Element with subcoordinate_y enabled, a dedicated wheel_zoom is created per group. The order of the zoom tools matches how the groups are displayed, i.e. the top tool controls the group displayed at the top of the chart. Each tool has a custom tooltip that contains the group name.

import numpy as np
import panel as pn
import holoviews as hv
hv.extension('bokeh')

x = np.linspace(0, 10*np.pi)
curves = []
j = 0.5
for group in ['A', 'B', 'C']:
    for i in range(2):
        yvals = j * np.sin(x)
        curve = hv.Curve((x + i*np.pi/2, yvals), label=f'{group}{i}', group=group).opts(
            subcoordinate_y=True
        )
        curves.append(curve)
        j += 1

hv.Overlay(curves).opts(show_legend=False, width=400, height=400)

Kapture 2024-02-20 at 15 18 03

Users can add extra zoom_in and zoom_out tools. The UX isn't great but we agreed not to spend too much time trying to improve it (I've seen worse!).

for group in ['A', 'B', 'C']:
    for i in range(2):
        yvals = j * np.sin(x)
        curve = hv.Curve((x + i*np.pi/2, yvals), label=f'{group}{i}', group=group).opts(
            subcoordinate_y=True, tools=['zoom_in', 'zoom_out']
        )
        curves.append(curve)
        j += 1

hv.Overlay(curves).opts(show_legend=False, width=400, height=800)

Kapture 2024-02-20 at 15 21 29

When overlaid with other non subcoordinate_y elements, the default zoom tool is displayed.

for group in ['A', 'B', 'C']:
    for i in range(2):
        yvals = j * np.sin(x)
        curve = hv.Curve((x + i*np.pi/2, yvals), label=f'{group}{i}', group=group).opts(
            subcoordinate_y=True,
        )
        curves.append(curve)
        j += 1

(hv.Overlay(curves) * hv.Overlay([hv.VSpan(i, i + 2) for i in range(2, 20, 4)])).opts(
    show_legend=False, width=400, height=500
)

Kapture 2024-02-20 at 15 25 40

Implementation-wise, I opted for adding a post-processing step, instead of having to changing the structure of the zoom handle directory (accounting for groups) as it required changes in various places that are hard to follow. In the end, the implementation isn't so complex.

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2024

Codecov Report

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

Project coverage is 88.69%. Comparing base (bf4592e) to head (81e507b).
Report is 2 commits behind head on main.

❗ Current head 81e507b differs from pull request most recent head a93c67e. Consider uploading reports for the commit a93c67e to get more accurate results

Files Patch % Lines
holoviews/plotting/bokeh/element.py 94.28% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6122      +/-   ##
==========================================
- Coverage   88.70%   88.69%   -0.01%     
==========================================
  Files         316      316              
  Lines       66121    66129       +8     
==========================================
+ Hits        58650    58654       +4     
- Misses       7471     7475       +4     
Flag Coverage Δ
ui-tests 23.77% <7.07%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@droumis
Copy link
Member

droumis commented Feb 20, 2024

I made the normalization per group a separate issue. I think it could probably go into your postprocessing step.

@droumis
Copy link
Member

droumis commented Feb 20, 2024

If not all the curves are assigned a group, then none of the group-wise zooming takes effect. We should either issue a warning/error to assign all curves to a group, or just group any that aren't already assigned into a new group. Probably the former is good enough and the simpler approach.

For example, in the code below, not including a group arg for the B group breaks all the intended grouping behavior.

import numpy as np
import panel as pn
import holoviews as hv
hv.extension('bokeh')

x = np.linspace(0, 10*np.pi)
curves = {}
j = 0.5
for i, group in enumerate(['A', 'B', 'C']):
    for i in range(2):     
        yvals = j * np.sin(x)
        label = f'{group}{i}'
        if group == "B":
            curve = hv.Curve((x + i*np.pi/2, yvals), label=label, group=group).opts(
                subcoordinate_y=True, tools=['zoom_in', 'zoom_out', 'hover']
            )
        else:
            curve = hv.Curve((x + i*np.pi/2, yvals), label=label).opts(
                subcoordinate_y=True, tools=['zoom_in', 'zoom_out', 'hover']
            )
        curves[(group, label)] = curve
        j += 1

pn.Row(hv.Overlay(curves, kdims=['group', 'label']).opts(show_legend=False, width=600, height=500)).servable()

image

@droumis droumis requested a review from philippjfr April 5, 2024 16:08
@maximlt maximlt added the type: enhancement Minor feature or improvement to an existing feature label Apr 15, 2024
@maximlt
Copy link
Member Author

maximlt commented Apr 17, 2024

We decided yesterday not to try to depend on bokeh/bokeh#13826 for now, since it may take a little while until this is refined and released, and may need more upstream changes in Panel (to adapt to Bokeh 3.5) before we can even start using the new Bokeh feature.

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@maximlt maximlt merged commit 4042923 into main Apr 17, 2024
11 of 12 checks passed
@maximlt maximlt deleted the subcoordy_group branch April 17, 2024 11:38
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply subcoordinate zoom by group
4 participants