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

feat: Extend .interactive() with tooltip and legend #3394

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1e85a59
Add tooltip and legend to .interactive()
joelostblom Apr 4, 2024
e76a3e2
Avoid using `configure` to be compatible with layered specs
joelostblom Apr 4, 2024
7c127fd
Avoid calling `to_dict()` since `validate=False` is currently not sup…
joelostblom Apr 4, 2024
d20ecc8
Detect encodings used in spec from possible legend encodings
joelostblom Aug 9, 2024
7fea185
Indicate which legend encodings are currently working
joelostblom Aug 9, 2024
343bb94
Switch to using the `react: false` approach
joelostblom Aug 9, 2024
0b5b688
Format docstring as per ruff
joelostblom Aug 9, 2024
cbf51c4
Support only one legend encoding channel
joelostblom Aug 9, 2024
29994be
Ruffify
joelostblom Aug 9, 2024
8d85e02
Note that name can be None
joelostblom Aug 9, 2024
f3a5a17
Ignore mypy type error for `next` built-in
joelostblom Aug 9, 2024
f2305bd
Add additional legend encoding channel and indicate types
joelostblom Aug 9, 2024
2981860
Use `is_undefined` instead of `isinstance`
joelostblom Aug 11, 2024
32f900e
Simplify types
joelostblom Aug 11, 2024
cc3df47
Skip checking function complexity
joelostblom Aug 11, 2024
706f374
Don't expect str type
joelostblom Aug 11, 2024
3eb743e
Adjust type to indicate that it is a list of channels
joelostblom Aug 11, 2024
4794028
Indicate that there is only a subset of encodings allowed for the leg…
joelostblom Aug 11, 2024
a8001da
refactor: Simplify typing, split up `.interactive()` for reuse
dangotbanned Aug 11, 2024
49bd736
Draft test suite
joelostblom Aug 17, 2024
b125c19
Do not try to iterate over encodings in the spec when there are none
joelostblom Aug 17, 2024
01644a2
Ensure that tooltips are added
joelostblom Aug 17, 2024
24b8aab
Merge remote-tracking branch 'upstream/main' into interactive_tooltip…
dangotbanned Aug 17, 2024
708457c
test(typing): Add annotations
dangotbanned Aug 17, 2024
51fd920
test: Adds `all_chart_types`, `color_data` fixtures
dangotbanned Aug 17, 2024
885767a
test: Add `xfail`(s) for unimplemented in `test_interactive_for_chart…
dangotbanned Aug 17, 2024
728d3cb
test: Update `test_interactive_with_no_encoding` to use `all_chart_ty…
dangotbanned Aug 17, 2024
61281d7
test: Use `color_data` in `test_interactive_legend_made_interactive_f…
dangotbanned Aug 17, 2024
5204759
test: Adds implementation for `test_interactive_legend_made_interacti…
dangotbanned Aug 17, 2024
ab7dab4
Merge remote-tracking branch 'upstream/main' into interactive_tooltip…
dangotbanned Aug 21, 2024
c6dac9d
test: Add repro for failing test
dangotbanned Aug 21, 2024
5f47fa0
Merge branch 'main' into interactive_tooltip_legend
joelostblom Aug 25, 2024
bd7ccfb
test: Remove test case fixed by #3553
dangotbanned Aug 25, 2024
4b12df6
Merge remote-tracking branch 'upstream/main' into interactive_tooltip…
dangotbanned Aug 25, 2024
d9f8850
Merge remote-tracking branch 'upstream/main' into interactive_tooltip…
dangotbanned Oct 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 103 additions & 9 deletions altair/vegalite/v5/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -3897,33 +3897,117 @@ def add_selection(self, *params) -> Self: # noqa: ANN002
return self.add_params(*params)

def interactive(
joelostblom marked this conversation as resolved.
Show resolved Hide resolved
self, name: str | None = None, bind_x: bool = True, bind_y: bool = True
) -> Self:
self,
name: str | None = None,
bind_x: bool = True,
bind_y: bool = True,
tooltip: bool = True,
legend: bool | LegendChannel_T = False,
dangotbanned marked this conversation as resolved.
Show resolved Hide resolved
) -> Chart:
dangotbanned marked this conversation as resolved.
Show resolved Hide resolved
"""
Make chart axes scales interactive.
Add common interactive elements to the chart.

Parameters
----------
name : string
name : string or None
The parameter name to use for the axes scales. This name should be
unique among all parameters within the chart.
unique among all parameters within the chart
bind_x : boolean, default True
If true, then bind the interactive scales to the x-axis
Bind the interactive scales to the x-axis
bind_y : boolean, default True
If true, then bind the interactive scales to the y-axis
Bind the interactive scales to the y-axis
tooltip : boolean, default True,
Add a tooltip containing the encodings used in the chart
legend : boolean or string, default True
A single encoding channel to be used to create a clickable legend.
The deafult is to guess from the spec based on the most commonly used legend encodings.

Returns
-------
chart :
copy of self, with interactive axes added
copy of self, with interactivity added

"""
encodings: list[SingleDefUnitChannel_T] = []
if bind_x:
encodings.append("x")
if bind_y:
encodings.append("y")
return self.add_params(selection_interval(bind="scales", encodings=encodings))
chart: Chart = self.copy().add_params(
dangotbanned marked this conversation as resolved.
Show resolved Hide resolved
selection_interval(bind="scales", encodings=encodings)
)
# We can't simply use configure_mark since configure methods
# are not allowed in layered specs
if tooltip:
chart = _add_tooltip(chart)
legend_encodings_missing = utils.is_undefined(chart.encoding)
if legend and not legend_encodings_missing:
facet_encoding: FacetedEncoding = chart.encoding
if not isinstance(legend, str):
legend = _infer_legend_encoding(facet_encoding)

facet_legend = facet_encoding[legend]
legend_type = facet_legend["type"]
if utils.is_undefined(legend_type):
legend_type = facet_legend.to_dict(context={"data": chart.data})["type"]

if legend_type == "nominal":
# TODO Ideally this would work for ordinal data too
legend_selection = selection_point(bind="legend", encodings=[legend])
initial_computed_domain = param(expr=f"domain('{legend}')")
nonreactive_domain = param(
react=False, expr=initial_computed_domain.name
)
scale = facet_legend["scale"]
if utils.is_undefined(scale):
scale = {"domain": nonreactive_domain}
else:
scale["domain"] = nonreactive_domain
chart = chart.add_params(
legend_selection,
initial_computed_domain,
nonreactive_domain,
).transform_filter(legend_selection)
else:
msg = f"Expected only 'nominal' legend type but got {legend_type!r}"
Copy link
Member

@dangotbanned dangotbanned Aug 11, 2024

Choose a reason for hiding this comment

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

Same reasoning as for a8001da (#3394)

Edit

Not exactly the same reasoning, as I think cases other than "nominal" are planned to be handled.
Just unsure on which ones, so this could be updated with each elif case as they're completed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that sounds good. I think the only other type we will support it 'ordinal'. I don't see how either 'quantitative' or 'temporal' would make use of interactivity (I guess one could technically imagine an interval selection on a quantitative legend but that is currently not possible in VegaLite).

raise NotImplementedError(msg)
return chart


LegendChannel_T: TypeAlias = Literal[
dangotbanned marked this conversation as resolved.
Show resolved Hide resolved
"color",
"fill",
"shape",
"stroke",
"opacity",
"fillOpacity",
"strokeOpacity",
"strokeWidth",
"strokeDash",
"angle", # TODO Untested
"radius", # TODO Untested
"radius2", # TODO Untested
# "size", # TODO Currently size is not working, renders empty legend
]


def _add_tooltip(chart: _TChart, /) -> _TChart:
joelostblom marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(chart.mark, str):
chart.mark = {"type": chart.mark, "tooltip": True}
dangotbanned marked this conversation as resolved.
Show resolved Hide resolved
else:
chart.mark.tooltip = True
return chart


def _infer_legend_encoding(encoding: FacetedEncoding, /) -> LegendChannel_T:
"""Set the legend to commonly used encodings by default."""
_channels = t.get_args(LegendChannel_T)
it = (ch for ch in _channels if not utils.is_undefined(encoding[ch]))
if legend := next(it, None):
return legend
else:
msg = f"Unable to infer target channel for 'legend'.\n\n{encoding!r}"
Copy link
Member

Choose a reason for hiding this comment

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

It wasn't clear to me what the intention was for this case.

At a minimum I think there should be a warning displayed, rather than silently continuing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what is the best approach here. I agree that a warning/error is more explicit than silently continuing, but since .interactive() is a sort of convenient method to quickly add basic interactivity to charts, I also want to keep it simple to use. To me, the least friction for the user is if legend interactivity is applied when it makes sense and not applied when it doesn't make sense without being too noisy with warnings and errors. I'm thinking that people would not expect a gradient color legend to be interactive in any way, but they would expect to click on discrete points/lines to select and unselect groups. This is also how e.g. plotly works, see the examples on this page https://plotly.com/python/plotly-express/#gallery. If we go this path, one case that might be confusing in altair is quantitative scales in the size encoding since these appear to have discrete categories in the legend but clicking them would not select anything (since there are rarely observations with those exact values). I think this is minor, and am still leaning towards the "apply interactivity where it makes sense" approach for .interactive() and I'm ok with this method being less explicitly verbose since I see it as more of a convenience.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what is the best approach here. I agree that a warning/error is more explicit than silently continuing, but since .interactive() is a sort of convenient method to quickly add basic interactivity to charts, I also want to keep it simple to use. To me, the least friction for the user is if legend interactivity is applied when it makes sense and not applied when it doesn't make sense without being too noisy with warnings and errors.

I do see the argument for low-friction. The issue is the cost the solution has.
If I'm a new user and I call Chart.interactive(legend=True), but result doesn't contain a legend, how am I supposed to know what went wrong?

We have useful info in .interactive() that could quickly nudge the user, e.g. set one of the expected channels.
The end result would be similar to the below example:

Screenshot from discussion

image


Without providing feedback like this, I'd assume we'd need to write the equivalent of that into the docstring anyway.
If that were the case, it would need repeating + maintaining in 7 places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm a new user and I call Chart.interactive(legend=True), but result doesn't contain a legend, how am I supposed to know what went wrong?

Good point, and it brings up an interesting distinction I didn't think of before. Whereas setting tooltip=True actually creates a tooltip, setting legend=True converts an existing legend into an interactive/clickable format (but doesn't create one from scratch as you pointed out). I will make this distinction explicit via the docstring and/or a more suitable parameter name. Then we can consider whether that is clear enough or we also need the warning/error; I appreciate your reasoning and suggestion with the nudging message, but I want to avoid that users need to pass legend=False just because there color scale is using a quantitative encoding (so eventually I will likely favor either a warning or an explicit docstring, but I will leave the error raise for now until next time I can work on this PR)

Copy link
Member

Choose a reason for hiding this comment

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

That's fair!

If I'm a new user and I call Chart.interactive(legend=True), but result doesn't contain a legend, how am I supposed to know what went wrong?

Good point, and it brings up an interesting distinction I didn't think of before. Whereas setting tooltip=True actually creates a tooltip, setting legend=True converts an existing legend into an interactive/clickable format (but doesn't create one from scratch as you pointed out)

My bad for writing the last comment without testing.
I was assuming it created a legend (forgetting that it would already be there).

I guess the starting point would be the same though, encountering that error currently would require legend=True and a Chart without a channel displaying a legend?

raise NotImplementedError(msg)


def _check_if_valid_subspec(
Expand Down Expand Up @@ -5003,6 +5087,16 @@ def sphere() -> SphereGenerator:
return core.SphereGenerator(sphere=True)


_TChart = TypeVar(
"_TChart",
Chart,
RepeatChart,
ConcatChart,
HConcatChart,
VConcatChart,
FacetChart,
LayerChart,
)
ChartType: TypeAlias = Union[
Chart, RepeatChart, ConcatChart, HConcatChart, VConcatChart, FacetChart, LayerChart
]
Expand Down
158 changes: 153 additions & 5 deletions tests/vegalite/v5/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from datetime import date, datetime
from importlib.metadata import version as importlib_version
from importlib.util import find_spec
from typing import TYPE_CHECKING
from typing import TYPE_CHECKING, Any, Literal

import ibis
import jsonschema
Expand All @@ -28,15 +28,20 @@
from tests import skip_requires_vl_convert, slow

if TYPE_CHECKING:
from altair.typing import ChartType, Optional
from altair.vegalite.v5.api import _Conditional, _Conditions
from altair.vegalite.v5.schema._typing import Map
from altair.vegalite.v5.schema._typing import Map, SingleDefUnitChannel_T

ibis.set_backend("polars")

PANDAS_VERSION = Version(importlib_version("pandas"))

_MakeType = Literal[
"layer", "hconcat", "vconcat", "concat", "facet", "facet_encoding", "repeat"
]

def getargs(*args, **kwargs):

def getargs(*args, **kwargs) -> tuple[tuple[Any, ...], dict[str, Any]]:
return args, kwargs


Expand All @@ -47,7 +52,7 @@ def getargs(*args, **kwargs):
}


def _make_chart_type(chart_type):
def _make_chart_type(chart_type: _MakeType) -> ChartType:
data = pd.DataFrame(
{
"x": [28, 55, 43, 91, 81, 53, 19, 87],
Expand Down Expand Up @@ -81,8 +86,24 @@ def _make_chart_type(chart_type):
raise ValueError(msg)


@pytest.fixture(
params=(
"layer",
"hconcat",
"vconcat",
"concat",
"facet",
"facet_encoding",
"repeat",
)
)
def all_chart_types(request) -> ChartType:
"""Use the parameter name ``all_chart_types`` to automatically parameterise."""
return _make_chart_type(request.param)


@pytest.fixture
def basic_chart():
def basic_chart() -> alt.Chart:
data = pd.DataFrame(
{
"a": ["A", "B", "C", "D", "E", "F", "G", "H", "I"],
Expand All @@ -93,6 +114,18 @@ def basic_chart():
return alt.Chart(data).mark_bar().encode(x="a", y="b")


@pytest.fixture
def color_data() -> pl.DataFrame:
"""10 rows, 3 columns ``"x:Q"``, ``"y:Q"``, ``"color:(N|O)"``."""
return pl.DataFrame(
{
"x": [0.32, 0.86, 0.10, 0.30, 0.47, 0.0, 1.0, 0.91, 0.88, 0.12],
"y": [0.11, 0.33, 0.01, 0.04, 0.77, 0.1, 0.2, 0.23, 0.05, 0.29],
"color": list("ACABBCABBA"),
}
)


@pytest.fixture
def cars():
return pd.DataFrame(
Expand Down Expand Up @@ -1657,3 +1690,118 @@ def test_ibis_with_vegafusion(monkeypatch: pytest.MonkeyPatch):
{"a": 2, "b": "2020-01-02T00:00:00.000"},
{"a": 3, "b": "2020-01-03T00:00:00.000"},
]


# TODO These chart types don't all work yet
@pytest.mark.parametrize(
"chart_type",
[
"chart",
pytest.param(
"layer", marks=pytest.mark.xfail(reason="Not Implemented", raises=TypeError)
),
pytest.param(
"facet", marks=pytest.mark.xfail(reason="Not Implemented", raises=TypeError)
),
],
)
def test_interactive_for_chart_types(chart_type: _MakeType):
chart = _make_chart_type(chart_type)
assert chart.interactive(legend=True).to_dict() # type: ignore[call-arg]


def test_interactive_with_no_encoding(all_chart_types):
# Should not raise error when there is no encoding
assert all_chart_types.interactive().to_dict()


def test_interactive_tooltip_added_for_all_encodings():
# TODO Loop through all possible encodings
# and check that tooltip interactivity is added for all of them
assert "tooltip" in alt.Chart().mark_point().interactive().to_dict()["mark"]
assert (
"tooltip"
not in alt.Chart().mark_point().interactive(tooltip=False).to_dict()["mark"]
)


@pytest.mark.parametrize(
("encoding", "err"),
[
("xOffset", NotImplementedError),
("yOffset", NotImplementedError),
("x2", NotImplementedError),
("y2", NotImplementedError),
("longitude", NotImplementedError),
("latitude", NotImplementedError),
("longitude2", NotImplementedError),
("latitude2", NotImplementedError),
("theta", NotImplementedError),
("theta2", NotImplementedError),
("radius", None),
("radius2", KeyError),
("color", None),
("fill", None),
("stroke", None),
("opacity", None),
("fillOpacity", None),
("strokeOpacity", None),
("strokeWidth", None),
("strokeDash", None),
("size", NotImplementedError),
("angle", None),
("shape", None),
("key", NotImplementedError),
("text", NotImplementedError),
("href", NotImplementedError),
("url", NotImplementedError),
("description", NotImplementedError),
],
)
def test_interactive_legend_made_interactive_for_legend_encodings(
color_data, encoding: SingleDefUnitChannel_T, err: type[Exception] | None
) -> None:
"""Check that legend interactivity is added only for the allowed legend encodings."""
chart = (
alt.Chart(color_data).mark_point().encode(x="x", y="y", **{encoding: "color"}) # type: ignore[misc]
)
if err is None:
assert chart.interactive(legend=True).to_dict()
else:
with pytest.raises(err):
chart.interactive(legend=True).to_dict()


def test_interactive_legend_made_interactive_for_appropriate_encodings_types(
color_data,
) -> None:
chart = alt.Chart(color_data).mark_point().encode(x="x", y="y")

# TODO Reverse legend=False/True once we revert the default arg to true
assert len(chart.encode(color="color:N").interactive().to_dict()["params"]) == 1
chart_with_nominal_legend_encoding = (
chart.encode(color="color:N").interactive(legend=True).to_dict()
)
assert len(chart_with_nominal_legend_encoding["params"]) == 4
for param in chart_with_nominal_legend_encoding["params"]:
if "expr" in param:
assert param["expr"] == "domain('color')" or "react" in param

# TODO These tests currently don't work because we are raising
# when the legend is not nominal. To be updated if we change that behavior
# TODO Could change this first test if we get interactivity working with nominal
# chart_with_ordinal_legend_encoding = (
# chart.encode(color="color:O").interactive(legend=True).to_dict()
# )
# assert len(chart_with_ordinal_legend_encoding["params"]) == 1

# chart_with_quantitative_legend_encoding = (
# chart.encode(color="color:Q").interactive(legend=True).to_dict()
# )
# assert len(chart_with_quantitative_legend_encoding["params"]) == 1


def test_interactive_legend_encoding_correctly_picked_from_multiple_encodings():
# TODO The legend should be chosen based on the priority order
# in the list of possible legend encodings
...
Loading