Polar coordinate system: heatmap, sectors, log r, and error bars (protocol v12) - #370
Open
Alek99 wants to merge 31 commits into
Open
Polar coordinate system: heatmap, sectors, log r, and error bars (protocol v12)#370Alek99 wants to merge 31 commits into
Alek99 wants to merge 31 commits into
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
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.
…zoom Every major from the PR review reproduced before fixing; each fix carries a regression test asserting the behaviour the review named. Exporters: - Reversed radial axes dropped every wedge from SVG/PNG: norm_radius is decreasing under reverse, so taking the normalized endpoints positionally made outer <= inner in both shared wedge functions while the shader (which min/maxes) kept drawing. The normalized fractions are now ordered before clamping. - PDF export crashed for any polar chart with a hole or sector: those clips are a single <path> clipPath, which the converter refused. It now lowers the parsed path (arcs already cubic) to PDF clip ops, with SVG's clip-rule mapped onto W/W* so the annular hole stays a hole. - The raster polar area branch only clamped radii; it now applies the same position mask as SVG's _curve_path and the shader, splitting the fill into visible runs — no more chords across a sector boundary, and NaN no longer reaches the display list (§19). Ranges: - Constant-radius data bypassed the centre-origin default via the singleton early-return: line(theta, [5,5,5,5]) resolved to [4.75, 5.25] and drew a unit circle as a ring floating mid-disc. The singleton path now applies the same centre-origin/no-pad rule as the main polar branch. One existing cross-renderer test placed its marks via the old padded band and needed an explicit domain to keep its negative probe on empty canvas. pyplot: - set_rmax/set_rmin/getters snapshotted the cartesian-PADDED preview: set_rmax(2.0) shipped [0.85, 2.0] where matplotlib gives [0, 2]. The radial auto-domain preview now matches the engine's polar autorange. - set_rlim accepts matplotlib's documented rmin/rmax keywords, and refuses unknown keywords by name instead of TypeErroring inside set_ylim. - plt.subplot(111, projection="polar") on a claimed slot bounced off the stale pre-polar NotImplementedError in Axes.set; both that path and activate_subplot now delegate to _set_projection, which stays loud for genuinely unsupported projections. Interaction (the review's "pan and zoom do not work", root-caused): - Radial wheel zoom was dead on arrival: polar disables pan/box/select, so its RESOLVED default drag tool is "none", and the wheel gate treated that identically to the user choosing the none tool from the modebar. The gate now distinguishes user opt-out (releases page scroll, unchanged) from a chart that simply has no drag tools. Verified with real WheelEvents in headless Chromium: r_hi scales, r_lo stays pinned at 0 — the §8 contract, now asserted by a browser test rather than after != before. - Bar/rect hover was not seam-aware: |dataX - centre| in unwrapped data space missed a wind-rose "N" sector on its wrap side (|355 - 0| = 355). Both hovers now re-base through the same positive-mod the heatmap inverse uses; a browser test hovers both sides of the seam. - The radial zoom precision floor was defeated at r_lo = 0 (minSpan ~ 1e-42): the floor now scales with the interval magnitude, which also fixes centre-anchored cartesian zoom on symmetric ranges. Sustained deep zoom ultimately needs §16-style radial re-centering, recorded in the deferred table rather than half-built. Spec currency: §6's shader table now says RECT_VS is polar-capable (it is — it sweeps the unequal-slice path) and stops listing error bands under AREA_VS; the PDF hole/sector capability is recorded in §6; §8 documents the wheel-with-no-drag-tool rule; the deferred table gains the radial re-centering row. Suite 3,608 passing, polar GLSL parity smoke green, ruff clean.
Hover values arrive f32-decoded (§4/§16), so theta = pi/2 comes back ~2e-8 off its f64 self — and fmtAngle's pi-fraction match used a 1e-9 gate, so the tooltip on a radians chart said "1.57" while the tick at the same spoke said "pi/2". Loosened to 1e-6 in both mirrors (js/src/30_ticks.ts and python/xy/_svg.py); no real tick sits within 1e-6 of a pi-fraction without being one. Verified by hovering every polar chart type in Chromium at this head: radians line/scatter/area (name + pi-fraction + r), degrees bar (theta with the degree sign), radar (authored spoke label, "power" not 1.5708), wind rose, 90-degree sector, donut with hole, and the polar heatmap (theta/r plus the color value). Two browser tests pin the tooltip content pipeline — the coverage gap the review named.
…heta
A pie encodes its value in ANGULAR WIDTH, so the generic bar readout has
nothing meaningful to print for a slice: theta is layout, the radius is the
constant rim, and the category lives only in the mark's name. Hovering the
Market Share donut said "theta: 257, r: 1" under the name — truthful and
useless.
Rather than teach the tooltip machinery to guess when a wedge is a pie,
the pie becomes a first-class composition that owns its readout:
- xy.pie_chart(labels, values, *, hole=0.55, pad=3.0, colors=None,
corner_radius=6.0, show_values=True, show_percent=True) composes polar
wedge bars — hole=0 for a full pie — and names each slice
"label value (share%)", which is also what the legend shows (the same
convention the reference designs use).
- The composition passes tooltip(title="{name}"), so hovering a slice shows
exactly that one line. A user-supplied xy.tooltip child still wins.
- "{name}" is a new tooltip-template pseudo-field resolving to the hovered
trace's series name — generally useful for any composition whose category
lives in the mark name (wind-rose bands, gauge segments).
Verified by real hovers in Chromium on the donut and a hole=0 pie: the
tooltip reads "Streamforge 12 (12%)" / "Affiliate 16300 (12%)" — no theta,
no r. Refusals for mismatched lengths, negative values, zero totals, and
holes outside [0, 1). Suite 3,616 passing, ruff clean, roadmap rows 6/14
updated.
Four audit findings, all reproduced first.
P1 — wheel zoom and double-click reset were dead on every default polar
chart. Polar resolves to dragMode "none" because no drag tool applies, and
the dblclick handler bailed on that unconditionally. The wheel handler
already drew the right distinction (only a USER-CHOSEN "none" releases
navigation, since that is the modebar's page-scroll escape hatch); the
dblclick handler now draws the same one. Examples that hide the modebar had
no reset path at all. Verified in Chromium on polar line, wind rose and a
pie: wheel takes r_hi 1.4 -> 0.48 and a double-click restores home exactly.
P1 — wind rose bands were the WRONG SIZE, not merely mislabelled. `bar`
measures its value as a height above `base`, so authoring `base + counts`
stacked each band on its own cumulative offset a second time: three
observations reached radius 5, and every band above the first was too thick.
The height is the band's count. Two existing tests encoded the bug — one
subtracted the bases back out so the error cancelled — and now assert the
real semantics.
P2 -> default — the numeric angle row is gone from every polar readout. On
most polar charts the angle is where layout put the mark and the cursor is
already sitting on it, so "theta: 72" answers a question nobody asked. What
shows is the values: series name, radial value, colour/size. Two things
survive because they are not numeric angles: an authored spoke label (a
radar category reads "power"), and any row the user names explicitly via
labels={"x": ...}, which opts the angle back in formatted through the axis's
own text function.
Compositions that genuinely need the angle now say so. A wind rose reads
"<= 15.6 / direction 45 deg / count 38" — the bearing IS its data. A pie or
gauge band reads its label alone, and a named single-wedge trace suppresses
theta/r generally, since one named wedge's angle and radius are layout.
Suite 3,619 passing, parity smoke green, ruff clean. Verified visually
across pie, wind rose, gauge, polar line and radar.
Not addressed here (docs, not engine): the two fixed-grid radial-bar demos
that overflow at 375px, responsive legend placement, mobile anchor offsets
under the sticky header, and pie slice keyboard traversal.
The pie's seam narrowed toward the centre because the gap was a constant ANGLE: its width is r*dtheta, so it was 35 px at the rim and 20 px inboard, tapering to nothing at the hole. Measured on a five-slice donut before the fix: 19.8 / 26.8 / 34.2 px at r = 0.42 / 0.55 / 0.68. A gap is a LENGTH, so the angular inset has to grow as the radius shrinks — the same construction as d3's padAngle/padRadius pair. After: 10.6 / 10.4 / 10.1 px. New `wedge-gap` style property (px), threaded through bar/column and applied in one place per renderer — the unrolled (arc, radial) frame the corner radius already uses, where it is a single subtraction from the arc half-width. Both exporters keep exact `A` arcs: only the endpoints move inward and the radial edges become straight offset lines, which `L` draws. Registered in xy.styling.capabilities, matrix regenerated. `pie_chart(pad=...)` is now px, and each slice ships its FULL angular span, so the wire carries true shares. A named single wedge now reports its share: a gauge band reads "At risk / share 45.0%", which is exactly 0-450 of 1000, because the denominator is the span the wedges cover between them rather than the axis range — four bands sweeping 240 degrees of a full-turn axis must total 100% of the gauge, not 67% of a circle. Skipped when the name already carries a percentage, so a pie slice never reads "40% ... 40%". From the continued audit, the three findings that were real on this branch: - Modebar zoom moved the radial minimum. `_zoomBy` anchored at 0.5 for every coordinate system while the wheel path anchors polar radial zoom at r_lo; the documented fixed-minimum contract (polar-axes.md §8) only held for the gesture that was itself unreachable until the previous commit. Verified: 0..48 -> 0..24 with r_lo fixed. - radar_chart ignored an authored `theta_axis(unit="degrees")`: spokes were always generated in radians, so 0..2pi samples sat in a 0..360 frame and the whole radar compressed into the first 6.28 degrees. Spokes now follow the declared unit. Already fixed earlier on this branch, and re-verified rather than assumed — the audit was run against a different checkout (PR #374's worktree): - The wind-rose double-add (P0). A 300-observation rose renders its largest sector at 48, which is the true count; the audit saw 77 -> 142. - Hover across the north seam. A band centred on 0 degrees and 40 wide is hit at 340/350/0/10/20 degrees and correctly missed at 30. Suite 3,623 passing, parity smoke green, ruff clean. Still open from the audit, none of it engine-side: keyboard traversal for polar bar/area marks, CSV export of angular width and radial base, a11y summary staleness after zoom, the two fixed-grid radial-bar demos at 375px, and responsive legend placement.
Alek99
marked this pull request as ready for review
July 29, 2026 22:57
CI's "Docs tests and lint" failed on
test_documented_factories_describe_every_parameter with ('bar', 'wedge_gap'):
the generated API tables are sourced from docstrings, so every parameter needs
an Args: entry. `bar`, `column` and `wind_rose` (which gained *children_in for
its tooltip override) were missing theirs.
Checked by applying the same rule the test applies to every public factory in
xy.__all__ — no factory has an undocumented parameter now. The docs test env
is not installable locally (needs reflex_site_shared), so that check plus
codespell over docs/ stand in for the job.
Suite 3,623 passing, ruff check/format and the repo hooks clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify_ci_workflow.py (1)
324-362: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing gate enforcement for the new "Polar GLSL parity smoke" step.
.github/workflows/ci.ymladds two new polar smoke steps ("Polar GLSL parity smoke" at lines 122-126 and "Polar phase 6/7 live examples" at lines 204-208), but only the second is added to this job's required-markers list. The GLSL-parity probe (binding the shippedxyPolarPosto the fixture) has no wiring guard, so it could be silently removed from CI withoutverify_ci_workflow.pycatching it — defeating the purpose of this gate.🔧 Proposed fix
"scripts/verify_ci_workflow.py", "scripts/check_public_api.py", "ruff check .", "scripts/smoke_render.py", + "Polar GLSL parity smoke", + "scripts/polar_parity_smoke.py", "Polar phase 6/7 live examples", "scripts/polar_phase7_smoke.py",🤖 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 `@scripts/verify_ci_workflow.py` around lines 324 - 362, Update the required-markers list in the test job validation call to include unique markers for the “Polar GLSL parity smoke” step, including its step label and the relevant xyPolarPos parity probe or script invocation. Keep the existing “Polar phase 6/7 live examples” markers unchanged so both polar smoke gates are enforced.
🧹 Nitpick comments (13)
tests/test_polar_charts.py (1)
1134-1163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
monkeypatch.setattrover hand-rolled class-attribute swapping.The try/finally restores correctly, but
monkeypatchremoves the possibility of leaking a patched_Cmd.fill/gradinto the rest of the session if this block is ever edited or an earlyreturn/skip is added.♻️ Proposed refactor
-def test_raster_polar_area_culls_vertices_outside_the_sector() -> None: +def test_raster_polar_area_culls_vertices_outside_the_sector(monkeypatch) -> None: ... captured: list[int] = [] original_fill = _raster._Cmd.fill original_grad = _raster._Cmd.grad def spy_fill(self, pts, color): captured.append(len(pts)) return original_fill(self, pts, color) def spy_grad(self, pts, g0, g1, stops): captured.append(len(pts)) return original_grad(self, pts, g0, g1, stops) - _raster._Cmd.fill = spy_fill - _raster._Cmd.grad = spy_grad - try: - ... - finally: - _raster._Cmd.fill = original_fill - _raster._Cmd.grad = original_grad + monkeypatch.setattr(_raster._Cmd, "fill", spy_fill) + monkeypatch.setattr(_raster._Cmd, "grad", spy_grad) + ...🤖 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_polar_charts.py` around lines 1134 - 1163, Update the test’s _raster._Cmd.fill and _raster._Cmd.grad patching to use pytest’s monkeypatch.setattr fixture instead of manual original_* storage and try/finally restoration. Preserve the existing spies, captured point counts, and chart rendering behavior while letting monkeypatch manage cleanup automatically.spec/design/polar-axes.md (1)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTag the three plain fences with a language.
markdownlint flags MD040 on these blocks.
text(orglsl/pseudocode) keeps the lint lane clean and matches the tagged GLSL block at line 125.Also applies to: 105-105, 154-154
🤖 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 `@spec/design/polar-axes.md` at line 50, Tag each of the three plain fenced code blocks in the polar-axes document with an explicit language identifier, using text, glsl, or pseudocode as appropriate and matching the existing tagged GLSL style.Source: Linters/SAST tools
js/src/51_annotations.ts (1)
690-694: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the local
projecthelper instead of_dataPxPointinside this loop.
projectalready closes over thepolarGeomcomputed once at line 600;_dataPxPointre-resolves polar geometry per call (four extra resolutions per arrow/marker annotation) and gives the same result, so the mixed spelling in the arrow/marker branches is redundant and invites the two paths to diverge.♻️ Proposed change
} else if (ann.kind === "arrow") { - const [arrowX0, arrowY0] = this._dataPxPoint(Number(ann.x0), Number(ann.y0)); - const [arrowX1, arrowY1] = this._dataPxPoint(Number(ann.x1), Number(ann.y1)); + const [arrowX0, arrowY0] = project(ann.x0, ann.y0); + const [arrowX1, arrowY1] = project(ann.x1, ann.y1); this._drawArrowLine(ctx, arrowX0, arrowY0, arrowX1, arrowY1, style); @@ } else if (ann.kind === "marker") { - const [markerX, markerY] = this._dataPxPoint(Number(ann.x), Number(ann.y)); + const [markerX, markerY] = project(ann.x, ann.y); this._drawAnnotationMarker(ctx, markerX, markerY, style, ann);Also applies to: 705-706
🤖 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/51_annotations.ts` around lines 690 - 694, Replace the _dataPxPoint calls in the arrow and marker annotation branches with the local project helper, passing each annotation coordinate through project before drawing. Update both affected locations, including the branch around the callout handling, while preserving the existing coordinate order and drawing behavior.js/src/40_gl.ts (2)
652-684: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPolar clip-space→data inversion duplicates
xyPolarFragmentVisible.Lines 659-671 re-derive
local,displayedRadius,radialFraction,rCoordand the radial-window test with the same epsilons asPOLAR_FRAGMENT_CLIP_GLSL(lines 207-221). Two copies of the inverse transform will drift; consider factoring the shared inverse into one GLSL helper (returningvec2(thetaCoord, rCoord)plus a visibility flag) that both the heatmap branch and the fragment clip call.🤖 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/40_gl.ts` around lines 652 - 684, Factor the duplicated polar clip-space inversion from the heatmap branch and xyPolarFragmentVisible/POLAR_FRAGMENT_CLIP_GLSL into one shared GLSL helper that computes thetaCoord and rCoord and reports visibility. Update both callers to use this helper, preserving the existing radial/angular bounds and epsilon behavior while removing the duplicate local, displayedRadius, radialFraction, and rCoord calculations.
1332-1369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStroke/coverage block is a near-copy of the cartesian path above.
Lines 1361-1368 duplicate 1321-1330 verbatim (
aa, stroke source resolution,inner,mix, finalpremult *=). Onlyddiffers between the two branches, so computingdin each branch and running one shared stroke+coverage tail would remove the copy and keep the two paths from diverging on the alpha stack.♻️ Sketch
- if (u_coordMode != 1 && (radius.x > 0.0 || radius.y > 0.0 || strokeWidth > 0.0)) { + bool hasSdf = false; + float d = 0.0; + if (u_coordMode != 1 && (radius.x > 0.0 || radius.y > 0.0 || strokeWidth > 0.0)) { ... - premult *= 1.0 - smoothstep(-aa, aa, d); + hasSdf = true; } - if (u_coordMode == 1) { + if (u_coordMode == 1) { /* compute d for the annular sector */ hasSdf = true; } + if (hasSdf) { /* one stroke + coverage tail */ }🤖 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/40_gl.ts` around lines 1332 - 1369, Refactor the fragment shader so the u_coordMode == 1 branch computes only its annular-sector distance d, then exits the branch before the shared stroke and coverage tail. Move the duplicated aa, stroke source/alpha construction, inner mix, and final premult coverage operations out of the branch and execute them once for both coordinate paths, preserving the existing d-specific behavior.js/src/52_tooltip.ts (1)
256-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
_isNamedSingleWedgewrapper Nothing calls it, and_defaultTooltipItemsalready uses_namedWedgedirectly.🤖 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/52_tooltip.ts` around lines 256 - 258, Remove the unused _isNamedSingleWedge method. Keep _defaultTooltipItems using _namedWedge directly and leave the existing _namedWedge behavior unchanged.python/xy/components.py (3)
1682-1682: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
wedge_gapparameter.
bar()'s Args block documents every other keyword;wedge_gapis missing, so the polar-only pixel-gap semantics are undiscoverable from the public docstring.📝 Proposed docstring addition
corner_radius: Bar corner radius in pixels. + wedge_gap: Gap between neighbouring polar wedges, in pixels. Ignored + unless the owning chart is ``coords="polar"``. stroke: Optional bar outline color.Also applies to: 1741-1741
🤖 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/components.py` at line 1682, Update the bar() docstring Args block to document the wedge_gap parameter, including that it applies only to polar charts and specifies the pixel gap between wedges. Apply the same documentation update to the corresponding second bar-related declaration.
1767-1767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame missing
wedge_gapdoc entry incolumn().Also applies to: 1824-1824
🤖 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/components.py` at line 1767, Add the missing wedge_gap documentation entry to the column() docstring, matching the existing parameter documentation style and accurately describing its float value and default behavior. Apply the same update to the corresponding second column() definition or overload identified near the referenced symbol.
1682-1741: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
wedge_gapis undocumented on both public mark factories. Both signatures gained the keyword and both forward it intoprops, but neither Args block mentions it, so the polar-only pixel semantics are invisible to users of the public API.
python/xy/components.py#L1682-L1741: add awedge_gap:entry tobar()'s Args block, betweencorner_radiusandstroke.python/xy/components.py#L1767-L1824: add the same entry tocolumn()'s Args block.🤖 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/components.py` around lines 1682 - 1741, Document the existing wedge_gap parameter in the Args sections of bar() and column() in python/xy/components.py at lines 1682-1741 and 1767-1824, respectively. Add the same entry between corner_radius and stroke, describing its polar-only pixel semantics; no implementation changes are needed because both factories already forward wedge_gap through props.python/xy/pyplot/_mplfig.py (1)
394-398: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFour copies of the same
projection/polarnormalization. Each axes-creation path repeats the identical pop-and-map block, so a future change to the aliases (matplotlib'spolar=plusprojection=) has to land in four places.Suggested shape: a module-level
_pop_projection(kwargs)returning the resolved name orNone.
python/xy/pyplot/_mplfig.py#L394-L398: replace the block inadd_subplotwithprojection = _pop_projection(kwargs).python/xy/pyplot/_mplfig.py#L449-L456: same replacement inactivate_subplot, keeping the existing explanatory comment.python/xy/pyplot/_mplfig.py#L472-L476: same replacement inadd_axes.python/xy/pyplot/_mplfig.py#L554-L562: same replacement insubplots, applied tosubplot_kwbefore the per-axes loop.🤖 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/pyplot/_mplfig.py` around lines 394 - 398, Extract the duplicated projection/polar normalization into a module-level _pop_projection(kwargs) helper that removes both aliases and returns the resolved projection name or None, with polar taking precedence. Replace the blocks in python/xy/pyplot/_mplfig.py lines 394-398 (add_subplot), 449-456 (activate_subplot; preserve its explanatory comment), 472-476 (add_axes), and 554-562 (subplots; apply to subplot_kw before the per-axes loop) with calls to the helper.js/src/50_chartview.ts (2)
22-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment block at Lines 32-34 documents
POLAR_RLABEL_DEG/POLAR_TICK_GAPbut is attached toPOLAR_LABEL_ROOM_MAX.Move
POLAR_LABEL_ROOM_MAXabove that comment (next toPOLAR_LABEL_ROOM, whose ceiling it is) so each constant sits under the text describing it.🤖 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 22 - 41, Move POLAR_LABEL_ROOM_MAX to immediately follow POLAR_LABEL_ROOM, before the comment describing POLAR_RLABEL_DEG and POLAR_TICK_GAP. Keep the existing constant values and comments unchanged so each constant is positioned under its corresponding documentation.
5649-5656: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_polarGeometry()is recomputed several times per draw call.
_setPolarUniformsalready builds the geometry, and_drawRects/_drawBarsbuild it again purely to decide the segment count; the same happens once per trace per frame. Consider caching the geometry for the frame (invalidated in_layout/_setView) and passing it down.Also applies to: 5748-5762
🤖 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 5649 - 5656, Cache the result of _polarGeometry for the current frame instead of recomputing it in _setPolarUniforms, _drawRects, and _drawBars. Add invalidation in _layout and _setView, then pass or reuse the cached geometry when determining polarSegments and setting uniforms, preserving the existing draw behavior.python/xy/pyplot/_plot_types.py (1)
1662-1667: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mark_kwargsis built even though the polar path never uses it.The polar branch returns before the triangle-mesh
@mark, and re-spellscolor/opacity/namein its own dict. Moving themark_kwargsconstruction below the polar branch keeps the duplication out of the polar path.🤖 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/pyplot/_plot_types.py` around lines 1662 - 1667, Move the mark_kwargs construction below the polar branch in the surrounding plotting function so it is created only for the triangle-mesh `@mark` path. Preserve the polar branch’s existing return behavior and its own color, opacity, and name handling.
🤖 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/40_gl.ts`:
- Around line 827-866: Update the polar branch in the main shader to clip
contour/error-bar segments against the authored angular sector as well as the
radial window, preventing xyPolarPos from receiving invisible endpoints and
culling the segment. Interpolate the radial coordinate at angular boundary
intersections, preserving segment continuity and clamp-not-cull behavior;
otherwise explicitly document this as a deferred limitation after confirming
SVG/raster behavior.
In `@js/src/50_chartview.ts`:
- Around line 7134-7145: Update the polar heatmap wrapping logic near
_polarGeometry and the sampleX bounds check to anchor wrapped dataX at
Math.min(x0, x1) and use the absolute x-range span as the wrapping period.
Preserve the existing ascending-axis behavior while allowing descending xRange
values to produce samples within the valid cell bounds instead of returning
null.
- Around line 7103-7122: Update the polar containment branch in _rectHover to
measure the wrapped dataX offset from x0, matching the forward span computed
from x1 - x0; do not use Math.min(x0, x1) for this directional calculation.
Preserve the cartesian min/max logic and add a regression case alongside
test_polar_bar_hover_wraps_across_the_seam for a descending, seam-crossing rect
wedge.
In `@js/src/53_interaction.ts`:
- Around line 564-576: Update the culled-row branch in the keyboard traversal
method around _projectDataPoint so non-finite coordinates still trigger a
concise live-region announcement identifying the point as outside the visible
range, while continuing to skip _showTooltip and coordinate-based placement.
Preserve the advanced _a11yPointIndex and existing traversal flow so the next
key proceeds from the current source position.
In `@python/xy/_figure.py`:
- Around line 306-345: Reject type_="log" for polar theta axes in
Figure.set_axis(), before the existing log-range validation can run. Use the
visible axis_dim, type_, and self.coords checks to raise a clear ValueError for
a polar x-axis, while preserving log support for non-polar or radial axes.
In `@python/xy/_svg.py`:
- Around line 3820-3833: Update the area/error_band path construction around
_curve_path so empty polar top_path or base_path results skip the trace instead
of emitting malformed path data. When either path contains multiple
space-separated subpaths, pair corresponding top and base runs and close each
visible run independently, rather than applying the existing single-path join
with base_path[2:]. Preserve the current behavior for ordinary single-run paths.
- Around line 5341-5356: The polar branch in _bar_marks must not render
horizontal bars with the current pos/v0/v1 mapping. Reject horizontal
orientation for polar traces, or swap the polar inputs so horizontal bars
receive the correct theta and radius values; preserve existing vertical polar
bar behavior.
In `@python/xy/components.py`:
- Around line 6394-6409: Update the quartile-derived edge construction in the
speed_bins fallback so the top-edge adjustment is followed by a final np.unique
pass. This must remove any duplicate created when the recomputed upper edge
equals an existing rounded edge, while preserving the existing explicit
speed_bins handling and empty-edge validation.
In `@python/xy/pyplot/_axes.py`:
- Around line 5790-5794: Update get_theta_offset to return the canonical
positive angle for the "S" theta-zero location, matching Matplotlib’s 270°
representation (approximately 3π/2) instead of -π/2. Preserve the existing
mappings for "E", "N", and "W", as well as numeric zero values.
In `@spec/design/polar-axes.md`:
- Around line 69-98: Update the introductory count in the list to state six
properties instead of five; leave the six property bullets and their content
unchanged.
In `@spec/matplotlib/compat-changelog.md`:
- Line 7: Align the Matplotlib reference versions between the “Polar projection
depth” entry in compat-changelog.md and the corresponding version pin in
compat-matrix.md. Update either reference so both spec files consistently use
the same version, preserving the existing formatting.
In `@src/raster.rs`:
- Around line 60-82: Update the Sector constructor new to accept a zero sweep as
a valid empty clip instead of returning None, while preserving rejection of
non-finite inputs and invalid radii. In pixel_coverage, return 0.0 when inner >=
outer or the computed span is zero, so degenerate annuli and f32-collapsed
sectors mask no pixels without causing rasterize_with_spans to discard the
framebuffer.
---
Outside diff comments:
In `@scripts/verify_ci_workflow.py`:
- Around line 324-362: Update the required-markers list in the test job
validation call to include unique markers for the “Polar GLSL parity smoke”
step, including its step label and the relevant xyPolarPos parity probe or
script invocation. Keep the existing “Polar phase 6/7 live examples” markers
unchanged so both polar smoke gates are enforced.
---
Nitpick comments:
In `@js/src/40_gl.ts`:
- Around line 652-684: Factor the duplicated polar clip-space inversion from the
heatmap branch and xyPolarFragmentVisible/POLAR_FRAGMENT_CLIP_GLSL into one
shared GLSL helper that computes thetaCoord and rCoord and reports visibility.
Update both callers to use this helper, preserving the existing radial/angular
bounds and epsilon behavior while removing the duplicate local, displayedRadius,
radialFraction, and rCoord calculations.
- Around line 1332-1369: Refactor the fragment shader so the u_coordMode == 1
branch computes only its annular-sector distance d, then exits the branch before
the shared stroke and coverage tail. Move the duplicated aa, stroke source/alpha
construction, inner mix, and final premult coverage operations out of the branch
and execute them once for both coordinate paths, preserving the existing
d-specific behavior.
In `@js/src/50_chartview.ts`:
- Around line 22-41: Move POLAR_LABEL_ROOM_MAX to immediately follow
POLAR_LABEL_ROOM, before the comment describing POLAR_RLABEL_DEG and
POLAR_TICK_GAP. Keep the existing constant values and comments unchanged so each
constant is positioned under its corresponding documentation.
- Around line 5649-5656: Cache the result of _polarGeometry for the current
frame instead of recomputing it in _setPolarUniforms, _drawRects, and _drawBars.
Add invalidation in _layout and _setView, then pass or reuse the cached geometry
when determining polarSegments and setting uniforms, preserving the existing
draw behavior.
In `@js/src/51_annotations.ts`:
- Around line 690-694: Replace the _dataPxPoint calls in the arrow and marker
annotation branches with the local project helper, passing each annotation
coordinate through project before drawing. Update both affected locations,
including the branch around the callout handling, while preserving the existing
coordinate order and drawing behavior.
In `@js/src/52_tooltip.ts`:
- Around line 256-258: Remove the unused _isNamedSingleWedge method. Keep
_defaultTooltipItems using _namedWedge directly and leave the existing
_namedWedge behavior unchanged.
In `@python/xy/components.py`:
- Line 1682: Update the bar() docstring Args block to document the wedge_gap
parameter, including that it applies only to polar charts and specifies the
pixel gap between wedges. Apply the same documentation update to the
corresponding second bar-related declaration.
- Line 1767: Add the missing wedge_gap documentation entry to the column()
docstring, matching the existing parameter documentation style and accurately
describing its float value and default behavior. Apply the same update to the
corresponding second column() definition or overload identified near the
referenced symbol.
- Around line 1682-1741: Document the existing wedge_gap parameter in the Args
sections of bar() and column() in python/xy/components.py at lines 1682-1741 and
1767-1824, respectively. Add the same entry between corner_radius and stroke,
describing its polar-only pixel semantics; no implementation changes are needed
because both factories already forward wedge_gap through props.
In `@python/xy/pyplot/_mplfig.py`:
- Around line 394-398: Extract the duplicated projection/polar normalization
into a module-level _pop_projection(kwargs) helper that removes both aliases and
returns the resolved projection name or None, with polar taking precedence.
Replace the blocks in python/xy/pyplot/_mplfig.py lines 394-398 (add_subplot),
449-456 (activate_subplot; preserve its explanatory comment), 472-476
(add_axes), and 554-562 (subplots; apply to subplot_kw before the per-axes loop)
with calls to the helper.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 1662-1667: Move the mark_kwargs construction below the polar
branch in the surrounding plotting function so it is created only for the
triangle-mesh `@mark` path. Preserve the polar branch’s existing return behavior
and its own color, opacity, and name handling.
In `@spec/design/polar-axes.md`:
- Line 50: Tag each of the three plain fenced code blocks in the polar-axes
document with an explicit language identifier, using text, glsl, or pseudocode
as appropriate and matching the existing tagged GLSL style.
In `@tests/test_polar_charts.py`:
- Around line 1134-1163: Update the test’s _raster._Cmd.fill and
_raster._Cmd.grad patching to use pytest’s monkeypatch.setattr fixture instead
of manual original_* storage and try/finally restoration. Preserve the existing
spies, captured point counts, and chart rendering behavior while letting
monkeypatch manage cleanup automatically.
🪄 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: cf08f0b9-d70e-4bca-9d45-6e1a1c0cc4d0
📒 Files selected for processing (53)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mddocs/styling/capabilities.mdjs/src/00_header.tsjs/src/30_ticks.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/51_annotations.tsjs/src/52_tooltip.tsjs/src/53_interaction.tspython/xy/__init__.pypython/xy/_figure.pypython/xy/_hosts.pypython/xy/_native.pypython/xy/_payload.pypython/xy/_pdf.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_validate.pypython/xy/components.pypython/xy/config.pypython/xy/marks.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pypython/xy/styles.pypython/xy/styling/capabilities.pyscripts/polar_parity_smoke.pyscripts/polar_phase7_smoke.pyscripts/verify_ci_workflow.pyspec/api/capability-matrix.mdspec/api/chart-roadmap.mdspec/design/polar-axes.mdspec/design/renderer-architecture.mdspec/design/wire-protocol.mdspec/matplotlib/compat-changelog.mdspec/matplotlib/compat-matrix.mdspec/matplotlib/compat.mdspec/matplotlib/shim-todo.mdsrc/lib.rssrc/raster.rstests/fixtures/polar_transform.jsontests/pyplot/corpus/55_polar_projection.pytests/pyplot/test_boundaries.pytests/pyplot/test_polar_projection.pytests/pyplot/test_tick_side_rendering.pytests/test_polar_charts.pytests/test_polar_client_regressions.pytests/test_polar_phase7_api.pytests/test_polar_phase7_static.pytests/test_polar_transform.py
Comment on lines
+827
to
+866
| ${POLAR_GLSL_UNIFORMS} | ||
| void main() { | ||
| vec2 p0 = vec2(xyMap(ax0, u_xmap, u_x0meta, u_x0mode, u_x0constant), xyMap(ay0, u_ymap, u_y0meta, u_y0mode, u_y0constant)); | ||
| vec2 p1 = vec2(xyMap(ax1, u_xmap, u_x1meta, u_x1mode, u_x1constant), xyMap(ay1, u_ymap, u_y1meta, u_y1mode, u_y1constant)); | ||
| vec2 p0; | ||
| vec2 p1; | ||
| if (u_coordMode == 1) { | ||
| float th0 = xyAxisCoord(ax0, u_x0meta, u_x0mode, u_x0constant); | ||
| float th1 = xyAxisCoord(ax1, u_x1meta, u_x1mode, u_x1constant); | ||
| float r0 = xyAxisCoord(ay0, u_y0meta, u_y0mode, u_y0constant); | ||
| float r1 = xyAxisCoord(ay1, u_y1meta, u_y1mode, u_y1constant); | ||
| float rmin = min(u_rrange.x, u_rrange.y); | ||
| float rmax = max(u_rrange.x, u_rrange.y); | ||
| if (max(r0, r1) < rmin || min(r0, r1) > rmax) { | ||
| p0 = vec2(uintBitsToFloat(0x7fc00000u)); | ||
| p1 = p0; | ||
| } else { | ||
| // Independent contour/error-bar segments clip at the radial window. | ||
| // Interpolating theta at the clipped endpoint keeps a diagonal segment | ||
| // attached to the ring instead of bending it onto a radial clamp. | ||
| float dr = r1 - r0; | ||
| float t0 = 0.0; | ||
| float t1 = 1.0; | ||
| if (abs(dr) > 1e-30) { | ||
| float ta = (rmin - r0) / dr; | ||
| float tb = (rmax - r0) / dr; | ||
| t0 = max(0.0, min(ta, tb)); | ||
| t1 = min(1.0, max(ta, tb)); | ||
| } | ||
| float cth0 = mix(th0, th1, t0); | ||
| float cth1 = mix(th0, th1, t1); | ||
| float cr0 = clamp(mix(r0, r1, t0), rmin, rmax); | ||
| float cr1 = clamp(mix(r0, r1, t1), rmin, rmax); | ||
| p0 = xyPolarPos(cth0, cr0, u_polar, u_rrange, u_zdir, | ||
| u_trange, u_turn, u_rshape); | ||
| p1 = xyPolarPos(cth1, cr1, u_polar, u_rrange, u_zdir, | ||
| u_trange, u_turn, u_rshape); | ||
| } | ||
| } else { | ||
| p0 = vec2(xyMap(ax0, u_xmap, u_x0meta, u_x0mode, u_x0constant), xyMap(ay0, u_ymap, u_y0meta, u_y0mode, u_y0constant)); | ||
| p1 = vec2(xyMap(ax1, u_xmap, u_x1meta, u_x1mode, u_x1constant), xyMap(ay1, u_ymap, u_y1meta, u_y1mode, u_y1constant)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Segments straddling the sector edge are dropped rather than angularly clipped.
The branch clips against the radial window only; xyPolarPos then applies xyPolarThetaVisible to each endpoint and returns NaN, so a contour/errorbar segment with one endpoint outside an authored sector culls entirely instead of being trimmed at the sector boundary. Radial clipping got the "clamp, don't cull" treatment (AREA_VS/BAR_VS/RECT_VS comments); the angular direction still culls. Worth confirming this matches the SVG/raster behaviour for sectored polar contours, or noting it as a documented deferral.
🤖 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/40_gl.ts` around lines 827 - 866, Update the polar branch in the main
shader to clip contour/error-bar segments against the authored angular sector as
well as the radial window, preventing xyPolarPos from receiving invisible
endpoints and culling the segment. Interpolate the radial coordinate at angular
boundary intersections, preserving segment continuity and clamp-not-cull
behavior; otherwise explicitly document this as a deferred limitation after
confirming SVG/raster behavior.
Comment on lines
+564
to
+576
| const [chartX, chartY] = this._projectDataPoint( | ||
| g.xAxis || "x", | ||
| g.yAxis || "y", | ||
| xValue, | ||
| yValue, | ||
| ); | ||
| // A polar view can legitimately cull source rows outside its authored | ||
| // sector or radial window. Keyboard traversal still advances over that row, | ||
| // but it must not publish a hover or feed NaN coordinates to tooltip | ||
| // placement; the next traversal key continues from this source position. | ||
| if (!Number.isFinite(chartX) || !Number.isFinite(chartY)) return; | ||
| const x = chartX - this.plot.x; | ||
| const y = chartY - this.plot.y; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Culled polar rows leave keyboard traversal silent.
The early return skips _showTooltip and the live-region update, so a screen-reader user pressing Arrow over a row outside the sector/radial window gets no announcement at all — indistinguishable from a stuck focus. _a11yPointIndex has already advanced, so a short announcement ("point N of M is outside the visible range") would keep the traversal legible without publishing a hover.
🛠️ Proposed fix
- if (!Number.isFinite(chartX) || !Number.isFinite(chartY)) return;
+ if (!Number.isFinite(chartX) || !Number.isFinite(chartY)) {
+ this._hideTooltip();
+ this._hoverId = -1;
+ this._hoverTarget = null;
+ if (this.a11yLive) {
+ this.a11yLive.textContent =
+ `Point ${flat + 1} of ${total} is outside the visible range.`;
+ }
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [chartX, chartY] = this._projectDataPoint( | |
| g.xAxis || "x", | |
| g.yAxis || "y", | |
| xValue, | |
| yValue, | |
| ); | |
| // A polar view can legitimately cull source rows outside its authored | |
| // sector or radial window. Keyboard traversal still advances over that row, | |
| // but it must not publish a hover or feed NaN coordinates to tooltip | |
| // placement; the next traversal key continues from this source position. | |
| if (!Number.isFinite(chartX) || !Number.isFinite(chartY)) return; | |
| const x = chartX - this.plot.x; | |
| const y = chartY - this.plot.y; | |
| const [chartX, chartY] = this._projectDataPoint( | |
| g.xAxis || "x", | |
| g.yAxis || "y", | |
| xValue, | |
| yValue, | |
| ); | |
| // A polar view can legitimately cull source rows outside its authored | |
| // sector or radial window. Keyboard traversal still advances over that row, | |
| // but it must not publish a hover or feed NaN coordinates to tooltip | |
| // placement; the next traversal key continues from this source position. | |
| if (!Number.isFinite(chartX) || !Number.isFinite(chartY)) { | |
| this._hideTooltip(); | |
| this._hoverId = -1; | |
| this._hoverTarget = null; | |
| if (this.a11yLive) { | |
| this.a11yLive.textContent = | |
| `Point ${flat + 1} of ${total} is outside the visible range.`; | |
| } | |
| return; | |
| } | |
| const x = chartX - this.plot.x; | |
| const y = chartY - this.plot.y; |
🤖 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/53_interaction.ts` around lines 564 - 576, Update the culled-row
branch in the keyboard traversal method around _projectDataPoint so non-finite
coordinates still trigger a concise live-region announcement identifying the
point as outside the visible range, while continuing to skip _showTooltip and
coordinate-based placement. Preserve the advanced _a11yPointIndex and existing
traversal flow so the next key proceeds from the current source position.
Comment on lines
+6394
to
+6409
| if speed_bins is None: | ||
| # Quartile bands, rounded to three significant figures: the raw | ||
| # quantiles are readable as a legend only by accident ("<= 2.76651"). | ||
| quartiles = np.quantile(magnitudes, [0.25, 0.5, 0.75, 1.0]) | ||
| edges = np.unique([float(f"{value:.3g}") for value in quartiles]) | ||
| # The top edge rounds *up*, never down: it has to cover the fastest | ||
| # observation, and restoring the raw maximum would put "27.2197" in the | ||
| # legend next to "2.77". | ||
| top = float(magnitudes.max()) | ||
| if top > 0: | ||
| unit = 10.0 ** (math.floor(math.log10(top)) - 2) | ||
| edges[-1] = math.ceil(top / unit) * unit | ||
| else: | ||
| edges = np.unique(np.asarray(speed_bins, dtype=float).reshape(-1)) | ||
| if edges.size == 0: | ||
| raise ValueError("wind_rose speed_bins must contain at least one edge") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Speed-band edges can collapse to a duplicate upper edge.
Quartiles are rounded to three significant figures before edges[-1] is rounded up, so a top quantile that rounds up (e.g. 2.7665 → "2.77") can equal the recomputed final edge. That yields two identically labelled bands (≤ 2.77) with the last one empty. Re-uniquing after the top-edge adjustment closes it.
🐛 Proposed fix
top = float(magnitudes.max())
if top > 0:
unit = 10.0 ** (math.floor(math.log10(top)) - 2)
edges[-1] = math.ceil(top / unit) * unit
+ edges = np.unique(edges)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if speed_bins is None: | |
| # Quartile bands, rounded to three significant figures: the raw | |
| # quantiles are readable as a legend only by accident ("<= 2.76651"). | |
| quartiles = np.quantile(magnitudes, [0.25, 0.5, 0.75, 1.0]) | |
| edges = np.unique([float(f"{value:.3g}") for value in quartiles]) | |
| # The top edge rounds *up*, never down: it has to cover the fastest | |
| # observation, and restoring the raw maximum would put "27.2197" in the | |
| # legend next to "2.77". | |
| top = float(magnitudes.max()) | |
| if top > 0: | |
| unit = 10.0 ** (math.floor(math.log10(top)) - 2) | |
| edges[-1] = math.ceil(top / unit) * unit | |
| else: | |
| edges = np.unique(np.asarray(speed_bins, dtype=float).reshape(-1)) | |
| if edges.size == 0: | |
| raise ValueError("wind_rose speed_bins must contain at least one edge") | |
| if speed_bins is None: | |
| # Quartile bands, rounded to three significant figures: the raw | |
| # quantiles are readable as a legend only by accident ("<= 2.76651"). | |
| quartiles = np.quantile(magnitudes, [0.25, 0.5, 0.75, 1.0]) | |
| edges = np.unique([float(f"{value:.3g}") for value in quartiles]) | |
| # The top edge rounds *up*, never down: it has to cover the fastest | |
| # observation, and restoring the raw maximum would put "27.2197" in the | |
| # legend next to "2.77". | |
| top = float(magnitudes.max()) | |
| if top > 0: | |
| unit = 10.0 ** (math.floor(math.log10(top)) - 2) | |
| edges[-1] = math.ceil(top / unit) * unit | |
| edges = np.unique(edges) | |
| else: | |
| edges = np.unique(np.asarray(speed_bins, dtype=float).reshape(-1)) | |
| if edges.size == 0: | |
| raise ValueError("wind_rose speed_bins must contain at least one edge") |
🤖 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/components.py` around lines 6394 - 6409, Update the
quartile-derived edge construction in the speed_bins fallback so the top-edge
adjustment is followed by a final np.unique pass. This must remove any duplicate
created when the recomputed upper edge equals an existing rounded edge, while
preserving the existing explicit speed_bins handling and empty-edge validation.
| meaning of xy's compatibility levels. It complements the project changelog, | ||
| which covers user-visible releases across the whole package. | ||
|
|
||
| ## Polar projection depth — 2026-07-28 (Matplotlib 3.11.1 reference) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'compat-changelog.md|compat-matrix.md|compat.md' spec | xargs rg -n '3\.11|Pinned upstream'Repository: reflex-dev/xy
Length of output: 4346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' spec/matplotlib/compat-matrix.md
printf '\n--- compat-changelog ---\n'
sed -n '1,120p' spec/matplotlib/compat-changelog.mdRepository: reflex-dev/xy
Length of output: 17725
Align the Matplotlib reference version across the spec files. spec/matplotlib/compat-changelog.md now cites Matplotlib 3.11.1, but spec/matplotlib/compat-matrix.md still pins v3.11.0-348-gbde111fb4e; update one side so the spec stays consistent.
🤖 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 `@spec/matplotlib/compat-changelog.md` at line 7, Align the Matplotlib
reference versions between the “Polar projection depth” entry in
compat-changelog.md and the corresponding version pin in compat-matrix.md.
Update either reference so both spec files consistently use the same version,
preserving the existing formatting.
Source: Coding guidelines
Comment on lines
+60
to
+82
| fn new(cx: f32, cy: f32, inner: f32, outer: f32, start: f32, sweep: f32) -> Option<Self> { | ||
| if ![cx, cy, inner, outer, start, sweep] | ||
| .into_iter() | ||
| .all(f32::is_finite) | ||
| || inner < 0.0 | ||
| || outer < inner | ||
| || sweep == 0.0 | ||
| { | ||
| return None; | ||
| } | ||
| let span = sweep.abs(); | ||
| Some(Self { | ||
| cx, | ||
| cy, | ||
| inner, | ||
| outer, | ||
| start: [start.cos(), start.sin()], | ||
| end: [(start + sweep).cos(), (start + sweep).sin()], | ||
| direction: sweep.signum(), | ||
| wide: span > std::f32::consts::PI, | ||
| full: span >= std::f32::consts::TAU * (1.0 - 1e-6), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A degenerate sweep aborts the whole render rather than masking marks.
inner >= outer was deliberately treated as an empty-but-valid clip (see pixel_coverage), but sweep == 0.0 returns None, which propagates through ok? and makes rasterize_with_spans drop the entire framebuffer — a blank export instead of an empty annulus. An f32-collapsed sector span reaches the same state as the collapsed annulus.
🛡️ Treat a zero sweep as an empty clip
- || inner < 0.0
- || outer < inner
- || sweep == 0.0
+ || inner < 0.0
{
return None;
}
+ // A collapsed sweep is an empty visible sector, not a malformed
+ // command stream: mask marks and let the later rectangular clip reset.with pixel_coverage returning 0.0 when inner >= outer || span == 0.0.
🤖 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 `@src/raster.rs` around lines 60 - 82, Update the Sector constructor new to
accept a zero sweep as a valid empty clip instead of returning None, while
preserving rejection of non-finite inputs and invalid radii. In pixel_coverage,
return 0.0 when inner >= outer or the computed span is zero, so degenerate
annuli and f32-collapsed sectors mask no pixels without causing
rasterize_with_spans to discard the framebuffer.
The probe raced the frame it was waiting for. Wheel deltas are coalesced and applied inside a requestAnimationFrame, but the probe read the radial range after a fixed 260 ms setTimeout. Headless probes run under --virtual-time-budget, which fast-forwards timers while rAF still needs a real frame, so the timed read could land before the zoom committed and report [0, 1.4] -> [0, 1.4]. It failed that way on both "Python 3.11 floor" and "Test (Rust + Python + JS)" while passing locally every time. Use the idiom the repo's other two wheel probes already use: stub requestAnimationFrame, dispatch, then drain the queue synchronously. No wall clock, no frame dependency. The assertion is unchanged and still live. Reintroducing the original gate (dragMode === "none" without the _dragModeUserSet distinction) fails the test with exactly the CI signature, and neutering the drain also fails it — confirming the synchronous drain, not an incidental real frame, is what commits the zoom.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/xy/components.py (2)
6417-6449: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent explicit speed bins from silently dropping observations.
Values above the largest supplied edge satisfy none of the loop predicates, so they disappear from the rose without an overflow band or error. Reject incomplete bins or add a labeled overflow tier.
Proposed validation
if edges.size == 0: raise ValueError("wind_rose speed_bins must contain at least one edge") + if not np.all(np.isfinite(edges)): + raise ValueError("wind_rose speed_bins must be finite") + if magnitudes.max() > edges[-1]: + raise ValueError("wind_rose speed_bins must cover every finite speed")As per coding guidelines, “Record every decimation and tier decision in the specification; decisions must never be silent.”
🤖 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/components.py` around lines 6417 - 6449, Update the wind_rose speed-bin construction so observations above the largest supplied speed_bins edge cannot be silently discarded. Either validate the input and raise a clear ValueError for incomplete bins, or add a labeled overflow tier covering values above the final edge; preserve existing band counts and cumulative stacking for all in-range observations.Source: Coding guidelines
1745-1745: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMove
wedge_gapoff the JSON spec — both bar constructors still write it intostyle, and the renderer readsg.trace.style?.wedge_gap, so it travels as a numeric JSON field instead of a rawf32buffer.🤖 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/components.py` at line 1745, Remove wedge_gap from the style JSON emitted by both bar constructors, and update the corresponding renderer path that reads g.trace.style?.wedge_gap to consume wedge_gap through the raw f32 buffer instead. Ensure both constructors and the renderer use the same buffer-backed representation.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@python/xy/components.py`:
- Around line 6417-6449: Update the wind_rose speed-bin construction so
observations above the largest supplied speed_bins edge cannot be silently
discarded. Either validate the input and raise a clear ValueError for incomplete
bins, or add a labeled overflow tier covering values above the final edge;
preserve existing band counts and cumulative stacking for all in-range
observations.
- Line 1745: Remove wedge_gap from the style JSON emitted by both bar
constructors, and update the corresponding renderer path that reads
g.trace.style?.wedge_gap to consume wedge_gap through the raw f32 buffer
instead. Ensure both constructors and the renderer use the same buffer-backed
representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 06335ed4-59ab-47a8-a7cc-f8d498e2fc18
📒 Files selected for processing (2)
python/xy/components.pytests/test_polar_client_regressions.py
Three defects from an external audit, each reproduced first. The audit's two
headline claims (PDF path clips, constant-radius [-0.5, 0.5]) were generated
against a pre-9d0abbee tree and are already fixed and pinned here; four more
findings reproduce identically on origin/main and are not this PR's.
Secondary axes. A polar figure accepted y_axis="y2", validated the binding as
legitimate, then every renderer read only the primary pair: an overlapping
secondary range drew pixel-identical to the primary, inviting the reader to
decode it against a tick ladder it does not belong to, and a disjoint one
culled the series away entirely — while the axis still got a straight
Cartesian spine and title in the gutter of a disc. Payload build now rejects
any axis id outside {x, y} under coords="polar", per the rule this method
already states: a plausible wrong picture is worse than an error.
Non-linear theta. theta_axis(type_="log"/"symlog") was accepted, serialized,
and honoured by exactly one renderer — the client scales theta before
projecting, the static exporters ignore the scale outright (their SVG is
byte-identical across linear/log/symlog). The same datum landed at 4.000 rad
in SVG and 0.602 rad in the browser. The angle must be linear; a log or symlog
radial axis is untouched.
Seam ticks. Tick trimming was linear while mark culling is modular, so a
sector crossing 0/turn threw away every tick authored on the far side of it:
sector=(-30, 30) plotted a data point at theta=20 while silently dropping the
tick at 20. Both trims now use the same modular containment as
_angular_value_visible_mask, mirrored in the client's _axisTicks.
Verified by mutation: reverting the client's modular filter fails the new
browser probe, and both refusals build cleanly before the change. Cartesian
secondary axes and Cartesian tick trimming are unaffected, each with a test.
Suite 3,632 passing (+9), hooks and ruff clean, spec updated.
Four defects from CodeRabbit and a second audit pass, each reproduced first. Empty and split polar areas. _curve_path returned "" for a fully culled trace and the area join stitched that into " L Z" — malformed path data that also reached the PDF converter's _parse_path — while a trace splitting into several visible runs had its first top run stitched onto the base with a stray L. An empty vertex array separately reached the native poly-path builder, which rejects a zero-length buffer, so a log radial axis that annihilates every row and an all-NaN radar polygon raised instead of drawing nothing. Each visible run now closes against its own base, and an empty curve yields "". This is one root cause behind three separately reported symptoms. Wedge hover span. _rectHover measured a directional span, mod(x1 - x0, turn), while anchoring the offset at min(x0, x1). Both renderers draw the band as the direct unwrapped interval between the edges — GLSL takes abs(a1 - a0) with dir = a1 >= a0 ? 1 : -1, and wedge_angles takes min..max — so edge order carries no meaning and the two disagreed: a descending pair (350, 300) covered 300..610 instead of 300..350. Only the offset needs wrapping, so a seam-crossing bar whose edges are emitted unwrapped (-15..15) stays reachable from 355. Note the review's suggested shortest-arc fix was not taken: it breaks any wedge wider than 180 degrees, such as a dominant pie slice. The new test fails against both the original code and that suggestion. theta_axis(reverse=True) was accepted, rode the wire, and was ignored by every renderer — the same accepted-but-inert trap as a secondary axis. Refused, with a pointer to direction='clockwise'. r_axis(reverse=True) is honoured and untouched. wind_rose no longer leaks a raw NumPy TypeError about "a sequence of integers" for a fractional sector count. Suite 3,638 passing (+6), hooks and ruff clean, spec updated.
Third audit pass, 19 claims, each reproduced before acting. Nine of twelve groups were pre-existing on main or wrong about their baseline; these four are this PR's. pie_chart crashed at render on any zero-valued slice. The factory validates values as finite and non-negative, so 0 is legal input by its own gate, but a zero span reached bar(width=...) and died as "bar width must be positive" — an error from a layer below naming neither pie_chart nor the label, and a 500 at page render rather than an authoring error. A slice of no size draws nothing, so it is skipped. coords="cartesian" silently un-polared five helpers. `coords` is the only thing making them polar (Chart.kind is inert), so the keyword returned unlabelled rects with no axes and dropped any authored theta/r axis. Worse, it re-opened every refusal this PR added: _validate_coords returns early for a non-polar figure, so histogram marks, theta reverse and secondary radial axes all built cleanly through it. Verified no test, example, doc, script or pyplot call site passes coords to these five. A time angular axis shipped when inferred. The declared spelling theta_axis(type_="time") was already refused, but a datetime column resolved to kind="time" pinned to a fixed 0..2pi range — twelve consecutive days wrapped the disc billions of times under radian spoke labels, with no escape hatch since theta_axis(domain=) is aliased to sector. The check now uses the resolved kind and explains the mapping to write instead. A time radial axis is unaffected. bar/column/errorbar had fallen out of POLAR_DIRECT_CEILING — a regression inside this PR, not something the audit found: 8d887a2 narrowed the gate to {line, scatter, area} so heatmap/contour cell grids could exceed the point ceiling, and un-capped the three most expensive marks as collateral. A polar bar costs 2*(96+1) verts per wedge against a cartesian quad's 4, and a million of them built without a word. Also: radar's fill=False outline treated a legal line_width=0 as unset and substituted the 2.0 default, so asking for no outline gave the thickest one. Suite 3,647 passing (+9), hooks and ruff clean, spec updated.
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 |
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.