Add polar coordinate system and chart types (phase 6/7) - #388
Closed
Alek99 wants to merge 33 commits into
Closed
Conversation
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.
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.
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.
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".
…ie legend (#387) Restores alek/polar-axes to green and adds polar benchmark coverage. Fixes the five test failures the merged polar audit round introduced: the triangle-mesh buffer-cleanup assertion, the compact-colorbar plot width, the two SVG legend truncations, and the pyplot import-line check. - Widen the polar legend gutter to 22% of the canvas width, clamped to 120-200 px and floored so all three renderers land on the same integer pixel. - Restack the compact colorbar's two extreme ticks above and below the gradient, so the reservation stays at GAP + THICKNESS + 8 and the plot keeps its width. - Type the colorbar tick node that carries the stashed beside-bar CSS, so tsc accepts it and js/build.mjs can emit a bundle again. - Skip the DPR rescale for a trace whose CPU style/radius mirror no longer spans every row on the GPU, which a streaming tail append leaves behind; the existing append-time rebuild renormalizes those instead. - Fix the pie legend's sideways scrollbar with shrinkable grid columns, and stop pie_chart printing the value and an identical share. - Add benchmarks/test_codspeed_polar.py (6 rows) and the polar_coordinate_system category, taking the CodSpeed suite to 109 rows, with a test that pins the count to the methodology spec. - Raise the widest render smoke's chromium timeout, which sat 22 s from its cap.
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (48)
📝 WalkthroughWalkthroughPolar support is added across the declarative API, pyplot compatibility layer, WebGL, SVG, PDF, and native raster renderers. The change introduces polar chart factories, axis controls, wedge styling, polar interactions, protocol/ABI updates, documentation, and extensive automated smoke and regression coverage. ChangesPolar chart API and contracts
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🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes
|
Alek99
force-pushed
the
claude/resolve-merge-conflicts-88drf7
branch
from
July 30, 2026 18:35
407d940 to
6d4e99c
Compare
Member
Author
|
What fails The Why it isn't from this branch PR #370 fails the identical assertion at Already cleared by the rebase onto
Why I am not pushing a fix The failure is in the polar static-export path owned by #370, and I have no way to verify a change to it from this environment: PyPI and the npm registry are both unreachable here, so numpy/Pillow cannot be installed to run the smoke script and the client bundle cannot be rebuilt. A speculative rasterizer edit I could not execute once would not be worth the risk. Fixing it alongside the rest of #370's polar work looks like the right home for it. Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/pyplot/_axes.py (1)
3824-3860: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Axes.set()'s docstring still says projection must stay rectilinear.Line 3831 contradicts the new behaviour, which accepts
"polar".📝 Proposed fix
- ``aspect``, ``adjustable``, ``facecolor``, and ``axisbelow`` - (``projection`` must stay rectilinear). Unknown names raise loudly. + ``aspect``, ``adjustable``, ``facecolor``, ``axisbelow``, and + ``projection`` (``"polar"`` or ``"rectilinear"``). Unknown names + raise loudly.🤖 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/_axes.py` around lines 3824 - 3860, Update the Axes.set() docstring to reflect that the projection argument accepts both "polar" and "rectilinear", while retaining the existing behavior for unsupported projection values.
🧹 Nitpick comments (20)
docs/app/tests/test_docs_site.py (1)
2281-2312: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
/charts/pie-chart/entry in the factory-API-table coverage.
radar-chart,radial-bar-chart, andwind-roseall got new entries here, but/charts/pie-chart/— proven elsewhere in this same file (line 1461) to be a standalone page carryingxy.polar_bar_chart(— isn't covered by this test, so its generated API section isn't validated at all.✅ Proposed addition
"/charts/radial-bar-chart/": ("xy.polar_bar_chart",), + "/charts/pie-chart/": ("xy.polar_bar_chart",), "/charts/wind-rose/": ("xy.wind_rose",),🤖 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 `@docs/app/tests/test_docs_site.py` around lines 2281 - 2312, Add a "/charts/pie-chart/" entry to the expected mapping in test_chart_gallery_pages_append_factory_api_tables, associating it with the standalone xy.polar_bar_chart factory API. Keep the existing chart-page coverage and assertion flow unchanged.scripts/polar_phase7_smoke.py (2)
579-579: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the decoded image.
Image.openon aBytesIOleaves the image object unclosed; wrap it so repeated cases do not accumulate open decoders.♻️ Context manager
- image = np.asarray(Image.open(io.BytesIO(png)).convert("RGB")) + with Image.open(io.BytesIO(png)) as opened: + image = np.asarray(opened.convert("RGB"))🤖 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/polar_phase7_smoke.py` at line 579, Update the image decoding in the smoke-test flow around Image.open so the opened image is managed by a context manager and closed after conversion to RGB. Preserve the existing np.asarray result while ensuring repeated cases do not retain open image decoders.
447-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing wire keys surface as
KeyErrorrather than the intended diagnostic.x_axis["grid_shape"],x_axis["sector"], andy_axis["hole"]are indexed directly, so an omitted field (exactly the regression this script guards) fails with a traceback instead of the "did not reach the wire" message.♻️ Use `.get` with sentinels
- if x_axis["grid_shape"] != case.grid_shape: + if x_axis.get("grid_shape") != case.grid_shape: raise AssertionError(f"{case.name}: grid shape did not reach the wire") - if case.sector is not None and tuple(x_axis["sector"]) != case.sector: + if case.sector is not None and tuple(x_axis.get("sector") or ()) != case.sector: raise AssertionError(f"{case.name}: sector did not reach the wire") - if not math.isclose(float(y_axis["hole"]), case.hole): + if not math.isclose(float(y_axis.get("hole", math.nan)), case.hole): raise AssertionError(f"{case.name}: hole did not reach the wire")🤖 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/polar_phase7_smoke.py` around lines 447 - 465, Update the wire-field assertions in the spec validation block to use sentinel-backed get lookups for x_axis["grid_shape"], x_axis["sector"], and y_axis["hole"]. Preserve the existing comparisons and assertion messages, ensuring omitted keys produce the intended “did not reach the wire” diagnostics instead of KeyError.tests/test_trace_buffer_lifecycle.py (2)
56-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExact-string teardown assertions are formatter-brittle. Matching full call text including the trailing semicolon and single spaces means any reformatting of
js/src/50_chartview.ts/js/src/45_lod.tsbreaks the test without a real regression. Consider whitespace-tolerant regexes (e.g._deleteBuffers\(\s*g\.drill\s*,\s*TRACE_GPU_BUFFERS\s*\)).🤖 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_trace_buffer_lifecycle.py` around lines 56 - 69, Update test_every_teardown_path_reads_the_shared_buffer_list to use whitespace-tolerant regular-expression assertions for each _deleteBuffers call instead of exact strings with fixed spacing and semicolons. Preserve validation of the same receivers and buffer lists, including the retained _homeDecimated geometry buffers.
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse failure raises
IndexErrorinstead of the intended assertion message. If the declaration is ever reformatted (e.g.export const TRACE_GPU_BUFFERS: string[] = [),split(...)[1]raises before the friendly check at Line 45 can run.♻️ Regex-based extraction with a clear failure
def _trace_gpu_buffers() -> list[str]: source = (JS_SRC / "00_header.ts").read_text(encoding="utf-8") - block = source.split("export const TRACE_GPU_BUFFERS = [", 1)[1].split("];", 1)[0] - return re.findall(r'"([^"]+)"', block) + match = re.search( + r"export const TRACE_GPU_BUFFERS\b[^=]*=\s*\[(.*?)\]", source, re.DOTALL + ) + assert match, "TRACE_GPU_BUFFERS declaration not found in js/src/00_header.ts" + return re.findall(r'"([^"]+)"', match.group(1))🤖 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_trace_buffer_lifecycle.py` around lines 28 - 31, Update _trace_gpu_buffers to extract TRACE_GPU_BUFFERS with a regex that tolerates declaration formatting such as an optional type annotation, and explicitly assert or raise with a clear message when the declaration cannot be found before accessing capture groups. Preserve the existing quoted-string extraction behavior for valid declarations.tests/test_polar_transform.py (1)
136-144: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion is weaker than the documented behaviour.
px < 200.0also passes for any point left of centre; the mirroring claim would be pinned exactly bypx == approx(0.0)(r=-1 of range [0,1] mirrors to the full radius on the opposite side).♻️ Tighter pin
- px, _ = project(0.0, -1.0) - assert float(px) < 200.0 + px, py = project(0.0, -1.0) + assert float(px) == pytest.approx(0.0, abs=1e-9) + assert float(py) == pytest.approx(200.0, abs=1e-9)🤖 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_transform.py` around lines 136 - 144, Strengthen test_negative_radius_falls_inside_the_ring_it_came_from by asserting that px equals pytest.approx(0.0), preserving the documented exact mirrored position for r=-1 with range [0,1] instead of only checking that it lies left of centre.tests/test_type_surface.py (1)
359-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrive the required arguments from a table. The
if/elifchain and the exclusion set at Line 371 repeat the same three names, so a new argument-taking factory has to be added in two places.♻️ Single source for the special cases
def test_chart_factories_construct_named_lazy_charts() -> None: + required_args: dict[str, tuple] = { + "radar_chart": (["a", "b", "c"],), + "wind_rose": ([0.0], [1.0]), + "pie_chart": (["a", "b"], [1.0, 2.0]), + } for name in CHART_FACTORIES: - if name == "radar_chart": - chart = components.radar_chart(["a", "b", "c"]) - elif name == "wind_rose": - chart = components.wind_rose([0.0], [1.0]) - elif name == "pie_chart": - chart = components.pie_chart(["a", "b"], [1.0, 2.0]) - else: - chart = getattr(components, name)() + args = required_args.get(name, ()) + chart = getattr(components, name)(*args) assert isinstance(chart, components.Chart), name assert chart.kind == name - if name not in {"radar_chart", "wind_rose", "pie_chart"}: + if name not in required_args: assert chart.children == ()🤖 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_type_surface.py` around lines 359 - 372, Refactor test_chart_factories_construct_named_lazy_charts so required factory arguments come from a single table mapping factory names to their call arguments. Use that table to invoke special-case factories and determine which factories should skip the chart.children assertion, eliminating the duplicated if/elif chain and exclusion set.scripts/polar_parity_smoke.py (1)
53-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the colour-palette bound.
COLORS[i]raises a bareIndexErroras soon as a fixture case carries a seventh point; the "max 6 points per case" invariant lives only in a comment.🛡️ Explicit failure
for i, point in enumerate(case["points"]): + if i >= len(COLORS): + raise SystemExit( + f"{case['name']}: {len(case['points'])} points exceeds the " + f"{len(COLORS)} distinguishable probe colours" + ) rn = (point["r"] - r_lo) / spanAlso applies to: 97-103
🤖 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/polar_parity_smoke.py` around lines 53 - 54, Enforce the six-point palette limit in the fixture-case handling that indexes COLORS, including the logic around COLORS[i]. Validate the number of points before indexing and raise an explicit, descriptive failure when a case exceeds six points, while preserving normal processing for valid cases.tests/test_polar_charts.py (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
POLAR_DIRECT_CEILINGis imported three times.The module-level import on line 22 already binds it; the function-local re-imports on lines 584 and 1511 are redundant.
♻️ Drop the local re-imports
def test_polar_point_ceiling_is_enforced() -> None: - from xy.config import POLAR_DIRECT_CEILING - theta = np.zeros(POLAR_DIRECT_CEILING + 1)Also applies to: 584-584, 1511-1511
🤖 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` at line 22, Remove the redundant function-local imports of POLAR_DIRECT_CEILING from the functions near the referenced locations, and rely on the existing module-level import from xy.config. Leave the module-level binding and all usages unchanged.tests/test_polar_audit_fixes.py (1)
454-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion pins exact indentation, not behaviour.
"node.hidden = compactVertical\n && Number.isFinite(fraction)"bakes in the eight-space continuation indent, so a Prettier width change or a wrapped-line reflow breaks it with no behaviour change. The neighbouring assertions on lines 455-456 already cover the endpoint-preserving conditions; matching without the embedded newline/indent (e.g. a whitespace-insensitive regex) keeps the intent without the formatter coupling.🤖 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_audit_fixes.py` around lines 454 - 456, Update the CHARTVIEW assertion in the test around the node.hidden condition to avoid matching the embedded newline and exact continuation indentation. Use whitespace-insensitive matching while preserving verification of the compactVertical and Number.isFinite(fraction) conditions; leave the neighboring endpoint assertions unchanged.docs/app/xy_docs/sidebar.py (1)
27-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the gallery sidebar less brittle.
The gallery section still depends on exactDOCS_SECTIONS[3]titles and positional indexes, so a rename or reorder will break the sidebar at import time. A small helper that resolves the gallery section by name, or a single upfront assertion for the expected titles, would make this safer.🤖 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 `@docs/app/xy_docs/sidebar.py` around lines 27 - 68, The chart gallery sidebar setup is brittle because _chart_gallery relies on DOCS_SECTIONS[3] and positional fields. Update the initialization around _chart_gallery to resolve the gallery section by its identifying title (or add one upfront validation covering the expected structure) before accessing its routes, while preserving CHART_GALLERY_SIDEBAR_LINK and CHART_FAMILY_SIDEBAR_SECTIONS behavior.docs/app/xy_docs/api_reference.py (1)
236-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared
**propsexpansion.The forwarded-axis branch and the chart-factory branch below are structurally identical (merge descriptions → collect
existing_names→ filter donor params → splice at theVAR_KEYWORDslot). One helper parameterized by(donor_callable, excluded_names)would remove the duplication and keep both paths in step.♻️ Sketch
+def _expanded_parameters( + parameters: tuple[inspect.Parameter, ...], + donor: Callable[..., Any], + descriptions: Mapping[str, str], + excluded: frozenset[str] = frozenset(), +) -> tuple[tuple[inspect.Parameter, Mapping[str, str]], ...]: + merged = {**_parameter_descriptions(inspect.getdoc(donor) or ""), **descriptions} + existing = {p.name for p in parameters if p.kind is not inspect.Parameter.VAR_KEYWORD} + forwarded = tuple( + p + for p in inspect.signature(donor).parameters.values() + if p.name not in existing + and p.name not in excluded + and p.kind is not inspect.Parameter.VAR_KEYWORD + ) + documented: list[tuple[inspect.Parameter, Mapping[str, str]]] = [] + for parameter in parameters: + if parameter.kind is inspect.Parameter.VAR_KEYWORD: + documented.extend((f, merged) for f in forwarded) + continue + documented.append((parameter, merged)) + return tuple(documented)🤖 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 `@docs/app/xy_docs/api_reference.py` around lines 236 - 287, Extract the duplicated VAR_KEYWORD expansion logic from the forwarded-axis and chart-factory branches into a shared helper parameterized by the donor callable and excluded parameter names. Have the helper merge descriptions, collect existing names, filter donor parameters, and splice them at the **props slot; update both branches to reuse it while preserving their current donor-specific exclusions and description precedence.python/xy/components.py (1)
2703-2712: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEmpty sequences are refused as if they were authored values.
value == {}special-cases only an empty dict, sotheta_axis(minor_tick_values=[])raises even though it asks for nothing. A plain falsiness test covers both, and for these four keywords a falsy value is always "no-op" (tick_label_min_gap=0included).♻️ Proposed tweak
- value = kwargs.get(key) - if value is None or value == {}: + value = kwargs.get(key) + if not value: continue🤖 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 2703 - 2712, Update _refuse_inert_polar_axis_kwargs to skip inert keyword arguments whenever their retrieved value is falsy, replacing the current None-or-empty-dict check. This must allow empty sequences and zero-valued options such as tick_label_min_gap=0 while continuing to reject truthy unsupported values.python/xy/pyplot/_axes.py (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe shim now imports a private components helper.
_polar_axis_kwargsis documented inpython/xy/components.pyas existing specifically for this adapter, so the intent is clear and the dependency direction is still one-way. Still, consider promoting it (or a thin public wrapper) so the shim keeps depending only on the public composition API.As per path instructions: "Keep the matplotlib shim fully contained and dependent only one way on the public composition API."
🤖 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/_axes.py` at line 31, Update the matplotlib shim’s import in _axes.py so it no longer depends directly on the private _polar_axis_kwargs helper. Promote that helper or add a thin public wrapper in the composition API, then import and use the public symbol from the shim while preserving the existing behavior and one-way dependency.Source: Path instructions
python/xy/marks.py (1)
798-805: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
wedge_gapby keyword.
_rect_mark_styletakes five positional parameters before it; a keyword here keeps the call site from silently binding to the wrong slot if that signature grows again.♻️ Proposed tweak
fill, - wedge_gap, + wedge_gap=wedge_gap, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/marks.py` around lines 798 - 805, Update the _rect_mark_style call in the surrounding mark-style construction to pass wedge_gap as a keyword argument, while leaving the existing positional arguments unchanged.python/xy/_svg.py (2)
5666-5671: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
out = [].
outis already initialized at line 5666; the polar branch re-assigns it at 5671.🧹 Drop the duplicate initialization
if polar is not None: # Four edge columns are an annular sector: (x0, x1) is the angular span # and (y0, y1) the radial one. This is the path unequal-width slices (a # pie or donut) take, since the compact bar path ships one scalar width. - out = [] for i in range(len(x0v)):🤖 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/_svg.py` around lines 5666 - 5671, Remove the redundant out = [] assignment inside the polar is not None branch, while preserving the initial out initialization and the branch’s existing behavior.
3858-3910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the polar guard out of the four tick/grid loops.
for v in xmt: if polar is not None: breakreads as loop control but is really a precondition, and it is repeated four times. A singleif polar is None:block around the cartesian grid emission says the same thing once.🤖 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/_svg.py` around lines 3858 - 3910, In the grid-generation code, replace the repeated polar guards inside the xmt, ymt, xt, and yt loops with one outer if polar is None block enclosing all four Cartesian grid-emission loops. Keep the existing hide_x and hide_y checks and line-generation behavior unchanged, while leaving _polar_grid handling outside this block.python/xy/_payload.py (1)
596-601: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider recording the declined density tier on the wire.
A polar scatter that would otherwise have shipped
tier="density"silently shipsdirectinstead. The shippedtiershows the outcome but not the decision; the density path in this same file records its own reductions explicitly (overlay_omitted,dropped_channels). A smallentry["density_omitted"] = "polar"keeps §28 literally true for this branch.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/_payload.py` around lines 596 - 601, Update the polar branch in the trace payload construction around use_density() to record the declined density decision as entry["density_omitted"] = "polar" when density is requested but self.coords is not polar. Preserve the existing direct-tier behavior and existing density reduction metadata.Source: Coding guidelines
js/src/53_interaction.ts (1)
2167-2177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment says "CENTRE", code anchors the radial minimum.
anchor = 0pinsc0(r_lo) and scalesc1— which is what the rest of the comment and_zoomBydescribe. "Anchors at the CENTRE" reads as the disc centre and contradicts theanchorFracsemantics two functions away.✏️ Reword
- // Polar radial zoom anchors at the CENTRE, always: an interior anchor + // Polar radial zoom anchors at the radial MINIMUM, always: an interior anchor // lifts r_lo and carves a hole in the middle of the disc — an annulus view🤖 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 2167 - 2177, Update the comments immediately above the polarRadial anchor calculation to describe anchoring at the radial minimum or fixed inner boundary, rather than claiming it anchors at the disc “CENTRE.” Keep the existing polarRadial behavior and anchor assignment unchanged, and align the wording with _zoomBy and anchorFrac semantics.js/src/52_tooltip.ts (1)
256-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
_isNamedSingleWedge._defaultTooltipItemsalready calls_namedWedgedirectly, and nothing else references this helper.🤖 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 _namedWedge implementation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/charts/pie-chart.md`:
- Around line 692-696: The pie guide must document the shipped public
xy.pie_chart(labels, values) API while explaining that it composes xy.bar()
marks inside xy.polar_bar_chart(). Update docs/charts/pie-chart.md lines 692-696
accordingly, and align spec/api/chart-roadmap.md line 123 with the same
implemented contract and composition wording.
In `@docs/overview/gallery.md`:
- Around line 42-47: Update the “pie and donut charts” description in the Polar
families list to use the grammatically consistent wording “for shares, progress
rings, and gauges,” while preserving the existing linked chart-family name and
surrounding entries.
In `@js/src/50_chartview.ts`:
- Around line 7299-7326: Optimize _nearestPolarCpuIndex by hoisting polar
geometry and axis-related lookups outside the per-point loop, avoiding repeated
resolution through _projectDataPoint/_polarProject. Compare squared coordinate
distances inside the loop instead of calling Math.hypot for every point, while
ensuring the returned distance remains in the units expected by _hoverAt’s
hit.dist comparisons.
In `@python/xy/_figure.py`:
- Around line 1394-1404: Align the polar centre-origin logic across both sites:
in python/xy/_figure.py lines 1394-1404, apply the singleton rule only when lo
>= 0.0 and self._axis_kind(axis_id) is not "time"; in python/xy/pyplot/_axes.py
lines 4520-4528, apply the same exemptions before adjusting lo. Prefer a shared
Figure helper used by the singleton and padded branches, with the pyplot preview
delegating to it, so all paths produce the same radial range.
In `@python/xy/components.py`:
- Around line 6322-6352: The _radar_outline function must remove or remap
area-only style properties before converting the mark to kind="line". Update the
replace call to provide a line-compatible style, excluding fill and fill-opacity
while preserving supported style properties and the existing outline props
mapping.
In `@python/xy/pyplot/_axes.py`:
- Around line 5755-5795: Invalidate the cached chart after updating polar
options in set_theta_zero_location, set_theta_direction, and set_theta_offset by
calling the class’s established invalidation method. Preserve validation and
option assignment behavior, and ensure projection setup remains safe when these
setters are used after a chart has already been built.
- Around line 1097-1127: Update _cached_axis to accept a polar boolean and use
it to apply _polar_axis_kwargs for both x and y axes whenever the owning axes is
polar, regardless of authored theta, hole, or r_origin keys. Pass
self._projection-derived polar state from _build_chart_uncached, preserving the
existing Cartesian behavior and axis-specific theta/radial handling.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 1668-1695: The polar branch in fill() currently uses _add("area")
with base=0.0, incorrectly closing polygons to the origin. Update the branch
around PolyCollection creation to preserve the edge between the first and last
vertices by using the existing closed polar-polygon path mechanism, or
explicitly reject polar fill() instead of routing it through area.
In `@README.md`:
- Around line 299-301: Update the “Pie / donut” roadmap bullet in README.md to
reflect that xy.pie_chart(xy.pie(...)) is now available, leaving only automatic
labels described as pending. Remove the outdated wording that presents the
dedicated convenience as outstanding.
In `@scripts/polar_parity_smoke.py`:
- Around line 160-162: Update the smoke probe’s canvas JavaScript and comparison
logic to account for the plot origin: include view.plot.x and view.plot.y
alongside width and height in the exported title, and derive expected centers
using plot_x + plot_w / 2 and plot_y + plot_h / 2. Preserve the existing DPR
normalization and centroid output behavior.
- Line 38: Update the import bootstrap in scripts/polar_parity_smoke.py before
the _protocol import so the scripts directory is added to sys.path, matching the
established pattern used by the other smoke probes. Ensure both direct execution
and python -m scripts.polar_parity_smoke or repo-root module imports can resolve
_protocol.
In `@spec/design/polar-axes.md`:
- Line 87: Add language identifiers to all four fenced code blocks in the
polar-axes specification: use text for the transform/inverse pseudocode blocks
and glsl for the shader block, ensuring every fence satisfies MD040.
In `@spec/matplotlib/compat-matrix.md`:
- Line 29: Update the errorbar entry in the matplotlib compatibility matrix to
include the existing 55_polar_projection.py corpus alongside
47_statistical_families.py, so the inventory reflects documented polar errorbar
coverage.
In `@tests/test_polar_charts.py`:
- Around line 1458-1467: Update the pie-chart label rendering exercised by
test_pie_chart_renders_a_zero_valued_slice so the full “Organic” label remains
present in the SVG instead of being truncated to “Orga...”. Reuse the same
label-width or truncation fix required by the related legend path, while
preserving omission of the zero-valued “Partner” slice.
- Around line 166-169: Update the shared legend-label rendering or layout logic
exercised by test_polar_legend_survives_the_disc_clip so labels are not
ellipsised before SVG generation; preserve the complete “series one” text in the
resulting document and apply the fix at the common root cause shared with
pie-label rendering.
---
Outside diff comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 3824-3860: Update the Axes.set() docstring to reflect that the
projection argument accepts both "polar" and "rectilinear", while retaining the
existing behavior for unsupported projection values.
---
Nitpick comments:
In `@docs/app/tests/test_docs_site.py`:
- Around line 2281-2312: Add a "/charts/pie-chart/" entry to the expected
mapping in test_chart_gallery_pages_append_factory_api_tables, associating it
with the standalone xy.polar_bar_chart factory API. Keep the existing chart-page
coverage and assertion flow unchanged.
In `@docs/app/xy_docs/api_reference.py`:
- Around line 236-287: Extract the duplicated VAR_KEYWORD expansion logic from
the forwarded-axis and chart-factory branches into a shared helper parameterized
by the donor callable and excluded parameter names. Have the helper merge
descriptions, collect existing names, filter donor parameters, and splice them
at the **props slot; update both branches to reuse it while preserving their
current donor-specific exclusions and description precedence.
In `@docs/app/xy_docs/sidebar.py`:
- Around line 27-68: The chart gallery sidebar setup is brittle because
_chart_gallery relies on DOCS_SECTIONS[3] and positional fields. Update the
initialization around _chart_gallery to resolve the gallery section by its
identifying title (or add one upfront validation covering the expected
structure) before accessing its routes, while preserving
CHART_GALLERY_SIDEBAR_LINK and CHART_FAMILY_SIDEBAR_SECTIONS behavior.
In `@js/src/52_tooltip.ts`:
- Around line 256-258: Remove the unused _isNamedSingleWedge method. Keep
_defaultTooltipItems using _namedWedge directly and leave the _namedWedge
implementation unchanged.
In `@js/src/53_interaction.ts`:
- Around line 2167-2177: Update the comments immediately above the polarRadial
anchor calculation to describe anchoring at the radial minimum or fixed inner
boundary, rather than claiming it anchors at the disc “CENTRE.” Keep the
existing polarRadial behavior and anchor assignment unchanged, and align the
wording with _zoomBy and anchorFrac semantics.
In `@python/xy/_payload.py`:
- Around line 596-601: Update the polar branch in the trace payload construction
around use_density() to record the declined density decision as
entry["density_omitted"] = "polar" when density is requested but self.coords is
not polar. Preserve the existing direct-tier behavior and existing density
reduction metadata.
In `@python/xy/_svg.py`:
- Around line 5666-5671: Remove the redundant out = [] assignment inside the
polar is not None branch, while preserving the initial out initialization and
the branch’s existing behavior.
- Around line 3858-3910: In the grid-generation code, replace the repeated polar
guards inside the xmt, ymt, xt, and yt loops with one outer if polar is None
block enclosing all four Cartesian grid-emission loops. Keep the existing hide_x
and hide_y checks and line-generation behavior unchanged, while leaving
_polar_grid handling outside this block.
In `@python/xy/components.py`:
- Around line 2703-2712: Update _refuse_inert_polar_axis_kwargs to skip inert
keyword arguments whenever their retrieved value is falsy, replacing the current
None-or-empty-dict check. This must allow empty sequences and zero-valued
options such as tick_label_min_gap=0 while continuing to reject truthy
unsupported values.
In `@python/xy/marks.py`:
- Around line 798-805: Update the _rect_mark_style call in the surrounding
mark-style construction to pass wedge_gap as a keyword argument, while leaving
the existing positional arguments unchanged.
In `@python/xy/pyplot/_axes.py`:
- Line 31: Update the matplotlib shim’s import in _axes.py so it no longer
depends directly on the private _polar_axis_kwargs helper. Promote that helper
or add a thin public wrapper in the composition API, then import and use the
public symbol from the shim while preserving the existing behavior and one-way
dependency.
In `@scripts/polar_parity_smoke.py`:
- Around line 53-54: Enforce the six-point palette limit in the fixture-case
handling that indexes COLORS, including the logic around COLORS[i]. Validate the
number of points before indexing and raise an explicit, descriptive failure when
a case exceeds six points, while preserving normal processing for valid cases.
In `@scripts/polar_phase7_smoke.py`:
- Line 579: Update the image decoding in the smoke-test flow around Image.open
so the opened image is managed by a context manager and closed after conversion
to RGB. Preserve the existing np.asarray result while ensuring repeated cases do
not retain open image decoders.
- Around line 447-465: Update the wire-field assertions in the spec validation
block to use sentinel-backed get lookups for x_axis["grid_shape"],
x_axis["sector"], and y_axis["hole"]. Preserve the existing comparisons and
assertion messages, ensuring omitted keys produce the intended “did not reach
the wire” diagnostics instead of KeyError.
In `@tests/test_polar_audit_fixes.py`:
- Around line 454-456: Update the CHARTVIEW assertion in the test around the
node.hidden condition to avoid matching the embedded newline and exact
continuation indentation. Use whitespace-insensitive matching while preserving
verification of the compactVertical and Number.isFinite(fraction) conditions;
leave the neighboring endpoint assertions unchanged.
In `@tests/test_polar_charts.py`:
- Line 22: Remove the redundant function-local imports of POLAR_DIRECT_CEILING
from the functions near the referenced locations, and rely on the existing
module-level import from xy.config. Leave the module-level binding and all
usages unchanged.
In `@tests/test_polar_transform.py`:
- Around line 136-144: Strengthen
test_negative_radius_falls_inside_the_ring_it_came_from by asserting that px
equals pytest.approx(0.0), preserving the documented exact mirrored position for
r=-1 with range [0,1] instead of only checking that it lies left of centre.
In `@tests/test_trace_buffer_lifecycle.py`:
- Around line 56-69: Update
test_every_teardown_path_reads_the_shared_buffer_list to use whitespace-tolerant
regular-expression assertions for each _deleteBuffers call instead of exact
strings with fixed spacing and semicolons. Preserve validation of the same
receivers and buffer lists, including the retained _homeDecimated geometry
buffers.
- Around line 28-31: Update _trace_gpu_buffers to extract TRACE_GPU_BUFFERS with
a regex that tolerates declaration formatting such as an optional type
annotation, and explicitly assert or raise with a clear message when the
declaration cannot be found before accessing capture groups. Preserve the
existing quoted-string extraction behavior for valid declarations.
In `@tests/test_type_surface.py`:
- Around line 359-372: Refactor test_chart_factories_construct_named_lazy_charts
so required factory arguments come from a single table mapping factory names to
their call arguments. Use that table to invoke special-case factories and
determine which factories should skip the chart.children assertion, eliminating
the duplicated if/elif chain and exclusion set.
🪄 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: e378ae5d-50b1-4ab8-a963-7547324bed61
📒 Files selected for processing (87)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mddocs/api-reference/changelog.mddocs/api-reference/chart-factories.mddocs/api-reference/limitations-and-alpha-status.mddocs/app/scripts/check_html_routes.pydocs/app/tests/test_docs_site.pydocs/app/xy_docs/api_reference.pydocs/app/xy_docs/config.pydocs/app/xy_docs/gallery.pydocs/app/xy_docs/sidebar.pydocs/charts/contour-plot.mddocs/charts/heatmap.mddocs/charts/pie-chart.mddocs/charts/polar-chart.mddocs/charts/radar-chart.mddocs/charts/radial-bar-chart.mddocs/charts/uncertainty.mddocs/charts/wind-rose.mddocs/components/annotations.mddocs/components/modebars-and-interaction-controls.mddocs/components/tooltips.mddocs/core-concepts/interactions.mddocs/core-concepts/large-data-and-performance.mddocs/guides/display-and-export.mddocs/integrations/matplotlib.mddocs/overview/gallery.mddocs/styling/capabilities.mdjs/src/00_header.tsjs/src/30_ticks.tsjs/src/40_gl.tsjs/src/45_lod.tsjs/src/50_chartview.tsjs/src/51_annotations.tsjs/src/52_tooltip.tsjs/src/53_interaction.tsjs/src/56_animation.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/_textblock.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/check_public_api.pyscripts/polar_parity_smoke.pyscripts/polar_phase7_smoke.pyscripts/verify_ci_workflow.pyspec/api/capability-matrix.mdspec/api/chart-kind-contract.mdspec/api/chart-roadmap.mdspec/api/interaction.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_audit_fixes.pytests/test_polar_charts.pytests/test_polar_client_regressions.pytests/test_polar_phase7_api.pytests/test_polar_phase7_static.pytests/test_polar_transform.pytests/test_trace_buffer_lifecycle.pytests/test_type_surface.py
Comment on lines
+7299
to
+7326
| _nearestPolarCpuIndex(g, cssX, cssY) { | ||
| const cpu = g && g._cpu; | ||
| if (!cpu || !cpu.x || !cpu.y) return -1; | ||
| const xMeta = cpu.xMeta || g.xMeta; | ||
| const yMeta = cpu.yMeta || g.yMeta; | ||
| const progress = g._transitionPositionProgress; | ||
| const limit = Math.min(cpu.x.length, cpu.y.length, g.n || cpu.x.length); | ||
| const geom = this._polarGeometry(); | ||
| let best = -1; | ||
| let bestDist = Infinity; | ||
| for (let i = 0; i < limit; i++) { | ||
| const xEncoded = g._transitionPrevXValues && Number.isFinite(progress) | ||
| ? g._transitionPrevXValues[i] + (cpu.x[i] - g._transitionPrevXValues[i]) * progress | ||
| : cpu.x[i]; | ||
| const yEncoded = g._transitionPrevYValues && Number.isFinite(progress) | ||
| ? g._transitionPrevYValues[i] + (cpu.y[i] - g._transitionPrevYValues[i]) * progress | ||
| : cpu.y[i]; | ||
| const x = xEncoded / (xMeta.scale || 1) + xMeta.offset; | ||
| const y = yEncoded / (yMeta.scale || 1) + yMeta.offset; | ||
| const [chartX, chartY] = this._projectDataPoint(g.xAxis, g.yAxis, x, y, geom); | ||
| const dist = Math.hypot(chartX - this.plot.x - cssX, chartY - this.plot.y - cssY); | ||
| if (Number.isFinite(dist) && dist < bestDist) { | ||
| bestDist = dist; | ||
| best = i; | ||
| } | ||
| } | ||
| return best; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Hover projects every point through the polar transform on each pointermove.
_nearestPolarCpuIndex does a full _projectDataPoint (per-point cos/sin, axis-coord transform, geometry re-read) for up to g.n rows, and _hoverAt runs it for every non-density trace on every pointer move. With the documented 200k-point polar ceiling that is ~200k trig evaluations per move event, versus the cartesian path's cheap 1-D scan. Two cheap mitigations: hoist _polarGeometry()/axis lookups out of the loop body (already passed in, but _projectDataPoint re-resolves axes via _polarProject), and compare squared distances instead of Math.hypot.
⚡ Cheaper inner loop
- const [chartX, chartY] = this._projectDataPoint(g.xAxis, g.yAxis, x, y, geom);
- const dist = Math.hypot(chartX - this.plot.x - cssX, chartY - this.plot.y - cssY);
- if (Number.isFinite(dist) && dist < bestDist) {
- bestDist = dist;
+ const [chartX, chartY] = this._projectDataPoint(g.xAxis, g.yAxis, x, y, geom);
+ const dx = chartX - this.plot.x - cssX;
+ const dy = chartY - this.plot.y - cssY;
+ const d2 = dx * dx + dy * dy;
+ if (Number.isFinite(d2) && d2 < bestDist) {
+ bestDist = d2;
best = i;
}Note the caller compares hit.dist, so return/track the same units consistently.
📝 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
| _nearestPolarCpuIndex(g, cssX, cssY) { | |
| const cpu = g && g._cpu; | |
| if (!cpu || !cpu.x || !cpu.y) return -1; | |
| const xMeta = cpu.xMeta || g.xMeta; | |
| const yMeta = cpu.yMeta || g.yMeta; | |
| const progress = g._transitionPositionProgress; | |
| const limit = Math.min(cpu.x.length, cpu.y.length, g.n || cpu.x.length); | |
| const geom = this._polarGeometry(); | |
| let best = -1; | |
| let bestDist = Infinity; | |
| for (let i = 0; i < limit; i++) { | |
| const xEncoded = g._transitionPrevXValues && Number.isFinite(progress) | |
| ? g._transitionPrevXValues[i] + (cpu.x[i] - g._transitionPrevXValues[i]) * progress | |
| : cpu.x[i]; | |
| const yEncoded = g._transitionPrevYValues && Number.isFinite(progress) | |
| ? g._transitionPrevYValues[i] + (cpu.y[i] - g._transitionPrevYValues[i]) * progress | |
| : cpu.y[i]; | |
| const x = xEncoded / (xMeta.scale || 1) + xMeta.offset; | |
| const y = yEncoded / (yMeta.scale || 1) + yMeta.offset; | |
| const [chartX, chartY] = this._projectDataPoint(g.xAxis, g.yAxis, x, y, geom); | |
| const dist = Math.hypot(chartX - this.plot.x - cssX, chartY - this.plot.y - cssY); | |
| if (Number.isFinite(dist) && dist < bestDist) { | |
| bestDist = dist; | |
| best = i; | |
| } | |
| } | |
| return best; | |
| } | |
| _nearestPolarCpuIndex(g, cssX, cssY) { | |
| const cpu = g && g._cpu; | |
| if (!cpu || !cpu.x || !cpu.y) return -1; | |
| const xMeta = cpu.xMeta || g.xMeta; | |
| const yMeta = cpu.yMeta || g.yMeta; | |
| const progress = g._transitionPositionProgress; | |
| const limit = Math.min(cpu.x.length, cpu.y.length, g.n || cpu.x.length); | |
| const geom = this._polarGeometry(); | |
| let best = -1; | |
| let bestDist = Infinity; | |
| for (let i = 0; i < limit; i++) { | |
| const xEncoded = g._transitionPrevXValues && Number.isFinite(progress) | |
| ? g._transitionPrevXValues[i] + (cpu.x[i] - g._transitionPrevXValues[i]) * progress | |
| : cpu.x[i]; | |
| const yEncoded = g._transitionPrevYValues && Number.isFinite(progress) | |
| ? g._transitionPrevYValues[i] + (cpu.y[i] - g._transitionPrevYValues[i]) * progress | |
| : cpu.y[i]; | |
| const x = xEncoded / (xMeta.scale || 1) + xMeta.offset; | |
| const y = yEncoded / (yMeta.scale || 1) + yMeta.offset; | |
| const [chartX, chartY] = this._projectDataPoint(g.xAxis, g.yAxis, x, y, geom); | |
| const dx = chartX - this.plot.x - cssX; | |
| const dy = chartY - this.plot.y - cssY; | |
| const d2 = dx * dx + dy * dy; | |
| if (Number.isFinite(d2) && d2 < bestDist) { | |
| bestDist = d2; | |
| best = i; | |
| } | |
| } | |
| return best; | |
| } |
🤖 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 7299 - 7326, Optimize
_nearestPolarCpuIndex by hoisting polar geometry and axis-related lookups
outside the per-point loop, avoiding repeated resolution through
_projectDataPoint/_polarProject. Compare squared coordinate distances inside the
loop instead of calling Math.hypot for every point, while ensuring the returned
distance remains in the units expected by _hoverAt’s hit.dist comparisons.
Comment on lines
1394
to
+1404
| configured_margin = opts.get("margin") | ||
| if lo == hi and configured_margin is None: | ||
| if self.coords == "polar" and self._axis_dim(axis_id) == "y" and scale != "log": | ||
| # Constant-radius data must not bypass the centre-origin | ||
| # default below: padding a singleton r=5 to [4.75, 5.25] draws | ||
| # a unit circle as a ring floating mid-disc, exactly the | ||
| # picture the polar branch exists to forbid. Same rule as the | ||
| # non-singleton branch: centre origin, no outer pad. | ||
| lo_out = min(0.0, lo) | ||
| hi_out = hi if hi > lo_out else lo_out + 1.0 | ||
| return (hi_out, lo_out) if opts.get("reverse") else (lo_out, hi_out) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The polar centre-origin rule is spelled out three times with three different exemption sets. Figure._range's padded branch (python/xy/_figure.py lines 1446-1461) exempts a time radius and data reaching below zero from centre-origin, keeping the ordinary padded extent; the singleton branch and the pyplot preview both re-implement the rule without those exemptions, so the same data resolves to different radial ranges depending on which path runs.
python/xy/_figure.py#L1394-L1404: gate the polar singleton branch onlo >= 0.0 and self._axis_kind(axis_id) != "time"so a constant negative radius and a single-timestamp radius fall through to the ordinary padded path, exactly as their non-singleton counterparts do.python/xy/pyplot/_axes.py#L4520-L4528: apply the same two exemptions beforelo = min(0.0, float(lo)), soget_rmin()/set_rmax()snapshot the domain the engine would actually render rather than freezing a padless one.
Consider extracting the rule into one shared helper on Figure that both the singleton and padded branches call, and having the pyplot preview delegate to it, so a future exemption cannot be added to only one copy.
📍 Affects 2 files
python/xy/_figure.py#L1394-L1404(this comment)python/xy/pyplot/_axes.py#L4520-L4528
🤖 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/_figure.py` around lines 1394 - 1404, Align the polar centre-origin
logic across both sites: in python/xy/_figure.py lines 1394-1404, apply the
singleton rule only when lo >= 0.0 and self._axis_kind(axis_id) is not "time";
in python/xy/pyplot/_axes.py lines 4520-4528, apply the same exemptions before
adjusting lo. Prefer a shared Figure helper used by the singleton and padded
branches, with the pyplot preview delegating to it, so all paths produce the
same radial range.
Comment on lines
+6322
to
+6352
| def _radar_outline(mark: "Mark", angles: list[float], values: list[float]) -> "Mark": | ||
| """Rebuild a filled radar `area` as its outline, for `fill=False`. | ||
|
|
||
| The two marks do not share a prop vocabulary — an area carries | ||
| `line_color`/`line_width`/`line_opacity` where a line carries | ||
| `color`/`width`/`opacity` — so swapping only `kind` handed `_apply_line` an | ||
| area's dict and it died on `m.props["width"]`. The outline inherits the | ||
| area's stroke settings, falling back to its fill color when the stroke was | ||
| never given one. | ||
| """ | ||
| props = mark.props | ||
| return replace( | ||
| mark, | ||
| kind="line", | ||
| x=angles, | ||
| y=values, | ||
| props={ | ||
| "color": props.get("line_color") or props.get("color"), | ||
| # `or` treated a legal 0.0 as "unset" and silently substituted the | ||
| # 2.0 default, so an author asking for no outline got the thickest | ||
| # one. Fall back only on a genuinely absent value; an explicit 0 | ||
| # then meets the library-wide "line width must be positive" rule | ||
| # instead of being quietly overridden. | ||
| "width": 2.0 if props.get("line_width") is None else props["line_width"], | ||
| "opacity": props.get("line_opacity", 1.0), | ||
| "curve": props.get("curve", "linear"), | ||
| "dash": props.get("dash"), | ||
| "x_axis": props.get("x_axis", "x"), | ||
| "y_axis": props.get("y_axis", "y"), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A25 'def compile_mark_style' python/xy/styles.py
rg -n -B2 -A20 'def _apply_line\b' python/xy/components.pyRepository: reflex-dev/xy
Length of output: 2162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' python/xy/styles.py
printf '\n---\n'
sed -n '1,220p' python/xy/components.py
printf '\n---\n'
sed -n '5560,5595p' python/xy/components.pyRepository: reflex-dev/xy
Length of output: 17939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path('python/xy/styles.py').read_text()
for name in ['_supported_mark_style_properties']:
m = re.search(rf'def {name}\(.*?\n(?=def |\Z)', text, re.S)
print(f'## {name}')
print(m.group(0)[:8000] if m else 'not found')
print('---')
PYRepository: reflex-dev/xy
Length of output: 2289
Drop area-only style props when building the radar outline. _radar_outline() keeps mark.style unchanged, so xy.area(..., fill=False, style={fill: ...}) gets recompiled as a line style and can raise on unsupported area-only CSS. Strip or remap fill/fill-opacity before replace().
🤖 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 6322 - 6352, The _radar_outline
function must remove or remap area-only style properties before converting the
mark to kind="line". Update the replace call to provide a line-compatible style,
excluding fill and fill-opacity while preserving supported style properties and
the existing outline props mapping.
Comment on lines
+160
to
+162
| const dpr=v.dpr||1; | ||
| const out=sums.map(s=>s.n?[s.x/s.n/dpr,(h-1-s.y/s.n)/dpr,s.n]:[NaN,NaN,0]); | ||
| document.title="XY_OK plot="+v.plot.w+"x"+v.plot.h+" pts="+JSON.stringify(out); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scripts/polar_parity_smoke.py (around cited lines) ---'
nl -ba scripts/polar_parity_smoke.py | sed -n '120,240p'
echo
echo '--- search for plot/title parsing and view.plot usage ---'
rg -n "view\.plot|plot=|XY_OK|polar_parity_smoke" scripts . -g '!**/.git/**'Repository: reflex-dev/xy
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('scripts/polar_parity_smoke.py')
lines = p.read_text().splitlines()
for i in range(120, min(241, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo '--- search for view.plot / XY_OK / plot= ---'
python3 - <<'PY'
from pathlib import Path
import re
for path in Path('.').rglob('*'):
if not path.is_file():
continue
if '.git' in path.parts:
continue
if path.suffix not in {'.py', '.js', '.ts', '.rs', '.md', '.txt', '.json', '.sh'}:
continue
try:
text = path.read_text()
except Exception:
continue
if any(s in text for s in ('view.plot', 'XY_OK', 'plot=')):
print(path)
for m in re.finditer(r'view\.plot|XY_OK|plot=', text):
start = max(0, m.start()-80)
end = min(len(text), m.end()+160)
snippet = text[start:end].replace('\n', '\\n')
print(' ...' + snippet + '...')
break
PYRepository: reflex-dev/xy
Length of output: 10065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scripts/polar_parity_smoke.py (full relevant region) ---'
python3 - <<'PY'
from pathlib import Path
lines = Path('scripts/polar_parity_smoke.py').read_text().splitlines()
for i in range(1, len(lines)+1):
if 1 <= i <= len(lines):
if 1 <= i <= 260:
if i < 120 or i > 228:
continue
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo '--- tests/test_polar_client_regressions.py excerpts with view.plot.x/y ---'
python3 - <<'PY'
from pathlib import Path
text = Path('tests/test_polar_client_regressions.py').read_text().splitlines()
for start in [1]:
for i, line in enumerate(text, 1):
if 'view.plot.x' in line or 'view.plot.y' in line or 'view.plot.w' in line:
lo = max(1, i-8)
hi = min(len(text), i+12)
print(f'--- lines {lo}-{hi} ---')
for j in range(lo, hi+1):
print(f"{j:4d}: {text[j-1]}")
break
PY
echo
echo '--- scripts/polar_phase7_smoke.py excerpts with view.plot.x/y ---'
python3 - <<'PY'
from pathlib import Path
text = Path('scripts/polar_phase7_smoke.py').read_text().splitlines()
for i, line in enumerate(text, 1):
if 'view.plot.x' in line or 'view.plot.y' in line or 'view.plot.w' in line:
lo = max(1, i-8)
hi = min(len(text), i+12)
print(f'--- lines {lo}-{hi} ---')
for j in range(lo, hi+1):
print(f"{j:4d}: {text[j-1]}")
break
PYRepository: reflex-dev/xy
Length of output: 7112
Export the plot origin in the smoke probe scripts/polar_parity_smoke.py:160-162,210-221 The observed centroids are in canvas space, but the expected centers are derived from plot_w/plot_h only. When view.plot.x/view.plot.y are non-zero, every point is offset by the inset; include the full rect in the title and use plot_x + plot_w / 2, plot_y + plot_h / 2 for the comparison.
🤖 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/polar_parity_smoke.py` around lines 160 - 162, Update the smoke
probe’s canvas JavaScript and comparison logic to account for the plot origin:
include view.plot.x and view.plot.y alongside width and height in the exported
title, and derive expected centers using plot_x + plot_w / 2 and plot_y + plot_h
/ 2. Preserve the existing DPR normalization and centroid output behavior.
| origin `r_origin` (default `r_lo`), display-space hole fraction `h` (default | ||
| 0), and a plot rect `(x, y, w, h_px)` in CSS pixels: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to the specification fences.
Markdownlint reports four fenced blocks without languages. Use the appropriate identifier, such as text for the transform/inverse pseudocode and glsl for the shader block, so the specification passes MD040.
Also applies to: 170-170, 219-219, 293-293
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 87, Add language identifiers to all four
fenced code blocks in the polar-axes specification: use text for the
transform/inverse pseudocode blocks and glsl for the shader block, ensuring
every fence satisfies MD040.
Source: Linters/SAST tools
|
|
||
| - `plot` — [`01_basic_line.py`](../../tests/pyplot/corpus/01_basic_line.py), [`02_plot_fmt_red_dashed.py`](../../tests/pyplot/corpus/02_plot_fmt_red_dashed.py), [`03_plot_fmt_green_circles.py`](../../tests/pyplot/corpus/03_plot_fmt_green_circles.py), [`04_plot_fmt_cycle_dashdot_square.py`](../../tests/pyplot/corpus/04_plot_fmt_cycle_dashdot_square.py), [`05_multi_series_one_call.py`](../../tests/pyplot/corpus/05_multi_series_one_call.py), [`06_implicit_x.py`](../../tests/pyplot/corpus/06_implicit_x.py), [`07_labels_title_legend_grid.py`](../../tests/pyplot/corpus/07_labels_title_legend_grid.py), [`08_xlim_ylim.py`](../../tests/pyplot/corpus/08_xlim_ylim.py), [`09_log_scale.py`](../../tests/pyplot/corpus/09_log_scale.py), [`20_fill_between_band.py`](../../tests/pyplot/corpus/20_fill_between_band.py), [`23_axhline_axvline.py`](../../tests/pyplot/corpus/23_axhline_axvline.py), [`24_axvspan_band.py`](../../tests/pyplot/corpus/24_axvspan_band.py), [`25_annotate_text.py`](../../tests/pyplot/corpus/25_annotate_text.py), [`26_twinx_dual_axis.py`](../../tests/pyplot/corpus/26_twinx_dual_axis.py), [`27_subplots_2x2_mixed.py`](../../tests/pyplot/corpus/27_subplots_2x2_mixed.py), [`28_subplots_figsize.py`](../../tests/pyplot/corpus/28_subplots_figsize.py), [`29_implicit_state_savefig.py`](../../tests/pyplot/corpus/29_implicit_state_savefig.py), [`30_savefig_html.py`](../../tests/pyplot/corpus/30_savefig_html.py), [`31_rcparams_figsize.py`](../../tests/pyplot/corpus/31_rcparams_figsize.py), [`33_set_data_mutation.py`](../../tests/pyplot/corpus/33_set_data_mutation.py), [`34_gray_string_color.py`](../../tests/pyplot/corpus/34_gray_string_color.py), [`35_tab_colors.py`](../../tests/pyplot/corpus/35_tab_colors.py), [`36_color_cycle_c0_c9.py`](../../tests/pyplot/corpus/36_color_cycle_c0_c9.py), [`37_markers_only_fmt.py`](../../tests/pyplot/corpus/37_markers_only_fmt.py), [`38_close_all_hygiene.py`](../../tests/pyplot/corpus/38_close_all_hygiene.py), [`39_multiple_figures.py`](../../tests/pyplot/corpus/39_multiple_figures.py), [`40_subplots_row_sharex.py`](../../tests/pyplot/corpus/40_subplots_row_sharex.py), [`41_line_kwargs.py`](../../tests/pyplot/corpus/41_line_kwargs.py), [`43_grid_html_suptitle.py`](../../tests/pyplot/corpus/43_grid_html_suptitle.py), [`44_subplot_classic.py`](../../tests/pyplot/corpus/44_subplot_classic.py), [`45_xticks_positions_labels.py`](../../tests/pyplot/corpus/45_xticks_positions_labels.py) | ||
| - `plot` — [`01_basic_line.py`](../../tests/pyplot/corpus/01_basic_line.py), [`02_plot_fmt_red_dashed.py`](../../tests/pyplot/corpus/02_plot_fmt_red_dashed.py), [`03_plot_fmt_green_circles.py`](../../tests/pyplot/corpus/03_plot_fmt_green_circles.py), [`04_plot_fmt_cycle_dashdot_square.py`](../../tests/pyplot/corpus/04_plot_fmt_cycle_dashdot_square.py), [`05_multi_series_one_call.py`](../../tests/pyplot/corpus/05_multi_series_one_call.py), [`06_implicit_x.py`](../../tests/pyplot/corpus/06_implicit_x.py), [`07_labels_title_legend_grid.py`](../../tests/pyplot/corpus/07_labels_title_legend_grid.py), [`08_xlim_ylim.py`](../../tests/pyplot/corpus/08_xlim_ylim.py), [`09_log_scale.py`](../../tests/pyplot/corpus/09_log_scale.py), [`20_fill_between_band.py`](../../tests/pyplot/corpus/20_fill_between_band.py), [`23_axhline_axvline.py`](../../tests/pyplot/corpus/23_axhline_axvline.py), [`24_axvspan_band.py`](../../tests/pyplot/corpus/24_axvspan_band.py), [`25_annotate_text.py`](../../tests/pyplot/corpus/25_annotate_text.py), [`26_twinx_dual_axis.py`](../../tests/pyplot/corpus/26_twinx_dual_axis.py), [`27_subplots_2x2_mixed.py`](../../tests/pyplot/corpus/27_subplots_2x2_mixed.py), [`28_subplots_figsize.py`](../../tests/pyplot/corpus/28_subplots_figsize.py), [`29_implicit_state_savefig.py`](../../tests/pyplot/corpus/29_implicit_state_savefig.py), [`30_savefig_html.py`](../../tests/pyplot/corpus/30_savefig_html.py), [`31_rcparams_figsize.py`](../../tests/pyplot/corpus/31_rcparams_figsize.py), [`33_set_data_mutation.py`](../../tests/pyplot/corpus/33_set_data_mutation.py), [`34_gray_string_color.py`](../../tests/pyplot/corpus/34_gray_string_color.py), [`35_tab_colors.py`](../../tests/pyplot/corpus/35_tab_colors.py), [`36_color_cycle_c0_c9.py`](../../tests/pyplot/corpus/36_color_cycle_c0_c9.py), [`37_markers_only_fmt.py`](../../tests/pyplot/corpus/37_markers_only_fmt.py), [`38_close_all_hygiene.py`](../../tests/pyplot/corpus/38_close_all_hygiene.py), [`39_multiple_figures.py`](../../tests/pyplot/corpus/39_multiple_figures.py), [`40_subplots_row_sharex.py`](../../tests/pyplot/corpus/40_subplots_row_sharex.py), [`41_line_kwargs.py`](../../tests/pyplot/corpus/41_line_kwargs.py), [`43_grid_html_suptitle.py`](../../tests/pyplot/corpus/43_grid_html_suptitle.py), [`44_subplot_classic.py`](../../tests/pyplot/corpus/44_subplot_classic.py), [`45_xticks_positions_labels.py`](../../tests/pyplot/corpus/45_xticks_positions_labels.py), [`55_polar_projection.py`](../../tests/pyplot/corpus/55_polar_projection.py) | ||
| - `errorbar` — [`47_statistical_families.py`](../../tests/pyplot/corpus/47_statistical_families.py) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the polar corpus to the errorbar inventory.
spec/matplotlib/compat.md documents polar errorbar support, and spec/matplotlib/compat-changelog.md records its projected-segment contract, but this matrix still lists only 47_statistical_families.py. Add 55_polar_projection.py so the generated compatibility inventory and coverage claims include the new polar behavior.
🤖 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-matrix.md` at line 29, Update the errorbar entry in
the matplotlib compatibility matrix to include the existing
55_polar_projection.py corpus alongside 47_statistical_families.py, so the
inventory reflects documented polar errorbar coverage.
Source: Coding guidelines
Comment on lines
+166
to
+169
| def test_polar_legend_survives_the_disc_clip() -> None: | ||
| theta, r = _rose() | ||
| doc = _svg(_chart(children=[xy.line(theta, r, name="series one")])) | ||
| assert "series one" in doc |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Failing in CI: the legend label is being ellipsised before it reaches the SVG.
test_polar_legend_survives_the_disc_clip fails with the document containing a truncated series... instead of series one. See the consolidated comment for the shared root cause with the pie-label failure.
🧰 Tools
🪛 GitHub Actions: CI / 5_Python 3.11 floor.txt
[error] 169-169: Assertion failed: expected SVG document to contain legend/text 'series one' but it was truncated/altered (assert 'series one' in doc failed).
🪛 GitHub Actions: CI / Python 3.11 floor
[error] 169-169: AssertionError in test_polar_legend_survives_the_disc_clip: expected SVG to contain legend label text 'series one', but SVG contains truncated/modified text ('series...').
🤖 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 166 - 169, Update the shared
legend-label rendering or layout logic exercised by
test_polar_legend_survives_the_disc_clip so labels are not ellipsised before SVG
generation; preserve the complete “series one” text in the resulting document
and apply the fix at the common root cause shared with pie-label rendering.
Comment on lines
+1458
to
+1467
| def test_pie_chart_renders_a_zero_valued_slice() -> None: | ||
| """A zero-valued category is ordinary in aggregated data and `pie_chart` | ||
| accepts it (values are validated finite and non-negative), 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.""" | ||
| doc = xy.pie_chart(["Direct", "Partner", "Organic"], [40.0, 0.0, 20.0]).figure().to_svg() | ||
| assert "Direct" in doc and "Organic" in doc | ||
| # The empty category draws no wedge rather than a zero-width one. | ||
| assert "Partner" not in doc | ||
| xy.pie_chart(["a", "b"], [1.0, 0.0]).figure().to_image(format="pdf") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Failing in CI: the third pie label is truncated to Orga....
test_pie_chart_renders_a_zero_valued_slice fails on "Organic" in doc. Same truncation path as the legend failure above; consolidated below.
🧰 Tools
🪛 GitHub Actions: CI / 5_Python 3.11 floor.txt
[error] 1464-1464: Assertion failed: pie chart SVG should include both labels 'Direct' and 'Organic' but only 'Direct' was found and 'Organic' was missing (assertion failed due to truncated SVG output).
🪛 GitHub Actions: CI / Python 3.11 floor
[error] 1464-1464: AssertionError in test_pie_chart_renders_a_zero_valued_slice: expected SVG to include both category labels 'Direct' and 'Organic' for pie slices, but 'Organic' is not present (SVG shows truncated 'Orga...').
🤖 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 1458 - 1467, Update the pie-chart
label rendering exercised by test_pie_chart_renders_a_zero_valued_slice so the
full “Organic” label remains present in the SVG instead of being truncated to
“Orga...”. Reuse the same label-width or truncation fix required by the related
legend path, while preserving omission of the zero-valued “Partner” slice.
Alek99
force-pushed
the
claude/resolve-merge-conflicts-88drf7
branch
from
July 30, 2026 18:47
6d4e99c to
b025edd
Compare
Member
Author
|
Triaged the CodeRabbit review. Note that the 15-comment pass ran against Fixed (pushed as
Not actionable as written:
Deferred to #370, whose code they are — this branch adds one docs commit on top and touches no JS, Rust, or renderer Python. Several look worth a real look there: the I did not attempt any of those here: PyPI and the npm registry are unreachable from this environment, so I cannot run the suite or rebuild the client bundle to verify a renderer change. Everything I did push is text or a self-contained test refactor, checked with Generated by Claude Code |
This was referenced Jul 30, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the polar coordinate system implementation with full renderer support across Python, TypeScript, Rust, and static exporters. Adds
polar_chart,polar_bar_chart,radar_chart,wind_rose, andpie_chartcompositions alongside newtheta_axisandr_axisdeclarative components.Summary
This change implements a complete polar coordinate system as a first-class feature in XY, following the design in
spec/design/polar-axes.md. Polar is a coordinate system (not a chart type family), so existing marks (line,scatter,bar,heatmap,contour,errorbar) render through it unchanged. The implementation spans:PolarAxesprojection support viaplt.subplots(projection="polar")Key Changes
Python (
python/xy/)components.py: Addedpolar_chart(),polar_bar_chart(),radar_chart(),wind_rose(),pie_chart()chart factories;theta_axis()andr_axis()axis components with angular/radial configuration_figure.py: Addedcoordsparameter ("cartesian" or "polar"); polar validation and mark filtering_payload.py: Polar coordinate system wire protocol; validation that only legal marks appear in polar charts_svg.py:_PolarProjectionclass implementing (θ, r) → pixel transform; polar heatmap rasterization; wedge point generation for bars_raster.py: Polar clipping primitive in display list; annular sector masking for all marksmarks.py:wedge_gapparameter for radial barsconfig.py:POLAR_MARK_KINDS,POLAR_BAR_SEGMENTS,polar_bar_segments()functionpyplot/_axes.py: Polar projection routing for matplotlib compatibility_validate.py: Coordinate system validationTypeScript (
js/src/)30_ticks.ts:angularTicks()function for θ axis tick generation (15°, 30°, 45° steps)40_gl.ts:xyPolarPos()GLSL transform;xyPolarBarSegments()adaptive wedge subdivision;POLAR_FRAGMENT_CLIP_GLSLannular sector clipping50_chartview.ts: Polar bar segment scaling; pan axis policy enforcement for polar51_annotations.ts: Polar annotation clipping and projection52_tooltip.ts: Polar coordinate projection for hover; trace name readout53_interaction.ts: Pan axis policy check for coupled coordinates55_marks.ts: Polar mark filtering00_header.ts: Protocol version bump to 11Rust (
src/raster.rs)PolarClipstruct for annular sector stateOP_POLAR_CLIPdisplay-list opcodeDocumentation
spec/design/polar-axes.md: Normative specification for coordinate transform, conventions, wire shape, and legal marksdocs/charts/polar-chart.md: Polar line, scatter, area, heatmap, contour, errorbar examplesdocs/charts/radar-chart.md: Radar/spider chart compositiondocs/charts/radial-bar-chart.md: Radial bar, progress ring, capacity block examplesdocs/charts/pie-chart.md: Pie and donut chart compositiondocs/charts/wind-rose.md: Wind rose chart compositionTests
tests/test_polar_charts.py: Wire shape, refusal of unshttps://claude.ai/code/session_01FywmKazMdRTUKj9G9wJkb1
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests