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

Allow to link to an Overlay #5881

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Allow to link to an Overlay #5881

merged 4 commits into from
Sep 14, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Sep 11, 2023

Partly extracted from d690a2e in #5840

The goal of this PR is to allow linking (with a RangeToolLink) to an Overlay, the example in mind being a minimap targeting a plot with subcoordinates (subcoordinates being implemented as an Overlay).

Overriding .clone at the Overlay level is done to be more specific instead of what was done in #5840 at the LabelledData level.

Overriding clone is only required to get test_range_tool_link_callback_single_axis_overlay_target_image_source to pass, i.e. test_range_tool_link_callback_single_axis_overlay_target runs fine without it. While the two tests look very much alike, the former uses an Image as a source. I saw that there's a Compositor registered for the Image element, which makes the code run through a pretty hairy path when creating a simple Image (.map, .collapse_element) and leads to the creation of internal/intermediate overlays.

I would be happy to:

  • have someone more knowledgeable in the Compositor logic explain what is going on there :)
  • add more tests to:
    • cover the breadth of the envisioned feature; so far I've only tested the RangeToolLink to a simple overlay of curves, with either an Image or a Curve as source.
    • maybe cover what those changes (that still feel a little hacky to, because I feel I'm just playing with a house of cards!) could break

As for documenting these changes, I haven't found any place to update where links were described as limited to elements. There are two gallery examples that demonstrate how to use the RangeToolLink, it seems to me it wouldn't improve them so much to show how to link to an overlay. My suggestion instead would be to show that this can be done adding an EEG example to the gallery (in or after #5840).

closes #4472

@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

Merging #5881 (1486955) into main (2033109) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5881   +/-   ##
=======================================
  Coverage   88.35%   88.35%           
=======================================
  Files         310      310           
  Lines       63983    64013   +30     
=======================================
+ Hits        56529    56560   +31     
+ Misses       7454     7453    -1     
Flag Coverage Δ
ui-tests 23.40% <12.12%> (-0.01%) ⬇️

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

Files Changed Coverage Δ
holoviews/plotting/bokeh/plot.py 92.33% <ø> (ø)
holoviews/core/overlay.py 68.66% <100.00%> (+0.85%) ⬆️
holoviews/plotting/bokeh/links.py 73.59% <100.00%> (ø)
holoviews/plotting/plot.py 91.79% <100.00%> (+0.08%) ⬆️
holoviews/tests/plotting/bokeh/test_links.py 98.79% <100.00%> (+0.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@philippjfr
Copy link
Member

have someone more knowledgeable in the Compositor logic explain what is going on there :)

Compositors are a really, really old mechanism in HoloViews and were originally designed to take in an Overlay and run some operation to collapse multiple layers into a single layer (this was primarily used for combining two Image layers into a single RGB layer). Later we repurposed this as a general mechanism to allow operations to be applied as part of the plotting code. You can register a Compositor to match on one or more layers of a plot (this is done using .map I believe) and once a match is found .collapse_element is called to apply the operation (it's a misnomer for Compositors that work on a single Element but wasn't worth renaming). In the case of Image we have a Compositor that replaces the nodata values, which then results in the creation of the intermediate Overlay.

maybe cover what those changes (that still feel a little hacky to, because I feel I'm just playing with a house of cards!) could break

Agreed, unfortunately I have no good suggestions on what needs to be tested. When I made the changes in #5840 I did notice a few tests starting to fail with my changes, and it's a good sign that isn't happening in this PR.

My suggestion instead would be to show that this can be done adding an EEG example to the gallery (in or after #5840).

Agreed that sounds nice.

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.

I'm happy to see this merged. If we could think of more tests that would be nice but as mentioned above I have no good way of figuring out what tests would make sense to add.

@philippjfr philippjfr merged commit 3f96806 into main Sep 14, 2023
15 checks passed
@philippjfr philippjfr deleted the link_overlay branch September 14, 2023 18:15
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.

RangeToolLink doesn't work with overlays
3 participants