Polar coordinate system: heatmap, sectors, log r, and error bars (protocol v12) - #370
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:
📝 WalkthroughWalkthroughPolar support now spans Python and pyplot APIs, payload validation, WebGL, SVG, raster, PDF, and native rendering. The change adds polar compositions, wedge-gap styling, seam-aware ticks, interaction updates, compatibility documentation, fixtures, regression tests, and Chromium smoke gates. ChangesPolar coordinate system
Estimated code review effort: 5 (Critical) | ~120 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
Performance Changes
Comparing 
Footnotes
|
Alek99
changed the title
Alek99
force-pushed
the
alek/polar-axes
branch
2 times, most recently
from
July 28, 2026 22:09
4d84ddd to
399c24e
Compare
Alek99
changed the title
Alek99
added a commit
that referenced
this pull request
Jul 29, 2026
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).
Contributor
Review: polar coordinate system (protocol v12)Overall: the architecture is genuinely strong and the PR mostly delivers what it claims, but it is not ready to leave draft. Four confirmed majors in the shipping render/export paths and three in the pyplot shim, all with repros below. None are structural — each has a localized fix, usually with the correct pattern already present in a sibling code path. The verification claims in the PR description held up: Major findings (confirmed with repros)1. Reversed radial axis makes all wedge marks vanish in both static exporters — 2. Native PDF export crashes for any polar chart with a hole, raised origin, or partial sector — 3. Bar/rect hover is not seam-aware — 4. Constant-radius data bypasses the centre-origin radial default — 5. Raster polar area skips the theta/NaN cull that SVG and the browser apply — 6–8. pyplot polar majors —
Interaction: pan and zoomFrom manual testing of this branch: pan and zoom do not work on polar charts. Squaring that with what the code says:
Minor findingsAPI ergonomics (all verified):
Precision/performance:
Duplication hazard: the error-bar joint radial clip is ~30 near-identical lines in Spec currency: §6's shader table says RECT_VS is not polar-capable while §7 and the actual shader say it is; the AREA_VS row lists "error bands," which are outside the allowlist; line-number citations into Test coverage gapsThe suites are extensive and honest (real browsers, non-tautological fixtures, cross-exporter pixel assertions), but the confirmed majors map exactly onto the gaps:
What's sound (verified, not just claimed)
RecommendationRequest changes. Fix majors 1–5 and the three pyplot majors (each is small and localized), resolve the pan/zoom question above, and add the missing tests that would have caught them (reversed-r static wedge, PDF-with-hole, seam hover, unpadded |
Member
Author
|
All eight majors reproduced exactly as described — every one is fixed in 8923f90, each with a regression test asserting the behaviour this review named. Point-by-point:
On "pan and zoom do not work" — you were right, and it's root-caused: polar disables pan/box/select, so its resolved default drag tool is The zoom precision floor is fixed as suggested (interval magnitude, not anchor magnitude — which also covers centre-anchored cartesian zoom on symmetric ranges). Sustained deep zoom toward r=0 ultimately needs §16-style radial re-centering; that's recorded in the deferred table rather than half-built. Spec currency: §6's RECT_VS row corrected (it is polar-capable), AREA_VS no longer lists error bands, the PDF hole/sector capability is documented, and §8 records the wheel-with-no-drag-tool rule. Deferred from this round, deliberately: the minors (θ-zero unit inconsistency, Suite: 3,608 passing, polar GLSL parity smoke green, 🤖 Generated with Claude Code |
Polar is one coordinate system, not a family of chart types — which is how both incumbents work: matplotlib has a PolarAxes projection that ordinary plot/scatter/bar/fill calls render into, and Plotly composes radar, spider and wind roses out of three polar traces. So this adds a coordinate system and lets the existing mark registry render through it. MARK_KINDS gains no entries and no _emit_<K> is rewritten. `xy.polar_chart(...)` sets a chart-level `coords: "polar"`; each mark's first channel becomes the angle and its second the radius, so `xy.line` and `xy.scatter` are reused verbatim. `xy.theta_axis(unit=, zero=, direction=)` and `xy.r_axis(...)` configure the two axes — sugar over the x/y axes rather than new axis ids, since four separate places require an id to start with 'x'/'y' and the interaction axis policies are built on that grammar. The (theta, r) -> pixel transform is specified normatively in spec/design/polar-axes.md §3 and implemented twice: once in GLSL (xyPolarPos) and once in Python (_svg._PolarProjection, which both exporters share). Prose does not bind those; tests/fixtures/polar_transform.json does. The fixtures are authored from the spec rather than generated from either implementation, and every case is checkable by inspection — theta=0 lands due right, zero="N" with clockwise puts 90 degrees due east. This is deliberately stronger than the existing tick-math arrangement, where 30_ticks.ts and its hand port in _svg.py are bound by nothing executable. Details worth knowing: - The y term differs by sign between the two implementations on purpose: screen space grows downward, GL clip space grows upward. A mirrored chart is otherwise entirely plausible-looking, so a fixture pins it. - All three point-family shaders are transformed, not just POINT_VS. Scatter silently switches to POINT_SIMPLE_VS on its fast path, and PICK_VS feeds the hover hit test — untransformed, the picture stays right while hover reports the wrong row. - _PolarProjection reports affine=False. Several emitters bake a straight-line data->pixel map into Rust behind `sx.affine and sy.affine`, which a polar chart on linear axes would otherwise satisfy while being non-affine. - Data lines are chords between projected points (Plotly semantics), which is what makes radar edges straight; grid rings and the frame are true arcs. SVG draws rings as <circle>, and the raster path flattens the same rings to polylines because its display list has no arc opcode. - Angular ticks need their own ladder: niceStep's [1, 2, 2.5, 5, 10] cannot reach 15/30/45/90, so degrees came out as 0/50/100/150. - The radial axis starts at the centre and the angular axis spans a full turn. An autoscaled radial axis puts the smallest datum at the centre; an autoscaled angular axis puts spokes at arbitrary angles. Scope is line and scatter. Every other kind is refused at build time with an error naming the supported set, rather than approximated — the rect, area, segment and mesh shaders expand geometry in pixel space after the coordinate map, so under polar they would draw chord-edged shapes where arcs belong. Polar traces ship tier="direct": M4 buckets on a monotonic screen-x column, which a spiral is not, and density binning in (theta, r) distorts by area near the origin. Protocol 10 -> 11: a v10 client ignores `coords` and draws the columns as cartesian x/y, so it must reject the payload rather than render a plausible wrong picture.
Extends the polar coordinate system to two more mark families, both of which Plotly composes rather than implements: radar/spider out of a filled Scatterpolar, and wind roses out of Barpolar. Area (and so radar): the quad interpolates in DATA space and projects the result, rather than interpolating already-projected clip coordinates, so the two radial edges run along true radii while the inner and outer edges come out as chords between projected corners — which is what makes a radar polygon's edges straight. `xy.radar_chart(categories, ...)` closes each series itself, because the seam is easy to get wrong: appending the first *angle* makes the closing segment sweep backwards through the whole circle, so the closing sample goes at a full turn instead. Spokes are labelled with the categories. Bars: a polar bar is an annular sector, which four corners cannot express. BAR_VS now sweeps a triangle strip of POLAR_BAR_SEGMENTS+1 vertex pairs across the bar's angular span; SVG draws the same wedge with real `A` arcs; the raster path flattens it, since its display list has no arc opcode. Subdivision is fixed rather than view-adaptive — bar counts are small, and a view-dependent count would have to be recorded per §28 rather than chosen silently. `xy.wind_rose(directions, speeds)` bins in Python (the arrangement `hist` already uses) and stacks polar bars, defaulting to the compass convention (zero="N", clockwise) that makes 90 degrees read as east. Band edges round to three significant figures, and the top edge rounds *up* so it still covers the fastest observation instead of putting "27.2197" in the legend next to "2.77". Two bugs this turned up, both found by comparing renderers rather than by tests: - The disc clip was reusing the id that also bounds every legend, so a legend sitting outside the circle vanished from the SVG while the raster still drew it. Marks now clip to the disc through a second id. - Angular tick labels called the angle formatter directly, so authored `tick_labels` — the category names on a radar chart — lost to pi notation. Both exporters now route through `_tick_text`, and `_fmt_axis` gained the angle branch its JS twin already had. Bars name their axis uniforms u_p/u_v rather than u_x/u_y, so `_drawBars` needed the polar uniform upload explicitly; without it a wind rose drew cartesian rectangles inside correct polar chrome. Also lands the start of pyplot `projection="polar"` — the Axes projection state, the PolarAxes method surface (set_theta_zero_location, set_theta_direction, set_rlim/rmin/rmax, set_rgrids, set_thetagrids), and `coords` flowing into the built chart. Sector limits raise NotImplementedError naming the spec rather than silently ignoring the call. Routing is not yet wired end to end.
Styling audit fixes, all found by exercising knobs against all three renderers with distinctive values: - The polar tick-label writers in both static exporters read only the chart-level `tick_label` slot, never the axis's own tick_label_color/tick_color. That silently disabled the `text=False` and `show=False` shorthands (which work by setting tick_label_color transparent) and any explicit per-axis label colour — while the browser client honoured them. Both exporters now apply the same axis-first precedence the cartesian labels use, per axis, so the theta and r labels style independently. - The plot rect kept the cartesian tick-label gutters (L=76 R=38 T=36 B=66 on a 400x400 chart), which exist to hold edge-hugging labels a polar chart does not have — its labels ring the disc. The disc sat off-centre (cx=219) and smaller than it needed to be (r=143). `_inset_polar_plot` now gives those gutters back symmetrically: radius 143 -> 167 on a 400x400 chart, centred. Reservations that still mean something keep their room — the title band, a colorbar's right gutter, and the left gutter when the radial axis has a title (which is drawn there and would otherwise land at x = -10, off the canvas). - The client re-cuts its rect identically (`_recutPolarPlot`), and bumps its `_topAxisRoom` the same way the exporters bump `top_axis_room` — without that the figure title anchored onto the topmost angular label. Regression tests pin the per-axis independence (ring colour vs spoke colour, theta text=False vs r text=False) in both SVG attributes and raster pixels.
tests/test_polar_transform.py and the fixture file both named scripts/polar_parity_smoke.py as the consumer that binds the client's GLSL to the shared fixtures — but the script did not exist, so the two-implementation contract was only half enforced: the Python projection was fixture-bound while xyPolarPos was verified by eye. The probe renders one single-point scatter trace per fixture sample, each in a unique saturated colour, reads the pixels back, and compares each colour's centroid to the fixture value within 1.5 px. Two details make it exact: - The fixture stores positions for its authored plot rect; the client computes its own. They reconcile without a third copy of the transform because a point's offset from the centre in units of the disc radius is rect-independent — pure arithmetic on fixture data rescales it to the runtime canvas. - Points sit at 60% of each fixture radius so no sprite clips at the canvas edge; a half-clipped disc's centroid shifts inward, which would read as a transform error. Under a linear radial scale the rescale stays exact. Scatter colour rides the channel dict (`t.color.color`), not `style.color` — the probe's first version set only the style and every point rendered in the palette fallback, which is worth a comment because hand-rolled specs will hit it again. Runs in the stdlib-only CI lane next to the other Chromium smokes.
CLAUDE.md treats a change as incomplete while its spec is stale, and three increments had outrun the design doc: the shader table still called AREA_VS and bars future, §7 still said line+scatter only, and the deferred table still listed area/radar and bars/wind rose. Also records the plot-rect re-cut, the POLAR_BAR_SEGMENTS contract, the radar full-turn closure rule, and the parity probe's name now that it exists. Roadmap items 18/27/29/32/34 move to their shipped statuses.
…nups Two parallel audits (styling/customizability across the three renderers, and structure/abstraction against the repo's conventions) plus a reported zoom problem. Findings were verified by reproducing each one before fixing. Interaction — the reported problem. Polar inherited the full cartesian gesture set, so a drag panned the theta range and rescaled the disc as if the chart were rectilinear, and wheel zoom anchored at the cursor's screen HEIGHT rather than its radius. Polar now resolves its own axis policy: pan disabled, zoom radial-only, and box-zoom/select/brush/crosshair off (a screen rectangle neither matches a (theta, r) region nor reads as one — polar-axes.md §8). The wheel anchor is the cursor's normalized radius, and xyPolarPos returns NaN below the radial minimum so zoom-in culls those points instead of reflecting them through the centre. Renderer parity (each was: one renderer honoured it, another silently did not): - A colormapped or size-channelled polar scatter took a SECOND Rust affine fast path — `affine_channel_points` — that projected (theta, r) as cartesian x/y: a diagonal line of points outside the frame ring. All six such gates now go through one `affine_fast_path(sx, sy, polar)` predicate rather than a `polar is None` conjunct repeated per site, which is how this one was missed. - Cartesian edge tick marks leaked into polar in SVG and the client (the raster drew none). Guarded in both; tick_length/width/direction are documented as ignored under polar rather than half-drawn. - `curve="smooth"` was honoured by the client and skipped by both exporters — two visibly different shapes from one chart. The client now chords too, for the reason the exporters already did. - A constant `base=` on polar bars was dropped by the client (full pie slices from the centre) because the cartesian baseline uniform is clip-space. Bars now carry a data-space baseline uniform. - PDF export of ANY polar chart raised: the converter's clip subset was rect-only. It now accepts a single <circle> clip, emitted as four Bezier quarter-arcs, and tolerates inert data-* markers. - `tick_label_strategy="off"` and `tick_label_angle` were ignored by the polar label writers in all three renderers. - radar_chart silently replaced its category spokes with numeric angles as soon as any theta_axis child was supplied; an authored axis now merges. - The theta-axis title was drawn below the canvas edge because the rect re-cut reclaimed the bottom gutter it lives in. Structure: - Restore @cached_measurements to render_raster. Inserting a helper above it had silently transplanted the decorator onto that helper, costing every raster export its text-measurement cache. - The polar label placement was copied between the two exporters despite a docstring claiming otherwise. Extracted `polar_tick_label_layout` in _svg.py returning renderer-neutral placements; each exporter keeps only its sink. - Hoisted the GLSL cartesian/polar dispatch, pasted verbatim into four shaders, into POLAR_XYPOS_GLSL. AREA_VS/BAR_VS stay separate on purpose — they interpolate in data space before projecting. - Fixed a dead guard in the rect re-cut (clamping before testing made the small-chart escape unreachable), renamed _inset_polar_plot to _recut_polar_plot to match its client twin, hoisted the client's duplicated rlabel/gap/compass constants, and corrected mirror comments that named symbols which do not exist (xyPolar, THETA_ZERO "in 30_ticks.ts"). - POLAR_DIRECT_CEILING was a constant the spec advertised and nothing enforced; polar traces past it now refuse with the reason. - radar_chart's `fill=` was a documented no-op: it now rebuilds area children as line outlines. Non-area/line marks and column-name values raise instead of failing anonymously inside numpy. Dropped a no-op setdefault in wind_rose and a docstring frozen at the line+scatter increment.
Interactive testing and the styling audit's layout dimension caught four more:
- Zooming in drew marks past the outer ring into the rect corners (the GL
canvas is the plot RECT; the disc clip only existed in the SVG exporter).
xyPolarPos now culls rn > 1 the same way it culls rn < 0 — NaN position,
which is the client's equivalent of the exporters' disc clipPath. Verified
by pixel readback: zoomed to [0.59, 0.84], zero lit pixels outside the ring.
- A horizontal colorbar hangs off the plot's bottom edge, so the rect re-cut
extending the plot downward pushed it clean off the canvas. The bottom band
is kept whole when a horizontal colorbar (or a theta title) claims it, in
both the exporters and the client.
- The re-cut's small-chart escape fell back to the cartesian rect, whose own
40px floor can exceed a tiny canvas — an 80x80 chart drew its circle out to
x=86. Too-small charts now take the largest centred box the canvas itself
allows. (Also fixes the guard that clamped before testing, making the escape
unreachable.)
- The angular label allowance was a fixed 30px, so authored radar category
names ("EAST-NORTH-EAST") were hard-clipped at the canvas edge. The room is
now measured from the widest authored label, capped so a pathological label
shrinks the disc rather than erasing it. Generated angle text keeps the
floor. Mirrored in the client.
Also finishes the pyplot polar routing: set_theta_zero_location /
set_theta_direction / set_theta_offset collected into _polar_options which
nothing read — the write-only state the structure review flagged. The options
now land on the built figure's theta axis, so
subplot(projection="polar") + compass conventions render correctly end to end.
Interactive testing found the NaN cull was the wrong tool for filled marks. Culling is right for a point or a line vertex — there is no honest position outside the range — but a fill or a bar has an EXTENT, and its visible extent at a given angle is [base, top] intersected with [r_lo, r_hi]. Culling on one out-of-range endpoint threw the whole primitive away: - a radar polygon vanished the moment radial zoom lifted r_lo above its baseline, because every quad's inner corner went NaN; - a wind-rose bar disappeared whole as soon as its tip crossed the outer ring, rather than clipping at the ring the way matplotlib and Plotly do. AREA_VS and BAR_VS now clamp their radial span into the visible annulus, and a span entirely outside collapses to zero and draws nothing. Both static exporters clamp identically — the wedge builders bound outer and inner radius (the raster has no disc clip to save it), and the area emitters clamp their r columns before projecting, which also stops a below-minimum baseline mirroring through the centre INSIDE the disc where the SVG clip cannot catch it. Radial zoom now anchors at the CENTRE rather than the cursor's radius. The cursor anchor was the natural reading of "anchor where you point", but on a disc it lifts r_lo and carves a hole in the middle — an annulus view that reads as broken rather than as zoom, and only looked right when the pointer happened to be dead centre. Scaling the maximum about a fixed minimum is Plotly's radial semantics and stays legible from any cursor position. Applied at _zoomAt, so the wheel, the modebar buttons and axis-band gestures agree.
…ions Probing customizability by rebuilding four ECharts-style donut/gauge designs (evilcharts.com pie blocks) turned up the two things that stood between the polar system and a pie chart. Unequal angular widths. A bar's width may vary per bar; equal widths take the compact path (one scalar width) while unequal ones ship four edge columns — which under polar ARE an annular sector: (x0, x1) is the angular span and (y0, y1) the radial one. That path was not polar-capable, so a donut drew as cartesian rectangles inside polar chrome. RECT_VS now sweeps a sector exactly as BAR_VS does, and both exporters build the same wedge (SVG real arcs, raster flattened). This is what makes pie/donut a composition rather than a chart type: a slice is one bar carrying its own width. Point-anchored annotations. Centre text is `(any angle, r = 0)`, and the separable scales read that as the bottom-left corner — a donut's centre label landed outside the disc. `text`, `label`, `marker` and `arrow` now project jointly through the transform in both exporters. `rule` and `band` deliberately do not: a theta rule is a spoke, an r rule is a ring, a band is an annulus or a sector, and drawing them as straight cartesian bars would be a wrong picture rather than a missing one. Recorded in the spec's deferred table along with sector layout — a gauge drawn as a partial arc still gets a full-circle plot rect, so the unused portion is dead space. All four reference blocks now reproduce: a 6-slice donut with a 52% hole, 3° padding and centre text; dotted progress rings (40 sectors, 85-92% band); a revenue donut with a 62% hole and right-hand legend; and a -30°..210° gauge with four colour bands. Build script and a side-by-side page live outside the repo, under /tmp/evil.
CI's ruff format gate caught tests/test_polar_charts.py: the last test block was appended after the formatting pass, and only ruff check ran before the commit. No behaviour change.
…/arc fixes Review fixes for three confirmed defects: - Out-of-range radial data was neither culled nor clamped by the exporters' line and scatter paths. Below r_lo a point normalizes negative and mirrors through the centre to a position INSIDE the disc — no clip can hide it — and above r_hi the raster path (which has no disc clip) drew past the outer ring. The client shader NaN-culls both. _PolarProjection.visible_mask now applies the same predicate (same 1e-6 epsilon) in both exporters: scatter drops the rows, lines split into visible runs so a chord with a culled endpoint is dropped whole in every renderer. Spec §8 updated — it previously claimed the exports clip at the ring, which only SVG's above-range half actually delivered — and now also records the fill/bar clamp semantics it never stated. - A full-turn sector (a 100% donut slice) rendered as nothing in SVG: the A arc's endpoints coincide and SVG omits such segments entirely. Spans of a full turn or more now draw each circle as two half-turn arcs, the inner ring wound oppositely so nonzero fill keeps the hole open. The raster polygon and the BAR_VS sweep were already correct. - _fmt_angle/fmtAngle hardcoded degree precision to a step of 1, so an authored 22.5-degree grid labelled itself 22deg/68deg (round-half-even). The tick step now threads through from fmtAxis/_fmt_axis on both sides.
Radial bar edges rendered jagged in the client: the GL context runs with antialias: false, so every smooth edge is fragment-shader coverage — and the polar wedge branch switched the rect SDF off (v_half = 1e6), leaving all four edges of every wedge hard-aliased. Wide slices additionally showed the 24-segment arc flattening as visible facets. POLAR_WEDGE_GLSL now places the strip vertices XY_POLAR_AA px outside the true sector (computed from the same uniforms as xyPolarPos, in pixel form, because xyPolarPos's rn > 1 cull would eat the expanded outer vertices), and RECT_FS trims the expansion back against the true annular-sector SDF in device px. The fringe gets room to ramp on both sides of each edge, and because the expanded chords stay outside the true outer arc the trimmed arc is exactly round rather than faceted. Collapsed spans (radial clamp, zero width) cull explicitly — the expansion would otherwise leave a ghost sliver where nothing drew before. A full turn skips angular expansion and angular coverage: its two ends are one seam, not edges. POLAR_BAR_SEGMENTS rises 24 -> 96, sized so a full-turn wedge's chord sagitta stays inside the expansion up to a ~1400-device-px disc; the raster flattens the same count through its coverage-scanline fill, and SVG's real arcs never needed one. Verified: polar GLSL parity smoke green, full suite 3,541 passing, and pixel-level headless-Chrome crops of a wind rose, a 100% donut ring and a 90-degree slice at dpr 1 and 2; cartesian bars (shared RECT_FS) unchanged. render_smoke_nonumpy times out on this machine on the unmodified HEAD bundle too — pre-existing local SwiftShader issue, covered by CI.
…e blocks Rebuilding evilcharts' four ECharts pie blocks (market-share donut, dotted progress rings, gradient revenue donut, banded gauge) as a customizability probe surfaced four defects. All four were silent — every block built and exported without a warning. - The client projected NO annotation through the polar transform: js/src/51_annotations.ts had zero polar references and routed every kind through the separable _dataPxX/_dataPxY. Both exporters place them correctly, so the browser strung a donut's slice labels out in a horizontal row in theta order and dropped centre text at the left edge. This is the divergence the coordinate system exists to prevent, and the fix had landed only on the export side. A new _dataPxPoint mirrors the exporters' point() helper for text/label/marker/arrow/callout and the authored-scatter glyph pass; rule/band stay cartesian and deferred, as they are in Python. - `padding=` was discarded under polar. _recut_polar_plot symmetrised the authored gutters away and gave the disc the whole canvas, so the band every donut composition reserves for its legend or caption vanished — measured on one 400x420 chart, cartesian moved the plot bottom 384 -> 280 while polar moved 383 -> 380. An authored box is now only inset by the label room. - A gradient `fill=` reached the SVG and the browser but the raster painted it flat: the polar branches called cmd.fill(poly, flat) and never consulted style["fill"] the way the cartesian path does. - `corner_radius` was accepted, shipped on the wire as style.corner_radius, and ignored by all three renderers — a silent approximation. It is now defined in the unrolled (arc, radial) frame, where a wedge is a rectangle and the standard rounded-rect profile applies: the client evaluates it in the annular-sector SDF, the exporters sample the same profile through _rounded_wedge_points. Plain wedges keep their exact A arcs. Three of the four blocks depend on this (6px dots, 12px slices, 10px bands). Wedge strokes in the client fall out of the same SDF, which now carries the stroke ring the rectangle path always had. Suite 3,547 passing, polar GLSL parity smoke green, ruff/ty clean. Remaining from the probe and unchanged here: sector layout (a 240-degree gauge still gets a full-circle rect), polar rule/band, and polar select.
`xy.radar_chart(..., fill=False)` — the documented switch for outline radars, and the whole "Lines Variant" family in the competitor gallery — raised `KeyError: 'width'` for every input. Swapping only `kind` from "area" to "line" handed `_apply_line` an area's prop dict, and the two marks do not share a vocabulary: an area carries line_color/line_width/line_opacity where a line carries color/width/opacity. `_radar_outline` now translates them, falling back to the fill colour when the stroke was never given one. The existing `test_radar_fill_false_outlines_instead_of_filling` passed throughout because it asserts on the child mark kinds and never builds the figure, so the crash sat one `.figure()` call beyond its reach. Found by rebuilding the evilcharts ECharts radar blocks. The raster marker/arrow/callout half of this fix landed independently in 3d41a74. Suite 3,547 passing, ruff/ty clean.
Hovering a donut slice reported "x: 102.6, y: 0.94" for a slice whose whole identity is "Cloudpeak $13B". Three separate reasons, all fixed here. - The default readout never showed the series name. The hover row carries `trace` (an id) and \_defaultTooltipItems emitted only x/y/color/size, so a mark was identified purely by its coordinates. It now leads with the trace's name when it has one, which is what every comparable library does and the only label that means anything on a pie. - Under polar the channels were still called x and y. They are now theta and r (an explicit `labels=` override still wins). - Angular values were raw numbers: "x: 1.5708" on a radar spoke the chart itself labels "power", and no degree sign anywhere. The angular value now goes through the axis's own text function, so degrees read "338deg", radians read as pi-fractions, and an authored tick label wins outright. Authored labels match with a tolerance of an eighth of the spoke spacing: the hovered angle arrives as decoded offset-encoded f32 while the tick was authored in f64, so pi/2 missed its own label by ~1e-7 and fell back to a number. Verified by hovering real charts in headless Chrome across polar line, scatter, area, polar bar, radar and a six-slice donut; cartesian readouts are unchanged apart from now naming their series. Suite 3,597 passing, polar GLSL parity smoke green, ruff clean.
CodSpeed flagged test_png_export_line_pyplot at -16.84% (34 -> 40.9 ms sim) on this branch — a cartesian benchmark regressed by polar work. The annular- sector clip (OP_POLAR_CLIP) was applied inside Canvas::blend_u8 and Surface::blend_u8 by calling apply_polar_clip(self.polar_clip, ..), which passes the ~40-byte Option<PolarClip> BY VALUE — one struct copy per blended pixel, for every mark on every chart, polar or not. Local interleaved A/B on the benchmark's own workload (100k-point pyplot line, PNG): main 1.58-1.62 ms, branch 1.89-1.92 ms (+18%, matching the sim number), across six alternating-order rounds. The hot path now tests only the Option discriminant — one byte load and a branch — and the clipped blend is outlined behind #[cold]/#[inline(never)], so only pixels painted while a polar clip is actually active take the call. Fixed dylib: 1.61-1.67 ms on the same interleaved harness, recovering ~90% of the regression; the residual byte-test is the price of the clip existing at all short of monomorphizing every painter loop. Output is byte-identical before/after on both a cartesian export and a polar wedge chart (sha256-verified), the Rust polar-clip parity tests pass, and the Python suite is 3,597 passing. Diagnosis note for the next reader: the branch adds only ~1% Python calls to this benchmark (5,828 -> 5,884 profiled calls); the regression was entirely native. A local checkout that had an unpushed emit_tick_labels optimization masqueraded as "main" during the first bisect — measure baselines from origin/main, not from whatever a sibling worktree happens to have.
Three confirmed polar defects from the third audit pass. Polar bar update transitions rendered a hybrid matching neither old nor new data. BAR_VS mixes the transition into p/v0/v1 in clip space, but its polar branch needs data space and re-derives theta and both radii from the raw attributes, so only the scalar half-width survived: the wedge jumped to its destination angle on frame 0 while its angular width animated. polar-axes.md already defers polar animation, so the deferral is now guarded like its siblings — _prepareBarPositionInterpolation bails under polar and records "snap:polar-unsupported". Polar scatter and line interpolate correctly (they mix before projecting) and entrance grow still honours u_animationProgress. Negative radial autorange threw the pad away. min(0.0, lo) collapses to lo once the data goes negative, so four readings within 0.7% of each other resolved to [-100.8, -100.1] and drew as a full-disc star — the exact picture the adjacent comment says the branch exists to forbid. Centre origin is only meaningful when zero ends the range; below zero it is vacuous, so the ordinary padded extent stands. Non-negative data is byte-identical. Radial tick labels overlapped. They march along a 22.5-degree spoke, so their usable run is the annulus width projected onto it — about a fifth of the plot — while the tick request was sized off full plot height, and the polar path skips the collision pass that would thin them. Measured 6 overlapping pairs at 390px and 10 at 700px, now 0 at both. Note the first attempt thinned the tick list itself, which also thinned the GRID: rings and labels share one list, and a 520px disc dropped from three rings to two. Ring density stays tied to the plot; only the labels are strided. Also corrects the audit's framing: this is not a 390px effect — 700px was worse. Suite 3,651 passing (+4), hooks and ruff clean.
get_theta_offset() read "S" out of the render tables, which spell it -pi/2. Matplotlib maps the compass to 0..2pi counter-clockwise from east, so it returns 3*pi/2 — the same angle, but a compat getter has to return matplotlib's number: `get_theta_offset() > 0` and a round-trip through set_theta_offset both break on the negative. Verified against matplotlib 3.11.0 directly; all eight compass points now agree to 1e-9. The render tables keep -pi/2. They are angle math, where the two are interchangeable, and they are mirrored across THETA_ZERO in _svg.py and 50_chartview.ts. Also normalizes an explicitly-authored radian offset into the same range.
Nine reported failures across the polar surface and the responsive chrome it shares with every other chart. **P1 — repeated data updates leaked GPU buffers.** Trace teardown walked a hand-kept list of geometry buffer names, so every rebuilt trace (a state-driven update, an append that could not patch in place, an animated spec swap) orphaned its style, direct-RGBA colour, stroke, corner-radius, LOD-blend and dashed-line-length buffers. All three teardown paths now read one shared TRACE_GPU_BUFFERS list, pinned against the build paths by a test so a new channel cannot reintroduce the leak. **P1/P2 — mobile Wind Rose chrome.** The browser wraps a long title while layout measured one line, so a compact title lost ~10 px off the canvas top. Titles now wrap at one shared width in all three renderers and the browser caps the element at that same width; single-line titles are byte-identical. A polar legend now reserves a gutter beside the disc — 96 px on the loc's side, or a 64 px band beneath at compact widths — instead of covering the north-east sectors and the outer radial label. An authored anchor or four-tuple padding still wins. **P2 — wedge vertex cost.** Subdivision is span-proportional: clamp(ceil(96 * |span| / turn), 2, 96) over the *authored* angular width, in every renderer. Sagitta is quadratic in the per-segment angle, so this holds the flattening bound while a 16-sector wind-rose bar costs 14 vertices instead of 194; a full turn still uses 96. **P2 — inert polar axis options.** theta_axis/r_axis refuse minor_tick_values, minor_style, tick_label_min_gap, tick_label_anchor and the collision spellings of tick_label_strategy, each naming the control that does work. pyplot's projection="polar" drops the same keywords instead, because every matplotlib Axes carries an rcParam minor style it never authored; recorded in the compat spec. theta_axis(format=) now wins over the built-in degree text, and an r_axis(margin=) is honoured instead of discarded. **P2 — radial time axes.** A time radius autoranges from its data instead of epoch zero, which had squeezed every modern instant into a hairline ring at the rim. The signed-radius contract (a position, never a mirrored direction) is now normative in the spec and in the r_axis docstring. **P2 — compact colorbars.** They keep their two extreme tick labels and the scale title; only the interior ladder and the text-free minor ticks drop. Hiding everything left an unlabelled gradient. **P2 — redundant frames.** Data animations use the view animation's 80 ms label cadence instead of rebuilding the whole tick-label DOM every frame, and force one settled rebuild at the end. A DPR change coalesces into a single resize frame and rescales the per-instance widths and corner radii that are baked in device pixels. **P2 — pie_chart.** Listed in the generated chart-factory API reference with the other polar compositions. A zero-width bar is now legal and draws nothing, like line_width=0, so a 0% progress ring or an empty aggregated category no longer dies with "bar width must be positive".
Alek99
added a commit
that referenced
this pull request
Jul 30, 2026
CodSpeed on #370 reported "103 untouched benchmarks" for a change that added a whole coordinate system, rewrote wedge geometry in three renderers, and shipped the most expensive mark in the engine. Nothing in the suite could see any of it, which is why a ~50k-polar-bar performance cliff was found by hand instead. `benchmarks/test_codspeed_polar.py` — six rows, all Python cost (what simulation mode measures): - payload prep for the three shapes with materially different validation and emit paths: a 100k polar line, a 16-sector / 50k-observation wind rose (Python-side binning plus stacked wedges), and a 24-slice pie, whose unequal widths take the four-edge column path rather than the compact scalar-width one; - SVG and native-PNG export of the same rose, bracketing the arc-flattening term. SVG draws real `A` arcs and needs no subdivision count, so it is the control; PNG flattens every wedge at `config.polar_bar_segments(span, turn)` vertices, so a regression back to a flat full-turn count lands here as an arc-flattening step change rather than as a bug report; - a polar heatmap's bounded screen-space inverse raster, which has no Cartesian twin. The payload rows assert bounds rather than sizes — the rose's bytes must stay bounded by sector and band count, never by observation count — so a row cannot get cheaper by shipping a different chart. The report's other item was 2 skipped rows falling back to baseline results. Those are orphans in the dashboard, not skipped tests: the suite collects exactly 103 rows and CodSpeed measured 103, so the 2 extra rows have no code behind them and need archiving there by hand. What the repo can do is stop it recurring — `test_codspeed_row_count_matches_the_methodology_spec` collects the modules by AST and gates the total against `spec/benchmarks/methodology.md` §8, whose count was itself already one row stale. Deleting a benchmark stays allowed; deleting one silently does not. Also adds a `polar_coordinate_system` benchmark category, documents the module in §8 and `benchmarks/README.md`, and repoints the triangle-mesh cleanup assertion at the shared `TRACE_GPU_BUFFERS` list the buffer names moved into.
Member
Author
Addressing the CodSpeed reportBoth items from the CodSpeed comment are handled in #380 (stacked on this branch). "Merging this PR will not alter performance — 103 untouched benchmarks." That is the finding, not the reassurance: this PR adds a coordinate system, rewrites wedge geometry in three renderers, and ships the most expensive mark in the engine (an annular sector per bar rather than a quad), and the suite has no row anywhere near it. That blind spot is why the ~50k-polar-bar cliff was found by hand.
The payload rows assert bounds, not sizes, so a row cannot get cheaper by shipping a different chart. "2 skipped benchmarks — baseline results were used instead." These are orphan rows in the dashboard, not skipped tests. The suite collects exactly 103 rows (100 functions, two parametrized: +1 and +2), and CodSpeed measured 103 — so the 2 extra rows in the report have no code behind them and need archiving in the dashboard by hand. I can't do that from here; it's one click on the skipped filter. What the repo can do is stop it recurring. Also in #380: a Not verifiable from my sandbox: it has no egress to PyPI/npm/crates.io, so the native core and the benchmark deps cannot be installed and I could not execute the new rows. The row-count arithmetic and the AST collection logic were checked directly (103 → 109 with the new module, matching the spec); the CodSpeed job itself is the first real run. Generated by Claude Code |
Contributor
|
On pie chart the legend overflow horizontally becomes scrollable . It is repeating percentage text. 
|
Contributor
|
Created a followup for the mpl shim that is showing legend. |
Alek99
added a commit
that referenced
this pull request
Jul 30, 2026
Two defects reported against the pie chart on #370, with one shared cause. **The legend scrolled horizontally.** The box is capped at `--xy-legend-max-width`, but its grid columns were `max-content` and so refused to shrink below their content: an over-wide row overflowed and `overflow:auto` answered with a sideways scrollbar, hiding the label it was meant to be showing. Columns are now `minmax(0, max-content)` and the box scrolls on the block axis only. Overflow on the inline axis ellipsizes per row instead, which is what the static exporters already do — and the full text stays reachable in `title`/ARIA, the same rule categorical tick labels use. Only the label clips: the swatch is `flex:none` and keeps `overflow:visible`, so an authored oversized marker still draws outside its 18x14 box. **The legend repeated the percentage.** `pie_chart` printed the value and the share, and for values that already sum to 100 — how most pie data arrives — those are the same digits: `[40, 30, 20, 10]` rendered `Direct 40 (40%)`. The share keeps the unit, so the bare value is dropped when it says nothing new. Decided once for the whole pie rather than per slice, because a legend where one row carries a bare value and the next does not is harder to read than either consistent shape; zero slices draw no wedge and get no row, so they cannot veto it. The doubled label was also what overflowed the box, so the two reports share a root cause. The polar legend gutter added earlier in this branch was a flat 96 px, which ellipsized `Partner (30%)` — an ordinary slice's default name — while being a fifth of a phone canvas and a fifteenth of a wide one. It is now 22% of the canvas width clamped to 120-200 px: still derived from the canvas rather than measured from the label set, so all three renderers reserve the identical box instead of drifting with their font metrics, and `floor` rather than `round` so Python and JavaScript land on the same integer pixel. `legend_item` and `legend_label` join the `:where()` chrome layer, so an author's utility class still wins, and `test_static_client_security.py` now requires both rules to stay defeatable.
Alek99
added a commit
that referenced
this pull request
Jul 30, 2026
CodSpeed on #370 reported "103 untouched benchmarks" for a change that added a whole coordinate system, rewrote wedge geometry in three renderers, and shipped the most expensive mark in the engine. Nothing in the suite could see any of it, which is why a ~50k-polar-bar performance cliff was found by hand instead. `benchmarks/test_codspeed_polar.py` — six rows, all Python cost (what simulation mode measures): - payload prep for the three shapes with materially different validation and emit paths: a 100k polar line, a 16-sector / 50k-observation wind rose (Python-side binning plus stacked wedges), and a 24-slice pie, whose unequal widths take the four-edge column path rather than the compact scalar-width one; - SVG and native-PNG export of the same rose, bracketing the arc-flattening term. SVG draws real `A` arcs and needs no subdivision count, so it is the control; PNG flattens every wedge at `config.polar_bar_segments(span, turn)` vertices, so a regression back to a flat full-turn count lands here as an arc-flattening step change rather than as a bug report; - a polar heatmap's bounded screen-space inverse raster, which has no Cartesian twin. The payload rows assert bounds rather than sizes — the rose's bytes must stay bounded by sector and band count, never by observation count — so a row cannot get cheaper by shipping a different chart. The report's other item was 2 skipped rows falling back to baseline results. Those are orphans in the dashboard, not skipped tests: the suite collects exactly 103 rows and CodSpeed measured 103, so the 2 extra rows have no code behind them and need archiving there by hand. What the repo can do is stop it recurring — `test_codspeed_row_count_matches_the_methodology_spec` collects the modules by AST and gates the total against `spec/benchmarks/methodology.md` §8, whose count was itself already one row stale. Deleting a benchmark stays allowed; deleting one silently does not. Also adds a `polar_coordinate_system` benchmark category, documents the module in §8 and `benchmarks/README.md`, and repoints the triangle-mesh cleanup assertion at the shared `TRACE_GPU_BUFFERS` list the buffer names moved into.
Alek99
added a commit
that referenced
this pull request
Jul 30, 2026
Two defects reported against the pie chart on #370, with one shared cause. **The legend scrolled horizontally.** The box is capped at `--xy-legend-max-width`, but its grid columns were `max-content` and so refused to shrink below their content: an over-wide row overflowed and `overflow:auto` answered with a sideways scrollbar, hiding the label it was meant to be showing. Columns are now `minmax(0, max-content)` and the box scrolls on the block axis only. Overflow on the inline axis ellipsizes per row instead, which is what the static exporters already do — and the full text stays reachable in `title`/ARIA, the same rule categorical tick labels use. Only the label clips: the swatch is `flex:none` and keeps `overflow:visible`, so an authored oversized marker still draws outside its 18x14 box. **The legend repeated the percentage.** `pie_chart` printed the value and the share, and for values that already sum to 100 — how most pie data arrives — those are the same digits: `[40, 30, 20, 10]` rendered `Direct 40 (40%)`. The share keeps the unit, so the bare value is dropped when it says nothing new. Decided once for the whole pie rather than per slice, because a legend where one row carries a bare value and the next does not is harder to read than either consistent shape; zero slices draw no wedge and get no row, so they cannot veto it. The doubled label was also what overflowed the box, so the two reports share a root cause. The polar legend gutter added earlier in this branch was a flat 96 px, which ellipsized `Partner (30%)` — an ordinary slice's default name — while being a fifth of a phone canvas and a fifteenth of a wide one. It is now 22% of the canvas width clamped to 120-200 px: still derived from the canvas rather than measured from the label set, so all three renderers reserve the identical box instead of drifting with their font metrics, and `floor` rather than `round` so Python and JavaScript land on the same integer pixel. `legend_item` and `legend_label` join the `:where()` chrome layer, so an author's utility class still wins, and `test_static_client_security.py` now requires both rules to stay defeatable.
Member
Author
Pie legend, and this branch is red from my merge — both fixed in #387Heads up first:
That last test's I also reverted the DPR-change deferral from #380. The reported pie legend issues
Both reproduce, and they share a cause. Sideways scrollbar. The legend box is capped at Repeated percentage. xy.pie_chart(["Direct", "Partner", "Organic", "Other"], [40, 30, 20, 10])
# before: "Direct 40 (40%)"
# after: "Direct (40%)"The share keeps the unit, so the bare value is dropped when it says nothing new. Decided once for the whole pie rather than per slice — a legend where one row carries a bare value and the next doesn't is harder to read than either consistent shape — and zero slices draw no wedge, so they can't veto it. Counts still show both ( That doubled label was also what overflowed the box, so the two reports are one root cause plus one layout bug. I could not execute the suite — this sandbox has no egress to PyPI/npm/crates.io, so the native core and client bundle can't be built. Each fix was verified arithmetically instead (the two truncations reproduced through Generated by Claude Code |
…ie legend (#387) Restores alek/polar-axes to green and adds polar benchmark coverage. Fixes the five test failures the merged polar audit round introduced: the triangle-mesh buffer-cleanup assertion, the compact-colorbar plot width, the two SVG legend truncations, and the pyplot import-line check. - Widen the polar legend gutter to 22% of the canvas width, clamped to 120-200 px and floored so all three renderers land on the same integer pixel. - Restack the compact colorbar's two extreme ticks above and below the gradient, so the reservation stays at GAP + THICKNESS + 8 and the plot keeps its width. - Type the colorbar tick node that carries the stashed beside-bar CSS, so tsc accepts it and js/build.mjs can emit a bundle again. - Skip the DPR rescale for a trace whose CPU style/radius mirror no longer spans every row on the GPU, which a streaming tail append leaves behind; the existing append-time rebuild renormalizes those instead. - Fix the pie legend's sideways scrollbar with shrinkable grid columns, and stop pie_chart printing the value and an identical share. - Add benchmarks/test_codspeed_polar.py (6 rows) and the polar_coordinate_system category, taking the CodSpeed suite to 109 rows, with a test that pins the count to the methodology spec. - Raise the widest render smoke's chromium timeout, which sat 22 s from its cap.
The polar phase 6/7 CI step failed on symlog_origin: the native PNG had zero #0f766e pixels where it wanted at least ten. The teal in that case is a single scatter marker at 0.8 opacity, which never matched the exact-colour probe; the pixels the probe had always been counting were the legend swatch, and the legend had disappeared from the raster. Both exporters clip their legend to a rect so an oversized one ellipsizes instead of escaping the file — static files have no scrollbar to fall back on. The polar legend gutter, however, is reserved OUTSIDE the plot rect by construction, so bounding the legend to the plot rect does not shrink it, it deletes it. The SVG emitter already unioned the two boxes; the raster still clipped to the plot alone, so every polar figure with a legend exported a PNG without one while its SVG kept one. That went unnoticed because the gutter only arrived with #380 — before it, the legend sat inside the plot rect. The union is now `legend_clip_rect`, read by the SVG clipPath and the raster clip command alike, so the two cannot drift again. SVG output is byte-identical. Pinned three ways: the rect covers the gutter, a cartesian rect is still exactly the plot, and the PNG pixels carry the swatch — the last is the one that fails on the old code. Suite 3,706 passing (+3), hooks and ruff clean, and the full polar phase 6/7 smoke (browser, SVG and PNG) is green on all six cases.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 67 files
Not reviewed (too large): python/xy/_svg.py (~1,944 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
…ape hatch Five findings from the cubic review, each reproduced first. Polar paths kept the caller's sequence. Theta is the order marks are JOINED in, not a domain to be scanned, but Figure.line/area/error_band sorted x at ingest to satisfy the M4 precondition. An authored track of [350, 10, 30, 5] shipped as [5, 10, 30, 350] with the radii permuted to match, so a path crossing the 0/turn seam or doubling back was redrawn as an ascending-angle fan. Skipping the sort is safe under polar because polar forces tier="direct" — config.py already says as much: "M4 decimation buckets on a monotonic screen-x column, which a spiral is not". Cartesian still sorts. wind_rose dropped observations above an authored top edge. The default path rounds its top edge UP precisely so it covers the fastest observation; authored edges were taken as given, so speed_bins=[10, 20] counted 2 of 3 observations when one blew at 25 and the rose under-reported its own input with no warning. Non-covering and non-finite edges are now refused. The declarative drag escape hatch was broken by my own earlier fix. Freeing polar's wheel zoom gated on _dragModeUserSet, which only the modebar sets, so interaction_config(default_drag_action="none") — the documented opt-out that releases page scroll for embedded charts — still wheel-zoomed AND still called preventDefault, trapping the page. Both gates now ask whether `none` was REQUESTED, in either spelling. Probed: declared "none" no longer zooms and no longer prevents default; polar radial zoom and the modebar behaviour are unchanged (34 client tests). polar_bar_segments now mirrors the client's finite check on span. math.ceil raised ValueError on NaN and OverflowError on infinity where JS falls back to the full-turn count, so the renderers disagreed about a degenerate wedge — one drew it, the other crashed. The seven polar names are in the TYPE_CHECKING block, so `from xy import polar_chart` no longer resolves to Any and loses its signature. Suite 3,712 passing (+61), hooks and ruff clean.
Zero is the Unix epoch on a time axis, not a centre. Pinning a time radius there spanned the disc from 1970 to the data and parked every ring on the rim; a singleton instant was the worst case, resolving to [0.0, 1767225600000.0]. Time now takes the ordinary padded window, byte-identical to what the cartesian axis resolves for the same column. Both polar radial branches needed the guard — the singleton early return has its own copy of the centre-origin rule, so fixing only the general branch left the worst case untouched. Numeric radii, including constant and negative, are unchanged.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The client half of both fixes was sitting uncommitted; the Python twins were never made, so each was a one-renderer fix — the divergence this coordinate system keeps having to be defended against. Both reproduce in the exporters: - `_polar_label_room` measured only `tick_labels`, but a category axis carries its names in `categories` (`axis_ticks` hands those straight to `_category_ticks`) and has no `tick_labels` at all. Long names therefore fell back to the uniform default and spilled over the disc. `radar_chart` hid this because it authors `tick_labels` explicitly; a categorical `polar_bar_chart` measured 30 px where it needed 90. - `_recut_polar_plot` returned early when the angular tick labels were hidden, which skipped `_polar_legend_reserve` as well as the label inset. The legend then fell back to the plain plot rect and drew over the marks, and the disc kept the cartesian gutters the recut exists to give back. Hiding the labels removes the LABEL inset only; track the flag and skip just that. Tests pin both, including the client source strings, so the two halves cannot drift apart again.
Alek99
added a commit
that referenced
this pull request
Jul 30, 2026
* Sankey: ribbon primitive + layout engine (protocol v11) 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). * Improve Sankey docs and interactions * Sankey/ribbon review fixes: axis modes, outlines, alpha ramps, alignments 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. * Ribbon outlines: match-fill per band, close the band's end faces 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. * Ribbon review batch: transformed-space cubic, resolved paints, scalar 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. * Fix ribbon hover and raster regression probes
`POLAR_MARK_KINDS` admits `bar`, and the compact bar payload happily serialized `orientation="horizontal"` for a polar trace — but every renderer reads the position column as theta and the value column as radius regardless, so the bar came out transposed rather than rotated. A polar bar's position IS its angle and its value IS its radius, so the orientation is not a free choice; refuse it rather than draw a plausible wrong picture (§28). Caught by review. Also corrects the §3 property count in the polar spec, which said "Five" over eight bullets after later increments added to the list.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
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
2 participants
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.
Completes a reusable polar coordinate system and renders existing xy marks through it across the browser, SVG, and native raster paths.
Why a coordinate system, not chart types
Radar, wind rose, pie/donut, gauges, and mixed radial views are compositions over one projection. This PR therefore reuses the existing mark grammar instead of adding parallel polar-only renderers:
The allowlist is explicit:
line,scatter,area,bar,column,heatmap,contour, anderrorbar. Unsupported geometry is rejected at payload build time rather than approximated.Status
coords), axes, chrome in all three renderersradar_chart)polar_bar_chart,wind_rose)projection="polar"Phase 6: heatmap and contour
Phase 7: axis depth
holeand data-space radialorigin.grid_shape="linear") radial grids.The pyplot compatibility layer exposes polar factory routing plus theta/r controls, including degree-based theta limits and radial origin accessors.
Rendering and compatibility
Verification
Explicitly deferred
Generic segment/mesh marks, polar rule/band annotations, polar LOD, facets/animation, and angular pan/sector selection remain disabled or out of scope. Rectangular box selection remains off because it has no honest polar meaning.
Summary by CodeRabbit
coords="polar"support across radar, pie, wind rose, polar bars/rects, heatmaps, contours, and error bars.wedge-gapstyling.wedge-gap.