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 group and label to hover tooltip if provided by user #6125

Closed
wants to merge 9 commits into from

Conversation

droumis
Copy link
Member

@droumis droumis commented Feb 20, 2024

Adds the group and label args to the bokeh hover tooltip if provided by user.

It seems to me like a good idea to have this enabled by default, but I'm guessing that there's a reason why it wasn't already in place. Opinions welcome.

Fixes #3603

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

x = np.linspace(0, 10*np.pi)
curves = {}
j = 0.5
yvals = j * np.sin(x)

curves = []
curves.append(hv.Curve((x, yvals)).opts(tools=['hover'], color='blue'))
curves.append(hv.Curve((x + 2 * np.pi/2, 2 * yvals), label='curve2').opts(tools=['hover'], color='red'))
curves.append(hv.Curve((x + 3 * np.pi/2, 3 * yvals), group='A').opts(tools=['hover'], color='green'))
curves.append(hv.Curve((x + 4 * np.pi/2, 4 * yvals), label='curve4', group='A').opts(tools=['hover'], color='orange'))
pn.Row(hv.Overlay(curves).opts(width=600, height=600)).servable()
Screen.Recording.2024-02-20.at.11.59.48.AM.mov

TODO:

  • add tests

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.55%. Comparing base (c3c211e) to head (000f159).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6125      +/-   ##
==========================================
- Coverage   88.70%   88.55%   -0.15%     
==========================================
  Files         314      316       +2     
  Lines       65993    66050      +57     
==========================================
- Hits        58537    58490      -47     
- Misses       7456     7560     +104     
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.

Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like a good idea to have this enabled by default, but I'm guessing that there's a reason why it wasn't already in place.

A reason why it hasn't been implemented can be as simple as there has been no demand for it.

I don't really have high hopes for this getting merged

You shouldn't put down the effort you have put into the PR. If you don't believe in it yourself, it is hard to get others to believe in you 👍


I would like to see a unit test. To get the tooltips after creation you can play look around with hv.renderer('bokeh').get_plot(hv.Curve([])).handles (in test we have bokeh_renderer as shorthand for the first part).


This has nothing to do with this PR, but the legend seems wrong for your example.

holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
holoviews/plotting/bokeh/element.py Outdated Show resolved Hide resolved
@hoxbro hoxbro added the type: enhancement Minor feature or improvement to an existing feature label Feb 21, 2024
@droumis
Copy link
Member Author

droumis commented Feb 21, 2024

You shouldn't put down the effort you have put into the PR. If you don't believe in it yourself, it is hard to get others to believe in you 👍

Thanks, I adjusted my language.

@droumis
Copy link
Member Author

droumis commented Feb 21, 2024

This has nothing to do with this PR, but the legend seems wrong for your example.

What's wrong about it? seems accurate to me and reflects the labels of the curves for which a label was given

@hoxbro
Copy link
Member

hoxbro commented Feb 21, 2024

What's wrong about it? seems accurate to me and reflects the labels of the curves for which a label was given

It looks like it has a blue border at the top. But I can't recreate it locally, so just ignore that comment.
 Screenshot 2024-02-21 16 55 49

@droumis
Copy link
Member Author

droumis commented Feb 21, 2024

Tests added

@hoxbro
Copy link
Member

hoxbro commented Feb 23, 2024

One last thing: Can you try setting the group/label to something like hel lo, {hello (testing for edge-cases)

@droumis
Copy link
Member Author

droumis commented Feb 27, 2024

One last thing: Can you try setting the group/label to something like hel lo, {hello (testing for edge-cases)

added a test for whitespace and special chars

image

@maximlt
Copy link
Member

maximlt commented Feb 27, 2024

Just tried hvPlot from that branch.

Before:
image

After:
image

I think it's coming from these lines:
https://github.com/holoviz/hvplot/blob/f17f4705afe40f4c8e4e388823004b14ec938519/hvplot/converter.py#L1567-L1568

There are maybe more cases, I just did a very quick check.

@maximlt
Copy link
Member

maximlt commented Feb 27, 2024

Another case, before this branch there's no Label field in the tooltip.

image


Btw I'm just exposing changes in hvPlot. I don't yet have a firm opinion on whether they're good or bad, and whether hvPlot itself should be improved to better deal with labels/groups/titles.

@ahuang11
Copy link
Collaborator

Maybe if isinstance element, 2D, then don't include the label?

@droumis
Copy link
Member Author

droumis commented Apr 2, 2024

At a HoloViz meeting, there was discussion about supporting a better syntax for a custom hover tool.

Here is the unearthed thread on the topic: #1816

I would prefer a more fully featured custom hover tool syntax as originally proposed, as long as it supports adding arbitrary entries like 'label' and 'group' arg values, in addition to standard data dims.

I will close this current PR, unmerged.

@droumis droumis closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include label in hover tool
5 participants