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

Fix linking elements that are transformed by a Compositor #6003

Merged
merged 9 commits into from
Dec 12, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Dec 1, 2023

Fixes #5908

The issue to fix could basically be simplified as linking a rasterized element to any other element:
image

rasterize generates Image elements which have at least one Compositor registered, e.g. apply_nodata. The Compositor transformation is applied in the following lines. I noticed that, in the example above, the _plot_id value of the DynamicMap changed after obj.map(...) is executed, but not of the left (target) Curve. And indeed, re-setting "manually" the original _plot_id value of the DynamicMap allowed the link to be correctly setup.

if element_compositors and obj.traverse(lambda x: x, element_patterns):
obj = obj.map(lambda obj: cls.collapse_element(obj, mode=mode, backend=backend),
element_patterns)

The obj.map(...) call is pretty complex, however, the issue can be reproduced with a much simpler version that doesn't involve any Compositor. In this example the mapped operation is a bare identity function and datashade is used instead of rasterize to avoid involving any Compositor. This is enough for the code to fail at linking the elements:

import numpy as np
import holoviews as hv
from holoviews.plotting.links import RangeToolLink
from holoviews.operation.datashader import datashade
hv.extension('bokeh')

curve = hv.Curve(np.arange(10))
dcurve = datashade(curve)
RangeToolLink(dcurve, curve, axes=['x', 'y'])
layout = (curve + dcurve).opts(shared_axes=False)
layout = layout.map(lambda obj: obj)  # Breaks the link
layout

image

I tried to adjust LabelledData.map() so that it doesn't break the links but I wasn't very successful. So instead I decided to just try to reset the _plot_id values after obj.map(...) is called, and it worked. Interestingly that also allowed to remove the Overlay.clone override added in #5881, as in that PR this override was only necessary to link from an Image to an Overlay, which is now covered by the fix from this PR.

Trying this fix against the code provided in the issue:
Kapture 2023-12-01 at 21 08 13

@philippjfr :

  • How robust do you think that change is?
  • It might well be this is more of a workaround for a proper fix in LabelledData.map() (that calls obj.clone(shared=False) which prevent the links from being preserved IIUC) or in some other place.

@maximlt maximlt added the type: bug Something isn't correct or isn't working label Dec 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7a4fc21) 88.61% compared to head (5363a70) 88.62%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6003      +/-   ##
==========================================
+ Coverage   88.61%   88.62%   +0.01%     
==========================================
  Files         315      315              
  Lines       65593    65630      +37     
==========================================
+ Hits        58122    58165      +43     
+ Misses       7471     7465       -6     
Flag Coverage Δ
ui-tests 23.27% <13.79%> (-0.01%) ⬇️

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.

@maximlt
Copy link
Member Author

maximlt commented Dec 5, 2023

I reverted the changes made and in 96fa479 applied another better fix found by @philippjfr (thanks!)

@droumis
Copy link
Member

droumis commented Dec 6, 2023

great work! works well for me

holoviews/plotting/util.py Show resolved Hide resolved
holoviews/tests/plotting/bokeh/test_links.py Outdated Show resolved Hide resolved
holoviews/util/__init__.py Outdated Show resolved Hide resolved
@droumis droumis merged commit e23c3e9 into main Dec 12, 2023
11 checks passed
@droumis droumis deleted the fix_link_dmap_img_as_src branch December 12, 2023 16:33
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: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using rasterize on the source plot breaks the RangeToolLink
5 participants