holoviz / holoviews

With Holoviews, your data visualizes itself.
https://holoviews.org
BSD 3-Clause "New" or "Revised" License
2.7k stars 403 forks source link

Custom BoxSelectTool is broken when using in AdjointLayout #4997

Open rafgonsi opened 3 years ago

rafgonsi commented 3 years ago

ALL software version info

Python 3.8.11, bokeh==2.3.2, holoviews==1.14.4

Description of expected behavior and the observed behavior

I have an image for which I compute adjoint histogram. For that histogram I want to have my custom box select tool (I want to change the displayed "Box Select" tool name to my custom name). The problem is that now clicking the tool icon does not make it marked active. It is turned on and I can use it, although the icon looks like tool was inactive. I also noticed that clicking it again does not disable it.

Also when using only histogram (without adjoining) it works as expected.

Complete, minimal, self-contained example code that reproduces the issue

import numpy as np
import holoviews as hv
from bokeh.models import BoxSelectTool

hv.extension("bokeh")

box_select = BoxSelectTool(description="Foo")

data = np.arange(10000).reshape((100, 100))
img = hv.Image(data)
hist = img.hist(adjoin=False).opts(tools=[box_select])
hist  # Works fine
# img << hist  # Does not work

Screenshots or screencasts of the bug in action

Peek 2021-07-01 13-59

peterroelants commented 3 years ago

I ran into a similar issue with linked brushing tools such as box-select and AdjointLayout. I'm not even using a custom box select tool.

I have been able to replicate this based on an example from the linked brushing documentation by changing the final layout to a AdjointLayout:

import numpy as np
import pandas as pd
import datashader as ds
import holoviews.operation.datashader as hd
import holoviews as hv
from holoviews.selection import link_selections

hv.extension('bokeh')

num = 100000
np.random.seed(1)

dists = {
    cat: pd.DataFrame({
        'x': np.random.normal(x, s, num), 
        'y': np.random.normal(y, s, num), 
        'val': np.random.normal(val, 1.5, num), 
        'cat': cat
    }) for x,  y,  s,  val, cat in 
     [(  2,  2, 0.03, 10, "d1"), 
      (  2, -2, 0.10, 20, "d2"), 
      ( -2, -2, 0.50, 30, "d3"), 
      ( -2,  2, 1.00, 40, "d4"), 
      (  0,  0, 3.00, 50, "d5")]
}

points = hv.Points(pd.concat(dists), ['x', 'y'], ['val', 'cat'])
datashaded = hd.datashade(points, aggregator=ds.count_cat('cat'))
spreaded = hd.dynspread(datashaded, threshold=0.50, how='over').opts(tools=['box_select', 'lasso_select'])

# Declare dim expression to color by cluster
dim_expr = ((0.1+hv.dim('val')/10).round()).categorize(hv.Cycle('Set1').values)
histogram = points.hist(num_bins=60, adjoin=False, normed=False).opts(color=dim_expr)

link_selections(hv.AdjointLayout({'main':spreaded, 'top': histogram, 'right': histogram}))  # Does not work
# link_selections(spreaded + histogram)  # works

Which looks like:

drawing

Similarly, brushing (via box_select or lasso_select) in the main plot does not seem to have any effect.

However, avoiding AdjointLayout by changing the last line to:

link_selections(spreaded + histogram)

Works as expected.

Tested with:

Python: 3.9.6
numpy     : 1.21.0
bokeh     : 2.3.3
datashader: 0.13.0
holoviews : 1.14.4
pandas    : 1.3.0
peterroelants commented 3 years ago

However, I just noticed that replacing the link_selections line with following works in my case:

ls = link_selections.instance()
ls(spreaded) << ls(histogram)
jbednar commented 3 years ago

@rafgonsi , thanks for the report; I can replicate it on my system as well. Seems like a bug to me! For now I guess use a regular Layout rather than an AdjointLayout.

@peterroelants , I think your issue is different and should be in its own issue as a feature request, i.e., a request that link_selections(spreaded << histogram) be supported and for it to work like ls = link_selections.instance() ; ls(spreaded) << ls(histogram).opts(width=125). I haven't tried working on that, but I do see that link_selections has code like:

        elif isinstance(hvobj, Element):
           ...
        elif isinstance(hvobj, (Layout, Overlay, NdOverlay, GridSpace)):

that knows how to process four different container types. AdjointLayout isn't in that list, but could presumably be added.

peterroelants commented 3 years ago

@jbednar I created its own issue as you suggested: https://github.com/holoviz/holoviews/issues/5028