Repository navigation
Conversation
Add gridSpatialIndex at @tanstack/charts/spatial/grid. It returns the same point as the linear anchor scan, including the earliest-point tie, radius rules, and non-finite anchors. It is nearest-center lookup, not shape hit testing. Extend benchmarks/interaction/nearest.ts with 1k, 10k, and 100k point scaling rows against the linear scan and D3 quadtree.
Visit only the in-grid cells of each search ring and bound unvisited cells by their distance in both axes. A one-column or one-row grid no longer walks every ring row: 10k points on a vertical strip went from 754 us to 2 us per query at radius 48 and from 16.5 ms to 0.9 us with an unbounded radius. Rename the subpath to @tanstack/charts/spatial/grid-index, budget it in bundle:check, and add strip and cluster layouts to the pointer benchmark. Document the dense-cluster worst case.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 8 files. (9 skipped: 9… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding an opt-in grid spatial index for spatial interactions. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 8 files. (9 skipped: 9 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Thanks @greglum, the opt-in boundary and strip-query coverage are useful. I reproduced one remaining numeric edge case at With two finite anchors at
Please handle non-finite derived grid dimensions and add these exact-anchor regressions before merging. The public promise is equivalence to the linear scan, including finite input coordinates whose extent overflows. This is a correctness edge case, not a measured security issue. I have not completed the full PR validation yet. |
|
Thanks @greglum, the numeric fix at I checked 22,056 queries against the linear scan in an isolated Chromium context, including overflowing extents, tiny coordinates, zero-width axes, duplicate points, empty inputs and distance limits. They all matched. The standalone production ESM build measures 1.076 kB gzip and 0.970 kB Brotli, below this new entry's 1.2288 kB gzip ceiling. The performance tradeoff is worth keeping in the docs: for 256 queries over 10,000 tightly clustered points, my seven-trial median was 4.3 ms for the grid versus 2.1 ms for the linear scan. Uniform points at the same count were below 0.1 ms for the grid versus 1.9 ms for the scan. This stays opt-in, so it does not replace the default scan. Full repository and packed-consumer validation are still pending. The two GitHub workflows currently require approval, so this is confirmation of the numeric fix, not a merge-ready claim. |
🦋 Changeset detectedLatest commit: 7d8c770 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks @greglum, I found an optional improvement to the clustered-point case at Inlining the distance comparison inside const index = members[slot]!
const dx = xs[index]! - x
const dy = ys[index]! - y
const distance = dx * dx + dy * dy
if (
distance < bestDistance ||
(distance === bestDistance && (best < 0 || index < best))
) {
best = index
bestDistance = distance
}For 4,096 queries over 100,000 points, with half the points in a 2-pixel cluster, seven paired trial medians were 135.9 → 46.2 ms in Chromium, 192 → 57 ms in Firefox, and 58 → 42 ms in WebKit. Queries mixed near-cluster and uniform positions. These are lookup-only measurements, not chart frame times. The prototype matched the linear scan on 22,056 edge-case queries in each browser. I also checked multiple cluster densities and positions, plus radii 0, 1, 48 and Infinity. The standalone production ESM entry changes from 1.076 to 1.110 kB gzip and 0.970 to 0.989 kB Brotli, still below the proposed new-entry gzip ceiling. Existing-entry isolation and full repository validation are not covered by this prototype. This is a suggestion, not another correctness blocker. The documented clustered-data caveat should stay, and CI approval and full packed-consumer validation are still pending. |
|
Thanks @greglum, the full isolated checkout now passes all 2,245 DOM tests, the six additional framework targets, TypeScript, docs and bundle checks. The full packed-package gate also passes, including bare React Native, Expo, seven adapters and the final unified artifact. I checked the grid export from that final tarball against a linear scan for 112 edge-case queries, including ties, overflowing extents, tiny coordinates and distance limits. A deliberately broken lookup fails the verifier. This adds packed-package evidence to the earlier browser checks, without changing the optional performance suggestion above. Required CI and the proposed new-entry budget remain separate merge gates. Verified head: |
This branch has not been deployed
2 participants
This adds
gridSpatialIndexat@tanstack/charts/spatial/grid-index, an opt-in factory for the existingspatialIndexoption. It returns exactly the point the default linear scan returns:maxDistancerulesIt looks up the nearest point center; it does not hit-test shapes, and the docs say so. Only the default
'nearest'focus mode reaches the hook. The axis and group modes resolve before it.Default bundles do not change.
bundle:checkbudgets the subpath at 1.2 KiB gzip; it measures 1.01 kB.nearest.tsis untouched, so this should not conflict with #42 or #153. If either changes point-anchor semantics, this index and its tests should follow.Verification:
nearestPointon uniform, lattice, clustered, strip and non-finite layouts, including ties across cells.maxDistancevalues, with zero mismatches againstnearestPoint. It was not committed.pnpm performance:pointerchecks every index against the linear result before timing.typecheck,test,docs:check,bundle:checkandformat:checkpass.Benchmark (
pnpm performance:pointer, 960×540 plot, radius 48, query medians):The grid keeps about 24 bytes per point. The quadtree keeps about 90 per point on uniform data and about 145 on strips.
These numbers come from one Apple M5 Max running Node 24.18.0 while other work was running (load average 6 to 9), so read them as relative. The worst case is a pointer inside a dense cluster: about 2× slower than linear at 1k points, roughly even at 10k, and 2× faster at 100k. D3 quadtree does better there, and the large-data guide says so.
Includes a minor changeset, a PLAN.md note that the default stays the linear scan, and API-FRICTION entry F-307. That ID will need renumbering if another friction entry lands first.
Summary by CodeRabbit