Add funnel charts to the core declarative XY API (protocol v13) - #474
Open
Alek99 wants to merge 9 commits into
Open
Add funnel charts to the core declarative XY API (protocol v13)#474Alek99 wants to merge 9 commits into
Alek99 wants to merge 9 commits into
Conversation
`xy.funnel_chart(stages, values)` / `xy.funnel(...)` draw one centered segment per stage of an ordered process. Stage order is the DECLARED order — a funnel is a categorical business process and is never sorted — and the conversion arithmetic ships with every stage rather than being left to the reader. The kernel (python/xy/_funnel.py) owns all of it at build time, the way `_sankey` owns the Sankey placement: validation, conversion/drop-off math, quad construction and the label ladder. Renderers only ever see quads plus semantic rows. Geometry is explicit because the two classic funnel drawings encode differently: `geometry="area"` tapers each segment toward the next stage (so painted area is NOT proportional to value, and the docs say so), while `geometry="bar"` gives constant-width segments whose widths carry the values exactly. `neck="rect"|"taper"` decides the spout, `gap` resolves per geometry (0 for area, 0.2 for bar), and `min_width` floors DRAWN widths so a zero stage stays visible and hoverable while its reported value stays exact. Arithmetic: overall share, previous-stage conversion, and drop-off, with `None` — rendered as an em dash — wherever a denominator is zero, never an invented number or an infinity. Increasing stages are legal and drawn honestly (conversion > 1, a signed `+12%` boundary label); negative and missing values are refused by stage name. Renderers: a new `funnel` kind. The client sweeps a 4-vertex strip per stage through FUNNEL_VS sharing RIBBON_FS — that fragment stage is where the slanted edges get their fwidth coverage, without which the long diagonals staircase on the antialias:false context. Both exporters build their corners from `_scene.funnel_quad`, the single reference the golden tests pin, so SVG and PNG cannot drift from each other or from the client. Interaction: CPU trapezoid containment returns the STAGE index with its semantic row (picking stays off, so box/lasso selection is absent rather than wrong, as for ribbon); the tooltip follows the cursor inside a segment because a segment is an area, not a point; legend rows toggle a stage's geometry AND its labels, which needed an annotation ownership tag on the wire; and `stageNav` puts the per-stage centers into keyboard traversal so a screen reader hears "Stage 2 of 5" plus that stage's arithmetic. Formatting is Python's alone: `value_format`/`percent_format` are applied once and shipped as preformatted `*_text` twins beside the numeric event fields, so a label, a tooltip and a static export cannot disagree. Two parity bugs fixed on the way, both pre-existing and both visible the moment a funnel drew its own labels: `_scene`/`_svg` had a load-order cycle that broke the capability-matrix generator, and annotation labels in both exporters ignored `--chart-annotation-text`/`--chart-text` while the live client honored them — a themed dark chart printed its labels in the light-mode default. PROTOCOL_VERSION 12 -> 13 in lockstep with the client: markOf() falls back to scatter for unknown kinds, so a v12 client would silently render funnel quads as a point cloud. Tests: 71 new (stage math incl. zero/increasing/repeated/negative, declared-order determinism, quad geometry, label ladder, wire shape, event semantics, styling surface, legend ownership, export parity against the scene reference) plus a funnel probe in the render smoke asserting per-stage palette ink, containment hover, and a11y stage nav. Suite 3,904 passing, docs-site suite green, ruff/ty clean, capability matrix regenerated.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded declarative funnel charts with configurable geometry, labels, styling, tooltips, accessibility, WebGL rendering, static exports, protocol support, documentation, and tests. ChangesFunnel chart support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant funnel_chart
participant Figure_funnel
participant _emit_funnel
participant ChartView
participant tooltip_handling
funnel_chart->>Figure_funnel: create funnel stages and layout
Figure_funnel->>_emit_funnel: emit geometry and tooltip metadata
_emit_funnel->>ChartView: provide funnel trace payload
ChartView->>tooltip_handling: decode semantic stage rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR adds funnel charts across the declarative Python API, protocol payload, WebGL client, native exporters, interaction layer, documentation, and tests.
Confidence Score: 5/5The PR appears safe to merge because no eligible blocking failure remains established in the available follow-up context. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| python/xy/_funnel.py | Implements validated stage arithmetic, area/bar quad construction, width flooring, and deterministic label placement. |
| python/xy/marks.py | Integrates funnel construction into the shared Figure mark implementation with rollback, semantic rows, color channels, and owned labels. |
| python/xy/_payload.py | Emits protocol-v13 funnel geometry columns, semantic tooltip rows, orientation, and categorical paint metadata. |
| js/src/50_chartview.ts | Adds funnel GPU-buffer construction, drawing, CPU containment, legend filtering, and theme-aware paint refresh. |
| js/src/40_gl.ts | Adds the instanced funnel vertex shader while sharing ribbon edge antialiasing and stroke behavior. |
| js/src/51_annotations.ts | Suppresses mark-owned labels when their trace or funnel stage is hidden. |
| js/src/52_tooltip.ts | Displays preformatted funnel arithmetic, cursor-following area tooltips, and stage-specific accessibility announcements. |
| python/xy/_scene.py | Defines the shared funnel-quad reference used by native exporters. |
| python/xy/_svg.py | Adds vector funnel paths using shared geometry and aligns annotation text-color fallback with chart theme variables. |
| python/xy/_raster.py | Adds raster funnel rendering using the same quad and paint semantics as SVG. |
| tests/test_funnel.py | Covers validation, arithmetic, geometry, wire output, rendering parity, ownership, and API behavior. |
Reviews (2): Last reviewed commit: "Docs: show the funnel legend with a runn..." | Re-trigger Greptile
The legend was described in prose only, so a reader had no way to see what `xy.legend(...)` on a funnel actually does. Adds an "Add a Legend" section with a live demo whose rows are clickable, and says what the click does (hides the stage's segment AND its labels, leaves the other stages' arithmetic untouched), which `loc`/`title`/`ncols` control, which chrome slots style it, and why a theme palette mapping keeps a swatch and its segment the same colour across charts.
Alek99
marked this pull request as ready for review
August 6, 2026 22:49
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
tests/test_accessibility_contract.py (1)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the funnel noun derivation in the accessibility contract test.
Assert
const noun = g && g.trace && g.trace.kind === "funnel" ? "Stage" : "Point";. The client derives the noun fromg.trace.kind, not fromMARK_KINDS.🤖 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_accessibility_contract.py` at line 69, Update the accessibility contract test’s noun derivation to use g.trace.kind, assigning “Stage” only when it equals “funnel” and “Point” otherwise; do not derive the noun from MARK_KINDS.python/xy/marks.py (1)
740-749: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMaterialize the stage sequence once.
_materialize_sequenceexists so the category probe and the label extraction do not each repeat the O(n) conversion. This block calls it two or three times on the same input. Bind it to a local first.♻️ Proposed refactor
- if self._is_category_like(self._materialize_sequence(stage)): - stage_names = self._category_axis_labels(self._materialize_sequence(stage), "funnel stage") + stage_input = self._materialize_sequence(stage) + if self._is_category_like(stage_input): + stage_names = self._category_axis_labels(stage_input, "funnel stage") else: # Numeric stages are legal input (quarter numbers, ordinal codes) but # a funnel's stage axis is categorical by contract, so they become # labels in the declared order. stage_names = [ channels.category_label(raw) - for raw in np.asarray(self._materialize_sequence(stage)).reshape(-1) + for raw in np.asarray(stage_input).reshape(-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 `@python/xy/marks.py` around lines 740 - 749, Update the stage handling block to call _materialize_sequence(stage) once and bind the result to a local variable before the _is_category_like check. Reuse that materialized sequence for _category_axis_labels and the np.asarray label extraction, preserving the existing categorical and numeric-stage behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/app/tests/test_docs_site.py`:
- Around line 1282-1293: Update INLINE_SVG_PREVIEW_COUNT to 35 and adjust the
inline SVG route validator fixture to include 35 previews, using the expectation
text “34 previews, expected 35”. Preserve len(chart_section) == 21 because the
funnel route remains a Chart Gallery leaf.
In `@docs/charts/funnel-chart.md`:
- Around line 199-209: Update the xy.funnel_chart example in the documentation
to use tilde-style Markdown fences: replace both backtick fences with an opening
~~~python fence and a closing ~~~ fence, leaving the example content unchanged.
In `@js/src/50_chartview.ts`:
- Around line 3280-3284: Update _reapplyLegendVisibility so its non-density
branch calls _applyCategoryVisibility(i) instead of directly invoking
_filterScatterRows(g, cats). Preserve the existing density handling, ensuring
funnel traces are routed through the shared dispatch and their legend visibility
is reapplied after rebuilds.
- Around line 4770-4799: Update _funnelPaint to derive the full row count from
the retained CPU funnel color record (_funnelCodes or _funnelRgba) rather than
g.n, so recoloring indexes all original stages correctly. After rebuilding
_funnelRgbaFull and uploading the color buffer, re-apply the active funnel
filter using the existing filtering symbol so visible GPU rows and retained
full-length data stay synchronized.
In `@python/xy/_raster.py`:
- Around line 1108-1118: Update the raster and SVG annotation emission paths to
share the same fallback text color when neither annotation nor chart text color
is configured, using the SVG fallback “#667085” instead of the raster-only _TEXT
value. In the raster flow around _emit_annotations, bind the repeated
_css(dom_style.get("--chart-annotation-text"), "") lookup once and reuse it for
both annotation calls.
In `@python/xy/components.py`:
- Around line 7182-7194: Update the mark-selection logic around the funnel child
check so a non-empty mark_kwargs combined with an explicit funnel Mark raises a
direct conflict error instead of appending an empty funnel(). Preserve the
existing behavior for positional stages/values and for creating a funnel when no
explicit funnel child is present.
In `@python/xy/marks.py`:
- Around line 949-968: Update _funnel_label_color to detect unsupported or
non-static CSS forms such as var() before calling _parse_color, returning None
for them so the theme default is preserved. Keep the existing luminance-based
contrast selection for statically parseable colors, while retaining the current
exception fallback for parsing failures.
- Around line 820-836: Move the self._checkpoint() call before
self._axis_positions(stage_names, stage_dim) so stage-category registration is
covered by rollback. Add a regression test using an invalid funnel value_format
that fails after category registration, then assert the failed operation leaves
"y" absent from fig._axis_categories.
In `@spec/api/chart-roadmap.md`:
- Line 132: Update or remove the stale P3 “Funnel / funnel area” roadmap entry
so it no longer describes the chart as pending or caveated, consistent with the
implemented Funnel row and current behavior. Keep the roadmap’s status and
implementation details consistent throughout the specification.
In `@spec/design/wire-protocol.md`:
- Around line 472-478: Update the protocol version references near
PROTOCOL_VERSION and PROTOCOL from 12 to 13, and revise the compatibility text
to state that the v13 handshake rejects cached v12 clients. Keep the existing
v13 funnel schema description and ensure the compatibility section consistently
reflects the v13 handshake behavior.
In `@tests/test_type_surface.py`:
- Line 22: Add "funnel_chart" to the CHART_FACTORIES collection in the factory
type-surface test so this root export is included in the tested chart factories.
---
Nitpick comments:
In `@python/xy/marks.py`:
- Around line 740-749: Update the stage handling block to call
_materialize_sequence(stage) once and bind the result to a local variable before
the _is_category_like check. Reuse that materialized sequence for
_category_axis_labels and the np.asarray label extraction, preserving the
existing categorical and numeric-stage behavior.
In `@tests/test_accessibility_contract.py`:
- Line 69: Update the accessibility contract test’s noun derivation to use
g.trace.kind, assigning “Stage” only when it equals “funnel” and “Point”
otherwise; do not derive the noun from MARK_KINDS.
🪄 Autofix
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: 5ca1b815-83cb-43f4-b102-2f526bb3ba96
📒 Files selected for processing (43)
CHANGELOG.mddocs/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/funnel-chart.mddocs/components/marks.mddocs/overview/gallery.mddocs/styling/capabilities.mddocs/styling/mark-styles.mdjs/src/00_header.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/51_annotations.tsjs/src/52_tooltip.tsjs/src/53_interaction.tsjs/src/55_marks.tspython/xy/__init__.pypython/xy/_annotations.pypython/xy/_figure.pypython/xy/_funnel.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_scene.pypython/xy/_svg.pypython/xy/components.pypython/xy/config.pypython/xy/marks.pypython/xy/styles.pyscripts/render_smoke_nonumpy.pyspec/api/capability-matrix.mdspec/api/chart-kind-contract.mdspec/api/chart-roadmap.mdspec/design/wire-protocol.mdtests/pyplot/test_tick_side_rendering.pytests/test_accessibility_contract.pytests/test_api_parity.pytests/test_check_typing.pytests/test_funnel.pytests/test_polar_phase7_api.pytests/test_sankey.pytests/test_type_surface.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Merging this PR will not alter performance
Comparing 
Footnotes
|
Two independent adversarial reviews of #474 (an external one and a seven-dimension workflow run over the diff) agreed on a set of real defects. Both flagged the horizontal label transposition first, and each found things the other did not. Correctness - Conversion ratios could overflow to +/-inf on a wide dynamic range (1e-300 -> 1e10), contradicting the documented "never inf" contract and putting a non-JSON value on the wire. Undefined is undefined whether the denominator was zero or the quotient overflowed: both are None, both print the em dash (_funnel._ratio). - Horizontal funnels tested label fit on the WRONG axis: text width was measured against the segment's cross height and the line height against the stage pitch. Text never rotates, so both budgets were transposed and the inside/outside/hidden ladder silently did not hold — a ten-stage 400px funnel marked every ~101px label as fitting inside a 34px stage. The fit now measures the box the segment actually offers, and horizontal boundary labels take their own row so they cannot overprint the values beside them. - Inside-label contrast read the palette even when a constant `color=` was painting every segment, so a white funnel got near-white labels. It now reads the fill actually drawn — and defers to the theme's text colour for browser-only fills, where `_parse_color` silently substitutes its fallback blue rather than failing (the previous None branch was dead code). - A failed build (a bad `value_format`, say) left its stage names in the axis category registry, shifting the next valid funnel's positions. The checkpoint now precedes the commit. - The theme `palette={...}` reorder raised KeyError when stage names are themselves CSS colours: the shared resolver reads such a column as per-point paint, so there were no categories to reorder. The map is keyed by stage name either way. - Multidimensional stage arrays were silently flattened into an invented stage order; refused now. Axis validation - A funnel's segments are centered on zero, so half its corners are negative: a log cross axis mapped them hundreds of thousands of pixels offscreen and a forced time stage axis stripped the stage labels. Both refuse at payload build, beside the polar check and for the same §28 reason — a plausible wrong picture is worse than an error. Client - A theme refresh while a stage was legend-hidden rebuilt the paint cache at the VISIBLE length, recolouring survivors by their drawn index and losing the hidden rows for good. Paint is now always resolved over the full stage count, and one shared upload path applies the filter — so refresh, filter and hover-dim cannot disagree about row order. - A GPU rebuild (context restore, streaming append) un-hid filtered stages: _reapplyLegendVisibility routed everything to the scatter filter, which no-ops on a funnel. It now routes funnels to their own filter. - Legend hover dimmed the rows but not the funnel: the generic path swaps a palette LUT and a funnel carries resolved RGBA. It recolours the rows with the same blend rule, and restores them on leave. - Keyboard traversal walked legend-hidden stages that pointer hover already refused, and the "Stage i of n" total counted them. Traversal now walks the visible rows and reports the SHIPPED row index. - A legend-hidden series answered CPU hover for every kind that uses it (bar, rect, ribbon, funnel), reporting invisible geometry. Animation (the API was advertised and inert) - `key=`/`animation=` reached the payload but funnel geometry was excluded from position interpolation and the draw ignored transition progress, so key-matched updates snapped and `enter="grow"` did nothing. Funnel now has its own interpolation prep (start values are the old trace's currently displayed geometry, mid-flight retargets included) and a per-frame CPU mix that writes the live buffers in place — one quad per stage makes that cheaper than a second attribute set, and the shader stays untouched. The default entrance grows cross edges out of the segment spine, the bar family's baseline rule transposed. Export parity - Browser-only palette entries collapsed onto ONE fallback colour in the native rasterizer while SVG/PDF gave them distinct built-ins, so a PNG painted two stages the same. Both now resolve per index. - SVG funnel outlines joined as miter while the rasterizer's distance-field stroke is round by construction — visible where a taper meets its neck. - The annotation default-colour change from round 1 was too wide: it painted uncoloured annotation SHAPES (rule strokes, band fills, arrows) with the theme text colour in SVG only, diverging from the raster and the client. Only the LABEL follows --chart-annotation-text/--chart-text now. Composition - `name=` raised an unexpected-argument error instead of forwarding. - Chart-level `data=` with an explicit `xy.funnel(...)` child prepended a second empty funnel and failed; the data now reaches the child. Forwarded keywords that would build a ghost mark are refused by name. - Mixing vertical and horizontal funnel children silently mangled one of them through last-child-wins axis defaults; refused. Also: the tooltip dropped undefined-ratio rows instead of printing the em dash (the row set changed between stages, reading as missing data), and the wire-protocol spec still said v12 in the section describing v13. Tests: 17 more (88 total in test_funnel.py), each pinning one of the above, plus the render-smoke funnel probe extended from a static check into an executed one — legend filtering, the a11y row translation, the theme-refresh paint rebuild, hidden-trace hover refusal, a hand-stepped mid-flight interpolation frame, and the em-dash tooltip. Suite 3,921 passing, docs-site 111 passing, ruff/ty clean, render + ABI smokes pass.
…el tile check_html_routes.py carries its own hardcoded count, separate from the pytest copies updated with the tile itself, and it only runs against a production export — so the funnel tile passed every local gate and failed the Production docs job.
The test hardcoded a second copy of the preview count, so bumping the constant for the funnel tile broke it — the same duplicated-number failure one layer down. It now sizes its fixture from INLINE_SVG_PREVIEW_COUNT and tests the validator's behaviour rather than restating the number.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/50_chartview.ts (1)
4938-4943: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
_transitionOpacityin the funnel fill and stroke opacity.
_drawFunnelsmultiplies only byg._legendDim. Every other mark path, including the ribbon path updated in this PR at Line 4655, also multiplies byg._transitionOpacity.
_setTransitionVisualsetsg._transitionOpacity = 0for theexitphase. A funnel trace that is removed byupdatePayloadtherefore keeps drawing at full opacity for the whole animation instead of disappearing.🐛 Proposed fix
- gl.uniform1f(u("u_opacity"), this._fillOpacity(g.trace.style) * (g._legendDim ?? 1)); + const transitionAlpha = (g._transitionOpacity ?? 1) * (g._legendDim ?? 1); + gl.uniform1f(u("u_opacity"), this._fillOpacity(g.trace.style) * transitionAlpha); const stroke = g.stroke || [0, 0, 0, 0]; gl.uniform4f(u("u_stroke"), stroke[0], stroke[1], stroke[2], stroke[3]); gl.uniform1i(u("u_strokeMode"), g.stroke ? 0 : 1); gl.uniform1f(u("u_strokeWidth"), (g.strokeWidth || 0) * this.dpr); - gl.uniform1f(u("u_strokeOpacity"), this._strokeOpacity(g.trace.style || {}) * (g._legendDim ?? 1)); + gl.uniform1f(u("u_strokeOpacity"), this._strokeOpacity(g.trace.style || {}) * transitionAlpha);🤖 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 4938 - 4943, Update _drawFunnels so both fill opacity and stroke opacity multiply by g._transitionOpacity in addition to g._legendDim, matching the transition handling used by other mark paths. Preserve the existing opacity calculations and ensure exit-phase funnels fade out when _setTransitionVisual sets the transition opacity to zero.
🧹 Nitpick comments (5)
tests/test_funnel.py (4)
946-959: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the docstring with what this test asserts.
The test computes the mid-segment edge itself and compares it to constants. It does not call
_funnelHoveror any containment helper, so it does not check the containment rule. It checks thatcompute_layoutproducedhi0=5andhi1=1. The real containment assertion lives inscripts/render_smoke_nonumpy.pyat Line 1264 (fnMissat cross 4.5). Reword the docstring to state that this test pins the geometry source that the client rule consumes.🤖 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_funnel.py` around lines 946 - 959, The docstring for test_hover_containment_rejects_the_bounding_box_corner incorrectly claims the test verifies containment behavior. Reword it to state that the test validates compute_layout’s taper geometry, specifically the hi0/hi1 values consumed by the client’s containment rule, without claiming it calls _funnelHover or a containment helper.
764-768: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the pixel oracle so a background sample cannot pass.
The test asserts only
top != bottom. If one sample lands on the chart background and the other lands on a stage fill, the assertion still passes. That hides a real collapse of bothvar()entries onto one fallback. Assert that both samples are stage ink first.♻️ Proposed hardening
top = tuple(int(v) for v in pixels[int(h * 0.32), int(w * 0.5)][:3]) bottom = tuple(int(v) for v in pixels[int(h * 0.72), int(w * 0.5)][:3]) + background = tuple(int(v) for v in pixels[2, 2][:3]) + assert top != background and bottom != background, "sampled the canvas, not the stages" assert top != bottom🤖 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_funnel.py` around lines 764 - 768, Strengthen the pixel assertions in the image-color test by first verifying that both the top and bottom samples represent stage ink rather than the chart background, then retain the top-versus-bottom inequality check. Reuse the test’s existing background or stage-color symbols if available, and update the assertions around _decode_rgba and the top/bottom samples only.
924-927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree funnel assertions can pass without observing the behavior under test. Each site asserts a difference or an absence without first establishing that the sampled element is the element under test. A control assertion fixes all three.
tests/test_funnel.py#L924-L927: scope the SVG and PNG checks to the annotation label element, becausexy.theme(text_color=...)also colors the stage-axis tick labels that satisfy the current match.tests/test_funnel.py#L764-L768: assert that both sampled pixels differ from the canvas background before asserting that they differ from each other.scripts/render_smoke_nonumpy.py#L1287-L1289: capture a non-null_hoverAt(100,80)result before setting_legendHidden, so the null result proves the legend rule rather than a coordinate miss.🤖 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_funnel.py` around lines 924 - 927, Strengthen the three funnel checks: in tests/test_funnel.py:924-927, scope SVG and PNG assertions to the annotation label element rather than stage-axis tick labels; in tests/test_funnel.py:764-768, first verify both sampled pixels differ from the canvas background, then compare them; in scripts/render_smoke_nonumpy.py:1287-1289, capture and validate a non-null _hoverAt(100,80) result before setting _legendHidden so the null assertion tests the legend rule rather than a coordinate miss.
845-846: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the invalid format error in the rollback test.
Use
pytest.raises(ValueError, match="Invalid format specifier")so an unrelatedValueErrorcannot satisfy the test.🤖 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_funnel.py` around lines 845 - 846, Update the pytest.raises assertion around fig.funnel in the rollback test to require ValueError with the message pattern “Invalid format specifier,” preserving the existing invalid value_format scenario.scripts/render_smoke_nonumpy.py (1)
1308-1310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit per-check diagnostics for the funnel aggregate.
Fourteen independent checks collapse into one
funnelflag. When CI reportsfunnel=0, the title carries no information about which check failed, and theSystemExittext at Line 1700 names only three of the fourteen concerns. The neighbouringthrashandzoomoutprobes publish auxiliary values (tj,td) for exactly this reason.Ship a compact per-check string alongside the flag.
♻️ Proposed change
const funnel=(fnInk && fnHit && fnHit.index===1 && !fnMiss && fnRowOk && fnNav && fnFilterN && fnA11y && fnHitHidden && fnPaintFull && fnRestored && fnHiddenHover && fnMix && fnDash)?1:0; + const funnelBits=[fnInk,fnHit?1:0,fnHit&&fnHit.index===1?1:0,fnMiss?0:1,fnRowOk,fnNav, + fnFilterN,fnA11y,fnHitHidden,fnPaintFull,fnRestored,fnHiddenHover,fnMix,fnDash] + .map((b)=>b?1:0).join("");Then add
funnelBits=${{funnelBits}}to the title at Line 1312 and include it in theSystemExitmessage at Line 1700.🤖 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/render_smoke_nonumpy.py` around lines 1308 - 1310, Update the funnel aggregation near fnInk/fnHit to build a compact funnelBits diagnostic string containing each of the fourteen check results, then retain the existing funnel flag calculation. Append funnelBits to the title and to the SystemExit message so failures identify the specific checks that did not pass.
🤖 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 `@spec/api/chart-kind-contract.md`:
- Line 179: Update the funnel contract documentation to explicitly require
linear cross and stage axes, matching Figure._validate_funnel_axes and the
renderer behavior. Clarify that log, symlog, and time axes are unsupported for
funnels.
---
Outside diff comments:
In `@js/src/50_chartview.ts`:
- Around line 4938-4943: Update _drawFunnels so both fill opacity and stroke
opacity multiply by g._transitionOpacity in addition to g._legendDim, matching
the transition handling used by other mark paths. Preserve the existing opacity
calculations and ensure exit-phase funnels fade out when _setTransitionVisual
sets the transition opacity to zero.
---
Nitpick comments:
In `@scripts/render_smoke_nonumpy.py`:
- Around line 1308-1310: Update the funnel aggregation near fnInk/fnHit to build
a compact funnelBits diagnostic string containing each of the fourteen check
results, then retain the existing funnel flag calculation. Append funnelBits to
the title and to the SystemExit message so failures identify the specific checks
that did not pass.
In `@tests/test_funnel.py`:
- Around line 946-959: The docstring for
test_hover_containment_rejects_the_bounding_box_corner incorrectly claims the
test verifies containment behavior. Reword it to state that the test validates
compute_layout’s taper geometry, specifically the hi0/hi1 values consumed by the
client’s containment rule, without claiming it calls _funnelHover or a
containment helper.
- Around line 764-768: Strengthen the pixel assertions in the image-color test
by first verifying that both the top and bottom samples represent stage ink
rather than the chart background, then retain the top-versus-bottom inequality
check. Reuse the test’s existing background or stage-color symbols if available,
and update the assertions around _decode_rgba and the top/bottom samples only.
- Around line 924-927: Strengthen the three funnel checks: in
tests/test_funnel.py:924-927, scope SVG and PNG assertions to the annotation
label element rather than stage-axis tick labels; in
tests/test_funnel.py:764-768, first verify both sampled pixels differ from the
canvas background, then compare them; in
scripts/render_smoke_nonumpy.py:1287-1289, capture and validate a non-null
_hoverAt(100,80) result before setting _legendHidden so the null assertion tests
the legend rule rather than a coordinate miss.
- Around line 845-846: Update the pytest.raises assertion around fig.funnel in
the rollback test to require ValueError with the message pattern “Invalid format
specifier,” preserving the existing invalid value_format scenario.
🪄 Autofix
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: 1edeb3a1-44df-4697-b974-65432b6e5047
📒 Files selected for processing (18)
docs/charts/funnel-chart.mdjs/src/50_chartview.tsjs/src/52_tooltip.tsjs/src/53_interaction.tsjs/src/56_animation.tspython/xy/_figure.pypython/xy/_funnel.pypython/xy/_hosts.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/components.pypython/xy/marks.pyscripts/render_smoke_nonumpy.pyspec/api/chart-kind-contract.mdspec/design/wire-protocol.mdtests/test_accessibility_contract.pytests/test_funnel.py
🚧 Files skipped from review as they are similar to previous changes (7)
- python/xy/_payload.py
- docs/charts/funnel-chart.md
- python/xy/marks.py
- tests/test_accessibility_contract.py
- python/xy/_raster.py
- python/xy/components.py
- spec/design/wire-protocol.md
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/funnel-chart.md`:
- Around line 261-263: Update the funnel ratio behavior documentation near the
zero-denominator description to also cover ratios that overflow to infinity,
stating that events represent them as null and formatted tooltip text displays
an em dash (—). Preserve the existing zero-denominator behavior and wording.
In `@python/xy/_payload.py`:
- Around line 924-930: Update the funnel tooltip serialization around
t.tooltip_rows so numeric fields value, share, prior, conversion, and dropoff
are removed from JSON and emitted as raw f32 buffer columns with offset
encoding, while preserving canonical f64 values on the CPU. Keep only buffer
references and nonnumeric metadata in the JSON trace entry, and update the
protocol specification, client/static consumers, and payload tests consistently.
In `@spec/api/chart-kind-contract.md`:
- Line 179: Update the tooltip_rows schema entry so stage is explicitly declared
as a string stage name rather than a numeric field. Clarify that the numeric
semantic fields are the explicit small-N JSON exception to §29’s raw-buffer
rule, while preserving the existing formatting and null-value behavior.
🪄 Autofix
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: efa25ffb-c6dd-4bf9-ae7a-f58a9c599a43
📒 Files selected for processing (46)
CHANGELOG.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/funnel-chart.mddocs/components/marks.mddocs/overview/gallery.mddocs/styling/capabilities.mddocs/styling/mark-styles.mdjs/src/00_header.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/51_annotations.tsjs/src/52_tooltip.tsjs/src/53_interaction.tsjs/src/55_marks.tsjs/src/56_animation.tspython/xy/__init__.pypython/xy/_annotations.pypython/xy/_figure.pypython/xy/_funnel.pypython/xy/_hosts.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_scene.pypython/xy/_svg.pypython/xy/components.pypython/xy/config.pypython/xy/marks.pypython/xy/styles.pyscripts/render_smoke_nonumpy.pyspec/api/capability-matrix.mdspec/api/chart-kind-contract.mdspec/api/chart-roadmap.mdspec/design/wire-protocol.mdtests/pyplot/test_tick_side_rendering.pytests/test_accessibility_contract.pytests/test_api_parity.pytests/test_check_typing.pytests/test_funnel.pytests/test_polar_phase7_api.pytests/test_sankey.pytests/test_type_surface.py
🚧 Files skipped from review as they are similar to previous changes (43)
- tests/test_check_typing.py
- tests/test_polar_phase7_api.py
- tests/pyplot/test_tick_side_rendering.py
- docs/app/scripts/check_html_routes.py
- docs/overview/gallery.md
- tests/test_sankey.py
- docs/app/xy_docs/api_reference.py
- js/src/40_gl.ts
- spec/api/chart-roadmap.md
- tests/test_type_surface.py
- python/xy/_scene.py
- docs/components/marks.md
- docs/styling/mark-styles.md
- docs/app/xy_docs/gallery.py
- docs/app/xy_docs/config.py
- python/xy/config.py
- docs/styling/capabilities.md
- python/xy/_hosts.py
- python/xy/init.py
- CHANGELOG.md
- tests/test_api_parity.py
- docs/app/tests/test_docs_site.py
- python/xy/_figure.py
- python/xy/_raster.py
- docs/app/xy_docs/sidebar.py
- python/xy/_annotations.py
- python/xy/styles.py
- tests/test_accessibility_contract.py
- js/src/56_animation.ts
- python/xy/_svg.py
- spec/api/capability-matrix.md
- python/xy/marks.py
- js/src/53_interaction.ts
- python/xy/_funnel.py
- js/src/52_tooltip.ts
- js/src/00_header.ts
- spec/design/wire-protocol.md
- js/src/55_marks.ts
- js/src/51_annotations.ts
- js/src/50_chartview.ts
- python/xy/components.py
- tests/test_funnel.py
- scripts/render_smoke_nonumpy.py
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 20 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Two review batches hit the PR; the first reviewed the pre-fix code and is almost entirely covered by the round-2 commit. This addresses what was still live in the second batch, and skips what is not (reasons in the PR thread summary). Fixed: - animation match="append" paired every vertical-funnel stage with the LAST old stage: append matching keys on decoded x values, and a vertical funnel's x centers are all ~0. Stages are an ordered process, not a stream — the funnel prep now rebuilds the pairs by position and records "index:append-unsupported". A snap-strategy match (the 200k limit) now bails before preparing, instead of mixing six full-size arrays per frame for identical values. - horizontal OUTSIDE labels anchored "start" at the stage midpoint, hanging half the text over the neighbour and clipping the last stage; they center over their own stage now (vertical margin labels keep the start anchor). - the colour-shaped-stage-names palette fallback assigned spare colours silently; it now emits the same unmapped-category RuntimeWarning the resolver path does. - funnel_chart joins CHART_FACTORIES in the type-surface test (with the composing-factory exemption radar/wind-rose/pie already use). - contract wording: `stage` in tooltip_rows is the stage NAME (a string), not a numeric field. - the docs page documents overflow-to-infinity ratios beside the zero-denominator case, and its one backtick fence becomes the tilde style every other chart doc uses. - roadmap depth-table row 18 (the second Funnel row) now points at shipped row 15 instead of reading as still-planned. Skipped, with reasons: - "move numeric tooltip fields out of JSON" — the chart-kind contract documents the small-N semantic-row exemption for exactly these fields (ribbon precedent); §29's raw-buffer rule is for geometry that scales with data. - SVG-vs-raster ANNOTATION DEFAULT colours (token-less charts) — a pre-existing divergence this PR did not introduce and both reviews anchored on touched lines; spun off as its own task rather than widening this diff with cross-chart visual churn. - hover-during-animation reads settled geometry — the bar family behaves identically, and events should report real data, not a mid-flight blend. - theme-flip-while-legend-hovering drops the sibling dim until the next mouse move — matches the LUT marks' existing behaviour.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Fixed: - funnel stage centers leaked into the STANDALONE selection universe. They are retained (`retainCpu`) as a keyboard-navigation aid, but `_selectLocal` and `_selectLocalPolygon` iterate every trace holding `_cpu`, so a box or lasso select counted them and reported a selection the chart never drew — while the kind documents selection as absent. Both paths now skip stageNav marks. Introduced by this PR. - `stroke=` with no `stroke_width=` drew nothing: every renderer skips a zero-width stroke. Implies 1px now, matching the other mark builders. - a theme flip while a funnel legend row was hovered dropped that row's sibling emphasis, because the rebuild uploads undimmed rows. The dim is re-applied against the NEW backdrop (better than the LUT marks, which keep a dim computed against the old one). - the funnel tooltip now shows the prior stage's value, so "From previous 63%" is checkable rather than asserted; stage 0 omits the row. - `_scene.funnel_quad`'s normative comment named the wrong tessellation: the client sweeps A,B,D,C (triangles ABD/BDC), not ABC/ACD. Same quad, but a renderer reading the contract would have built the wrong strip. - the render smoke's a11y probe asserted GROUP MEMBERSHIP, which passed even with a broken walk; it now drives Home/ArrowRight and checks both the selected stage and the live-region text. Its palette check also skipped the middle stage, so only 2 of 3 shipped categories were verified. - spec/api/styling.md gained the funnel row (it was registered as a style surface with no entry in the authoritative table). - the Funnel tile joins the per-title gallery assertion, and the roadmap depth-table cross-reference no longer reads as pointing at rank 15 of its own table (which is Candlestick). Skipped, with reasons: - "move numeric tooltip fields out of JSON" — the chart-kind contract documents the small-N semantic-row exemption for exactly these fields (ribbon precedent); §29's raw-buffer rule targets geometry that scales with data, not five numbers per stage. - hover during animation reads settled geometry — `_barHover` does the same against `_cpu` while bars interpolate, and an event should report the real datum, not a mid-flight blend. - SVG-vs-raster annotation SHAPE defaults (`#667085` shared vs the raster's per-kind `#64748b`/`#2563eb`) — a pre-existing divergence this PR did not introduce; both bots anchored it on lines this PR happens to touch. Spun off rather than widening this diff with cross-chart visual churn.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The stroke-width fallback landed twice — a scripted edit applied to both sides of a near-identical block — so the second copy was dead code sitting under the first. One copy now, with the explicit-zero case spelled out. `prior_text` was added to `tooltip_rows` without reaching the normative wire table, so a client implementing from the contract could not discover it. Listed now, including its `null` on stage 0. The stroke test also grew teeth: it was checking only the SVG string, which left the reviewer's question — does the implied 1px reach the NATIVE exporters, or is it client-only? — unanswered by the suite. It now asserts the wire style, the SVG attribute, and red pixels in the PNG. (It does reach them: the implication happens at build time, so `stroke_width: 1.0` ships on the wire and both exporters gate on it normally.)
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The stroke test built the figure three times — once each for the wire, the SVG and the PNG. The point of the test is that the SAME built object reaches all three surfaces identically, so a single build states that better and drops the duplicated work.
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.
Closes ENG-10451.
xy.funnel_chart(stages, values)/xy.funnel(...)draw one centered segment per stage of an ordered process. Stage order is the declared order — a funnel is a categorical business process and is never sorted — and the conversion arithmetic ships with every stage rather than being left to the reader.Where the work lives
The kernel (
python/xy/_funnel.py) owns all of it at build time, the way_sankeyowns the Sankey placement: validation, conversion/drop-off math, quad construction, and the label ladder. Renderers only ever see quads plus semantic rows.Geometry is explicit
The two classic funnel drawings encode differently, so the mode is a parameter rather than a default nobody notices:
geometry="area"tapers each segment toward the next stage, so drop-off reads as slope — and painted area is therefore not proportional to value. The docs say so out loud.geometry="bar"gives centered constant-width segments whose widths carry the values exactly.neck="rect"|"taper"decides the spout,gapresolves per geometry (0 for area — a continuous silhouette; 0.2 for bar — bar-chart spacing), andmin_widthfloors drawn widths so a zero stage stays visible and hoverable while its reported value stays exact.Arithmetic
Overall share, previous-stage conversion, and drop-off, with
None— rendered as an em dash — wherever a denominator is zero. Never an invented number, never an infinity. Increasing stages are legal and drawn honestly (conversion above one, a signed+12%boundary label); negative and missing values are refused by stage name.Renderers
A new
funnelkind. The client sweeps a 4-vertex strip per stage throughFUNNEL_VSsharingRIBBON_FS— that fragment stage is where the slanted edges get theirfwidthcoverage, without which the long diagonals staircase on theantialias: falsecontext. Both exporters build their corners from_scene.funnel_quad, the single reference the golden tests pin, so SVG and PNG cannot drift from each other or from the client.Interaction
stageNavputs the per-stage centers into keyboard traversal, so a screen reader hears "Stage 2 of 5" followed by that stage's arithmetic.Formatting is Python's alone
value_format/percent_formatare applied once and shipped as preformatted*_texttwins beside the numeric event fields, so a label, a tooltip, and a static export cannot disagree about how a number reads. The client has nostr.format; re-implementing the spec in JS is exactly the divergence the single-reference rule exists to prevent.Two parity bugs fixed on the way
Both pre-existing, both visible the moment a funnel drew its own labels:
_scene/_svghad a load-order cycle that broke the capability-matrix generator.--chart-annotation-text/--chart-textwhile the live client honored them, so a themed dark chart printed its labels in the light-mode default.Protocol
PROTOCOL_VERSION12 → 13 in lockstep with the client:markOf()falls back to scatter for unknown kinds, so a v12 client would silently render funnel quads as a point cloud.Verification
ruffandtyclean; capability matrix regenerated; render + ABI smokes pass.Docs
New guide at
docs/charts/funnel-chart.md(basic chart, geometry modes, horizontal, neck/gap/floor, styling, interaction), plus gallery tile, sidebar entry, API reference, marks table, mark-styles row, chart-kind contract section (normative funnel geometry contract), roadmap row 15 → Implemented core, wire-protocol v13 note, and a CHANGELOG entry.Deferred
GPU picking, box/lasso selection, and multi-series comparison grouping (facets are the current answer) — recorded in the contract and the roadmap row.
Summary by CodeRabbit
New Features
Documentation