diff --git a/CHANGELOG.md b/CHANGELOG.md index 791c4f5f..8bd8f114 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,15 @@ in the README). that don't use them are byte-identical. ### Changed +- Stable animation `key=` identity planes are now retained and shipped only + when the resolved animation spec can actually key-match. `match` defaults to + `"index"`, so `key=` combined with a bare `xy.animation(...)`, with + `enabled=False`, or with no animation at all previously put two dead `u32` + columns in the payload — 8 B/row held for the widget lifetime and 8 B/row on + the wire (400 KB at 50k rows) that no client code read. Encoding still runs + in every case: duplicate-key and row-count errors are construction contract, + not animation policy, and are unchanged. Payloads that do key-match are + byte-identical. - Stable animation `key=` identity encoding now uses one native Rust row scan for homogeneous fixed-width strings, bytes, booleans, and signed or unsigned integers, plus finite floating arrays, including non-native-endian NumPy diff --git a/python/xy/components.py b/python/xy/components.py index 07e02102..e2e899ca 100644 --- a/python/xy/components.py +++ b/python/xy/components.py @@ -4069,6 +4069,12 @@ def _apply_mark_transition_metadata( ): raise ValueError(f"{mark.kind} animation match='key' requires key=") keys: np.ndarray | None = None + # `match` defaults to "index", so a bare `xy.animation(...)` — or no + # animation at all — never key-matches. Encoding still runs for its + # uniqueness and typing contract, but the identity planes are dead weight + # nothing reads: 8 B/row retained for the widget lifetime and 8 B/row on + # the wire. Only carry them when the client can actually match on them. + key_matching = effective.get("enabled") is not False and effective.get("match") == "key" if mark.key is not None: raw = ( _resolve(data, mark.key, context=f"{mark.kind}.key") @@ -4081,19 +4087,22 @@ def _apply_mark_transition_metadata( ) expected = int(traces[0].n_points) keys = _encode_transition_keys(raw, expected, f"{mark.kind} key") - if mark.kind in {"line", "area", "error_band"}: + if key_matching and mark.kind in {"line", "area", "error_band"}: positions = _original_mark_positions(fig, mark, data, expected) if positions is not None: keys = keys[np.argsort(positions, kind="stable")] for trace in traces: trace.animation = None if mark_spec is None else dict(mark_spec) if keys is not None: + # The per-trace row check is contract too, so it runs whether or + # not the planes are kept. if trace.n_points != len(keys): raise ValueError( f"{mark.kind} key has {len(keys)} rows but emitted trace {trace.id} " f"has {trace.n_points} logical rows" ) - trace.transition_keys = keys + if key_matching: + trace.transition_keys = keys def _continuous_color_label(mark: Mark) -> Optional[str]: diff --git a/spec/design/animation.md b/spec/design/animation.md index a4569a0a..cd858d1d 100644 --- a/spec/design/animation.md +++ b/spec/design/animation.md @@ -87,10 +87,18 @@ The kernel separates "declined this data, use the oracle" from "this layout is not in the ABI": only the first falls back, so a contract drift between the ctypes gate and the Rust layout check raises instead of silently costing the whole speedup. +Encoding runs whenever `key=` is given, because uniqueness and typing are +construction contract rather than animation policy. The resulting identity +planes are only retained and shipped when the *resolved* spec can key-match — +`match` defaults to `"index"`, so a bare `xy.animation(...)`, an +`enabled=False` chart, or a `key=` with no animation at all would otherwise +carry two u32 columns nothing reads, both in the widget's retained payload and +on the wire. Duplicate and row-count errors are unaffected by that skip. Line-like keys follow the same stable geometry sort as their coordinates; the encoder hands back Fortran-order planes that ship without a per-column copy, but that sort and any finite-row selection reorder through NumPy advanced -indexing, which returns C-order and restores the copy at ship time. +indexing, which returns C-order and restores the copy at ship time. The sort is +skipped along with the planes when nothing will match on them. Errorbar point keys are role-qualified after expansion so the main segment and caps remain unique and stable. diff --git a/tests/test_animation.py b/tests/test_animation.py index c37689d7..12b4c8dc 100644 --- a/tests/test_animation.py +++ b/tests/test_animation.py @@ -460,6 +460,95 @@ def test_native_transition_key_argument_errors_are_loud() -> None: _native._lib.xy_transition_keys_fixed = original +def _keyed_scatter(animation, n: int = 8): + x = [float(i) for i in range(n)] + marks = [xy.scatter(x=x, y=x, key=[f"k{i}" for i in range(n)])] + return xy.scatter_chart(*marks, *([animation] if animation is not None else [])).figure() + + +@pytest.mark.parametrize( + ("animation", "ships"), + [ + pytest.param(xy.animation(match="key"), True, id="match-key"), + pytest.param(xy.animation(match="index"), False, id="match-index"), + # `match` defaults to "index", so a bare animation() never key-matches. + pytest.param(xy.animation(duration=250), False, id="match-defaulted"), + pytest.param(xy.animation(match="key", enabled=False), False, id="disabled"), + pytest.param(None, False, id="no-animation-spec"), + ], +) +def test_identity_planes_ship_only_when_the_client_can_key_match(animation, ships) -> None: + """`key=` alone must not put two dead u32 columns on the wire.""" + figure = _keyed_scatter(animation) + spec, _buffers = figure.build_payload_split() + trace = spec["traces"][0] + + assert ("keys" in trace) is ships + assert (figure.traces[0].transition_keys is not None) is ships + + +@pytest.mark.parametrize( + "animation", + [ + pytest.param(xy.animation(match="index"), id="match-index"), + pytest.param(xy.animation(enabled=False), id="disabled"), + pytest.param(None, id="no-animation-spec"), + ], +) +def test_key_validation_still_runs_when_planes_are_skipped(animation) -> None: + """Uniqueness and typing are construction contract, not animation policy.""" + marks = xy.scatter(x=[1.0, 2.0, 3.0], y=[1.0, 2.0, 3.0], key=["a", "b", "a"]) + with pytest.raises(ValueError, match="duplicate value at rows 0 and 2"): + xy.scatter_chart(marks, *([animation] if animation is not None else [])).figure() + + bad = xy.scatter(x=[1.0, 2.0], y=[1.0, 2.0], key=[object(), object()]) + with pytest.raises(ValueError, match="animation key values must be"): + xy.scatter_chart(bad, *([animation] if animation is not None else [])).figure() + + +def test_key_matching_payload_is_unchanged_by_the_skip() -> None: + """The path that does key-match must be byte-identical to before.""" + spec, blob = _keyed_scatter(xy.animation(match="key"), n=32).build_payload() + trace = spec["traces"][0] + lo = _column(blob, spec, trace["keys"]["lo"]) + hi = _column(blob, spec, trace["keys"]["hi"]) + expected = _python_transition_key_reference([f"k{i}" for i in range(32)]) + np.testing.assert_array_equal(lo, expected[:, 0]) + np.testing.assert_array_equal(hi, expected[:, 1]) + + +def test_mark_animation_spec_clobbers_chart_level_fields() -> None: + """KNOWN BUG (reflex-dev/xy#329) — pinned so a fix is a deliberate change. + + `Animation.to_spec()` emits every field, and both the Python merge and the + client's `{...spec.animation, ...trace.animation}` are plain dict spreads. + So a mark-level `xy.animation(duration=90)` resets `match`, `easing`, + `enter`, `update`, and `interpolate` to their defaults, silently turning + off the chart-level `match="key"` the caller asked for. + """ + figure = xy.scatter_chart( + xy.scatter( + x=[1.0, 2.0], + y=[1.0, 2.0], + key=["a", "b"], + animation=xy.animation(duration=90), + ), + xy.animation(match="key", easing="linear"), + ).figure() + spec, _ = figure.build_payload_split() + resolved = {**spec.get("animation", {}), **(spec["traces"][0].get("animation") or {})} + + assert resolved["match"] == "index" # caller asked for "key" + assert resolved["easing"] == "ease-out" # caller asked for "linear" + assert resolved["duration"] == 90.0 # the one field they meant to set + + # The same clobbering suppresses the match='key' requires key= guard. + xy.scatter_chart( + xy.scatter(x=[1.0, 2.0], y=[1.0, 2.0], animation=xy.animation(duration=90)), + xy.animation(match="key"), + ).figure() + + def test_aggregate_tier_records_key_matching_fallback() -> None: chart = xy.scatter_chart( xy.scatter( @@ -467,7 +556,10 @@ def test_aggregate_tier_records_key_matching_fallback() -> None: y=[3.0, 4.0, 5.0], key=["a", "b", "c"], density=True, - animation=xy.animation(duration=90), + # `match="key"` has to be restated here: a mark-level animation + # spec is a full dict, so it resets every field the chart set. + # See test_mark_animation_spec_clobbers_chart_level_fields. + animation=xy.animation(duration=90, match="key"), ), xy.animation(match="key"), )