Data-bound chart components 8/8: full mark-kind coverage - #469
Data-bound chart components 8/8: full mark-kind coverage#469FarhanAliRaza wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Greptile SummaryThe PR expands data-bound plans to every standalone mark kind through empty structural probes and adds callable-aware plan addressing. It also adds flat Reflex factories, mixed-shape data support, and an all-kinds demo with browser-smoke coverage.
Confidence Score: 4/5The PR should not merge until callable digests account for behavior-affecting global state or reject reducers whose behavior is not fully addressable. A supported module-level reducer can read mutable or worker-specific global state that is absent from its fingerprint, allowing one plan digest to execute different aggregation behavior across reloads or workers. Files Needing Attention: python/reflex_xy/plan.py
|
| Filename | Overview |
|---|---|
| python/reflex_xy/plan.py | Extends plans to structural probes and named callables, but the Python callable fingerprint does not capture behavior-affecting global values. |
| python/xy/marks.py | Adds empty-data structural-probe exits while retaining real-data validation and aggregation paths. |
| python/reflex_xy/data_vars.py | Removes the global equal-length restriction so individual mark validators can enforce their own coupled shapes. |
| python/reflex_xy/factories.py | Adds flat data-bound factories for the remaining standalone mark kinds. |
| scripts/reflex_ws_smoke.py | Adds /kinds navigation and per-cell canvas paint checks to the existing browser smoke harness. |
| examples/reflex/xy_reflex_demo/xy_reflex_demo.py | Adds conditional data-source switching and a gallery route covering data-bound and static chart kinds. |
Reviews (2): Last reviewed commit: "fix(reflex): zero-row structural plan pr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| gap="1rem", | ||
| width="100%", | ||
| ), | ||
| rx.link("← the linking showcase", href="/"), | ||
| spacing="5", | ||
| width="100%", | ||
| ), | ||
| size="4", | ||
| padding_y="28px", | ||
| ) |
There was a problem hiding this comment.
The new /kinds route mounts all nineteen data-bound mark kinds, but the browser smoke test for this page remains unrun. Please exercise the running Reflex app and attach browser evidence so subscription, painting, and layout regressions are covered rather than only plan compilation.
Rule Used: You need to make an example svg/png, html, jupyter... (source)
Knowledge Base Used: Testing and Benchmarks
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Merging this PR will not alter performance
Comparing 
Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="examples/reflex/xy_reflex_demo/xy_reflex_demo.py">
<violation number="1" location="examples/reflex/xy_reflex_demo/xy_reflex_demo.py:513">
P3: In the new §10 `cond_summary` data var, `mean_y` is divided by `np.maximum(bincount(...), 1)` — the count is floor-clamped to 1 to dodge division-by-zero, not the true per-bin count. That means any empty bin renders a binned mean of exactly 0 (a false origin point at that x-centre) instead of being undefined/missing. With the current 1M-point seed every bin is populated so this is latent, but the summary's 'binned means' promise is only correct if no bin is ever empty. Consider dividing by the true bin counts and masking/omitting empty bins (e.g., leaving them NaN) rather than using the clamped count as the divisor.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| edges = np.linspace(x.min(), x.max(), 121) | ||
| idx = np.clip(np.digitize(x, edges) - 1, 0, 119) | ||
| counts = np.maximum(np.bincount(idx, minlength=120), 1) | ||
| mean_y = np.bincount(idx, weights=y, minlength=120) / counts |
There was a problem hiding this comment.
P3: In the new §10 cond_summary data var, mean_y is divided by np.maximum(bincount(...), 1) — the count is floor-clamped to 1 to dodge division-by-zero, not the true per-bin count. That means any empty bin renders a binned mean of exactly 0 (a false origin point at that x-centre) instead of being undefined/missing. With the current 1M-point seed every bin is populated so this is latent, but the summary's 'binned means' promise is only correct if no bin is ever empty. Consider dividing by the true bin counts and masking/omitting empty bins (e.g., leaving them NaN) rather than using the clamped count as the divisor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/reflex/xy_reflex_demo/xy_reflex_demo.py, line 513:
<comment>In the new §10 `cond_summary` data var, `mean_y` is divided by `np.maximum(bincount(...), 1)` — the count is floor-clamped to 1 to dodge division-by-zero, not the true per-bin count. That means any empty bin renders a binned mean of exactly 0 (a false origin point at that x-centre) instead of being undefined/missing. With the current 1M-point seed every bin is populated so this is latent, but the summary's 'binned means' promise is only correct if no bin is ever empty. Consider dividing by the true bin counts and masking/omitting empty bins (e.g., leaving them NaN) rather than using the clamped count as the divisor.</comment>
<file context>
@@ -489,6 +499,25 @@ def sensor_handles(self) -> list[DataHandle[SensorCols]]:
+ edges = np.linspace(x.min(), x.max(), 121)
+ idx = np.clip(np.digitize(x, edges) - 1, 0, 119)
+ counts = np.maximum(np.bincount(idx, minlength=120), 1)
+ mean_y = np.bincount(idx, weights=y, minlength=120) / counts
+ centers = (edges[:-1] + edges[1:]) / 2.0
+ return {"x": centers, "y": mean_y, "mag": np.hypot(centers, mean_y)}
</file context>
The data-bound plan tier refused the aggregating marks — box, violin, hexbin, contour, heatmap, stairs, ecdf — because their validators need at least one finite value and the Phase 3 decision judged a synthetic-row probe to be validating against made-up data. Answer that objection with discipline instead of exclusion. Each aggregating kind's channels bind fixed placeholder columns from a recorded table (plan._SYNTHETIC_CHANNELS): finite, positive, strictly increasing, one group for the grouped kinds, a square z grid with matching side coordinates, len+1 bin edges. The values satisfy every value-domain precondition a validator imposes, so a probe failure still indicts the structure and never the placeholders. Extending a kind means recording its minimal contract in that table — never a silent guess (§28 spirit). Landing with it: - Flat factories for the newly probed kinds plus triangle_mesh_chart (always zero-row-safe, just never wired). All 19 standalone mark kinds now have one. - validate_columns drops the shared-length requirement: a data var may carry mixed-length and 2-D columns (stairs edges, heatmap grids). Coupled-shape contracts stay with the mark validators at bind, where the errors name the mark and channels involved. - Module-level named callables in mark props (hexbin's reduce_C_function, np.mean by default) content-address as their import path; lambdas and closures have no stable one and are refused toward a module-level function or @reflex_xy.figure. Plan registration is last-write-wins so a hot reload replaces stale node objects behind an unchanged digest. Specs updated: the revision is recorded in the implementation doc's post-landing section, the options doc's decision record, and reflex-integration.md §3.6 "Kind coverage".
Two additions to the demo app, both covered by test_example_apps.py: - A /kinds page rendering all 19 standalone mark kinds as data-bound flat factories fed by a single @reflex_xy.data var — mixed column lengths and a 2-D grid in one var, which the relaxed column validation now permits — next to the composite kinds (pie, radar, sankey, polar, polar bars, wind rose, facet) on the static tier, since those still take data directly. - §10 on the main page: data=rx.cond(...) picking between two data vars. The cond sits at the data-var level, so one fixed plan swaps between two column sets from state and both branches stay compile-checked against the shared schema.
…er runs empty xy.structural_probe() (spec/api/chart-kind-contract.md 'Structural probe'): while active, a mark whose data channels are all empty validates its configuration — enums, bounds, colormaps, range/level shapes — and contributes no traces, instead of refusing zero rows or aggregating. This is the core seam compile gates need to validate chart structure with no data and no invented data; non-empty channels behave identically in and out of the mode. Each aggregating mark (stairs, ecdf, histogram, box, violin, hexbin, contour, heatmap) orders config validation before its data work and gates its zero-row refusal on the mode; _split_by_positions handles the empty grouping case. Pinned three ways in test_validation_timing.py: every kind compiles empty under the probe, still refuses empty normally, and still raises config errors under the probe.
Review hard-blocked the synthetic-column probe as structurally unsound: a column shared between an aggregating channel and a zero-row channel falsely failed on invented lengths (stairs edges len 9 vs scatter's 0), valid hexbin range=/mincnt= configurations falsely failed on invented values, and large gridsize ran real aggregation at page evaluation. - build_plan now compiles every kind zero-row under the core's new structural_probe() mode; _SYNTHETIC_CHANNELS and the shape table are gone. Config errors still fail reflex run; data-dependent outcomes and real-data shape couplings move (back) to bind. Repro pins: shared-column composition and hexbin range/mincnt/gridsize configs. - Plan callables are content-addressed, not name-addressed: import path + code fingerprint (bytecode/names/nested code/defaults) for pure-Python functions — editing a reducer body changes the digest, so rolling deployments resync instead of diverging; import path + distribution version for C-level callables that resolve back to themselves; bound methods, lambdas, closures, and partials refused. - @reflex_xy.data docstring drops the stale equal-length claim. - /kinds gains browser render coverage: reflex_ws_smoke.py step 7 navigates there and pixel-probes all 26 kind cells (19 data-bound + 7 static composites), each addressable as kind-<name>.
d775fd1 to
362394c
Compare
FarhanAliRaza
changed the title
|
Review addressed in b0437af + 362394c — both hard blocks are redesigned, not patched: Synthetic validation is gone entirely. The fix is the core structural-validation path the review asked for: Callables are content-addressed, not name-addressed. Bound methods are refused outright (your two-reducers repro is pinned: Also: the |
| "captured variables have no content address. Use a module-" | ||
| "level function, or build the chart with @reflex_xy.figure." | ||
| ) | ||
| return {"~callable": f"{module}.{qualname}", "code": _code_fingerprint(value)} |
There was a problem hiding this comment.
When a supported module-level reducer reads mutable or worker-specific global state, _code_fingerprint omits those values while assigning the same plan digest, causing workers or reload states to calculate different hexbin aggregates for one content address.
1 participant
Stacked on #467 (PR 8 of the reflex component-API stack). Review only the two commits here; everything below them is under review in #461–#467.
What this changes
The data-bound plan tier shipped without the aggregating marks — box, violin, hexbin, contour, heatmap, stairs, ecdf. The Phase 3 decision excluded them because their validators need at least one finite value, and a synthetic-row probe "would validate against made-up data". This lifts that exclusion.
The objection is answered by discipline instead of exclusion. Each aggregating kind's channels bind fixed placeholder columns from a recorded table (
plan._SYNTHETIC_CHANNELS) — finite, positive, strictly increasing, one group for the grouped kinds, a square z grid with matching side coordinates,len+1bin edges. Those values satisfy every value-domain precondition a validator imposes, so a probe failure still indicts the structure, never the placeholders. Extending a kind means recording its minimal contract in that table rather than guessing silently (§28 spirit).Riding along:
triangle_mesh_chart(always zero-row-safe, just never wired). All 19 standalone mark kinds now have one.validate_columnsdrops the shared-length requirement. A data var may carry mixed-length and 2-D columns — a stairs mark'slen+1edges or a heatmap grid beside ordinary row columns. Coupled-shape contracts stay with the mark validators at bind, where the errors name the mark and channels involved.reduce_C_function,np.meanby default) serializes as its import path. Lambdas and closures have no stable address and are refused toward a module-level function or@reflex_xy.figure. Plan registration becomes last-write-wins, so a hot reload replaces stale node objects behind an unchanged digest./kindspage rendering every kind (data-bound flat factories for the 19 marks; composite kinds — pie, radar, sankey, polar, wind rose, facet — on the static tier, since those still take data directly), and §10 on the main page showingdata=rx.cond(...)swapping two data vars under one fixed plan.Spec
Recorded in three places rather than edited in place, so the reversal is legible: the implementation doc's post-landing revision section, the options doc's §8 decision record, and reflex-integration.md §3.6 "Kind coverage (recorded decision, revised 2026-08)".
Verification
uv run pytest tests/reflex_adapter tests/test_validation_timing.py tests/test_example_apps.py— 261 passed, 1 skipped (250 on #467). New pins: the shape table and per-kind plan builds (test_plan.py), the full 19-kind flat table (test_factories.py),SHAPED_ROW_CHARTS— the xy-level half of the contract (test_validation_timing.py), and the/kindspage composing with mixed-length + 2-D columns (test_example_apps.py).pre-commit run --all-files,ruff check,ruff format --check,ty checkall clean.Not run: the browser E2E (
scripts/reflex_ws_smoke.py) against the/kindspage. Worth doing before this merges — the probe proves the plans compile, not that all 19 kinds render.