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 support for popups on selection streams #6168

Merged
merged 31 commits into from
Apr 18, 2024
Merged

Add support for popups on selection streams #6168

merged 31 commits into from
Apr 18, 2024

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Apr 3, 2024

This PR exposes the ability to pop up floating panes added in Bokeh 3.4 after a selection is made. The contents of the popup can be added to the Stream and will be populated when the user makes the first selection. The contents can be any Panel object or any component that can be rendered with Panel. To control the visibility of the popup this PR currently links the visible parameter of the provided component to the visibility of the popup itself.

Here's a simple example adding three distinct forms to each type of supported stream:

points = hv.Points(np.random.randn(1000, 2))

def form(name):
    text_input = pn.widgets.TextInput(name='Description')
    button = pn.widgets.Button(name='Save', on_click=lambda _: layout.param.update(visible=False))
    layout = pn.Column(f'# {name}', text_input, button)
    return layout

hv.streams.BoundsXY(source=points, popup=form('Bounds'))
hv.streams.Lasso(source=points, popup=form('Lasso'))
hv.streams.Tap(source=points, popup=form('Tap'))

points.opts(tools=['box_select', 'lasso_select', 'tap'], width=600, height=600, size=6, color='black', fill_color=None)

floating_panel

  • Decide whether the visible approach is sensible
  • Ensure this works on the server
  • Add documentation
  • Add tests

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

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

Project coverage is 88.11%. Comparing base (4e01bee) to head (26993fe).
Report is 27 commits behind head on main.

Files Patch % Lines
holoviews/tests/ui/bokeh/test_callback.py 18.32% 107 Missing ⚠️
holoviews/plotting/bokeh/callbacks.py 28.98% 98 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6168      +/-   ##
==========================================
- Coverage   88.69%   88.11%   -0.59%     
==========================================
  Files         316      318       +2     
  Lines       66017    66846     +829     
==========================================
+ Hits        58554    58899     +345     
- Misses       7463     7947     +484     
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.

@jlstevens
Copy link
Contributor

I know this is a draft but this looks like a decent API to me!

Initially I thought you were supplying a callback instead of a concrete layout to popup. While this is definitely a great start and it is good to support the basic case, do you think a callback could be supported as well? e.g. a callback that is supplied the stream instance so that you could make a pop-up that reflects the stream data (e.g. the position where you tapped)?

@philippjfr
Copy link
Member Author

I had the same thought and yes I think that should be supported.

@philippjfr
Copy link
Member Author

Here's an example with the callback support:

points = hv.Points(np.random.randn(1000, 2))

hv.streams.Selection1D(source=points, popup=lambda index: points.iloc[index].dframe().describe() if index else None)

points.opts(tools=['box_select', 'tap', 'lasso_select'], height=500, width=500, size=6, color='black', fill_color=None)

sel_popup

@jlstevens
Copy link
Contributor

How is the exact location of the popup decided? I assume there are multiple different policies that might be sensible depending on the stream? I suppose the default I would expect is the position of the cursor after the interaction generating the popup...

@philippjfr
Copy link
Member Author

How is the exact location of the popup decided? I assume there are multiple different policies that might be sensible depending on the stream? I suppose the default I would expect is the position of the cursor after the interaction generating the popup...

Currently it's:

Tap: x/y location
Box-select: Top-right corner
Lasso: Top-right coordinate

But yes, clearly for actual selections like Selection1D it'd be better if it was based on the overall selection rather than the geometry of the last selection.

@jbednar
Copy link
Member

jbednar commented Apr 3, 2024

Beautiful! I'd expect this example to show Tap selecting (and highlighting) a point in the same way that the lasso and box select do; is there something more that needs to be added to provide selection of a data point and not simply tapping a location?

@philippjfr
Copy link
Member Author

I'd expect this example to show Tap selecting (and highlighting) a point in the same way that the lasso and box select do; is there something more that needs to be added to provide selection of a data point and not simply tapping a location?

If you look closely it does show that, the default tap selection mode is to combine multiple selections though so you end up returning selections even when you click a random location (if a previous selection already exists).

@philippjfr
Copy link
Member Author

philippjfr commented Apr 3, 2024

This would be more intuitive if the popup was placed relative to the overall selection rather than the last selection geometry, as I had suggested above.

@jbednar
Copy link
Member

jbednar commented Apr 3, 2024

If you look closely it does show that, the default tap selection mode is to combine multiple selections though so you end up returning selections even when you click a random location (if a previous selection already exists).

Oh, so if Tap were the first thing you had used (and/or if you had been closer to a single point) it would have highlighted that point? If so, sounds good.

@philippjfr
Copy link
Member Author

Oh, so if Tap were the first thing you had used (and/or if you had been closer to a single point) it would have highlighted that point? If so, sounds good.

Yes, and if I hadn't hit a point it wouldn't have popped up anything.

@jbednar
Copy link
Member

jbednar commented Apr 3, 2024

Great. I guess there are two very different modes to think about how to use this functionality: (1) Annotating or calculating info or otherwise working with existing data points, where the tools are for data-point selection, and (2) Adding new entities, typically a range on an axis, or a bounding box or bounding region, then annotating that thing (e.g. "data drops out here", in a region where there are no data points). Presumably the same pop-up approach works for both, but figured I would bring it up just to be sure.

@ahuang11 ahuang11 self-assigned this Apr 3, 2024
@jlstevens
Copy link
Contributor

jlstevens commented Apr 4, 2024

In holonote, a DynamicMap creates an 'indicator' element to show where you want to add an annotation (which could well be somewhere where there is no existing data). As long as this approach could attach a popup to that element, then I think the second case mentioned can be handled as a consequence of the first. I think having the popup be associated with an existing element for selections (even if it is a transient indicator) is a sensible approach.

Also, I imagine you could use your stream to create whatever element in a DynamicMap and still have the popup appear (this is probably how you would do it, I imagine).

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 5, 2024

Just tried it on browser and it encountered the pending write issue. I think it has to due with the async task

from holoviz/panel#6521

There is still a clear failure case though, specifically writing an async callback that runs and modifies some Bokeh model directly will still run into the _pending_writes issue and there is effectively nothing we can do about it.

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


points = hv.Points(np.random.randn(1000, 2))

def form(name):
    text_input = pn.widgets.TextInput(name='Description')
    button = pn.widgets.Button(name='Save', on_click=lambda _: layout.param.update(visible=False))
    layout = pn.Column(f'# {name}', text_input, button)
    return layout

hv.streams.BoundsXY(source=points, popup=form('Bounds'))
hv.streams.Lasso(source=points, popup=form('Lasso'))
hv.streams.Tap(source=points, popup=form('Tap'))

pn.panel(points.opts(tools=['box_select', 'lasso_select', 'tap'], width=600, height=600, size=6, color='black', fill_color=None)).show()
Task exception was never retrieved
future: <Task finished name='Task-95' coro=<PopupMixin._populate() done, defined at [/Users/ahuang/repos/holoviews/holoviews/plotting/bokeh/callbacks.py:631](https://file+.vscode-resource.vscode-cdn.net/Users/ahuang/repos/holoviews/holoviews/plotting/bokeh/callbacks.py:631)> exception=RuntimeError('_pending_writes should be non-None when we have a document lock, and we should have the lock when the document changes')>
Traceback (most recent call last):
  File "/Users/ahuang/repos/holoviews/holoviews/plotting/bokeh/callbacks.py", line 664, in _populate
    self._panel.elements = [model]
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/core/has_props.py", line 338, in __setattr__
    return super().__setattr__(name, value)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/core/property/descriptors.py", line 333, in __set__
    self._set(obj, old, value, setter=setter)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/core/property/descriptors.py", line 621, in _set
    self._trigger(obj, old, value, hint=hint, setter=setter)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/core/property/descriptors.py", line 699, in _trigger
    obj.trigger(self.name, old, value, hint, setter)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/model/model.py", line 571, in trigger
    super().trigger(descriptor.name, old, new, hint=hint, setter=setter)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/util/callback_manager.py", line 188, in trigger
    self.document.callbacks.notify_change(cast(Model, self), attr, old, new, hint, setter, invoke)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 249, in notify_change
    self.trigger_on_change(event)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 413, in trigger_on_change
    invoke_with_curdoc(doc, invoke_callbacks)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 443, in invoke_with_curdoc
    return f()
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 412, in invoke_callbacks
    cb(event)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/callbacks.py", line 276, in <lambda>
    self._change_callbacks[receiver] = lambda event: event.dispatch(receiver)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/events.py", line 353, in dispatch
    super().dispatch(receiver)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/document/events.py", line 219, in dispatch
    cast(DocumentPatchedMixin, receiver)._document_patched(self)
  File "/Users/ahuang/miniconda3/envs/holoviews/lib/python3.10/site-packages/bokeh/server/session.py", line 244, in _document_patched
    raise RuntimeError("_pending_writes should be non-None when we have a document lock, and we should have the lock when the document changes")
RuntimeError: _pending_writes should be non-None when we have a document lock, and we should have the lock when the document changes

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 5, 2024

Multiple taps, without closing the first one, results in

UnknownReferenceError: can't resolve reference 'p1128'

Caused by

        elif self._popup:
            print("POPUP_2")
            if position:
                self._popup.visible = True
                self._panel.position = XY(**position)
                if self.plot.comm:
                    push_on_root(self.plot.root.ref['id'])
            return

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 5, 2024

Okay refactored with comments; the two issues above are fixed.

Base notebook; no more UnknownReferenceError: can't resolve reference 'p1128'

Screen.Recording.2024-04-04.at.9.32.02.PM.mov

Server; to fix RuntimeError: _pending_writes should be non-None, I dropped async

Screen.Recording.2024-04-04.at.9.32.34.PM.mov

Callbacks

Screen.Recording.2024-04-04.at.9.34.47.PM.mov

@ahuang11 ahuang11 added the type: feature A major new feature label Apr 5, 2024
@ahuang11 ahuang11 marked this pull request as ready for review April 5, 2024 23:55
@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 5, 2024

Okay, I think this is ready for review now.

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 9, 2024

@philippjfr I think this is in a good place for you to take back over to handle async / delayed stream results

https://github.com/holoviz/holoviews/pull/6168/files#diff-c0b814bedcee02b97de63fa13fc6a00858b3def2afc67eef7cdfaf04524be863R654-R656

@jbednar
Copy link
Member

jbednar commented Apr 10, 2024

In holonote, a DynamicMap creates an 'indicator' element to show where you want to add an annotation (which could well be somewhere where there is no existing data). As long as this approach could attach a popup to that element, then I think the second case mentioned can be handled as a consequence of the first.

This approach may also be how to support datashaded or rasterized plots, creating a bounding box around the region selected, then running a query against the original dataset.

I don't think matplotlib supports Taps, or any interactivity though

More precisely, our matplotlib backend doesn't support that. Matplotlib itself offers Jupyter-based interactivity via ipympl.

We should have a try/except based if the callable fails.

An app definitely needs to be robust against a failure in the callback or in any other computation resulting from a UI action. Maybe a JS console warning could be raised, at best, since everyone ignores those anyway. I think it's more likely to be noise than signal to print something on the Python terminal, because users can end up selecting things that just don't work with the callback, and the app shouldn't be affected by that.

@philippjfr
Copy link
Member Author

I think it's more likely to be noise than signal to print something on the Python terminal, because users can end up selecting things that just don't work with the callback, and the app shouldn't be affected by that.

The developer should be made aware of this so they can add the appropriate error handling themselves.

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 15, 2024

Thanks for looking into this. I couldn't get the actual contents to show up, but it might still be in progress.

Screen.Recording.2024-04-15.at.12.07.27.PM.mov

Also, using pn.serve results in
exception=RuntimeError('_pending_writes should be non-None when we have a document lock, and we should have the lock when the document changes')>

import panel as pn
import holoviews as hv
import numpy as np

hv.extension("bokeh")

points = hv.Points(np.random.randn(1000, 2))

hv.streams.Selection1D(source=points, popup=lambda index: points.iloc[index].dframe().describe() if index else None)

points.opts(tools=['box_select', 'tap', 'lasso_select'], height=500, width=500, size=6, color='black', fill_color=None)

Lastly, using box_select results in:

  File "/Users/ahuang/repos/holoviews/holoviews/plotting/bokeh/callbacks.py", line 661, in on_msg
    event = self._selection_event
AttributeError: 'Selection1DCallback' object has no attribute '_selection_event'

@philippjfr
Copy link
Member Author

Resolved the remaining issues, would appreciate more testing.

@ahuang11
Copy link
Collaborator

Thanks! I think most of them are resolved, but I couldn't get lasso to work on server / vscode:

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

points = hv.Points(np.random.randn(1000, 2))

def form(name):
    text_input = pn.widgets.TextInput(name='Description')
    button = pn.widgets.Button(name='Save', on_click=lambda _: layout.param.update(visible=False))
    layout = pn.Column(f'# {name}', text_input, button)
    return layout

hv.streams.BoundsXY(source=points, popup=form('Bounds'))
hv.streams.Lasso(source=points, popup=form('Lasso'))
hv.streams.Tap(source=points, popup=form('Tap'))

points.opts(tools=['box_select', 'lasso_select', 'tap'], width=600, height=600, size=6, color='black', fill_color=None)
Screen.Recording.2024-04-16.at.11.32.53.AM.mov

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 16, 2024

Separately, if I click too fast and furiously on init, I once got, but I can't reproduce now.
UnknownReferenceError: can't resolve reference 'a56e79fe-1414-4b63-97b4-e7af4febfb56'

def popup_distribution(index):
    x, y = points.iloc[index].data.T
    return hv.Distribution((x, y)).opts(
        width=100,
        height=100,
        toolbar=None,
        yaxis="bare",
        xlabel="",
        xticks=[-1, 0, 1],
        xlim=(-2, 2),
    )


points = hv.Points(np.random.randn(1000, 2))

hv.streams.Selection1D(
    source=points,
    popup=popup_distribution,
)

points.opts(
    tools=["box_select", "lasso_select", "tap"],
    active_tools=["lasso_select"],
    size=6,
    color="black",
    fill_color=None,
    width=500,
    height=500
)

However, there is an issue with positioning of x/y Tap; it sticks to the original. Oh I think _watch_position is just getting the farthest top right value of all the ones that are selected rather than the last clicked.

Screen.Recording.2024-04-16.at.11.36.58.AM.mov

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 16, 2024

I decided it'd make more sense to use the last point clicked as popup. However, I discovered Lasso event seems to have a bug; it's never final (see all the print(event.final) which results in False at the end)

Screen.Recording.2024-04-16.at.12.14.13.PM.mov

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 16, 2024

Okay, lasso select for Selection1D does show event.final occasionally, but only on init, when index = [].

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

def popup_distribution(index):
    print(index)
    x, y = points.iloc[index].data.T
    return hv.Distribution((x, y)).opts(
        width=100,
        height=100,
        toolbar=None,
        yaxis="bare",
        xlabel="",
        xticks=[-1, 0, 1],
        xlim=(-2, 2),
    )


points = hv.Points(np.random.randn(1000, 2))

hv.streams.Selection1D(
    source=points,
    popup=popup_distribution,
)

pn.serve(points.opts(
    tools=["box_select", "lasso_select"],
    active_tools=["lasso_select"],
    size=6,
    color="black",
    fill_color=None,
    width=500,
    height=500
))
Screen.Recording.2024-04-16.at.1.27.24.PM.mov

@philippjfr
Copy link
Member Author

Please test again, I think I've now resolved the lasso select issues. Code isn't the prettiest tbh.

@ahuang11
Copy link
Collaborator

ahuang11 commented Apr 17, 2024

Thanks! It works great. I added a few ui tests for it.

I think this is ready for review and merge.

@philippjfr philippjfr merged commit 0025713 into main Apr 18, 2024
11 of 12 checks passed
@philippjfr philippjfr deleted the popups branch April 18, 2024 12:02
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: feature A major new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants