Cast lasso rays in the core instead of once per polygon edge#265
Conversation
`select_polygon` bounded the work correctly — zone-pruned bbox first, ray
casting only over those candidates — and then spent all of it in NumPy, one
vectorized pass per polygon EDGE. Each pass materialized full-length
crossing and edge-intersection temporaries over every candidate, so a
64-vertex lasso over 160k candidates moved roughly half a gigabyte through
memory to answer a question that fits in registers, and cost grew linearly
with vertex count: a 2048-vertex freehand lasso (the API ceiling) took
~370 ms. Every other selection primitive was already native; this one was
not.
`kernels.polygon_select` walks edges inside the point loop. Crossing parity
is order-independent, so the answer is unchanged. On top of that it buckets
edges into y slabs, because an edge can only flip parity for a point inside
its own y-extent — the index hands back a superset of the edges that can
cross a given row and the rest are provably no-ops, so restricting the
candidate set is not an approximation. That is what flattens the vertex
scaling.
vertices numpy native speedup
16 3.70ms 2.56ms 1.4x
64 12.40ms 3.20ms 3.9x
256 48.30ms 3.03ms 15.9x
1024 188.18ms 4.47ms 42.1x
2048 377.57ms 5.39ms 70.0x
The full gesture unit the benchmark measures (dispatch, bbox prune, cast,
wire mask reply) goes 13.59ms -> 3.21ms, min-of-5.
The arithmetic is preserved term for term: `x_j - x_i` and `y_j - y_i` are
hoisted per edge (exact subtractions) but the division is NOT strength-
reduced to a reciprocal multiply, which would not be. The guarded division
covers exactly the lanes the vectorized form computed unconditionally and
then masked its inf/NaN away, since endpoints straddling the ray cannot
have equal y. Bucketing is declined for a non-finite polygon, where slab
bounds stop meaning anything.
Verified identical to the previous predicate on convex, concave, self-
intersecting (bowtie — even-odd parity is load-bearing), axis-aligned
(horizontal edges are the masked-division lanes), collinear, degenerate,
repeated-vertex and 2048-vertex polygons; on points landing exactly on
vertices and edges; on NaN/+-inf coordinates; and on sparse candidate
subsets. Both the Rust and Python tests carry the per-edge formulation as
an executable reference rather than describing it.
ABI 40 -> 41.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughPolygon lasso selection now uses a Rust native kernel exposed through ABI v41, with ctypes marshaling and Python integration. The kernel supports indexed edge traversal, non-finite inputs, candidate row subsets, and expanded predicate and end-to-end tests. ChangesPolygon selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Figure
participant select_polygon
participant kernels.polygon_select
participant xy_polygon_select
participant RustPolygonKernel
Figure->>select_polygon: select polygon candidates
select_polygon->>kernels.polygon_select: pass points, rows, and polygon
kernels.polygon_select->>xy_polygon_select: marshal native call
xy_polygon_select->>RustPolygonKernel: validate and select rows
RustPolygonKernel-->>xy_polygon_select: selected row IDs
xy_polygon_select-->>kernels.polygon_select: output count and IDs
kernels.polygon_select-->>select_polygon: selected rows
select_polygon-->>Figure: update selection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will improve performance by ×3.2
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_select_lasso_message_1m |
89.3 ms | 27.6 ms | ×3.2 |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing alek/native-polygon-select (6401310) with main (9b08085)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/design/rust-engine.md (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the lasso bucketing thresholds to
spec/design/rust-engine.md:28. Document the>=16-vertex gate, the256-slab cap, and the fallback for non-finite or zero-height polygons; that row also still saysRust (ABI v41), so update it to match the current ABI version (v36).🤖 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/rust-engine.md` at line 28, Update the Rust row in rust-engine.md to document lasso edge bucketing thresholds: enable it for polygons with at least 16 vertices, cap bucketing at 256 slabs, and fall back for non-finite or zero-height polygons. Change the row’s ABI label from Rust (ABI v41) to Rust (ABI v36).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/kernels.rs`:
- Around line 4885-4898: Add a local maximum vertex-count guard in the polygon
indexing path around n_edges before allocating CSR-related buffers, returning
unindexed() when the ceiling is exceeded. Ensure the limit prevents u32 count
overflow and excessive transient allocation while preserving normal indexing
below the ceiling and the existing unindexed scan behavior above it.
In `@src/lib.rs`:
- Around line 2831-2839: Update the polygon-processing function around the
existing n_poly pointer validation to return usize::MAX whenever n_poly is less
than 3, before any std::slice::from_raw_parts calls. Preserve the existing
null-pointer validation for non-empty polygons and ensure poly_x/poly_y are
never sliced on the empty or insufficient-polygon path.
---
Nitpick comments:
In `@spec/design/rust-engine.md`:
- Line 28: Update the Rust row in rust-engine.md to document lasso edge
bucketing thresholds: enable it for polygons with at least 16 vertices, cap
bucketing at 256 slabs, and fall back for non-finite or zero-height polygons.
Change the row’s ABI label from Rust (ABI v41) to Rust (ABI v36).
🪄 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: baf89600-51c5-4341-9407-7e465edd139d
📒 Files selected for processing (7)
python/xy/_native.pypython/xy/interaction.pypython/xy/kernels.pyspec/design/rust-engine.mdsrc/kernels.rssrc/lib.rstests/test_select_polygon.py
Two defects review caught in the polygon cast. `xy_polygon_select` built its polygon slices before checking the vertex count, so a caller passing null for an empty polygon reached `slice::from_raw_parts(null, 0)` — which requires a non-null pointer even at length zero. Answer the "fewer than three vertices encloses nothing" case first, before any slice exists. The slab index sized itself purely from the vertex count. `select_polygon` caps callers at 2048, but the kernel and its C export are public with no such ceiling, so a direct call with a very large polygon transiently allocated hundreds of MB, and past ~16.7M vertices the u32 span counters would have overflowed into a corrupt CSR. Cap the CSR at 2^20 entries and shed slabs to stay inside it, rather than refusing to index outright: the fallback scan is O(edges) for every point, which is exactly what hurts most at the vertex counts this guard is about. Below 8 slabs the index stops earning its build and declines. Neither path changes an answer: the budget only coarsens bucketing, and bucketing only ever restricts which edges are tested to a superset of those that can cross. Equivalence to the per-edge reference still holds across every shape, the 2048-vertex ceiling is now covered on the Python side, and a new Rust test pins the CSR invariant at 16 / 2048 / 65536 / 200000 vertices with every edge spanning the full height.
|
Both review findings were real and are fixed in 6401310. Zero-length Unbounded CSR ( I took a different fix than the suggested Neither change can move an answer: the budget only coarsens bucketing, and bucketing only ever restricts the tested edges to a superset of those that can cross the row. Added coverage: a Rust test pinning the CSR invariant at 16 / 2048 / 65536 / 200000 vertices with every edge spanning the full height (the worst case for the index), and On the spec nitpick: thresholds are now recorded in Re-verified: 118 Rust tests, 2241 Python tests, clippy clean, no new |
1 participant
What
select_polygonbounded the work correctly — zone-pruned bbox first, ray casting only over those candidates — and then spent all of it in NumPy, one vectorized pass per polygon edge. Each pass materialized full-length crossing and edge-intersection temporaries over every candidate, so a 64-vertex lasso over 160k candidates moved roughly half a gigabyte through memory to answer a question that fits in registers. Worse, cost grew linearly with vertex count: a 2048-vertex freehand lasso — the documented API ceiling — took ~370 ms. Every other selection primitive was already native; this one was not.kernels.polygon_selectwalks edges inside the point loop. Crossing parity is order-independent, so the answer is unchanged.On top of that it buckets edges into y slabs. An edge can only flip parity for a point inside its own y-extent, so the index hands back a superset of the edges that can cross a given row and the rest are provably no-ops — restricting the candidate set is not an approximation. That is what flattens the vertex scaling.
Numbers
1M uniform points, 160k bbox candidates, min-of-3:
The full gesture unit
test_select_lasso_message_1mmeasures — dispatch, bbox prune, cast, wire mask reply — goes 13.59 ms -> 3.21 ms (min-of-5).There is a second reason to want this.
test_select_lasso_message_1mhas been the suite's largest source of false alarms: it is bimodal at ~90 ms and ~123-141 ms and flips on changes that cannot reach it — docs-only #251 read "+39%", CI-only #236 read "-25%", and #264 (which touches onlymarks.pyand_figure.py) just read "-27%", each with the bot's "different runtime environments" warning attached. A memory-bound NumPy loop is exactly the workload whose modeled cost swings when the runner changes; collapsing it to a register-resident scan should shrink that sensitivity considerably.Correctness
The arithmetic is preserved term for term.
x_j - x_iandy_j - y_iare hoisted per edge — exact subtractions — but the division is not strength-reduced to a reciprocal multiply, which would not be. The guarded division covers exactly the lanes the vectorized form computed unconditionally and then masked its inf/NaN away, since endpoints straddling the ray cannot have equal y. Bucketing is declined for a non-finite polygon, where slab bounds stop meaning anything, and below 16 vertices, where it would not pay for itself.Verified identical to the previous predicate on:
Both the Rust unit test and
tests/test_select_polygon.pycarry the per-edge formulation as an executable reference rather than describing it in a comment.cargo test— 117 passed;cargo clippyclean; no newcargo fmtdiffsscripts/abi_smoke.py— 133 checks passedsrc/lib.rsandpython/xy/_native.pytogetherspec/design/rust-engine.mdupdatedSummary by CodeRabbit
New Features
Bug Fixes
Tests