Marginal distribution strips for all 2D plots#369
Merged
Conversation
Opt-in `marginals` prop on ScatterPlot, BinnedScatterPlot, BarPlot, BoxPlot/Violin and Histogram drawing distribution strips alongside the axes, backed by a pure core/marginals.ts model and a shared PlotMarginals.svelte renderer. Types: histogram, kde, cdf, rug, plus a custom snippet. Layered API (boolean / type string / per-side map). Strips reuse each plot's positional scale (track zoom/pan), self-clip, bind to the value axis per orientation, and portal hover tooltips above overlays. Supporting refactors and fixes: - Consolidate scale/colorbar defaults into SCALE_DEFAULTS / COLOR_BAR_DEFAULTS and plot padding into DEFAULT_PLOT_PADDING - Restructure axis-utils into create_axis_loader with per-axis get/set accessors - Share LineCurve->d3 resolution via line_curve_factory; widen x-axis label container - Theme-aware BoxPlot whisker/median/outline default colors (settings + vscode ext) - Fix histogram log count axis so singleton tail bins don't collapse to the baseline - Bump svelte/kit/js-yaml/knip/native-preview and merge prek fmt+lint into `vp check`
janosh
added
enhancement
|
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:
📝 WalkthroughWalkthroughAdds marginal distribution strips and shared defaults across 2D plots, refactors axis loading state, updates box theme colors, and refreshes pre-commit, package, and CI install tooling. ChangesMarginals Feature + Shared Defaults + Axis Refactor
Tooling and dependency updates
Sequence Diagram(s)sequenceDiagram
participant PlotComponent
participant marginals_ts as marginals.ts
participant PlotMarginals
participant Tooltip
PlotComponent->>marginals_ts: normalize_marginals(marginals)
marginals_ts-->>PlotComponent: ResolvedMarginals
PlotComponent->>marginals_ts: reserve_marginal_pad(resolved)
marginals_ts-->>PlotComponent: reserved padding
PlotComponent->>PlotMarginals: resolved_marginals, marginal_series, axes, pad
PlotMarginals->>marginals_ts: marginal_strip_rect(...)
PlotMarginals->>marginals_ts: compute_marginal_curve(...)
PlotMarginals->>marginals_ts: marginal_hit(...)
marginals_ts-->>PlotMarginals: hover datum
PlotMarginals->>Tooltip: portal tooltip content
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
.pre-commit-config.yaml (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
vite-plusin the hook command.Line 40 uses an unversioned
npx --package=vite-plus, which can cause pre-commit behavior to drift over time. Pin an explicit version for reproducible local/CI checks.Suggested diff
- entry: npx --package=vite-plus vp check --fix + entry: npx --package=vite-plus@<pinned-version> vp check --fix🤖 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 @.pre-commit-config.yaml at line 40, The pre-commit hook command is using an unversioned vite-plus package, which can drift over time and make checks non-reproducible. Update the hook entry to pin an explicit vite-plus version in the npx --package invocation so the pre-commit behavior stays consistent across local and CI runs; the change belongs in the hook configuration entry that currently runs vp check --fix.tests/playwright/plot/scatter-plot.test.ts (1)
108-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid mixing SVG user units with page-space mouse coordinates.
get_svg_rect()returns raw SVG attribute values, but Lines 347-359 combineclip.x/y/width/heightwithsvg.boundingBox()'s CSS-pixel coordinates to drive the zoom drag. That only stays correct while the SVG user space maps 1:1 to the rendered size, so a responsive/layout change can make this test drag the wrong region cross-browser. Please verify the gesture from a rendered page-space box instead of attribute space.Also applies to: 347-359
🤖 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/playwright/plot/scatter-plot.test.ts` around lines 108 - 116, The zoom-drag test is mixing SVG attribute coordinates from get_svg_rect with page-space coordinates from svg.boundingBox(), which can break when the rendered SVG is scaled differently. Update the scatter-plot test to derive the target drag rectangle from the rendered page-space box instead of raw SVG user-unit attributes, and make the gesture in the zoom interaction use the same coordinate space throughout. Use get_svg_rect and the zoom-drag logic near the affected scatter-plot test block to locate and adjust the coordinate calculations.tests/vitest/plot/axis-utils.test.ts (1)
69-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeed each mock axis with a different
selected_key.Right now all four accessors start with the same key, so a loader that accidentally reads
state.axes.x.get()for every axis would still pass the routing matrix unless it also misroutes the setter. Givingx,x2,y, andy2distinct initial keys here would make the regression guard both the read and write paths.🤖 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/vitest/plot/axis-utils.test.ts` around lines 69 - 88, The mock axis setup in create_mock_state currently seeds x, x2, y, and y2 with the same selected_key, which weakens the routing test. Update the axis accessors returned by create_mock_state so each of axes.x, axes.x2, axes.y, and axes.y2 starts with a distinct AxisConfig.selected_key, keeping the existing get/set mock shape intact. This will ensure the axis-utils tests validate that the loader reads from the correct axis as well as writing back to the right one.
🤖 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/lib/plot/bar/BarPlot.svelte`:
- Around line 1361-1365: The marginal axis setup in BarPlot.svelte is missing
the categorical label mapper for the secondary x axis, so x2 marginals fall back
to raw indices. Update the axes object built for PlotMarginals so the x2 entry
in marginal_axis receives the same category-index to label callback used by x1
when cat_axis is x, keeping the existing x1/y1 behavior unchanged and using the
existing category_list mapping logic.
In `@src/lib/plot/core/axis-utils.ts`:
- Around line 91-107: Keep callback failures out of the data_loader rollback
path in axis-utils’s loader flow. In the function that calls data_loader and
then updates state.axes and state.series, move on_axis_change out of the
try/catch that handles loading so only loader failures trigger the rollback
logic. Ensure the rollback in the catch restores all mutated axis state, not
just selected_key, and make on_axis_change run after state is committed so a
thrown callback cannot leave series or axis metadata partially updated.
- Around line 95-101: The axis metadata update in the axis-utils logic currently
skips empty strings, preventing loaders from clearing an existing label or unit.
Update the conditional in the axis update path to check for undefined instead of
truthiness, and keep using the current axis state merge so `result.axis_label`
and `result.axis_unit` can intentionally set values to an empty string. Refer to
the axis update block that reads `state.axes[axis].get()` and
`state.axes[axis].set(...)` when making the change.
In `@src/lib/plot/core/components/PlotMarginals.svelte`:
- Around line 228-246: The line rendering in PlotMarginals.svelte only filters
raw LinePt values before scaling, so points that become NaN/Infinity after
pos_scale/value scale still reach line<LinePt>() and area<LinePt>(), creating
invalid SVG paths. Update the line branch to validate the scaled coordinates
from px and py (or filter the points after mapping) before building line_path
and area_path, and keep the hit-testing behavior aligned with the same filtered
points.
In `@tests/vitest/plot/PlotMarginals.test.svelte.ts`:
- Around line 356-367: The marginal tooltip currently treats axis_title as HTML
in the PlotMarginals tooltip flow, which allows consumer-provided markup to be
parsed; update the tooltip rendering path to escape or sanitize axis labels
before display. Locate the axis title handling in the marginal tooltip code used
by hover_strip/root tooltip rendering, and remove any `@html-style` insertion so
the label is rendered as plain text. Then update the PlotMarginals test to
assert escaped text output rather than checking for parsed sub elements.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Line 40: The pre-commit hook command is using an unversioned vite-plus
package, which can drift over time and make checks non-reproducible. Update the
hook entry to pin an explicit vite-plus version in the npx --package invocation
so the pre-commit behavior stays consistent across local and CI runs; the change
belongs in the hook configuration entry that currently runs vp check --fix.
In `@tests/playwright/plot/scatter-plot.test.ts`:
- Around line 108-116: The zoom-drag test is mixing SVG attribute coordinates
from get_svg_rect with page-space coordinates from svg.boundingBox(), which can
break when the rendered SVG is scaled differently. Update the scatter-plot test
to derive the target drag rectangle from the rendered page-space box instead of
raw SVG user-unit attributes, and make the gesture in the zoom interaction use
the same coordinate space throughout. Use get_svg_rect and the zoom-drag logic
near the affected scatter-plot test block to locate and adjust the coordinate
calculations.
In `@tests/vitest/plot/axis-utils.test.ts`:
- Around line 69-88: The mock axis setup in create_mock_state currently seeds x,
x2, y, and y2 with the same selected_key, which weakens the routing test. Update
the axis accessors returned by create_mock_state so each of axes.x, axes.x2,
axes.y, and axes.y2 starts with a distinct AxisConfig.selected_key, keeping the
existing get/set mock shape intact. This will ensure the axis-utils tests
validate that the loader reads from the correct axis as well as writing back to
the right one.
🪄 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
Run ID: 3e78f1d8-1593-4deb-99cd-fe11ed2ff1e3
📒 Files selected for processing (42)
.pre-commit-config.yamlextensions/dash/matterviz_dash_components/typed.pyextensions/vscode/package.jsonpackage.jsonsrc/lib/plot/bar/BarPlot.sveltesrc/lib/plot/box/BoxPlot.sveltesrc/lib/plot/core/axis-utils.tssrc/lib/plot/core/components/AxisLabel.sveltesrc/lib/plot/core/components/ColorBar.sveltesrc/lib/plot/core/components/Line.sveltesrc/lib/plot/core/components/PlotAxis.sveltesrc/lib/plot/core/components/PlotMarginals.sveltesrc/lib/plot/core/components/index.tssrc/lib/plot/core/data-transform.tssrc/lib/plot/core/fill-utils.tssrc/lib/plot/core/index.tssrc/lib/plot/core/layout.tssrc/lib/plot/core/marginals.tssrc/lib/plot/core/scales.tssrc/lib/plot/core/types.tssrc/lib/plot/histogram/Histogram.sveltesrc/lib/plot/scatter-3d/ScatterPlot3D.sveltesrc/lib/plot/scatter-3d/ScatterPlot3DScene.sveltesrc/lib/plot/scatter/BinnedScatterPlot.sveltesrc/lib/plot/scatter/ScatterPlot.sveltesrc/lib/plot/scatter/binned-scatter-types.tssrc/lib/plot/sunburst/Sunburst.sveltesrc/lib/settings.tssrc/routes/(demos)/plot/binned-scatter/+page.sveltesrc/routes/(demos)/plot/box-plot/+page.mdsrc/routes/(demos)/plot/histogram/+page.mdsrc/routes/(demos)/plot/scatter-plot/+page.mdsrc/routes/(demos)/plot/violin/+page.mdsrc/routes/test/scatter-plot/+page.sveltetests/playwright/plot/scatter-plot.test.tstests/vitest/plot/BinnedScatterPlot.test.svelte.tstests/vitest/plot/BoxPlot.test.tstests/vitest/plot/Histogram.test.tstests/vitest/plot/PlotAxis.test.tstests/vitest/plot/PlotMarginals.test.svelte.tstests/vitest/plot/axis-utils.test.tstests/vitest/plot/marginals.test.ts
Fixes from cross-model review of the marginal-distribution-strips work: - marginals: drop negative weights in clean_pairs so weighted CDFs stay monotonic and histogram normalization isn't skewed; clamp the histogram bin domain to the smallest positive sample on a degenerate log range (mirrors the kde clip) via a shared smallest_positive helper. - Histogram: treat a non-positive explicit lower bound on a log y-axis as unset so it falls back to the positive count-based minimum instead of pinning the domain at <= 0 (a broken scale), via one log_safe_range helper. - BarPlot: use derived show_x2/show_y2 for secondary-axis visibility so no phantom ranges are written when the axes aren't rendered. - axis-utils: always clear the loading flag (finally), run on_axis_change only after state settles (a throwing callback no longer triggers rollback), and let a loader clear a label/unit by returning an empty string. - PlotMarginals: filter non-finite custom bins so SVG never gets Infinity dims. Add regression tests for each fix and fold/parametrize related cases across the marginals, PlotMarginals, Histogram, and axis-utils suites. svelte-check pre-commit hook bypassed: it fails repo-wide on a toolchain Snippet-type incompatibility unrelated to these changes.
svelte-check-rs@0.10.1 (the latest release, published 2026-01-04) predates
svelte 5.56.4 and mishandles its Snippet types, emitting ~660 false-positive
"{@render ...} must be called with a Snippet" errors across the repo. Official
svelte-check is accurate (0 errors after the fix below), so the pre-commit hook
is usable again.
- .pre-commit-config.yaml: run `svelte-check` instead of `svelte-check-rs`
- package.json: replace the svelte-check-rs devDependency with svelte-check,
drop @typescript/native-preview (only svelte-check-rs's peer), update knip
ignoreDependencies accordingly
- PeriodicTableDemo: allow null in custom_tooltip's value type to match
PeriodicTable's tooltip prop (HeatValue | null) -- the one genuine error
the accurate checker surfaced (the snippet body already handled null)
- log_safe_range: drop any non-positive bound (not just the lower) on a log axis so an invalid upper can't pin a broken (<= 0) domain either; both the auto (calc_y_range) and resolved range paths fall back accordingly. - DRY the scales and ticks derivations behind small per-axis helpers, dedupe the single-vs-multi bar-color ternary into a series_color helper, and collapse calc_y_range's duplicated empty-data fallback into one empty_range. - Tests: guard the log tick assertions against empty arrays (Math.min(...[]) is Infinity, a false pass) and add a non-positive upper-bound case.
Carry the Histogram scales/ticks simplification to the other 2D plots: replace the four near-identical create_scale calls with an axis_scale(axis, range, px) helper, and the four generate_ticks blocks with an axis_ticks(...) helper (the categorical branch stays at the call site). Also rename Histogram's ticks helper make -> axis_ticks so all three components share the same naming.
- Fix axis title overlapping wide tick labels: calc_auto_padding now reserves the rotated title width on l/r (mirrors the x2/top branch), and the trajectory scatter plot drops its fixed left padding so it auto-grows with tick width - DRY: extract create_axis_scales (Bar/Box/Histogram) and a side_pad helper in layout; remove redundant Vec2/number[] casts; tighten compute_bar_auto_ranges to return Vec2 - Single source of truth for viewer control-button icon size via the --ctrl-btn-icon-size token (ViewerChrome/Structure/ConvexHull/Trajectory) - Trajectory: ~g axis format (0.8 not "800m"), slider knob above step labels, circular checkmark badge
- Replace setInterval playback with a requestAnimationFrame loop: changing fps retargets the cadence without restarting, a clamped delta + at-most-one-advance per tick lets a background tab resume cleanly (no fast-forward jump), and the loop stops if the trajectory becomes invalid/single-frame - Add a bounded LRU frame cache (frame-count + total-atom budget) with 2-frame prefetch and in-flight de-duplication so scrub/playback over indexed/streaming trajectories doesn't re-read or double-load frames - Memoize per-trajectory property extraction (WeakMap) so visible_properties or label/config changes reuse the per-frame walk instead of redoing it - Show the FPS control for any multi-frame trajectory (set speed before play); add keyboard-shortcut hints to the nav button tooltips - calc_auto_padding reserves the rotated axis-title width on l/r so wide y-tick labels (e.g. -789.391) don't overlap the title, and reserves nothing extra when a side has no ticks - Tests for the extraction memoization and the axis-title padding reservation
Carve cohesive chunks out of three oversized plot files and re-export them
from the originals so every consumer import stays unchanged:
- types.ts -> types/{fills,reference-lines,plot-3d}.ts (barrel re-export)
- data-cleaning.ts -> data-cleaning-signal.ts (signal primitives; export *)
- Histogram.svelte -> histogram.ts (pure binning/range helpers, unit-testable)
Dedupe the histogram binning config via shared BinConfig/RangeLimit types and
import the cleaning config types from their canonical data-cleaning-signal home.
…s shading, keyboard toggle - multi-view active pane allows the hover tooltip to overflow into neighboring panes (overflow:visible + raised z-index) instead of being clipped - perf: memoize bond neighbor search across the 4 panes (compute_bonds), make highlight targets pulse-independent so the array isn't rebuilt every animation frame, and derive partial-occupancy atoms + negated rotation target - shared subtle canvas background shading for plots and structures via theme vars --plot-bg/--struct-bg (drop the trajectory plot surface-bg override) - Cmd/Ctrl+G shortcut to toggle the 2×2 grid
The max(window, order+2) / min(window, length) clamps could leave the effective window even, yielding an asymmetric kernel and a normalization mismatch against the coefficient row. Force it odd so SG reproduces low-degree polynomials at interior points. Adds interior-quadratic regression tests and tidies the data-cleaning tests (variance helper, in_place series reuse).
Fix camera and trajectory cache state handling while tightening browser/unit tests around the affected interactions.
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.
Summary
Adds an opt-in
marginalsprop to every 2D plot (ScatterPlot,BinnedScatterPlot,BarPlot,BoxPlot/Violin,Histogram) that draws distribution strips alongside theaxes. Built on a pure, unit-tested
core/marginals.tsmodel + a sharedPlotMarginals.svelterenderer.histogram,kde,cdf,rug, plus a customsnippetescape hatch.marginals={true}for sensible defaults, a single type string, or aper-side map (
{ top, bottom, left, right }) with full per-side config (size, bins,bandwidth, normalize, value_range, axis binding, styling, hover tooltip).
axis per orientation, and portal hover tooltips above overlays. Hit-testing requires the
pointer to be over the rendered bar/fill.
Supporting refactors & fixes (done alongside)
SCALE_DEFAULTS/COLOR_BAR_DEFAULTSand plotpadding into
DEFAULT_PLOT_PADDING.axis-utilsintocreate_axis_loaderwith per-axis get/set accessors.LineCurve→d3 curve resolution vialine_curve_factory; widen the x-axis labelcontainer so long horizontal titles fit on one line.
BoxPlotwhisker/median/outline default colors (settings + VS Code ext).svelte/@sveltejs/kit/js-yaml/knip/TS native-preview and merge the prekfmt+linthooks into a singlevp check.Demos updated for scatter, histogram, box, violin and binned-scatter; new unit tests for
marginals.tsandPlotMarginals.svelte.Summary by CodeRabbit
marginals) across 2D plots (scatter, histogram, bar, box, binned scatter), including hover tooltips and marginals-aware layout/value-axis rendering.var(--text-color, black)).