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

subcoordinate_y: respect ylim #6190

Merged
merged 3 commits into from
Apr 23, 2024
Merged

subcoordinate_y: respect ylim #6190

merged 3 commits into from
Apr 23, 2024

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Apr 15, 2024

Fixes #5952

image

I want to wait for #6122 to be merged to fully set this PR. At the moment I think you can't zoom-out from the ylim which isn't good.

@maximlt maximlt added the type: bug Something isn't correct or isn't working label Apr 15, 2024
@maximlt maximlt marked this pull request as draft April 15, 2024 17:15
@maximlt maximlt requested a review from philippjfr April 16, 2024 09:05
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.

@maximlt
Copy link
Member Author

maximlt commented Apr 17, 2024

Hmm so an issue at the moment is that when ylim is set on the Overlay the y-range cannot be changed by the user as the zoom tool is associated with the subcoordinate_y elements at a level of 1. I need to think more about this issue.

holoviews_subcoor_ylim

@maximlt
Copy link
Member Author

maximlt commented Apr 22, 2024

ylim (xlim too) isn't a hard-bound in HoloViews unless apply_hard_bounds is set to True (#6056), instead, it sets the initial y-bounds. So users can freely zoom and pan outside of the initial view when ylim is set.

Let's dive more into the zoom UX with subcoord-y overlays!

By default, the user gets global Box Zoom and Pan zoom tools controlling the whole viewport (level 0) and a Wheel Zoom controlling the subcoord-y subplots (y-axis at level 1) => Users can zoom in globally with the Box Zoom but can't zoom out at level 0 except with the Reset tool. This is a problem that was already there and that is made more obvious by this PR.

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

x = np.linspace(0, 10*np.pi)
curves = [
    hv.Curve((x + i*np.pi/2, np.sin(x)), label=f'Line {i}').opts(
        subcoordinate_y=True,
    )
    for i in range(3)
]
hv.Overlay(curves).opts(show_legend=False)

holoviews_subcoor_ylim4

Users can add Zoom In and Zoom Out tools that control the subcoord-y subplots (y-axis at level 1).

curves = [
    hv.Curve((x + i*np.pi/2, np.sin(x)), label=f'Line {i}').opts(
        subcoordinate_y=True, tools=['zoom_in', 'zoom_out'],
    )
    for i in range(3)
]
hv.Overlay(curves).opts(show_legend=False)
image

A little detour on a normal overlay behavior: it is not possible to directly add global tools to an overlay. This seems to be unrelated to subcoord-y.

overlay = hv.Curve([-1, -2, -3]) * hv.Curve([1, 2, 3])
overlay.opts(tools=['zoom_in'])
# No zoom_in tool!
image

For normal overlays, the usual workaround, I think, is to directly add a tool to a least one element of the overlay.

overlay = hv.Curve([-1, -2, -3]) * hv.Curve([1, 2, 3]).opts(tools=['zoom_in'])
overlay
image

This workaround doesn't work with subcoord-y overlays, it will not add a global Zoom In tool, instead, it'll just allow to zoom in on one subplot. That's because wheel_zoom, zoom_in and zoom_out tools are special-cased in the scope of a subcoord-y overlay, they're turned into subcoord-y zoom tools (y-axis at level 1). In this case, the workaround is to add a custom zoom instance to at least one element of the overlay.

from bokeh.models import WheelZoomTool

curves = [
    hv.Curve((x + i*np.pi/2, np.sin(x)), label=f'Line {i}').opts(
        subcoordinate_y=True, tools=[WheelZoomTool()],
    )
    for i in range(3)
]
hv.Overlay(curves).opts(show_legend=False, ylim=(0, 1))

holoviews_subcoor_ylim2

With grouped subcoord-y overlays, the same issues and workaround apply. Except that, at least at the moment, there are more zoom tools in the toolbar.

curves = [
    hv.Curve((x + i*np.pi/2, np.sin(x)), group=group, label=f'{group} Line {i}').opts(
        subcoordinate_y=True, tools=[WheelZoomTool()],
    )
    for group in 'ABC'
    for i in range(3)
]
hv.Overlay(curves).opts(show_legend=False, ylim=(0, 1))

holoviews_subcoor_ylim3

In conclusion:

  1. This PR exposes more the fact that setting a tool directly on an overlay is a no-op. I don't know if it's by design, a bug, or that it was just never implemented.
  2. The workaround for subcoord-y overlays is to pass a custom wheel zoom / zoom in / zoom out instance.
  3. This PR also exposes more of the issue users will have with differentiating a normal zoom tool with a subcoord-y zoom tool (y-axis at level 1). With @hoxbro we discussed creating a custom icon, derived from the existing icons.

My take on these:

  1. Looks like this deserves to be its own issue (I haven't found one)
  2. I'd be okay documenting this workaround, as it's similar to the usual workaround with setting a tool on an overlay, at least until 1. is resolved.
  3. I also think it deserves its own issue, the "hard" work being to create custom wheel zoom / zoom in / zoom out icons (no designer here 🙃 )

cc @droumis some food for thought

Co-authored-by: Philipp Rudiger <[email protected]>
@maximlt maximlt marked this pull request as ready for review April 22, 2024 17:43
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.55%. Comparing base (c3c211e) to head (1f7d510).
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6190      +/-   ##
==========================================
- Coverage   88.70%   87.55%   -1.15%     
==========================================
  Files         314      319       +5     
  Lines       65993    67167    +1174     
==========================================
+ Hits        58537    58808     +271     
- Misses       7456     8359     +903     
Flag Coverage Δ
ui-tests ?

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.

@philippjfr philippjfr merged commit c1824f7 into main Apr 23, 2024
12 of 13 checks passed
@philippjfr philippjfr deleted the fix_subcoordy_ylim branch April 23, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ylim does not work for a subcoordinates_y overlay
2 participants