feat: add hexbin transform and Hexgrid mark#552
Conversation
✅ Deploy Preview for svelteplot ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
📦 Preview package for this PR is published! Version: Install it with: npm install svelteplot@pr-552
# or install the specific version
npm install svelteplot@0.14.2-pr-552.0 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e0a8149d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const dx = explicitBinWidth ?? (xMax - xMin) / Math.max(1, bins); | ||
| // Vertical spacing between hex centers (pointy-topped hexagons) | ||
| const dy = (dx * 1.5) / sqrt3; |
There was a problem hiding this comment.
Handle zero x-extent before computing hexbin spacing
If all input points share the same x value (or binWidth is explicitly 0), dx becomes 0 here and dy also becomes 0, so later index calculations divide by zero and produce NaN bin centers. In that scenario, hexbin returns records with invalid x/y coordinates instead of a usable single-column bin result, which makes the transformed mark disappear or render unpredictably.
Useful? React with 👍 / 👎.
| const cols = Math.ceil(w / dx) + 1; | ||
| const rows = Math.ceil(h / dy) + 1; |
There was a problem hiding this comment.
Reject zero binWidth in Hexgrid path generation
When binWidth is 0, both dx and dy are 0, so cols/rows evaluate to Infinity and the nested loops below become non-terminating (j <= Infinity is always true). This can lock up rendering for any user-controlled binWidth input that allows zero, so the component should guard against non-positive widths before computing loop bounds.
Useful? React with 👍 / 👎.
Builds on top of #552. Adds the component-flavored `<Hexbin>` (Approach B from `HEXBIN_DESIGN_COMPARISON.md`, kept in my working tree) — a Svelte component that bins 2D scatter data into a pixel-space hex lattice and renders each bin as a regular hexagonal cell. Pairs with the existing `<Hexgrid>` at matching `binWidth`: a default `<Hexbin />` + default `<Hexgrid />` tile the same lattice without user coordination. ## Why a component, not just the transform The transform-flavored `hexbin()` (already in #552) cannot bin in pixel space because it runs at template-evaluation time, before scales exist. Three structural symptoms followed: 1. Cells distort under non-1:1 axis aspect ratios (transform bins in data units; projected cells aren't regular hexagons). 2. Lattice drift between the transform and `<Hexgrid>` under `nice`, `zero`, or explicit `domain` (transform reads `extent(rawData)`, grid reads `scale.domain()`). 3. User has to coordinate `bins` between the transform call, `<Hexgrid bins>`, and `r` on the Dot mark to get a coherent picture. `<Hexbin>` runs as a child of `<Plot>`, calls `usePlot()`, and builds its lattice in pixel space after scales exist. All three symptoms collapse: cells are regular by construction, the lattice is single-sourced, and there's nothing to coordinate. The transform stays for users who want the function-API ergonomics. ## What's in this PR - **Shared lattice helper** at `packages/svelteplot/src/helpers/hexLattice.ts` (102 lines, 7 helper tests). Both the existing transform and the new component import from here. `transforms/hexbin.ts` refactored 176→142 lines, `marks/Hexgrid.svelte` 92→73 lines — behavior unchanged, just deduplicated math. - **`<Hexbin>` component** at `packages/svelteplot/src/marks/Hexbin.svelte`. - **9 unit tests** in `packages/svelteplot/tests/hexbin.test.svelte{,.ts}` — including a sub-pixel center-alignment test (#6) between Hexbin and Hexgrid that catches a Y-origin bug fixed in this PR. - **3 examples** under `src/routes/examples/hexbin/` with light + dark VR baselines under `src/snapshots/hexbin/`. - **Showcase docs** at `src/routes/marks/hexbin/+page.{md,ts}` + sidebar entry. - **PlotDefaults wiring** so users can configure default Hexbin props via `setPlotDefaults`. - **`NEW_MARK_CHECKLIST.md`** at the repo root — checklist for adding new marks with monorepo paths (rebased from a doc on my main branch). The commit message has each box ticked with file:line refs. - **Pre-existing tooling fix:** `scripts/generate-api.js` had 13 stale `src/lib/...` paths from before the monorepo refactor (`3a1d5205`) — updated to `packages/svelteplot/src/...` so `pnpm docs:api:marks` works again. ## Bug fixes inline Two real bugs found and fixed during this work: 1. **Color scale was always "unknown".** `reduceOutputs` writes its result to `__fill`/`__stroke` (with the `__` prefix, see `helpers/reduce.ts:113`), not `fill`/`stroke`. The Symbol-copy step in the component was reading the wrong key, so every bin rendered as `#cccccc99`. Caught by the count-reducer test. 2. **Hexbin/Hexgrid alignment was off by `binWidth/√3`.** `Hexgrid.svelte:53` and `transforms/hexbin.ts:85` use `originY = mt`; the original Hexbin used `originY = mt + binWidth/√3` to "push cell (0,0) inside the rect" — but that breaks symmetry with the X half-pitch offset and breaks alignment with Hexgrid by exactly that amount. Caught by the alignment test (#6) — without it, screenshots looked plausibly close to aligned. ## Tests - 870/870 tests pass (861 baseline + 9 new for Hexbin). - `pnpm check`: 0 errors. - VR baselines were generated on a SLURM compute node; on a login node the Vite dev compile was too slow for puppeteer's default 60s navigation timeout. The full commit message has the complete `NEW_MARK_CHECKLIST` with file:line citations for every box. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
📦 Preview package for this PR is published! Version: Install it with: npm install svelteplot@pr-552
# or install the specific version
npm install svelteplot@0.14.2-pr-552.1 |
…558) Adds a `CONTRIBUTING.md` at the repo root following the convention used by Vega-Lite, Svelte, and shadcn/ui — single root file covering project layout, adding a new mark, adding a new transform, and per-PR expectations. Also fixes `scripts/generate-api.js`, which had stale `src/lib/...` paths from before the monorepo refactor (#551). `pnpm docs:api:marks` (referenced from `CONTRIBUTING.md`) won't work without this fix. Both pieces were originally bundled with #552 but they're project-wide tooling, not part of the hexbin feature, so splitting them out so they can land independently. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes for #552's `feat/hexbin-transform` branch: **Prettier formatting** — three Hexbin example files exceed the `printWidth: 60` override for `routes/examples/**/*.svelte`; ran `pnpm format`. Fixes the lint failure. **Per-facet binning** — `<Hexbin>` inside a faceted plot was sharing one bin map across all facets, so each panel rendered the union of all data. Records are now partitioned by `(fxVal, fyVal)` before binning. Mirrors `Density.svelte:272-286, 441-497`. New test in `tests/hexbin.test.svelte.ts`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
📦 Preview package for this PR is published! Version: Install it with: npm install svelteplot@pr-552
# or install the specific version
npm install svelteplot@0.14.2-pr-552.2 |
Summary
count,mean,sum, and other reducers via existing reduce infrastructureCloses #86
Closes #80
Split from #531 per reviewer request. Rebased onto main after monorepo restructure (#551), originally authored by @RobertFrenken in #542.
Test plan
pnpm testpasses (854 tests)pnpm buildsucceedspnpm lintpasses/transforms/hexbin