Sankey: ribbon primitive + layout engine - #375
Open
Alek99 wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds ChangesRibbon and Sankey rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Figure
participant SankeyLayout
participant PayloadWriter
participant Renderer as SVG/Raster/WebGL
User->>Figure: sankey(links, layout_options)
Figure->>SankeyLayout: compute_layout(links, align, iterations)
SankeyLayout-->>Figure: nodes with layer/value/bounds
Figure->>Figure: ribbon(src_lo/hi, dst_lo/hi, color, color_target)
Figure->>PayloadWriter: emit ribbon trace (direct tier)
PayloadWriter-->>Renderer: geometry + dual color channels
Renderer-->>User: cubic path or polygon with gradient/outline
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
A first-class Sankey, in two halves. The `ribbon` primitive: a flow band between two vertical spans, carrying a colour at each end with the gradient running along the flow. It is a new mark kind because no composition can express it: the seam-free triangle_mesh path in both exporters is gated on one uniform fill, so per-triangle colour reintroduces an antialiasing seam on every shared edge, and the client's MESH_VS reads colour per instance, so a mesh is flat-shaded by construction. The chart-kind contract gains a normative ribbon geometry section (wire shape, the curveBumpX cubic with control points at the horizontal midpoint, flow-axis paint, deferred picking) and its "a ribbon is plugin territory" sentence is amended with the evidence. Renderers: SVG emits exact cubics with a new two-point user-space gradient helper (already inside the PDF converter's allowlist); the raster flattens through _scene.ribbon_polygon — the single geometry reference the golden tests pin both exporters to — and paints through cmd.grad's arbitrary gradient vector; the client sweeps an instanced triangle strip of the same 24 segments per edge, mixing the two end colours by flow progress. Hover resolves on the CPU by bisecting the monotone cubic at the cursor's x and testing containment; picking stays off (the id pass is point-geometry only), so selection is absent rather than wrong. The layout (python/xy/_sankey.py): longest-path layering with cycle refusal that names the cycle, node value = max(inflow, outflow), alternating barycentre sweeps for crossing minimisation, value-proportional heights on one shared scale, and endpoint stacking ordered by the opposite end. Nodes draw as a second ribbon trace — a band whose two spans are equal is a rectangle — so the whole diagram is one primitive. Public surface: xy.ribbon, xy.sankey, xy.sankey_chart(links) with hidden axes and a y-inverted unit box. PROTOCOL_VERSION 10 -> 11 in lockstep with the client: markOf() falls back to scatter for unknown kinds, so a v10 client would silently render ribbons as a point cloud. NOTE: PR #370 (polar) also claims protocol v11 off the same base; whichever merges second rebases to v12. Same for the roadmap rows. Tests: 18 new (layout invariants, refusal messages, wire shape, golden geometry incl. a rect-fall-through tripwire and a raster ink probe against the reference polygon). Suite 3,466 passing, ruff clean, capability matrix regenerated. Verified visually in all three renderers. Deferred, recorded in the contract: GPU picking, hover highlight, legend swatches for two-colour bands, style.fill gradients on ribbon, cycle auto-breaking, LOD, matplotlib.sankey shim (recorded in shim-todo.md).
Alek99
marked this pull request as ready for review
July 29, 2026 07:08
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
python/xy/marks.py (1)
574-595: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
link_opacitycoercion precedes its own error message.
float(link_opacity)on a non-numeric value raises a bareTypeError/ValueErrorinstead of the namedsankey link_opacitymessage right below it. Wrap it (or route through_validate.opacity) so the failure names the argument like every other mark.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/marks.py` around lines 574 - 595, The link_opacity validation in the Sankey mark should normalize invalid values through the established opacity validator or an equivalent guarded conversion, ensuring non-numeric and out-of-range inputs raise an error that names “sankey link_opacity.” Update the link_alpha validation block while preserving the existing accepted range of (0, 1].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/src/50_chartview.ts`:
- Around line 3866-3902: The _drawRibbons method does not write the shared axis
scale uniforms required for log and symlog rendering. In js/src/50_chartview.ts
lines 3866-3902, add uniform writes for u_xmode, u_xconstant, u_ymode, and
u_yconstant using _axisMode and _axisConstant with g.xAxis and g.yAxis, matching
_drawRects and _drawBars. No direct change is needed in js/src/40_gl.ts lines
657-713; its shared-uniform shader design already provides the required
contract.
In `@js/src/55_marks.ts`:
- Around line 169-188: Update the ribbon refreshColor handler to also re-resolve
g.colorTarget from the target trace color using parseColor, matching how
_buildRibbonMark initializes it and how g.color is refreshed. Ensure theme
changes update both ends of the ribbon gradient consistently.
In `@python/xy/_payload.py`:
- Line 849: Pass the required pw argument to _transition_entry at this call
site, keeping sel_arg in the sel parameter so transition_keys are narrowed by
the null-row selection. Match the argument ordering used by _emit_rect and the
signature _transition_entry(entry, t, pw, sel=None, key_values=None).
In `@python/xy/_sankey.py`:
- Around line 216-222: Clamp each column’s computed usable room to a
non-negative value before calculating scale, so the min ratio cannot become
negative when node_padding and count exceed the available space. Update the
usable comprehension near scale computation; preserve the existing zero-total
handling and non-finite scale fallback.
In `@python/xy/_svg.py`:
- Around line 3998-4008: The ribbon SVG path around `same`, `gradient_vector`,
and `alpha` must preserve differing endpoint alpha values instead of always
using `fills[i]`’s alpha. Update the gradient emission to represent or
consistently approximate interpolation from `a[3]` to `b[3]` (extending
`gradient_vector` for per-stop opacity if appropriate), while retaining the
solid-fill behavior when endpoints match and documenting any unavoidable
approximation.
- Around line 4009-4013: Update the ribbon SVG attribute generation near _css so
a positive stroke_width uses the trace color as the stroke fallback when
stroke_css is unset, matching _raster.py’s _emit_ribbon and the area/error_band
outline’s line_color behavior. Preserve explicit stroke colors and continue
omitting the outline when stroke_width is not positive.
In `@python/xy/styles.py`:
- Around line 294-298: Extend the target-key mapping used by the style compiler
for `stroke` and `stroke-width` to include ribbon kinds, alongside the existing
`_POINT_KINDS`, `_RECT_KINDS`, `_MESH_KINDS`, and `"box"` handling. Ensure
ribbon styles compile to `stroke` and `stroke_width` so `marks.ribbon` consumes
them as outline properties rather than mapping them to fill color and width.
In `@spec/api/styling.md`:
- Line 1218: Update the ribbon row’s Dash capability in spec/api/styling.md from
supported to unsupported, matching the current implementations of _emit_ribbon
and _ribbon_marks and the generated capability matrix. Keep the remaining ribbon
capability declarations unchanged.
---
Nitpick comments:
In `@python/xy/marks.py`:
- Around line 574-595: The link_opacity validation in the Sankey mark should
normalize invalid values through the established opacity validator or an
equivalent guarded conversion, ensuring non-numeric and out-of-range inputs
raise an error that names “sankey link_opacity.” Update the link_alpha
validation block while preserving the existing accepted range of (0, 1].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3265e591-4220-4295-9634-df1a3f53b8d9
📒 Files selected for processing (27)
docs/styling/capabilities.mdjs/src/00_header.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/55_marks.tspython/xy/__init__.pypython/xy/_figure.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_sankey.pypython/xy/_scene.pypython/xy/_svg.pypython/xy/_trace.pypython/xy/components.pypython/xy/config.pypython/xy/marks.pypython/xy/styles.pyspec/api/capability-matrix.mdspec/api/chart-kind-contract.mdspec/api/chart-roadmap.mdspec/api/styling.mdspec/design/wire-protocol.mdspec/matplotlib/shim-todo.mdtests/pyplot/test_tick_side_rendering.pytests/test_api_parity.pytests/test_sankey.pytests/test_type_surface.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/app/xy_docs/api_reference.py`:
- Line 87: Update the marks API table in MARKS to include xy.ribbon alongside
the existing xy.sankey entry, ensuring the ribbon primitive is exposed in the
generated API reference.
In `@docs/charts/sankey.md`:
- Around line 69-74: Update the fenced Python examples in the Sankey
documentation, including the block around the sankey_chart example and the
second block at the corresponding later section, by replacing each opening and
closing backtick fence with the configured tilde style: ~~~python and ~~~.
Preserve the example contents unchanged.
In `@python/xy/interaction.py`:
- Around line 263-266: Update the row_dict logic in interaction.py to detect and
process specialized Sankey ribbon/semantic rows before accessing t.x.values or
t.y.values. In that early path, merge source, target, and value from the ribbon
row, while preserving the existing tooltip_rows handling and generic coordinate
projection for non-ribbon traces.
- Around line 263-266: Update the tooltip_rows handling in the interaction
payload construction to exclude numeric flow values from JSON instead of passing
them through _json_scalar. Retain only semantic labels and references in out,
while preserving numeric tooltip data in the binary f32 interaction payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 431d8950-3b18-4148-8459-9da06413bf9e
📒 Files selected for processing (21)
docs/app/scripts/check_html_routes.pydocs/app/tests/test_docs_site.pydocs/app/xy_docs/api_reference.pydocs/app/xy_docs/config.pydocs/app/xy_docs/gallery.pydocs/charts/sankey.mddocs/components/marks.mddocs/overview/gallery.mddocs/styling/mark-styles.mdjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/52_tooltip.tsjs/src/60_entries.tspython/xy/_payload.pypython/xy/_scene.pypython/xy/_trace.pypython/xy/interaction.pypython/xy/marks.pyspec/api/chart-kind-contract.mdtests/test_sankey.pytests/test_ui_issue_regressions.py
🚧 Files skipped from review as they are similar to previous changes (7)
- python/xy/_scene.py
- python/xy/_trace.py
- js/src/50_chartview.ts
- spec/api/chart-kind-contract.md
- python/xy/_payload.py
- tests/test_sankey.py
- python/xy/marks.py
…ents
Review pass over the ribbon primitive and Sankey layout:
- WebGL: _drawRibbons writes the shared u_xmode/u_xconstant/u_ymode/
u_yconstant uniforms RIBBON_VS reads, so log/symlog axes no longer
render ribbons with linear geometry; theme refresh re-resolves
color_target and the outline stroke, not just the source end.
- WebGL outlines: RIBBON_FS draws stroke/stroke-width/stroke-opacity as
an inset border along the curved edges — the capability registry
claimed webgl=full for stroke while the client drew nothing. The
end-face difference vs the exporters' closed-path stroke is recorded
in the divergence registry and the ribbon contract.
- styles: ribbon joins the stroke/stroke-width target sets, so
style={"stroke": ...} reaches the outline instead of repainting the
band, and stroke-width is no longer dropped.
- SVG/raster parity: stroke_width without a stroke colour falls back to
the trace colour in SVG (the raster already did); both exporters fold
opacity*stroke_opacity into the outline; flat-fill collapse compares
all four channels, and endpoint alphas interpolate along the flow
(SVG per-stop stop-opacity, raster RGBA gradient stops).
- Sankey layout: align="right" and align="center" are implemented
(d3-sankey semantics) instead of silently behaving as "left"; an
over-packed node_padding is refused by name instead of inverting
every span through a negative shared scale.
- interaction: semantic tooltip rows replace the x/y placement
projection in pick results, per the contract; the contract records
that tooltip_rows values are deliberately JSON scalars (small-N
semantic readouts, not bulk geometry).
- docs/spec: xy.ribbon exposed in the marks API reference and marks.md;
styling.md ribbon row corrects the Stroke/Dash cells; sankey.md fence
style (MD048) and alignment prose; capability matrix regenerated for
the new divergence entry.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/src/50_chartview.ts (2)
3844-3859: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease ribbon-owned GPU buffers during trace teardown.
t0Buf,t1Buf,rgbaBuf, andrgba2Bufare allocated here, but_destroyTraceResourcesdoes not delete them. Rebuilds/spec updates will accumulate WebGL buffers.Proposed fix
this._deleteBuffers(g, [ "xBuf", "yBuf", "cBuf", "sBuf", "selBuf", "baseBuf", - "x0Buf", "x1Buf", "x2Buf", "y0Buf", "y1Buf", "y2Buf", + "x0Buf", "x1Buf", "x2Buf", "y0Buf", "y1Buf", "y2Buf", + "t0Buf", "t1Buf", "rgbaBuf", "rgba2Buf", "posBuf", "value1Buf", "value0Buf",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/src/50_chartview.ts` around lines 3844 - 3859, Update _destroyTraceResources to delete the ribbon-owned GPU buffers t0Buf, t1Buf, rgbaBuf, and rgba2Buf when present, matching the allocations performed in the ribbon setup block. Ensure teardown clears these resources during rebuilds and spec updates without altering unrelated trace cleanup.
3924-3955: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPerform ribbon hit-testing in transformed axis coordinates.
The shader curves and interpolates after
xyMap; this method does both in raw data space. On log or symlog axes, hover containment diverges from the rendered ribbon. TransformdataX/dataYand all decoded endpoints through_axisCoord()before bisection and edge interpolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/src/50_chartview.ts` around lines 3924 - 3955, Update _ribbonHover to perform all hit-testing in transformed axis coordinates: pass dataX and dataY, plus every decoded x0/x1/y0/y1/t0/t1 endpoint, through _axisCoord() before containment checks, bisection, and edge interpolation. Preserve the existing ribbon iteration and interpolation logic while ensuring comparisons match the shader’s xyMap behavior on log and symlog axes.
🧹 Nitpick comments (1)
python/xy/_sankey.py (1)
142-144: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a deque for the reverse Kahn queue.
pop(0)is O(n) per iteration, making_heightsquadratic for wide graphs. Usedeque.popleft()to keep the traversal linear in nodes plus links.Suggested fix
+from collections import deque + - queue = [n.index for n in nodes if outdegree[n.index] == 0] + queue = deque(n.index for n in nodes if outdegree[n.index] == 0) while queue: - current = queue.pop(0) + current = queue.popleft()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/_sankey.py` around lines 142 - 144, Update the queue initialization and consumption in _heights to use collections.deque, replacing queue.pop(0) with queue.popleft(). Preserve the existing reverse Kahn traversal and enqueue behavior while ensuring queue operations remain linear-time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/src/50_chartview.ts`:
- Around line 3894-3898: Preserve per-band colors for implicit ribbon outlines
by adding an explicit-stroke flag in _drawRibbons and setting it based on
whether g.stroke is supplied; retain the existing fallback uniform for explicit
strokes. In js/src/50_chartview.ts lines 3894-3898, set the new uniform
alongside u_stroke. In js/src/40_gl.ts lines 708-735, update the ribbon outline
shader to derive the stroke color from v_rgba when the flag is unset, while
continuing to use u_stroke for explicit stroke colors.
In `@python/xy/styling/capabilities.py`:
- Around line 228-236: The WebGL ribbon endpoint-face limitation is incorrectly
classified as a default divergence. Reclassify or move ribbon_outline_end_faces
in the capabilities registry as a style-specific limitation, then regenerate
spec/api/capability-matrix.md so the WebGL ribbon stroke, stroke-opacity, and
stroke-width status and wording consistently indicate partial support; update
python/xy/styling/capabilities.py lines 228-236 and
spec/api/capability-matrix.md line 132.
In `@tests/test_sankey.py`:
- Around line 200-203: Update the Sankey specification generation and
serialization around tooltip_rows so numeric tooltip values are removed from the
JSON spec and transported through a raw f32 buffer with row references instead.
Retain canonical tooltip values as f64 on the CPU, and revise the assertions in
the test for exact_link and exact_node to verify the buffer-backed
representation and row references rather than only the absence of x/y fields.
---
Outside diff comments:
In `@js/src/50_chartview.ts`:
- Around line 3844-3859: Update _destroyTraceResources to delete the
ribbon-owned GPU buffers t0Buf, t1Buf, rgbaBuf, and rgba2Buf when present,
matching the allocations performed in the ribbon setup block. Ensure teardown
clears these resources during rebuilds and spec updates without altering
unrelated trace cleanup.
- Around line 3924-3955: Update _ribbonHover to perform all hit-testing in
transformed axis coordinates: pass dataX and dataY, plus every decoded
x0/x1/y0/y1/t0/t1 endpoint, through _axisCoord() before containment checks,
bisection, and edge interpolation. Preserve the existing ribbon iteration and
interpolation logic while ensuring comparisons match the shader’s xyMap behavior
on log and symlog axes.
---
Nitpick comments:
In `@python/xy/_sankey.py`:
- Around line 142-144: Update the queue initialization and consumption in
_heights to use collections.deque, replacing queue.pop(0) with queue.popleft().
Preserve the existing reverse Kahn traversal and enqueue behavior while ensuring
queue operations remain linear-time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77b813c4-ceb7-4dfb-9509-1b45320f45b7
📒 Files selected for processing (17)
docs/app/xy_docs/api_reference.pydocs/charts/sankey.mddocs/components/marks.mddocs/styling/capabilities.mdjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/55_marks.tspython/xy/_raster.pypython/xy/_sankey.pypython/xy/_svg.pypython/xy/interaction.pypython/xy/styles.pypython/xy/styling/capabilities.pyspec/api/capability-matrix.mdspec/api/chart-kind-contract.mdspec/api/styling.mdtests/test_sankey.py
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/api/styling.md
- docs/components/marks.md
- docs/styling/capabilities.md
- spec/api/chart-kind-contract.md
- docs/charts/sankey.md
Comment on lines
+3894
to
+3898
| gl.uniform1f(u("u_opacity"), this._fillOpacity(g.trace.style) * (g._legendDim ?? 1)); | ||
| const stroke = g.stroke || g.color; | ||
| gl.uniform4f(u("u_stroke"), stroke[0], stroke[1], stroke[2], stroke[3]); | ||
| gl.uniform1f(u("u_strokeWidth"), (g.strokeWidth || 0) * this.dpr); | ||
| gl.uniform1f(u("u_strokeOpacity"), this._strokeOpacity(g.trace.style || {}) * (g._legendDim ?? 1)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve per-band colors for implicit ribbon outlines. When style.stroke is absent, _drawRibbons falls back to uniform g.color; direct-RGBA ribbons therefore outline every band with the trace fallback rather than its interpolated endpoint color.
js/src/50_chartview.ts#L3894-L3898: add a uniform flag indicating whether an explicit stroke color was supplied.js/src/40_gl.ts#L708-L735: when that flag is unset, derive the stroke fromv_rgbainstead ofu_stroke, preserving each band’s source-to-target gradient.
📍 Affects 2 files
js/src/50_chartview.ts#L3894-L3898(this comment)js/src/40_gl.ts#L708-L735
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/src/50_chartview.ts` around lines 3894 - 3898, Preserve per-band colors
for implicit ribbon outlines by adding an explicit-stroke flag in _drawRibbons
and setting it based on whether g.stroke is supplied; retain the existing
fallback uniform for explicit strokes. In js/src/50_chartview.ts lines
3894-3898, set the new uniform alongside u_stroke. In js/src/40_gl.ts lines
708-735, update the ribbon outline shader to derive the stroke color from v_rgba
when the flag is unset, while continuing to use u_stroke for explicit stroke
colors.
Comment on lines
+228
to
+236
| RendererDivergence( | ||
| id="ribbon_outline_end_faces", | ||
| what="A ribbon outline at the band's two vertical end faces", | ||
| webgl="absent (the outline is an inset border along the curved edges)", | ||
| svg="stroked (the closed path includes the end faces)", | ||
| native="stroked (the closed path includes the end faces)", | ||
| visible_when="stroke on a bare ribbon whose ends do not abut node bands", | ||
| tracked_by="the ribbon geometry contract's Outline clause", | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Classify the WebGL ribbon-outline limitation as style-specific.
This occurs only after callers select stroke, so it is not a default divergence. It also conflicts with the matrix declaring WebGL ribbon stroke, stroke-opacity, and stroke-width as full; either render the endpoint faces or mark/document WebGL stroke support as partial.
python/xy/styling/capabilities.py#L228-L236: move or reclassifyribbon_outline_end_facesso the default-divergence registry remains accurate.spec/api/capability-matrix.md#L132-L132: regenerate the matrix with matching style-specific capability status and wording.
📍 Affects 2 files
python/xy/styling/capabilities.py#L228-L236(this comment)spec/api/capability-matrix.md#L132-L132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/styling/capabilities.py` around lines 228 - 236, The WebGL ribbon
endpoint-face limitation is incorrectly classified as a default divergence.
Reclassify or move ribbon_outline_end_faces in the capabilities registry as a
style-specific limitation, then regenerate spec/api/capability-matrix.md so the
WebGL ribbon stroke, stroke-opacity, and stroke-width status and wording
consistently indicate partial support; update python/xy/styling/capabilities.py
lines 228-236 and spec/api/capability-matrix.md line 132.
Comment on lines
+200
to
+203
| # Semantic rows REPLACE the coordinate projection: the x/y slots hold a | ||
| # ribbon's internal placement (its target span), never a data readout. | ||
| for row in (exact_link, exact_node): | ||
| assert "x" not in row and "y" not in row |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep tooltip numeric values out of the JSON spec.
tooltip_rows remains part of spec, and its value entries are JSON floats. Removing internal x/y projections does not meet the wire contract. Ship numeric tooltip values through a raw f32 buffer with row references, retain canonical f64 values CPU-side, and assert that representation here.
As per coding guidelines, “Never place JSON numbers on the wire; transport data as raw f32 buffers” and “Keep canonical data CPU-side as f64.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_sankey.py` around lines 200 - 203, Update the Sankey specification
generation and serialization around tooltip_rows so numeric tooltip values are
removed from the JSON spec and transported through a raw f32 buffer with row
references instead. Retain canonical tooltip values as f64 on the CPU, and
revise the assertions in the test for exact_link and exact_node to verify the
buffer-backed representation and row references rather than only the absence of
x/y fields.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A first-class Sankey for xy:
xy.sankey_chart(links).Two halves:
The
ribbonprimitive — a flow band between two vertical spans, a colour at each end, gradient along the flow. New mark kind because no composition can express it: the seam-freetriangle_meshroute in both exporters requires one uniform fill (np.all(fills == fills[0])), and the client'sMESH_VSreads colour per instance (flat-shaded by construction). The chart-kind contract gains a normative ribbon geometry contract (wire shape,curveBumpXcubic, flow-axis paint, deferred picking) so a fourth renderer implements from one text; its "a ribbon is plugin territory" sentence is amended with the evidence.userSpaceOnUsegradient helper (already in the PDF allowlist — PDF export works)_scene.ribbon_polygon(the single reference the golden tests pin both exporters to), painted viacmd.grad's arbitrary gradient vectorThe layout (
python/xy/_sankey.py) — longest-path layering with a cycle refusal that names the cycle,max(inflow, outflow)node values, alternating barycentre crossing-minimisation sweeps, value-proportional heights on one shared scale, endpoint stacking ordered by the opposite end. Nodes draw as a second ribbon trace: a band whose spans are equal is a rectangle, so the whole diagram is one primitive.Protocol v10 → v11 in lockstep (⚠️ PR #370 also claims v11 off the same base — whichever merges second rebases to v12.
markOf()falls back to scatter, so a stale client would silently render ribbons as a point cloud).18 new tests (layout invariants, refusals, wire shape, golden geometry including a rect-fall-through tripwire and a raster ink probe). Suite 3,466 passing, capability matrix regenerated, verified visually in all three renderers.
Deferred, recorded in the contract: GPU picking, hover highlight, legend swatches for two-colour bands,
style.fillgradients on ribbon, cycle auto-breaking, LOD, mplsankeyshim.🤖 Generated with Claude Code
Summary by CodeRabbit
xy.sankey_chartandxy.sankeywith automatic layout, labels, and alignment controls.