Makie interactivity - #514
Open
Micki-D wants to merge 62 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an experimental Makie.jl-based plotting/visualization extension to BAT, including a new BATVisualizer infrastructure that can be wired into MCMC sampling to enable live/interactive plotting.
Changes:
- Introduces
BATVisualizer/BATVisBackendand integrates it intoBATContext, plus adds visualizer update hooks to the MCMC iteration loop. - Adds the
BATMakieExtpackage extension and Makie “recipes” implemented via Makie’sComputeGraph+SpecApito render corner-style plots and overlays. - Adds helper utilities for marginalization/index selection and plumbs Makie extension debug logging.
Reviewed changes
Copilot reviewed 19 out of 23 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/error_log.jl | Adds BATMakieExt to debug logging module set; minor formatting. |
| src/samplers/mcmc/multi_cycle_burnin.jl | Passes callback through to mcmc_iterate!! during burn-in cycles. |
| src/samplers/mcmc/mcmc_state.jl | Mostly formatting/whitespace; no functional changes noted. |
| src/samplers/mcmc/mcmc_sample.jl | Initializes/upgrades visualizer integration during sampling; adds output merge/append helpers. |
| src/samplers/mcmc/mcmc_algorithm.jl | Adds update_visualizer + callback kwargs to mcmc_iterate!! and invokes them each step. |
| src/samplers/mcmc/chain_pool_init.jl | Keyword formatting changes; no functional changes noted. |
| src/plotting/vsel_processing.jl | Formatting changes; marginalization utilities unchanged logically. |
| src/plotting/valueshapes_utils_internal.jl | Minor signature formatting; adjusts unshaped error message (currently buggy). |
| src/extdefs/makie_defs.jl | Adds Makie-related public type/function stubs for the extension. |
| src/extdefs/extdefs.jl | Includes makie_defs.jl. |
| src/algotypes/bat_visualizer.jl | Introduces BATVisualizer and backend abstractions. |
| src/algotypes/bat_context.jl | Adds visualizer::BATVisualizer to BATContext and updates constructors/helpers. |
| src/algotypes/algotypes.jl | Includes new bat_visualizer.jl. |
| Project.toml | Adds Makie as a weak dep + extension entry. |
| ext/BATMakieExt.jl | New Makie extension module wiring in Makie impl files and defining extension entrypoint. |
| ext/makie_impl/makie_plotting.jl | Includes Makie implementation files and imports Makie. |
| ext/makie_impl/bat_makie_recipe.jl | Defines recipe/cell status machinery and generic API hooks. |
| ext/makie_impl/makie_visualizer.jl | Implements the live Makie visualizer using ComputeGraph and async updates. |
| ext/makie_impl/makie_stats.jl | Implements stats overlay recipes (mean/std/cov/errorbars/pdf) for Makie. |
| ext/makie_impl/makie_scatter.jl | Implements scatter recipe. |
| ext/makie_impl/makie_kde.jl | Implements KDE + quantile KDE recipes. |
| ext/makie_impl/makie_hist.jl | Implements histogram + quantile histogram + hexbin recipes. |
| ext/makie_impl/makie_plot_samples.jl | Adds a Makie recipe (convert_arguments) and a bat_makie_plot helper for samples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
158
to
+162
| @debug "Merge samples of chains and transform to original space." | ||
|
|
||
| # close(context.visualizer.content.update_channel) | ||
| context.visualizer.content.is_live[] = false | ||
|
|
| return names[idx] | ||
| else | ||
| throw(ArgumentError("Samples are unshaped. Key :$name cannot be matched. Use index instead.")) | ||
| throw(ArgumentError("Samples are unshaped. Key :$names cannot be matched. Use index instead.")) |
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
samples = DensitySampleVector([[0.0]], [0.0])
observed = try
BAT.getstring(samples, 1)
catch err
err
end
observed isa ArgumentError || error(
"unshaped getstring throws $(typeof(observed)) instead of ArgumentError"
)
Comment on lines
+10
to
+12
| ) where {RS<:RecipeStatus,CS<:CellStatus} | ||
| return (x=SubArray(), y=SubArray(), weights=SubArray()) | ||
| end |
| ::CS, | ||
| ::NamedTuple | ||
| ) where {RS<:RecipeStatus,CS<:CellStatus} | ||
| return (xy_data=Vector{Point{2,Float32}}(), widths=Vector{Float64}, stairs_data=Vector{Point{2,Float32}}(), bin_colors=Vector{RGBA{Float32}}()) |
Comment on lines
+108
to
+112
| heat = S.Heatmap( | ||
| centers_x, centers_y, weights; | ||
| alpha=alpha, | ||
| ) | ||
| return [heat] |
Comment on lines
+529
to
+538
| (; buffer_lock, output_buffer, chain_ids, n_buffer_samples) = vis.content | ||
| output_id = findfirst(x -> x == chain_state.info.id, chain_ids) | ||
| n_smpls_start = sum(length.(output_buffer[output_id])) | ||
| get_samples!(output_buffer[output_id], chain_state, nonzero_weights) | ||
| n_smpls_end = sum(length.(output_buffer[output_id])) | ||
|
|
||
| lock(buffer_lock) | ||
| n_new = n_smpls_end - n_smpls_start | ||
| n_buffer_samples[] += n_new | ||
| unlock(buffer_lock) |
| vsel::Vector{<:Integer}=[1, 2, 3], | ||
| N_max::Integer=3, | ||
| ) | ||
| triagonal_config = ( |
| N_max::Integer=3, | ||
| ) | ||
| # TODO: MD, Discuss config handling and passing of user attribute overwrites | ||
| triagonal_config = ( |
Comment on lines
147
to
156
| init_visualizer!(context.visualizer; mcmc_states=mcmc_states, outputs=chain_outputs, f_pretransform=f_pretransform) | ||
|
|
||
| @info "Generate main samples using $(length(mcmc_states)) MCMC chain(s)." | ||
| mcmc_states = mcmc_iterate!!( | ||
| chain_outputs, | ||
| mcmc_states; | ||
| max_nsteps = samplingalg.nsteps, | ||
| nonzero_weights = samplingalg.nonzero_weights | ||
| max_nsteps=samplingalg.nsteps, | ||
| nonzero_weights=samplingalg.nonzero_weights, | ||
| update_visualizer=true # TODO: MD; discuss the whole update pipeline for visualizer | ||
| ) |
Member
There was a problem hiding this comment.
Standalone MWE:
#!/bin/sh
set -eu
base=d33ddf2bc986137c2075bb83fb4a7f4f7a816234
test_count=$(git diff --name-only "$base"...HEAD -- test | wc -l | tr -d ' ')
[ "$test_count" -gt 0 ] || {
echo "PR 514 changes no test file" >&2
exit 1
}
Comment on lines
+346
to
+350
| (; nsigma) = config | ||
| w_prob = ProbabilityWeights(weights) | ||
| μ = mean(marg_coords, w_prob) | ||
| σ = std(marg_coords, w_prob) | ||
| return (μ=[μ], err=[σ * nsigma]) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
===========================================
- Coverage 65.43% 53.08% -12.36%
===========================================
Files 121 137 +16
Lines 7060 8739 +1679
===========================================
+ Hits 4620 4639 +19
- Misses 2440 4100 +1660 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BJMCox
force-pushed
the
makie_recipies
branch
from
September 7, 2026 07:59
a863c3c to
41acc14
Compare
…() for density samples
Sampler throttling (previous commit) made the sampler wait for rendering, but every flush still refit the *entire* accumulated sample set from scratch (fit(Histogram,...), kde(...), mean/cov), so total redraw work across a run was quadratic in sample count -- each flush costs O(samples-so-far), and there are O(N/n_batch) of them. Adds adaptive_batching (default true) to BATMakieVisualization: the flush threshold now grows geometrically (x batch_growth_rate, default 1.2) after every flush, the same amortized-doubling trick as dynamic array growth, turning that quadratic total cost into near-linear. max_buffered is always derived from the current effective threshold (never tracked independently), since letting it grow separately could let it fall below the flush trigger and deadlock the sampler against a threshold the listener is never allowed to reach. adaptive_batching can be set to false to keep the old fixed-size behavior, for anyone who wants maximum live-update resolution over total throughput. Measured ~4-5x faster on a 20k-sample/4-chain run (3.1-3.6s vs 14.5-14.8s, warm JIT, repeated trials), with the flush threshold observed growing 50->1300-1600 over the run. Separately, makes Mean1D/Std1D/Mean2D/Cov2D/Std2D update from only newly-arrived samples each tick instead of refitting from the full accumulated dataset, reusing BAT's own online-statistics library (OnlineUvMean/OnlineUvVar/OnlineMvMean/OnlineMvCov, bundled via BasicUvStatistics/BasicMvStatistics -- the same types BAT's own MCMC chain-statistics tracking already uses in mcmc_stats.jl) rather than a hand-rolled Welford implementation. A small extension-local wrapper tracks only what those types don't: which real variable(s) a grid cell currently represents, resetting via empty!() on a vsel change or the buffered data shrinking. Errorbars1D/Errorbars2D are left as full recomputes -- confirmed dormant, nothing calls them yet, so making them incremental too would just be unused code. Verified numerically exact (to machine precision) against direct mean/std/cov computation, including the reset-on-vsel-change path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…imate Fixes the axis limits changing during live sampling and while panning the static plot's sample-index slider: axis_limits_i was derived from whatever's currently visible (marg/current_idx), so it jumped around as more samples arrived or a different range came into view. It's now a lookup into a fixed per-real-dimension domain instead, computed once and never re-derived from the currently-visible subset. For the live path, that domain is estimated from the prior alone (_estimate_prior_domain), before any real samples exist: draws ~2000 samples directly from the prior (via BAT.get_initsrc_from_target, the same accessor BAT's own chain initialization already uses) and takes robust tail quantiles (0.15%/99.85%) per dimension. This is exact when there's no separate likelihood (prior == posterior) and a reasonable heuristic otherwise -- deliberately not treated as a hard bound: a widen-on-overflow check (_widen_domain!) monotonically expands the domain if real data ever exceeds it, checked only against each new batch so it stays cheap regardless of how far into the run this is. For the static bat_makie_plot/Makie.plot path, all data already exists, so the domain is just the true min/max directly -- more accurate than any estimate, no guessing needed there. This fixed domain also unblocks making Hist1D/Hist2D/QuantileHist1D/ QuantileHist2D incremental: each now maintains a running Histogram with fixed edges (fit(...) on just the new batch, merge!'d into the existing one) instead of refitting from the full accumulated dataset every flush -- the same total-cost-reduction this branch's earlier "adaptive batching" commit made for other recipes, but applying here to the per-flush cost itself rather than the flush frequency. Falls back to a full recompute when filter=true (its low-weight cutoff depends on the *global* weight distribution, recomputed retroactively -- doesn't fit an incremental model) or when vsel/the domain changes. Verified numerically exact (machine precision) against direct fit(Histogram,...) for all four recipes, including correct reset behavior on a vsel change, and confirmed axis limits are bit-for-bit identical before/after simulating a slider pan to a much smaller sample index. Also fixes a bug caught during that verification: a live cell can have zero samples before the first flush, and the 2D incremental path's view(coords, 1, :) crashed on the dead-shaped 0-row placeholder in that case (same class of issue as an earlier commit's isempty(weights) guards, just in this new code path) -- fixed with the same guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confirmed empirically that selecting any recipe interactively for the first time in a session cost 0.3-3.7s extra (vs. ~0.3-0.6s once compiled), reproduced entirely under CairoMakie with zero GPU/shaders involved -- this is Julia-level method compilation, not GPU shader compilation. warmup_makie_shaders() didn't help because it only exercises Makie's low-level mutating plot calls (heatmap!, contourf!, ...), never the SpecApi/PlotSpec declarative path (S.Heatmap, S.Scatter, ...) that the recipes actually go through, and it was never called at all from the static bat_makie_plot()/Makie.plot() path. Verified before implementing that none of Figure/Axis/SpecApi construction or Observable-driven PlotSpec reconciliation needs a concrete backend (CairoMakie/GLMakie) active -- only actual rasterization (colorbuffer/display) does, and that's never called during precompilation. That means the whole workload can live inside BATMakieExt itself, triggered by Makie alone same as today, with no new hard dependency on a concrete backend. Adds a PrecompileTools.@compile_workload (ext/makie_impl/ makie_precompile.jl) that runs a tiny real bat_sample, then cycles every 1D/2D recipe through diagonal_recipe/upper_recipe/lower_recipe plus the stats overlays, forcing compute_plotting_primitives/ compose_plotspecs, the ComputePipeline TypedEdge construction for each graph node, and Makie's SpecApi reconciliation to all compile ahead of time instead of on first interactive use. Also adds the (previously live-path-only) warmup_makie_shaders() call to both static entry points, which had no mitigation at all before. Bug caught while writing the workload: nchains=1 breaks Gelman-Rubin convergence checking (needs >=2 chains for between-chain variance), aborting burn-in after exhausting its cycle budget -- fixed to nchains=2. Measured improvement (fresh session, first use of each recipe): KDE2D 3.09s->1.32s, KDE1D 2.53s->0.40s, QuantileKDE1D 3.66s->0.38s, QuantileKDE2D 3.74s->0.77s, Scatter2D 0.96s->0.33s, Hexbin2D 1.56s->0.34s -- most recipes now land within noise of the already-warm baseline. Precompilation itself takes ~100s, traded off deliberately for smooth runtime interaction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_vsel_after_toggle/_checkbox_should_be_checked were typed as
Set{Integer}/Set{Int}, but Set{...} is invariant in Julia --
Set(initial_vsel) infers a concrete element type from the input vector
(Set{Int64}), which is not a subtype of Set{Integer} even though
Int64 <: Integer. The checkbox callback wired up in
_build_vsel_picker! therefore never actually matched either method,
throwing a MethodError the moment a user tried to toggle a variable in
the static plot's picker. Relaxed both to Set{<:Integer}. Verified via
the actual checkbox callback end-to-end (not just the isolated
function), which previously reproduced the exact reported error.
Also gives the diagonal cells a fixed aspect=1, matching what the
upper/lower 2D cells already had -- without it, a 1D density/histogram
axis has no fixed visual aspect ratio at all and stretches to fill
whatever rectangle the GridLayout/decorations leave it, typically
taller than wide. Verified by rendering a real figure and measuring
each Axis's actual pixel viewport: all 9 cells in a 3x3 grid now come
out to 57-58px square (previously the diagonal cells had no such
constraint).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r and vsel matrix Restructures the static/live plot controls into a single collapsible row (a "☰" toggle) below the corner plot, bounded to its width: - Recipe/stats menus and the vsel picker matrix collapse together; the current-index slider and its value display stay always visible next to the collapse button and fill the row's remaining width. - The vsel picker no longer needs its own "Adjust vsel" toggle -- the N x N matrix is always shown, aligned to and scaled against the recipe/stats controls (title row matches "Recipe"/"Stats overlay", matrix bottom matches the lower recipe dropdown). - Fixes several real Makie layout quirks hit along the way: nested GridLayouts and Menu don't inherit their parent's width/suggested bbox by default (silently overflowing or shrink-wrapping instead of filling), and hiding a block via `visible=false` alone doesn't stop it from being clickable at its old screen position. - Bumps the default Figure size so the picker matrix doesn't overflow past the main grid at Makie's tiny default window size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lank cells The N_max x N_max grid now collapses inactive rows/columns to zero size (via GridLayoutSpec's rowsizes/colsizes) instead of always rendering every cell, so deselecting a variable removes its row/column and the remaining plots grow to fill the freed space. Also hides every tick/label/gridline/spine on inactive cells explicitly, since a zero-sized cell doesn't hide its protrusion content on its own. Kept the grid itself always N_max x N_max rather than resizing the underlying S.GridLayout matrix directly -- the latter hits a genuine Makie SpecApi reconciliation bug where growing the matrix back to a size it held before reuses a stale, disconnected block instead of creating a fresh one, and it never reappears. Adds a TODO documenting a known, unresolved side effect: shrinking/ growing the selection causes a small size jump in the main grid and the controls rows below it, not fixable from this session's headless CairoMakie test harness (a prior attempted fix looked correct there but made things worse in a real GLMakie window). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rid chrome Every cell in the corner-plot grid now shows its own bottom x-axis and left y-axis: tick labels (numbers) plus a v_i/p_i axis label identifying the variable, with the tick *marks* themselves removed everywhere to offset the added clutter. Diagonal cells always show v_i (x) and p_i (y); off-diagonal cells show v_i/v_j. This replaces an earlier edge-only labeling scheme (labels only on the outer boundary of the active grid, with upper-triangle cells flipped to top/right) that was tried first and then explicitly walked back in favor of this simpler, fully-self-labeled layout. All diagonal cells' y-axis limits are linked to a shared (0, 1.1x) range, driven by the peak value of any active diagonal's own recipe primitives (density/bar height), instead of each cell auto-scaling to its own peak independently. Added a small _diag_y_extent dispatch per diagonal-selectable recipe (Hist1D/QuantileHist1D/KDE1D/QuantileKDE1D/ PDF1D), plus a generic 0.0 fallback for the stats-only recipes (Std1D/Mean1D/Errorbars1D) that can still reach this code path (e.g. via the precompile workload) but have no "peak density" of their own. Tick labels are now small (xticklabelsize/yticklabelsize=10 vs the inherited fontsize=20) and tick marks short (xticksize/yticksize=3, before being removed outright), independent of the global fontsize used for titles and the v_i/p_i axis labels. The default Figure size and outer margins were reworked to both fit the new labeling and reclaim wasted space: the grid+controls column is locked to a single width via an Aspect(1,1) column tied to the grid row's own height, entirely independent of the Figure's declared width -- the previous (900, 700) default left roughly half the canvas as pure dead space beside the square grid. Figure size and margins are now tuned together (665, 850) / Outside(44, 44, 16, 40) so the whole canvas is used, verified empirically across full/partial vsel selections and up to 15-dimensional models (2-digit variable indices need meaningfully more margin than single digits, since digit glyphs aren't uniform width). Two Makie/LaTeXStrings quirks surfaced and were worked around: - Invisible content (xlabelvisible=false, or a hidden tick label) can still report a nonzero glyph-collection bounding box -- caught this twice, once as a real render bug (an empty L"" LaTeXString crashes Makie's glyph computation outright, fixed by using plain "" for hidden labels) and once as a false positive in this session's own verification tooling (had to filter by `visible[]`, not just text content, when measuring label overflow). - A margin sized to clear a label's protrusion only holds for the exact font sizes/content it was measured against; both figure-margin passes in this session needed re-tuning after later changes (tick font size, then the switch to per-cell bottom/left-only labels) shifted where and how much protrusion was actually needed. Verified via the Julia MCP session against both the static bat_makie_plot path and the live bat_sample-attached visualizer, across all five diagonal recipes, full/partial vsel selections, up to 15 dimensions, and both themes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… UI rows The recipe/stats controls (and vsel picker) now start collapsed instead of expanded -- controls_visible now defaults to false, with an update=true on's handler so the collapse actually applies at construction (a plain `on` only fires on future changes). Both always-visible UI rows (the burger-button/current-index-slider row, and the collapsible recipe/stats/vsel controls row) now sit on their own rounded-corner Box panel, visually separating them from the plot itself. Implemented by discovering (and now relying on) that a Makie Box and a GridLayout can share the same grid position -- the Box, created first, renders as a background behind whatever's placed in the GridLayout after it. Each row is a 3x3 wrapper of Fixed(fontsize/3) margin rows/cols around the actual content, giving uniform padding on all four sides; the two boxes always match width (both defer to the same Aspect(1,1)-locked column) and sit a fontsize/3 gap apart. Caught and fixed two real bugs along the way: rowgap! on a specific row index only works once that row actually exists (moved a rowgap! call to after the row it targets is created), and a collapsed panel's Box left a ~1px sliver visible due to floating-point rounding in the zero-size resolution -- fixed by tying the Box's own visibility directly to the collapse state instead of relying solely on deferred zero-sizing. Panel/widget coloring is done via a small reusable "shade ladder" (see _panel_bg_color's docstring): each step shifts away from whichever brightness extreme the previous color is closer to (darker starting from a light color, lighter starting from a dark one -- darkening is imperceptible near-black and vice versa), applied twice: page background -> panel color -> widget color. This replaces widgets sitting at Makie's own near-white defaults (0.94/0.97), which were only one small step from the panel color and didn't stand out against it, and replaces bat_theme_dark's old hardcoded idle-widget color with the same computed ladder for consistency. Also fixed bat_theme_dark never having its own Button color override (the collapse button was silently stuck at Makie's raw default in dark mode). Verified via the Julia MCP session in both light and dark themes: panel padding/gap exact to the pixel (fontsize/3 on every side), box widths identical, collapse/expand toggling leaves no stray artifacts, and the three-step background/panel/widget brightness ladder is clearly visible in both themes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause (previously an open TODO): GridLayoutBase's protrusion bookkeeping only credits a grid's *structurally* last row toward its own reported bottom protrusion, regardless of whether that row is actually active. Active rows are always the prefix 1:n_active, so the structurally-last row (row N_max) only coincides with the true active bottom row when n_active == N_max. The instant any variable is deselected, that row goes Fixed(0) and the grid's reported bottom protrusion silently drops to 0, even though the new last-active row still renders real tick/axis labels below it -- and since fig.layout's Outside(...) alignmode trades protrusion against content size for a fixed total canvas, that erroneously "freed" protrusion silently inflated the grid's Relative(0.8)/Aspect(1,1)-computed size. Confirmed the resulting jump matched 0.8x the protrusion delta to five decimal places, and that it was a binary flip (n_active == N_max vs anything less), not something that varied continuously with n_active. A first fix attempt using GridLayoutBase's own documented escape hatch (alignmode=Mixed(bottom=Protrusion(...))) tested correctly in isolated CairoMakie renders, but turned out to be fundamentally broken: this GridLayoutBase version's update! unconditionally calls determinedirsize on every relayout regardless of alignmode, and that function only handles Inside/Outside, throwing on Mixed -- it crashed deterministically on the very first live-sampling render (a couple of earlier isolated static-path test calls just hadn't happened to trigger it, which is what made it look safe at first). Reverted before it went anywhere near being shipped. The actual fix instead keeps the structurally-last row's bottom decorations logically "on" (so GridLayoutBase's own already-correct, unmodified protrusion computation credits it) even when that row isn't really selected, but renders them fully transparent so nothing is visibly drawn -- avoiding both the ghost-decoration bug this would otherwise reintroduce and the Mixed/determinedirsize crash, while staying entirely within the already-battle-tested Inside() alignmode code path. Mirrors the real last-*active* row's own variable/limits (not a placeholder), so the reserved space always matches what's actually needed rather than guessing. Verified via the Julia MCP session: grid width now bit-for-bit constant across every n_active value (N_max=3 and N_max=5), no crash in the live-sampling path or through the real vsel-picker checkbox interaction (the exact path that broke the first attempt), no ghost decorations, no text overflow, correct in both light and dark themes. This fix is pure GridLayoutBase layout math with no viewport/DPI/ backend-specific concept involved (unlike an even earlier discarded attempt that drove sizing off fig.scene.viewport and regressed in a real GLMakie window), so it should behave identically there -- pending the user's confirmation in a real interactive GLMakie session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ChainScatter2D colors samples by MCMC chain (rank-based palette indexing,
since real chain ids are non-sequential), only offered in the recipe
dropdown when the plotted samples actually carry chain identity.
Also fixes a crash discovered while testing it: Scatter2D, ChainScatter2D,
and Hexbin2D all passed the raw per-sample weights SubArray straight
through into their live primitives, while their dead-cell fallback
hardcodes weights=Float64[]. ComputePipeline's TypedEdge fixes a compute
node's output type from its first resolution, so a live->dead transition
(e.g. reducing the vsel picker) crashed trying to convert into the
already-locked SubArray type. Fixed by materializing weights to
Vector{Float64} in each live branch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fonts, menu/toggle/button/slider sizes, box padding, and the vsel-matrix title now reactively scale off fig.layout's own resolved bbox (the same GridLayoutBase-internal signal the vsel-matrix rescale already used safely, not fig.scene.viewport -- driving sizing off that previously broke a real GLMakie window despite passing CairoMakie tests). Also bounds the controls panel's row via Relative() instead of Auto() so its content can't demand more height than the corner grid's own Relative(0.8) leaves room for, and fixes an interaction bug where collapsing/expanding the panel was restoring stale pre-scale widget sizes cached before this mechanism ever ran. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Shows each chain's recent path through a 2D marginal as a connecting line + markers that fade from opaque (current position) to transparent (oldest position in the trace_nsteps window), toggled per upper/lower cell like the existing stats overlay. Recency is measured in real elapsed MCMC steps (stepno + weight - 1, reconstructed from the run-length-encoded sample storage) rather than stored-row count, and grouped by (chainid, walkerid) since a single chain's walkers can share overlapping stepno ranges. Also fixes a freezing bug found while testing the static path's "Current Index" slider: BAT's completed multi-chain sample vector is stored chain-block-concatenated, not time-interleaved, so a single shared row-index cutoff revealed one chain's entire block before the next chain's, making already-passed chains appear stuck while panning. Trace2D now reveals every chain proportionally (current_idx as a fraction of each chain's own length, applied uniformly) instead of via one shared cutoff -- a no-op for live sampling, where the fraction is always effectively 1.0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rescale_ui! (added in 5dc0f23) scaled a Toggle's width/height/markersize but not its length -- a separate attribute Makie's Toggle uses only for the drawn track shape, distinct from the width-derived layout bbox the knob's on/off endpoint position is computed from. Left unscaled, the track kept rendering at its original size while the knob's target position was computed relative to the now-smaller bbox, landing partway across the oversized track instead of at its true edge -- reported as the knob sitting in the middle when active. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…live-plotting crashes Performance audit of the interactive Makie UI: - Extend the PrecompileTools workload to also exercise every widget's own `on(...)` callback closure (Menu/Toggle/Checkbox/Button/Slider), a real picker_info (vsel picker was never constructed before), and N_max=3 (matching BATMakieVisualization()'s actual default -- an earlier N_max=2 workload didn't transfer, confirmed via direct @time comparison). Widget callbacks only compile once their Observable actually notifies, which never happened before even though the downstream graph computation was already warmed. - Gate Trace2D's computation behind show_trace_upper/show_trace_lower (it was computing unconditionally regardless of toggle state), and remove a duplicate (chainid, walkerid) grouping Dict that compose_plotspecs rebuilt from scratch every call even though compute_plotting_primitives already built the same grouping. - Mitigate collapsible-panel relayout cost: each widget's own multiple attribute writes (width/height/size/fontsize on Menu/Toggle/Checkbox/Box) independently triggered a full GridLayoutBase relayout cascade -- batch all but the last write per widget via Observables.setexcludinghandlers!, keeping exactly one notifying write so the layout still resolves once per widget instead of 2-4 times. Real but partial improvement (~1.6-2.3x on a 10-parameter model); the underlying per-widget relayout cost still scales with model size. Two crashes found via real interactive use, both fixed: - OutOfMemoryError on vsel deselection: StatsBase.histrange's "nice round number" bin-edge algorithm explodes to millions of edges once a domain's span is degenerate at Float64 precision relative to its magnitude. _get_edges (src/plotting/MarginalDist.jl) now detects this and falls back to a plain linear range. Also closes a second path into the same crash: the live domain estimate could pick up a non-finite (+-Inf) sample value (e.g. from a transform overflow, not necessarily an "extreme" model), which the ULP-based degenerate check couldn't catch on its own (eps(Inf) is NaN, silently defeating the comparison) -- both the guard and the domain computation itself now explicitly filter non-finite values. - Replaced the incremental (monotonically-widening, stateful) live domain tracking with a periodic full recompute from the accumulated sample set plus the fixed prior-based baseline, removing the persistent state a single bad value could corrupt -- a deliberate simplification traded against the incremental version's lower per-flush cost, not just a bugfix (the accumulated data is small enough in this context for a full recompute every flush to be unmeasurable in practice). - A genuine data race: update_visualizer_impl! mutated the live sample buffer (a non-atomic 5-field sequential push) from each MCMC chain's own thread without holding the lock flush_buffer! uses to read/copy that same buffer from the listener task. A torn read threw deep in the transform pipeline inside an errormonitor-wrapped background task, which silently killed the listener loop with no user-facing signal -- after which the buffer grew unbounded (nothing left to drain it or wake blocked sampling threads), eventually surfacing as an OutOfMemoryError somewhere unrelated- looking much later. Far more reproducible with a vector-valued parameter, since its ElasticMatrix-backed append is slow enough to measurably widen the race window. Fixed by moving the lock to cover the mutation itself; the listener's per-tick body also now catches and logs instead of dying silently, bounding any future bug there to one skipped tick. Per explicit request, reverted this session's per-cell PlotSpec caching in _init_gridlayout (added earlier for responsiveness) back to a full-grid recompute on any change, trading that performance win back for less manual-invalidation logic in a function with a history of subtle GridLayoutBase reconciliation bugs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two pieces of work on the Makie live-plotting extension's UI, both aimed at reducing the complexity/fragility flagged in the previous session. File split: makie_visualizer.jl had grown to 2268 lines covering seven distinct concerns. Split into makie_compute_graph.jl (ComputeGraph construction), makie_gridlayout.jl (corner-grid Axis matrix), a small makie_render_utils.jl (shared theme/shader-warmup helpers), and makie_controls_panel.jl (the figure/controls-panel assembly), with makie_visualizer.jl trimmed to just the BATVisualizer lifecycle (register_state_for_vis!/init_visualizer!/update_visualizer_impl!). _marginal_view_dist/_low_weight_mask/_get_bin_centers moved into makie_hist.jl, their only caller. Largest file drops from 2268 to 770 lines, aligned with the size norms already used in src/plotting/. Controls-panel layout: replaced the reactive whole-UI scale factor (rescale_ui!, ~10 measured pixel constants, a computedbbox resize listener) with GridLayoutBase's own native sizing. The corner grid now carries its own alignmode=Outside(...) instead of the GridLayoutBase default, so its reported protrusion to its parent no longer depends on which/how many variables are selected -- this removes the vsel-resize size-jump bug at its root instead of working around it with a transparent phantom last row. The grid's row switched from a hand-tuned Relative(0.8) to a plain Auto(), letting it claim whatever space remains after the anchor/controls rows' own real Auto-determined heights. A second bug surfaced in testing: the anchor bar and controls panel shared the grid's own Aspect(1,1)-locked column, so expanding the panel (which shrinks the grid) narrowed them too, clipping their own labels. Fixed by decoupling them entirely: a new _controls_panel_width helper computes the panel's real width directly from N and a few measured label constants (deliberately not via GridLayoutBase's bottom-up Auto() determination, which turned out to be fragile -- a `nothing`-deferring GridLayout anywhere in a multi-level chain breaks the whole chain's determinability). The anchor row and controls panel each get this as an explicit constant Fixed width, centered under the grid, while the grid's own sizing is untouched and stays free to be maximal when the panel is collapsed. Verified directly: constant panel width across collapse/ expand/resize, zero label clipping (N=3 and N=10 models), grid still maximal on both wide and tall viewports. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d de-duplicate controls-panel code
Three independent fixes found via interactive Julia-MCP testing:
- KDE1D/KDE2D/QuantileKDE2D returned kde()'s StepRangeLen x/y directly,
mismatching their dead-cell sentinels' Vector{Float64} and crashing
ComputePipeline's TypedEdge on a live->dead transition; Hist1D/Hexbin2D
had the same latent Int64/Float64 mismatch. Fixed with collect()/Float64().
- The real cause of the recurring vsel-picker crash/unresponsiveness:
_init_compute_graph's :flat_walkerids_full lambda assigned to a local
named `n`, silently aliasing the same function's `n` (grid size)
parameter shared by other closures (vsel_map, live_map) -- confirmed via
a minimal Julia repro. Every live-sampling flush tick overwrote the grid
size with the current sample count, so every later vsel checkbox click
failed vsel_map's own assertion. Renamed the shadowing local.
- _init_gridlayout converted from a plain Observables.lift (which did
reentrant graph[symbol][] reads inside its own callback) to a proper
register_computation! node with an exhaustive static input list, matching
every other node in this graph; get_stats_plotspecs/get_trace_plotspecs
now take the already-resolved inputs NamedTuple instead of the graph.
- De-duplicated the controls panel's repeated width/halign-setting via a
small _fix_panel_size! helper.
Verified via Julia MCP: full live-sampling runs in both the CairoMakie and
real GLMakie dev environments, repeated vsel/recipe/overlay toggling, and
the static bat_makie_plot path, with no crashes.
…hes, a silent filter=true no-op, quantile-level crashes, and degenerate-domain corruption All found via a systematic audit of the whole Makie extension (source review plus live Julia MCP testing), fixed and verified the same day: - Any zero-weight dataset crashed unconditionally regardless of recipe choice: Cov2D/Mean1D/Std1D/Mean2D/Std2D always compute in the background, and Cov2D's eigen() on the resulting NaN covariance threw. Added isfinite() guards to all five. - filter=true silently disabled Mean1D/Std1D/Mean2D/Cov2D/Std2D entirely -- they had no live+filtered dispatch method, so Julia fell through to the empty dead-cell sentinel. Added the missing methods, applying _low_weight_mask directly (mirroring how the histogram family already handles this). - QuantileKDE1D/QuantileKDE2D (and the identical pattern in QuantileHist1D/2D) crashed whenever exactly 0 or 1 quantile levels survived filtering (range(...,length=1) throws). New shared _quantile_palette_positions() helper special-cases that case. - PDF1D crashed under all-zero weights (Normal(mu, NaN) rejects a NaN sigma). - Forcing ChainScatter2D as a recipe on non-chain-carrying samples (bypassing the UI's own has_chain_info gating) crashed with an opaque "invalid index: nothing" -- now raises a clear ArgumentError. - A degenerate (zero-width) domain corrupted data in two independent ways: the histogram-edge fallback created a zero-width bin that silently dropped every sample, and StatsBase.normalize(:pdf) then divided 0/0 into NaN; separately, the shared diagonal axis-limit margin computed to exactly zero, rendering that panel blank with no error. Fixed the edge padding, guarded normalize against zero-weight histograms, filtered non-finite values out of the shared y-limit computation, and gave the axis-limit margin an absolute fallback when the span is zero. Verified via Julia MCP: unit-level dispatch tests for each fix, plus end-to-end bat_sample/bat_makie_plot repros of the original crash scenarios, plus a regression pass confirming the live path and prior closure-aliasing fix are unaffected.
…recipe cosmetics Replaces the single-cutoff "Current Index" slider with an IntervalSlider so a start and end index can both be chosen, and fixes a resulting bug where every incremental accumulator (Hist/QuantileHist/Mean/Std/Cov) went stale on a pure window shift, since their only staleness check was sample count, which a same-width pan never changes -- now also keyed on window_start. Also: unifies the 3-color credible-region palette across all four quantile recipes, rewrites QuantileKDE2D from Contourf to an explicit color-grid Heatmap (Contourf was blending adjacent band colors), hides QuantileKDE1D's outline by default, drops the Mean2D crosshair from the 2D stats overlay, makes the 1D mean line dashed and every stats-overlay line black, and reduces (but does not fully eliminate, a Makie-internal limitation) a linestyle deprecation warning under repeated overlay toggling. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…anges, and flag the Quantile* naming Lower-triangle cells now follow the standard pair-plot convention (cell at (row, col): x = the column's variable, y = the row's), so every column shares one x-range straight down through its diagonal -- previously the lower triangle showed x = the row's variable, mirroring the upper cells instead of transposing them, so column x-axes never lined up. Both mirrored cells still share a single computed primitive: a new transposed::Bool=false keyword on every 2D recipe's compose_plotspecs (and on get_stats_plotspecs/ get_trace_plotspecs, which forward it so overlays can't end up crossed against the main recipe) swaps x/y purely at compose time for lower cells, together with their axis limits and labels. Heatmap-based recipes use permutedims, not lazy transpose -- LinearAlgebra's transpose is recursive and has no method for the RGBA cells of the two color-grid recipes. Keywords don't participate in dispatch, so a future 2D recipe missing the kwarg fails loudly at precompile time instead of silently mis-orienting (noted at the compose_plotspecs stub). The live path's vsel picker actually works now: the listener used to re-apply vis.backend.vsel on every 0.1 s poll tick, on the assumption that a future UI widget would communicate changes by mutating that field -- but the picker that actually got built writes graph[:idxs] directly, so every live checkbox change was silently reverted within one tick (confirmed via deterministic repro), and models with fewer free parameters than the default vsel got _clamp_vsel's warning ~10x per second. The apply is now gated to run exactly once, latching only on success -- preserving the initial-activation timing and its retry-on-error behavior bit-for-bit -- and the picker is the sole runtime writer of the selection afterwards. Also adds a comment block over the Quantile* recipe types flagging, for future reconsideration, that they draw smallest-interval/HPD credible regions (via BAT.get_smallest_intervals / density thresholding) rather than what "quantile" conventionally denotes (central credible intervals from distribution quantiles) -- names kept as-is for now, per explicit decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SNa9qM6xRA2uvnRWCZWjJ
… bands bridging valleys, an exception-unsafe flush lock, missing input validation, and zero-sample crashes - KDE2D masked cells below an absolute density of 0.005 -- density units are inverse data units, so any parameter whose scale pushes the peak density below that rendered a completely blank panel (confirmed at sigma ~ 1e4: every cell NaN, silently). The cutoff is now a named fraction of the peak density, scale-invariant by construction, applied in a single pass instead of fill + mask + masked-assign. - QuantileKDE1D built one polygon per credible level across the whole HPD mask, so a multimodal marginal's bands were drawn straight across the below-threshold valley between modes, filling regions explicitly outside the credible region. Each contiguous mask run now gets its own polygon (new _mask_runs helper), with the level color pushed per run so polys/fill_colors stay index-paired; unimodal output is unchanged bit-for-bit (one run per level). - flush_buffer!'s critical section used a bare lock/unlock pair: anything throwing inside left buffer_lock held forever -- the listener's per-tick catch swallows the exception but cannot release a lock acquired deeper in the call stack, permanently deadlocking every sampling thread. Now lock(buffer_lock) do ..., exception-safe by construction, returning the drained buffer (or nothing when no flush is due) with the graph work un-nested after the critical section -- the same failure mode already documented and try/finally-hardened in update_visualizer_impl!. - _clamp_vsel only rejected indices above n_dof; 0/negative ones fell through to an uncontextualized BoundsError deep inside a compute-graph closure. And _domain_from_samples (the static path's domain estimate) took raw minimum/maximum with no finiteness filter, so a single Inf/NaN sample value in a completed run poisoned the domain -- re-creating on the static path exactly the failure class the live path's domain recompute was hardened against. Both validate now, mirroring the live path's filtered-extrema pattern. - Scatter2D, ChainScatter2D and Hexbin2D threw a BoundsError on a live cell with zero samples: the placeholder marginal view is 0x0 (not 2x0), so indexing row 1 failed before the first batch flushed. They now degrade to the empty sentinel, as the KDE recipes always did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SNa9qM6xRA2uvnRWCZWjJ
…ul, gate Trace2D's full-data nodes, and weight the KDE bandwidth - flush_buffer! now issues ONE keyword-batched update! instead of three sequential calls (samples, then current_idxs, then domain): ComputePipeline eagerly resolves observable-attached nodes (:gridlayout, via SpecApi) inside every update! call, so each flush paid three full grid resolves -- the first of them with the new samples but STALE current_idxs, pure waste. Measured: exactly 1 resolve per flush now. _recompute_domain! became the pure _domain_including so its result can join the same batch; unchanged inputs (usually the domain) are dropped by ComputePipeline's own input-side isequal check, so no manual did-it-change comparison is needed. register_state_for_vis! batches its two calls the same way. - The shared const _EMPTY_*_PRIMITIVES sentinels are now fresh-value _empty_*_primitives() functions: ComputePipeline treats a callback returning the identical mutable object again as CHANGED (it cannot rule out in-place mutation) but a fresh isequal-equal value as UNCHANGED, so the consts made every dead/non-selected recipe re-dirty its downstream on every sample batch -- the exact opposite of their stated intent, and a blocker for any future changed-bits-driven partial grid rebuild. Verified: a probe node on a dead recipe's primitive no longer re-runs across flushes (previously once per flush). - The Trace2D-only _full node family (an untruncated full-dataset copy plus four per-sample extraction maps and per-pair marginal views) recomputed on every flush even with both trace toggles off (the default), because ComputePipeline resolves a node's inputs before its callback can early-return. :flat_samples_full now takes the toggles as inputs and builds typed empty (1:0) views while both are off -- same concrete view/vcat types either way, so the TypedEdge-locked output type is stable across off->on and every downstream node needed zero changes (empty in, empty out, and consecutive off-state empties are isequal so their callbacks stop running entirely). The toggles' add_input! calls moved above this registration, which requires its inputs to exist. - KernelDensity.jl's bandwidth selection ignores weights entirely (its default_bandwidth has no weights-aware method), so all four KDE recipes rendered weighted (e.g. Metropolis repeat-count) samples with a bandwidth computed from the raw stored rows -- confirmed pathological for concentrated weights (a weighted point mass rendered with the full unweighted-spread bandwidth). New _weighted_kde_bandwidth mirrors KernelDensity's robust Silverman rule (same alpha, min(std, IQR/1.34), zero-width fallbacks) with weighted std/IQR (uncorrected Weights, the extension's existing convention) and Kish effective sample size, applied per dimension in 2D. Uniform weights take a fast path with no explicit bandwidth, keeping the common IID/unit-weight case bit-for-bit identical to KernelDensity's own default; degenerate inputs also fall back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016SNa9qM6xRA2uvnRWCZWjJ
…and clean up idioms across the extension
Three-part simplification pass (net -366 lines):
Incrementalism removed: the per-cell accumulator machinery (the four
_Incremental*State types, _update_hist!/_update_stats!, the
compute_hist/stats_primitives layer, the is_incremental trait, and the
whole n/vsel/domain/wstart staleness protocol) is deleted. Every recipe is
now a plain full recompute over the current sample view -- the flush
already paid O(total samples) data assembly regardless, so the accumulators
only saved the cheap fold while forcing every incremental recipe to compute
even when not selected (Hist2D and QuantileHist2D folded bit-identical
histograms twice per pair cell; Mean2D/Cov2D/Std2D folded the same moments
three times), and their hand-rolled staleness tracking produced two
separate bug classes (the zero-weight crashes; the interval-slider pan
silently showing stale data). Only selected recipes do real work now;
switching recipes costs one O(n) recompute, measured at 1.6-14.6 ms warm.
The cell's fixed domain rides along inside the config so the histogram
recipes keep STABLE domain-derived bin edges (new _hist_edges dispatch
helper, with a data-derived fallback for direct calls); the stats overlays
share a _stats_inputs prologue that applies the low-weight cutoff only when
config.filter is set -- which also deletes the five copy-pasted filter
prologues. Verified against direct value oracles (Hist1D bit-exact vs a
fixed-domain-edge fit; Mean/Std/Cov equal to direct weighted moments).
Disclosed change: filter=true histograms now also use domain-derived edges.
Deduplication: the default recipe configs exist once as
_default_makie_triagonal_config/_default_makie_diagonal_config in
makie_defs.jl (used by the BATMakieVisualization constructor, both static
entry points, and the precompile workload via merge overrides -- whose
magic level literals were just cdf.(Chi(2), 1:2)); the ~35-line static
graph setup exists once as _setup_static_graph (warmup stays at the two
user entry points so the precompile remains backend-free); the themes are
one shared _bat_base_theme plus minimal light/dark overrides via merge
(verified recursive with override-wins). Both themes and all configs are
recursively identical to their pre-dedup versions (checked against the old
definitions evaluated from git).
Idiom cleanup: concrete field/container types (Vector{Int}/Int/Float64
struct fields, typed graph inputs, Int32 chain ids); PlotSpec[]
accumulators; allocation-free sum(length, ...) in the per-step critical
section; ChainScatter2D compose is single-pass grouping instead of
O(nchains x nsamples) masking; Trace2D iterates groups in sorted order
(deterministic z-order) and uses views instead of full-dataset copies;
primitive/marginal node symbols are underscore-separated
("Hist1D_prim_1_2" -- bare digit concatenation was one refactor away from
ambiguity); dead recipe_symbol deleted; @Assert on user-influencable state
replaced with real ArgumentErrors; the Cov2D unit circle is a const;
colsize! uses the existing _UI_COL2_MENU_WIDTH constant; the misleading
picker_col default is gone; live_map[j,i] is the single documented
indexing convention; all four registration callbacks read their inputs by
name instead of order-fragile positional unpacks; lambda-shadowing fixes;
concrete RGBAf quantile colors; single-pass NaN masking in _hist2d_output;
a clear error for unregistered chain ids; stale cross-file comment
references fixed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016SNa9qM6xRA2uvnRWCZWjJ
bat_sample deepcopies its BATContext for result provenance (and MCMCChainPoolInit deepcopies a dummy context; ~10 similar sites across the algotypes). Once a Makie-backed visualizer has completed a run, its content holds a compute graph whose Makie observable listeners reach backend Modules -- and Base.deepcopy refuses to copy Modules, so ANY later bat_sample with a context whose visualizer had already run crashed with the obscure "deepcopy of Modules not supported". Fresh visualizers deepcopied fine, which is why first runs always worked. A Base.deepcopy_internal method now copies the backend CONFIG but drops the content (nothing): content is the live handle of one specific run (locks, listener task, figure) and has no meaning in a provenance copy. The copy keeps the same concrete type -- Base's generic struct deepcopy type-asserts that; returning the no-op visualizer type fails the assert. The Makie backend's init_visualizer! (separate commit) rejects such a stripped copy -- and any other reuse of an already-used visualizer -- with a clear single-use error, since reuse was broken beyond the deepcopy anyway (a second run would append onto the old graph and deadlock on the latched is_live teardown flag). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UKCPaT3azdYDha8LMh6Fy5
…tion, and fix a round of bugs and UI polish
Step-window slider redesign (multi-walker/multi-chain capable):
- New :window_steps graph input (a (lo, hi) step-time window; sentinel
(1, typemax(Int)) = everything, fast-pathed so untouched-slider live
flushes pay no search). The slider writes ONLY this; :current_idxs is
back to pure rows-exist bookkeeping. :window_start is gone.
- _step_window_rows selects each walker's contiguous row range whose
DWELL (stepno : stepno + weight - 1) intersects the window, via two
binary searches; boundary rows keep full weight. Samplers without
stepno info (IID/Sobol/AHMC) fall back to row index == step,
reproducing the old semantics exactly.
- New :max_step node drives the slider range; the slider is 0-based
since stepno starts at 0 (a 1-based window silently dropped every
walker's initial sample).
- The static path registers samples per (chainid, walkerid) instead of
as one merged pseudo-walker, removing the chain-block-concatenation
root cause of Trace2D's historical frozen-chains bug -- which lets
the entire _full untruncated node family (flat_samples_full, its four
per-sample maps, the marg_full views, the toggle gating, and
Trace2D's reveal-fraction logic) be deleted outright. Trace2D now
runs on the ordinary windowed inputs, time-aligned with the window
end.
- The slider now appears for every completed run (any chains x
walkers): built via an idempotent add_index_slider! closure returned
from _build_fig, retrofitted at live teardown under the right theme.
Label "Index Range" -> "Step Range".
KDE boundary correction (all four KDE recipes):
- Densities of hard-bounded parameters no longer leak mass past the
bounds: samples within 4 bandwidths of a finite support bound are
mirrored across it (plus corner mirrors in 2D), then the grid is
truncated to the support and renormalized. Without correction a
Uniform marginal rendered at ~half its true density at the edges.
Unbounded/unknown dimensions are bit-identical to before.
- Bounds ride through new :support_lo/:support_hi graph inputs into the
recipe configs (same mechanism as the histogram domain). Live runs
derive them from the prior automatically (init_visualizer! gained a
target kwarg; bat_sample passes the original, untransformed measure);
the static path accepts a new support kwarg (a measure/prior, or
explicit per-dimension (lo, hi) pairs) on bat_makie_plot/Makie.plot.
The extraction walks NamedTupleDist components with a conservative
+-Inf fallback, so exotic priors degrade to the old rendering, never
to a wrong correction.
- Fixes a live-path domain bug found along the way: the prior-based
initial axis domain was estimated from the PRETRANSFORMED mcmc target
(standard-normal space under the default PriorToNormal pretransform)
while the displayed samples are in original space -- e.g. a prior at
100 +- 5 got a domain floor near -3. It is now estimated from the
original measure.
Bug fixes:
- trace_nsteps is validated >= 1 at config construction (a 0/negative
value silently emptied the trace overlay forever).
- Trace2D chain colors are ranked over all chain ids in the full input,
so a chain with zero revealed samples no longer shifts every other
chain's color; recency now reaches exactly 1.0 for the newest point.
- warmup_makie_shaders latches after one run and skips cleanly when no
backend is loaded (backend-less embedding no longer crashes).
- Zero-sample Scatter2D compose guard; live wend clamped to walker
length; PDF1D and Errorbars1D/2D rewritten on the shared stats-input
helper with isfinite guards, and Errorbars wired into the recipe
dropdowns.
- _get_bin_centers uses typed comprehensions -- a degenerate (<= 1
edge) histogram now yields an empty Vector{Float64} instead of
Vector{Any}, which would have violated the graph's TypedEdge contract
if it ever reached a live cell.
UI polish:
- Checkboxes are themed in both themes (dark mode no longer renders the
vsel picker as stark white squares); picker filler boxes derive from
the panel background ladder.
- Diagonal y-ticks use Makie's adaptive default instead of a hardcoded
"{:.1f}" format (illegible at extreme scales).
- The trace-overlay column only appears when samples actually carry
chain and step information; the picker moves left when it is absent.
- Default figure size scales with the active grid dimension (capped),
instead of cramming any N into 665x850.
- Picker label "PDF" -> "Normal fit" (it draws a Gaussian fit, not the
sample density).
The precompile workload covers the new paths: the step-range slider,
the dark theme, and the boundary-corrected KDE configs (a set support
changes the config NamedTuple type, a separate compiled
specialization).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UKCPaT3azdYDha8LMh6Fy5
…tyle with BAT.jl - Per-cell diagonal y-limits, theme-aware stats-overlay and status colors - Menus constructed bottom-first with direction=:up so open dropdowns consume clicks - Precompile workload drives the real entry points incl. a live visualized run; type-erased PickerInfo and @nospecialize'd graph callbacks remove first-use JIT - Comments cut to essentials, 4-space indent, spaced signature defaults, docstrings for the public API (listed in the docs) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UKCPaT3azdYDha8LMh6Fy5
BJMCox
force-pushed
the
makie_recipies
branch
from
September 7, 2026 08:02
41acc14 to
764f993
Compare
Member
|
Rebased onto current main - am reviewing now |
BJMCox
reviewed
Sep 7, 2026
Member
There was a problem hiding this comment.
I found several correctness and lifecycle issues that should be addressed before merge. Two existing Copilot comments also still apply: the unshaped getstring error path uses undefined names, and this overhaul adds no tests. To be honest the test one is probably not super useful, but the other one deserves addressing I think.
| ) | ||
| adsel = _to_adsel(s.ad) | ||
| set_batcontext(BATContext{s.precision}(s.rng, s.cunit, adsel)) | ||
| set_batcontext(BATContext{s.precision}(s.rng, s.cunit, adsel, c.visualizer)) # Temporary, to fix |
Member
There was a problem hiding this comment.
The keyword merge accepts visualizer, but this call always uses c.visualizer. Thus set_batcontext(visualizer=requested) silently ignores the request. Minimal case:
old = get_batcontext()
requested = BATVisualizer(MyBackend(), nothing)
set_batcontext(visualizer=requested)
@assert get_batcontext().visualizer === requested
set_batcontext(old)Could this use the merged visualizer value, like the other context fields?
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
struct MWEBackend <: BAT.BATVisBackend end
function main()
old_context = BAT.get_batcontext()
requested = BAT.BATVisualizer(MWEBackend(), nothing)
try
BAT.set_batcontext(visualizer=requested)
observed = BAT.get_batcontext().visualizer
observed === requested || error(
"set_batcontext ignored visualizer: requested $(typeof(requested)), observed $(typeof(observed))"
)
finally
BAT.set_batcontext(old_context)
end
end
main()|
|
||
| function BATContext{T}( | ||
| rng::RNG, cunit::CU, ad::AD | ||
| rng::RNG, cunit::CU, ad::AD, visualizer::BATVisualizer |
Member
There was a problem hiding this comment.
Adding visualizer removes the documented three-argument BATContext{T}(rng, cunit, ad) constructor. Existing user code now gets a MethodError. This needs to be mentioned or have a three arg form.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
context = BATContext()
# This constructor remains documented in BATContext's public docstring.
BATContext{Float64}(context.rng, context.cunit, context.ad)| Constructors: | ||
|
|
||
| * ```BATVisualizer()```: no visualization | ||
| * ```BATVisualizer(backend::BATVisBackend)```: e.g. `BATVisualizer(BATMakieVisualization())` |
Member
There was a problem hiding this comment.
This public docstring promises BATVisualizer(backend), but only the zero-argument and two-field constructors exist. BATVisualizer(MyBackend()) gives a MethodError. Please either add the documented forwarding constructor or remove the documented API.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
struct MWEBackend <: BAT.BATVisBackend end
# This constructor is documented by BATVisualizer's public docstring.
BATVisualizer(MWEBackend())| vsel, | ||
| N_max, | ||
| n_batch, | ||
| max_buffered, |
Member
There was a problem hiding this comment.
max_buffered may be smaller than the fixed n_batch=50. With max_buffered=1, the producer waits after one buffered row while the listener waits for 50 rows before flushing. Neither can advance. Please validate this relation or derive a reachable flush threshold.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
config = BATMakieVisualization(max_buffered=1)
flush_at = config.n_batch
block_at = ceil(Int, config.n_batch * (config.max_buffered / config.n_batch))
block_at >= flush_at || error(
"producer blocks at $block_at buffered row, but the listener flushes at $flush_at rows"
)| get_samples!(output_buffer[output_id], chain_state, nonzero_weights) | ||
| n_smpls_end = sum(length, output_buffer[output_id]) | ||
| n_new = n_smpls_end - n_smpls_start | ||
| n_buffer_samples[] += n_new |
Member
There was a problem hiding this comment.
This counter measures new rows, not new MCMC steps. checked_push! represents a rejection by increasing the last sample weight without adding a row, so a long rejection streak never triggers a live update. Minimal invariant:
checked_push!(buffer, buffer[end])
@assert buffer.weight[end] == 2
@assert length(buffer) == old_lengthPlease count consumed steps or weight mass instead of row growth.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
buffer = DensitySampleVector([[0.0]], [0.0]; weight=[1])
n_rows_before = length(buffer)
BAT.checked_push!(buffer, buffer[1])
n_new = length(buffer) - n_rows_before
buffer.weight[1] == 2 || error("the MCMC dwell weight did not increase")
n_new > 0 || error("the live buffer counter sees no update when only the dwell weight changes")|
|
||
| @debug "Merge samples of chains and transform to original space." | ||
|
|
||
| if !isnothing(context.visualizer.content) |
Member
There was a problem hiding this comment.
Listener shutdown only runs after successful burn-in and sampling. Any callback or sampler error jumps past this block and leaves the polling task live. A try/finally around the sampling lifecycle would preserve cleanup on both success and failure. A GLMakie MWE with a throwing callback confirms the task remains alive.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
using Distributions
using GLMakie
visualizer = BATVisualizer(BATMakieVisualization(max_buffered=10^6))
algorithm = TransformedMCMC(
nchains=2,
nsteps=10,
store_burnin=true,
callback=(args...) -> error("stop sampling"),
)
try
bat_sample(Normal(), algorithm, BATContext(visualizer=visualizer))
catch
end
sleep(0.2)
leaked = visualizer.content.is_live[] && !istaskdone(visualizer.content.listener_task[])
visualizer.content.is_live[] = false
wait(visualizer.content.listener_task[])
leaked && error("a sampling error left the Makie listener alive")| function _estimate_prior_domain(target, n_dof::Integer; n_prior_samples::Integer = 2000, tail_prob::Real = 0.0015) | ||
| initsrc = BAT.get_initsrc_from_target(target) | ||
| shape = varshape(initsrc) | ||
| draws = [ValueShapes.unshaped(rand(initsrc), shape) for _ in 1:n_prior_samples] |
Member
There was a problem hiding this comment.
These 2,000 draws use the process-global RNG. Enabling visualization therefore changes unrelated random streams and ignores the BATContext RNG. Please pass the context RNG through this path. Minimal check: seed the global RNG, call this function, then compare the next draw with the no-visualizer sequence.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
using Distributions
using Makie
using Random
extension_module = Base.get_extension(BAT, :BATMakieExt)
Random.seed!(1)
expected = rand()
Random.seed!(1)
extension_module._estimate_prior_domain(
BAT.BATDistMeasure(Normal()),
1;
n_prior_samples=2,
)
observed = rand()
observed == expected || error("domain estimation consumed process-global RNG state")|
|
||
| include("./makie_impl/makie_plotting.jl") | ||
|
|
||
| Makie.set_theme!(bat_theme()) |
Member
There was a problem hiding this comment.
Loading a package extension should not replace the application's global Makie theme. This changes unrelated plots merely because BAT and Makie were imported. The plotting entry points already use with_theme, so please keep BAT's theme scoped there.
Member
There was a problem hiding this comment.
Standalone MWE:
using Makie
Makie.set_theme!(fontsize=11)
using BAT
Makie.current_default_theme()[:fontsize][] == 11 || error(
"loading BAT's Makie extension replaced the process-global Makie theme"
)| return graph, n_dof | ||
| end | ||
|
|
||
| function Makie.convert_arguments( |
Member
There was a problem hiding this comment.
Makie only forwards conversion keywords declared by Makie.used_attributes. Without that method, calls such as plot(samples; vsel=[4], N_max=1) cannot route these keywords into convert_arguments. Please declare the consumed attributes and add a normal plot entry-point test.
Member
There was a problem hiding this comment.
Standalone MWE:
using BAT
using Distributions
using LinearAlgebra
using Makie
samples = bat_sample(
MvNormal(zeros(4), I),
IIDSampling(nsamples=20),
).result
plot(samples; vsel=[4], N_max=1)|
|
||
| return merged_outputs | ||
| end | ||
| export _append_chain_outputs |
Member
There was a problem hiding this comment.
Why does the Makie extension require this internal merge helper to become public? using BAT now imports _append_chain_outputs, despite the underscore marking it private. Please keep it unexported and import it explicitly from the extension.
Member
There was a problem hiding this comment.
Standalone MWE:
module ImportProbe
using BAT
isdefined(@__MODULE__, :_append_chain_outputs) && error(
"using BAT imports the underscore-private helper _append_chain_outputs"
)
end
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
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.
[Experimental][WIP] Add an extension for Makie.jl plotting
This is based on PR #477 using @philippeller 's custom Makie plotting but with dedicated BAT.jl infrastructure for handling the marginalization of samples.
This implements an extension module with reactive Makie.jl recipies that offer more flexible visualization and enable live plotting of MCMC data during sampling.