Sankey: ribbon primitive + layout engine - #375
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesRibbon and Sankey feature
Estimated code review effort: 5 (Critical) | ~90 minutes 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
Follow-up review pass on the ribbon outline added in 91655cc. An implicit outline (stroke_width with no stroke colour) resolved to a single trace-level colour. For a per-band ribbon that is wrong in every renderer and inconsistent between them: a direct_rgba colour channel ships no `color` field, so the client fell back to the hardcoded blue-gray while the exporters used the palette colour. Adopt the edgecolors="face" convention the point and rect programs already use — an omitted stroke colour matches each band's own source-end paint, flat, in all three renderers (cmd.stroke and SVG's stroke= take one colour per band, so the client must not ramp what the exporters cannot). RIBBON_FS also folds the two vertical end faces into the outline distance via v_t's derivative, so the client strokes the closed band exactly as the exporters' closed path does. That removes the end-face gap entirely, so the ribbon_outline_end_faces divergence entry is gone: a style-selected difference never belonged in a registry documented as default-only, and webgl stroke support is now genuinely full. The capability documents return to their previously generated content.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
3939-3958: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHit-test ribbons in transformed axis coordinates.
The shader builds the cubic after log/symlog mapping, but
_ribbonHoversolves and interpolates in raw data space. Tooltips therefore miss or select the wrong vertical span on non-linear axes. Transform the pointer and all decoded endpoints with_axisCoord()before solving and comparing edges.🤖 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 3939 - 3958, Update the ribbon hit-testing logic around _ribbonHover to operate in transformed axis coordinates: apply _axisCoord() to the pointer coordinates and every decoded x/y endpoint before defining xAt, solving for t, and comparing dataY against edgeLo and edgeHi. Preserve the existing bisection and interpolation behavior while ensuring log/symlog axes use transformed values consistently.
3856-3859: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease
rgba2Bufduring trace teardown.The target-color buffer is allocated here but omitted from
_destroyTraceResources, leaking a WebGL buffer on rebuild/destroy for dual-color ribbons.Proposed fix
- "x0Buf", "x1Buf", "x2Buf", "y0Buf", "y1Buf", "y2Buf", + "x0Buf", "x1Buf", "x2Buf", "y0Buf", "y1Buf", "y2Buf", "rgba2Buf",🤖 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 3856 - 3859, Update _destroyTraceResources to release the g.rgba2Buf WebGL buffer created by the direct_rgba branch in the trace setup, ensuring dual-color ribbon resources are freed during teardown, rebuild, and destroy while preserving existing cleanup behavior.
🤖 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 `@tests/test_sankey.py`:
- Around line 394-400: Update the raster assertions around _decode_rgba to
compare the rendered Sankey image against an otherwise identical stroke_width=0
baseline. Identify pixels changed by enabling outlines, then assert those
changed edge pixels include both red-dominant and green-dominant hues, rather
than checking colored fill pixels across the full image.
---
Outside diff comments:
In `@js/src/50_chartview.ts`:
- Around line 3939-3958: Update the ribbon hit-testing logic around _ribbonHover
to operate in transformed axis coordinates: apply _axisCoord() to the pointer
coordinates and every decoded x/y endpoint before defining xAt, solving for t,
and comparing dataY against edgeLo and edgeHi. Preserve the existing bisection
and interpolation behavior while ensuring log/symlog axes use transformed values
consistently.
- Around line 3856-3859: Update _destroyTraceResources to release the g.rgba2Buf
WebGL buffer created by the direct_rgba branch in the trace setup, ensuring
dual-color ribbon resources are freed during teardown, rebuild, and destroy
while preserving existing cleanup behavior.
🪄 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: 3b79b1fe-cf1e-4da8-a1be-9b3498008a35
📒 Files selected for processing (8)
docs/styling/capabilities.mdjs/src/40_gl.tsjs/src/50_chartview.tspython/xy/_raster.pypython/xy/_svg.pyspec/api/capability-matrix.mdspec/api/chart-kind-contract.mdtests/test_sankey.py
💤 Files with no reviewable changes (1)
- docs/styling/capabilities.md
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/api/chart-kind-contract.md
- spec/api/capability-matrix.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 40 files
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
… styles The cubic is now normative in axis-transformed space (the contract): the raster maps the six endpoints first and flattens the cubic they define, and CPU hover transforms the pointer and endpoints through _axisCoord — matching the SVG exporter's exact pixel-space C and the client's clip-space sweep on log/symlog axes, where flatten-then-map bowed a different curve. d3-sankey and ECharts evaluate curveBumpX the same way; under affine axes nothing changes. Ribbon paints ship resolved (constant or direct_rgba): numeric color encodings sample the shared exporter LUT at the factory (channels.resolve_direct_rgba), because the ribbon program's a_rgba2 shares its attribute slot with a_style and has no cval path — the live chart was silently painting the constant fallback for continuous/categorical ribbons. Per-band opacity/stroke/stroke_width arrays are refused by name instead of shipped-and-dropped; per-band alpha rides the RGBA color rows, which every renderer interpolates. Also: ribbon-owned GPU buffers (t0/t1/rgba/rgba2, plus style/stroke) join the trace teardown list; sankey cycle refusal names only the actual cycle members (iterative Tarjan) instead of everything downstream; both Kahn queues use deques and duplicate detection uses a Counter; sankey link_opacity failures name the argument; ribbon/sankey/sankey_chart join the TYPE_CHECKING imports; wire-protocol §7 gains the v11 entry. The raster outline test now diffs against a stroke_width=0 baseline so it sees only outline ink, and new tests pin the log-axis raster geometry, the factory paint resolution, the per-trace style refusals, and the hover transform.
Member
Author
|
5f8573b addresses the remaining review batch. Beyond the inline threads, the outside-diff comments are in too:
Verification: suite 3,486 passing; live client vs raster export compared on a log-y ribbon in-browser (same transformed-space curve; hover hits inside the drawn band and misses the old data-space region); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_sankey.py (1)
460-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBrittle source-text split fails opaquely.
view.split("_ribbonHover(g, dataX, dataY)", 1)[1]raisesIndexErrorif the signature is reformatted or the params renamed, hiding the real cause. Assert the anchor is present first (or match a regex on the signature) so a rename reports as a contract change.🤖 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 460 - 464, The source-text test around _ribbonHover should validate that its expected signature anchor exists before splitting the file content. Add an explicit assertion with a clear contract-change message, then retain the existing hover extraction and axis transformation assertions.
🤖 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 `@tests/test_sankey.py`:
- Around line 416-421: Update the data-space flattening probe in the Sankey
raster test to sample the interior side of the hypothetical data-space curve:
use the same downward row offset as the existing inside probe rather than
subtracting from data_space_mid_y. Keep the assertion and endpoint-mapping
validation unchanged.
---
Nitpick comments:
In `@tests/test_sankey.py`:
- Around line 460-464: The source-text test around _ribbonHover should validate
that its expected signature anchor exists before splitting the file content. Add
an explicit assertion with a clear contract-change message, then retain the
existing hover extraction and axis transformation assertions.
🪄 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: 7c58a8b4-7c3b-47fc-a26f-40f555c0ac6a
📒 Files selected for processing (11)
js/src/40_gl.tsjs/src/50_chartview.tspython/xy/__init__.pypython/xy/_raster.pypython/xy/_sankey.pypython/xy/_scene.pypython/xy/channels.pypython/xy/marks.pyspec/api/chart-kind-contract.mdspec/design/wire-protocol.mdtests/test_sankey.py
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/design/wire-protocol.md
- python/xy/init.py
- python/xy/_scene.py
- spec/api/chart-kind-contract.md
- js/src/40_gl.ts
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
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
New Features
ribbon,sankey, andsankey_chartpublic APIs.Bug Fixes
Documentation
Chores