feat(calib_dash): interactive dashboard for RT calibration fitting - #82
Open
jspaezp wants to merge 46 commits into
Open
feat(calib_dash): interactive dashboard for RT calibration fitting#82jspaezp wants to merge 46 commits into
jspaezp wants to merge 46 commits into
Conversation
FitEvent::PathFound previously paired a path that could include Pass 2's greedily-attached prefix/suffix nodes with a weight that only ever covered the DP's own chain, so a consumer had no way to tell which nodes the DP actually chose versus which were attached afterwards by the monotonic greedy extension. Rename total_weight to dp_weight and add dp_range, the index span within path that the DP itself selected; also add the real coordinate-based monotonicity check that the earlier DP test only claimed to make (its `j < i` assertion held by construction and could never fail).
… fit buffers Capacity equality alone can't distinguish a reused allocation from a same-sized reallocation, which is exactly what the pre-fix collect()-based code would produce against this test's fixed-survivor-count, identical-input loop. Add filtered_ptr/path_points_ptr test accessors and assert on them. Also fold find_optimal_path's four scratch/output parameters into a PathfindingScratch struct, dropping it from 8 arguments to 5 and removing the clippy too_many_arguments allow.
…t Vec::capacity() Vec::with_capacity only guarantees capacity() >= n and may over-allocate, so record() gated its safety bound on the wrong thing; store retained explicitly instead. Also make finish() verify the reserved slot actually holds last_chunk before promoting it, since a stale non-stride entry promoted on a mismatched last_chunk would corrupt the index's monotonic chunk order; a mismatch now panics loudly in debug builds and is dropped with a tracing::warn! in release. Strengthen the_slab_is_allocated_once to also check index_capacity(), and replace a circular dropped() assertion with a hand-computed value.
… regression The dp/path_indices capacity debug_asserts assumed survivors never exceed bins, but suppress_nonmax keeps every node tied for a row/column max, so weight ties legitimately exceed it — real tied data would abort every debug/test run. Capacity stays a with_capacity(bins) sizing hint, now documented on the fields instead of asserted. Also adds a regression test for the FitStarted suppression-bitset clear: no existing test re-fits at unchanged bins with a different suppression outcome, so the one place recording.rs deviates from a naive port of FitObserver for correctness had no coverage. And seeds an off-diagonal point in the geometry test so a row/col transposition in col_of/row_of would be caught.
…a domains The doc comment's collinearity-dependent algebra didn't actually rule out a direct hop beating a two-hop chain for a general monotone triple; replaced it with the bounding-box argument, which holds unconditionally. Added the missing curve_delta test for domains that never overlap at all (previously only partial overlap was exercised), and hardened weight wording plus a one-point-per-cell debug_assert in curve_from.
Holding a digit key overflowed the count accumulator (panics with overflow checks on, wraps silently in release); a huge count on `[`/`]` also looped once per requested step over a 5-state enum, which could freeze the UI.
…gles and DP pane coverage The half-block heatmap collapsed each cell's two grid rows to one glyph via upper.max(lower), discarding the vertical resolution heatmap_cells computes. Encode occupancy per half instead (▀/▄/█/space), with a documented rule for when both halves want different overlay marks. Overlay visibility was stage-derived only, with no way to toggle a single overlay independently as the brief specifies. App gains show_suppressed/ show_path/show_curve/show_ridge, defaulting to the stage-implied set via sync_overlays_to_stage and independently overridable after. The DP pane had no snapshot coverage. Add a fixture exercising it and assert the selected node's chose/acc_weight/considered all render. Also: shorten the status line (it clipped d:dp-pane at 100 columns), and use Fill instead of Ratio for the five sparkline rows so they split area height evenly.
…aseline across a failed fit The standalone binary passed grid_size straight from a user-supplied calibration.json into CalibrationState::new's .expect, so a syntactically valid but malformed file (grid_size: 0, or too few points to fit a curve) aborted the process instead of reporting a message. Validate before constructing CalibDash. Also: a batch whose fit fails (e.g. suppress_nonmax finds nothing survives) was unconditionally overwriting prev_curve with None, so the next successful batch compared against nothing and reported a NaN delta instead of the last real curve — a discontinuity on the Convergence tab that never happened. Only overwrite the baseline when a curve actually resulted. Also tightens a couple of no-reallocation claims to the codebase's existing pointer-identity convention (current_points/prev_points), and corrects a misleading .expect message that blamed the ever-valid placeholder range rather than a zero bins.
Wires calib_dash into timsseek_cli behind an opt-in `calib-dashboard` feature plus the `TIMSSEEK_CALIB_DASHBOARD=1` / `CALIB_DASH_FRAME_BUDGET_MB` env vars: on_batch is called once per Phase 1 chunk (Flow::Abort breaks the loop, leaving Phase 2 to proceed on whatever calibrants were collected so far), and the real Phase 2 fit is captured into a FitRecording and handed to show_final so the Tolerances tab reflects the actual derived tolerances. A plain build of timsseek_cli never links calib_dash; an ordinary dashboard-enabled run (env var unset) pays only one is_none() check per chunk.
…e, and Tolerances rendering The Convergence and Tolerances tabs, the batch scrubber, and the post-Phase-2 pause were all built and tested but unreachable: handle_key never routed to them and CalibDash had no entry point to open a second pause. Adds h/l (cycle tabs), </> (scrub retained frames via a new CalibDash::sync_scrub + App::active_recording), and s/p/c/w (toggle Fit-tab overlays) to the key map, each bounded to O(1) regardless of a typed count so a huge count cannot spin. Adds CalibDash::present() so the dashboard actually reopens after show_final. Moves ToleranceSummary onto App (App::set_final) so draw_tolerances_tab can finally render the m/z, mobility and RT summary instead of a placeholder. Fixes ui.rs's scaled_u64 mapping NaN samples to 0 (indistinguishable from "converged") by carrying the last finite value forward instead. FitRecording/BitSet gain #[derive(Clone)] to support handing the Fit tab an owned copy of a scrubbed frame's recording.
… ridge width processing.rs called show_final and moved straight to Phase 3, so the Tolerances/Convergence tabs were never actually shown after Phase 2 -- call the newly-added CalibDash::present() right after show_final. calibrate_from_phase1 also measured ridge width via the plain measure_ridge_width(0.1) even when recording into a dashboard FitRecording, so recording.ridge() stayed empty and the Tolerances tab's ridge overlay rendered "NaN over 0 ridge column(s)". Route the measurement through measure_ridge_width_with when a recording is present.
Neither calibrt nor calib_dash was ever built or tested in CI, and --features calib-dashboard was never compiled, so the #[cfg(feature = "calib-dashboard")]-gated parameters on phase1_prescore and calibrate_from_phase1 could rot the first time either signature changes without CI noticing. Guards two tests in the same commit that would break under a release-profile CI leg (debug_assert! compiled out, panic = 'abort' workspace-wide): finish_drops_a_mismatched_stale_entry_without_corrupting_order and catch_panics_converts_a_panic_into_detach both rely on catching a panic that only happens under panic = "unwind".
…ine within width present() called render_pause unconditionally, so a user who pressed q, r, or Ctrl-C at a Phase 1 pause -- explicitly asking to stop seeing the dashboard, or stop the search outright -- would still have it reopen after Phase 2 and block in event::read() for a keypress they already declined to give. Adds Stepper::is_stopped() and guards present() on it, mirroring how on_batch already honors it via should_render. Also fixes the status line regressing past 100 columns (and clipping d:dp) once a wide batch number and a typed count are both present -- the exact bug its terseness was introduced to prevent, just reintroduced by hardcoding one wording instead of budgeting against the actual render width. draw_status_line now drops the h/l/</>/s/p/c/w hints one at a time, in priority order, only as far as area.width requires, so d:dp always survives. Three low-severity follow-ups from the same review: the scrub banner no longer disappears when the Fit tab body is exactly one row tall; n clears a leftover scrub cursor instead of silently reopening the next pause still replaying an old frame; and a stale doc comment in bin/calib_dash.rs now reflects that the scrubber is wired in.
The live per-batch fit runs with ObserveOpts::NONE (dp_nodes is O(n * lookback) per chunk), so on_batch's recording never carries DP decisions. `d` only flipped App::dp_pane's boolean, so pressing it during a live pause opened an empty pane. Add CalibDash::sync_dp, a per-batch (not per-draw) on-demand refit into a separate CalibrationState/FitRecording pair that the DP pane reads while live; scrubbed frames are untouched since refit_frame already observes dp_nodes for them.
The Fit tab's status line advertises many single-letter bindings with no room to explain them. `App` now tracks whether the `?` overlay is open; `handle_key` checks it first so any keypress while it's showing dismisses the overlay (and clears a pending count) instead of also performing that key's usual action.
…the design-review pass
Item 0: grid_to_screen mapped grid row 0 (the lowest observed RT) to
screen row 0, but terminal rows grow downward, so a monotonically
increasing calibration rendered as a descending ridge instead of an
ascending one. heatmap_cells and mark_curve each recomputed the same
row-to-screen mapping independently and had the identical bug. Fixed with
one shared flip_display_row helper that both negates the display-row
index and re-derives the half-block Upper/Lower parity from the flipped
index (flipping reverses which half a row lands in). Added
increasing_ridge_renders_ascending_not_descending as a regression test.
Item 1: frames the heatmap in a Block stating the axes
("observed RT (s) up vs library RT (s) right"), the batch/stage in the
title, a left gutter of y-tick labels, and a centered/clipped x-tick
label row below the block, with tick marks written directly into the
border cells. Ticks use a "nice number" step and degrade (no gutter, no
labels) rather than crowd a small terminal. Kept the hand-painted
half-block buffer rather than ratatui's Chart/Canvas, which cannot
express this density field or symbol set.
Item 2: rebuilds the status line as three Paragraphs over
Length/Min/Length so key hints render as bold key + dark-gray action
instead of "key:action" punctuation, per-tab (Fit gets the full
stage/frame/overlay/dp set; Convergence and Tolerances only get n/r),
the pending count only takes a column when one is set (REVERSED instead
of colored), and "? keys" is pinned to its own fixed-width column so it
survives every overflow stage. Adds the "?" keys overlay itself.
Item 3: reorders the overlay-priority constants so evidence (DP chain,
greedy tail) beats derived fit (curve, ridge) when both share a
half-block cell, and switches to a matplotlib-flavored, single-glyph-
per-mark set (suppressed ., curve -, ridge ~, tail +, DP chain O) so
color is redundant reinforcement only.
Item 4: heat_color now returns DarkGray/Gray instead of Rgb (which most
terminals don't remap on a light background), and every other color in
the file is a named ANSI constant outside the 9-15 (Light*/White) range.
REVERSED is now reserved for mode (selected tab, scrub banner, pending
count) rather than doubling up with a mark's hue.
… the design pass Every snapshot's status line changes regardless of tab; the Fit-tab ones also change body content for the y-axis flip, the new axis frame, and the reordered marker glyphs. Reviewed each by eye — see design-pass-report.md.
Replace the five-overlay composited-priority system (independent suppressed/path/curve/ridge toggles resolved by a MARK_* priority ladder that overwrote a cell's glyph) with a single active mark layer cycled by m/M, and mark a cell by adding Modifier::REVERSED to its existing density glyph instead of overwriting it. This keeps the underlying weight visible under every mark, gives maximum contrast on any terminal theme, and fixes a mark landing on a zero-weight cell (a reversed space reads as a solid block, lying about the data) by substituting a minimum-ink `·` glyph instead. Path-layer nodes (DP chain vs. a greedily attached tail) still replace the glyph with O/X, since a path node is a point estimate rather than a region. Stage (`[`/`]`) and layer (`m`/`M`) are now independent axes: stepping the pipeline stage no longer implies an overlay set, since that cumulative coupling is exactly the composited-priority design being removed here. The active layer's name now lives in the Fit tab's block title, which gained a small width-based degrade so a widened title cannot garble against the DP pane's narrower block.
The active mark layer used to share heatmap_title_right's top-right title
with batch/stage and was the first of the three dropped under a narrow DP
pane -- backwards, since a reader who doesn't know which layer is active
can't interpret a single inverted mark on this tab at all, and the layer
has no other place on screen naming it.
Gives the layer a dedicated, bold subtitle row ("Showing: ridge -- tolerance
band") directly below the block's title, spelled out with a one-phrase
gloss of what its marks mean rather than abbreviated to a key hint, since
Item 4's move to REVERSED-only marking left no glyph-coded legend. Costs one
row of heatmap canvas whenever there's a spare one to give; elides its own
text (gloss first, then the label itself) rather than wrapping at any
width, and is only skipped when there is no row to spare at all.
`phase1_prescore` and `calibrate_from_phase1` each took a cfg'd parameter, so both had two arities and every call site had to be written twice — once per cfg arm, with only one of the two arms compiled by a default build. All dashboard interaction now goes through a `calib_dash_hook` module with a real arm and a no-op arm exposing the same items, so the pipeline calls them unconditionally and the only feature cfgs left in processing.rs are the two on the module itself. `ObserveOpts` comes from `calib_dash` now, which drops timsseek_cli's direct `calibrt` dependency — its only use was spelling `ObserveOpts::NONE`.
…hable helpers Every module was `pub mod`, which made every `pub fn` public API and turned dead-code analysis off for the whole crate. Demoting the modules to `pub(crate)` and trimming the re-export list to the six items the two external consumers actually use turns that analysis back on. Removed: - `Stage` and its `[`/`]` bindings. It had no reader left but two label renders; `Layer` took over the overlay job. The status line and the heatmap block title now show the batch alone, and the `Layer` subtitle already says what the user is looking at. - `App::set_tab`/`set_layer`/`set_stage`. Every caller was a test; the tests now drive `handle_key`, which is what a user does. - `FrameIndex.range`, `FrameStore::is_empty`/`last`, `FitRecording::dp_weight`/`reset` (the fit paths reconfigure through `FitStarted` instead), and `Dims.disp_cols`, which always equalled `w`. - `bitset.rs`. A hand-rolled bitset to save 9 KB in a dev tool; `Vec<bool>` covers it. - The `#[cfg(test)]` capacity accessors and the tests that pinned `Vec::capacity()` after a fill. Those asserted std's documented behavior, not this crate's. The allocate-once guarantee they were standing in for is now pinned by pointer identity on the slab, through `frame()`'s own slice, with no test-only accessor on the production type.
`CalibDash::on_batch` never read the chunk's speclib range — the frame store indexes by chunk number and derives its own grid ranges from the points. Removing the parameter also removes it from both arms of `calib_dash_hook`. Also drops calib_dash from the workspace default-members (a bare `cargo build` should not compile a dev-only TUI; CI names it with -p), and strengthens the calib-dashboard CI leg to build test targets rather than just check the lib and bin.
…lace Three re-fit paths (live batch, scrubbed frame replay, on-demand DP fit) ran the same reconfigure/update/fit/measure sequence. They now go through `fit_points`, which derives the grid ranges with `point_ranges` itself so no caller can replay a frame against a fold the live fit never used. Only the live path needs to tell the skip reasons apart, so that stays at its call site as a match on `RefitSkipped`. `heatmap_cells` re-derived the grid-to-screen y-flip (and the half-block parity that has to come from the flipped index) by hand instead of calling `flip_display_row`; that flip having two implementations is what shipped the transposed heatmap once already.
The Fit tab's mark layers invert the cell rather than replacing its glyph, so a `.symbol()`-only snapshot could not tell a layer marking the right cells from one marking none. Snapshots now carry a trailing section listing the inverted column spans per terminal row. `fit_tab_renders_a_diagonal_ridge` was byte-identical to `fit_layer_None` (`Layer::None` is the default layer) and is dropped in favor of the layer cycle's own frame.
`CALIB_DASH_FRAME_BUDGET_MB=64M` silently fell back to the default; a set but unparseable (or zero) value now warns and names the value. The two default budgets it fell back to were spelled as literals at their call sites and disagreed by 64x for reasons neither one stated: `frames.rs` now owns both, as `DEFAULT_RUN_BUDGET_BYTES` (a whole run's batches, decimated to fit) and `REPLAY_BUDGET_BYTES` (one saved batch, nothing to decimate). Also: `tempfile` in place of hand-rolled pid+nanos paths that leaked on a failing test, and `ratatui` moved to `[workspace.dependencies]` alongside every other shared pin.
`FitStarted::n_points` and `Suppressed::n_kept` had no readers outside the crate's own test recorder, and each cost a full bins-squared scan on every fit. `GridGeom::lookback` had none at all. The dashboard already destructured all three away.
…e table The two cells had grown to five sentences each; the detail now lives in a subsection under the table.
`find_optimal_path` allocated the DP chain and both greedy tails on every call while its scratch struct claimed nothing there allocates. They now come from `CalibrationState` like the other buffers. `calibrate_with_ranges` was a hand-rolled second copy of the fit sequence, which meant a second place to keep the scratch construction in sync; it now drives a `CalibrationState` and keeps only the heatmap and WRMSE reporting that make it a distinct entry point.
…configure `reconfigure` re-derived and re-checked the x/y spans, and also rejected `bins == 0` on a path where `bins` has already been compared against a grid-held `bins >= 1`.
Capacity equality cannot tell buffer reuse from a same-sized reallocation, so the pointer-identity assertions next to it were carrying those tests alone; the reconfigure test now makes its no-reallocation claim the same way. Also drops a DP assertion the loop bounds already guarantee and the duplicated dp_range arithmetic that pathfinding.rs owns.
Branch-prediction and const-generic reasoning on `dp_nodes`, a borrow-checker tutorial next to one of two identical `mem::take`s, and clone archaeology all went. The "weight ties can push survivors past `bins`" caveat was written out four times; it now lives in the debug assert that enforces it, with short pointers from the rest.
The status line's hint table and the `?` overlay's rows were two independent lists of the same bindings, free to drift apart. Both now read one `Binding` table carrying a terse `hint` and a full `help` label. `App::handle_key` stays a `match`: its arms differ in what they do, not only in which key they answer to, so driving them from the table would cost more machinery than the drift it removes.
… impossible `FitRecording::col_of`/`row_of` and `ui::bin_of` were three copies of the same value-to-bin arithmetic. It now lives once in `recording::bin_index`; `bin_of` keeps its zero-bins/non-finite/zero-span screening as a wrapper, which the recording's own placement does not need. The zero-bins guards in `draw_heatmap`, `mark_path`, `mark_curve` and `mark_ridge` are unreachable: `draw_fit_tab` and `paint_heatmap` already return early, and none of those four divides by `bins`. The guard in `mark_grid_indices` stays — it is the one that sits next to a `/` and `%`. `node_glyph_and_style`'s catch-all only ever saw the two node kinds, so it was suppressing the non-exhaustiveness error a new `Mark` variant should cause. `draw` and `heatmap_cells` are narrowed to their real visibility.
The `contains` assertions on frames that also carry an insta snapshot made a cosmetic change fail twice for the same reason. Two are kept because they say something the snapshot cannot: the scrub banner's 1-based frame count, and an equal-bounds RT tolerance collapsing to a single ±-prefixed number. `a_huge_count_before_l_does_not_spin` and its `m` twin exercise one mechanism (a count reduced mod the number of stops before any loop runs) and are merged. The `>` case stays separate: saturating arithmetic against the retained-frame count, not modular reduction.
The popup height was the literal 16, two short of the 18 the key table now needs, so the two bindings under "Convergence / Tolerances" were clipped and the heading rendered over nothing. Deriving the height from the rows keeps adding a binding from silently truncating the last group. The overlay had no test at all, which is why this survived a refactor of the renderer. It now has a snapshot plus an assertion that every entry in the binding tables reaches the screen.
Four places where the dashboard drew a shape but withheld the number behind it: - Convergence sparklines are each normalized to their own maximum, so every one fills its pane top to bottom regardless of the values. A wrmse that fell 5.0 -> 0.08 drew the same descent to the same floor as one that fell 5.0 -> 4.9. Each series is now titled with its peak (the value at the top of the plot) and its latest sample. - The DP pane rendered `chose` as a present/absent `->` marker, spending the columns and discarding the edge. It now prints the predecessor index, or `root` for a node the DP reached without one. - `ToleranceSummary::rt_seconds` was a `(f64, f64)` holding the same magnitude twice, under a type shared with the genuinely asymmetric `mz_ppm` and `mobility_pct`. It is a half-width now, so the two conventions are distinguishable and there is no second element for a renderer to drop. The m/z and mobility windows print as `-8.5 .. +9.5` rather than `(-8.5, 9.5)`, which read as a range of measured values. - The Tolerances tab wrapped with `trim: true`, stripping the indent that marks the ridge line as qualifying the one above it. Also pins that every screen draws at sizes down to 1x1. `panic = "abort"` in the release profile means `catch_panics` cannot contain a render panic there, so a user shrinking their terminal mid-pause would kill the search.
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.
Adds
calib_dash, a ratatui dashboard for inspecting RT calibration as it isfitted. It attaches to a real search under the opt-in
calib-dashboardfeatureand
TIMSSEEK_CALIB_DASHBOARD, pausing between Phase 1 batches to show theaccumulated grid, the DP path through it, and the resulting curve — plus a
post-Phase-2 pause for the derived tolerances. There is also a standalone
calib_dashbinary that replays a recording without a search.Calibration failures until now have only been visible as a bad number at the
end of a run, with no way to see which stage produced it. The dashboard makes
the intermediate state observable: whether the ridge is there at all, whether
suppression kept it, whether the DP followed it, and how the curve moved
between batches.
Cost when the feature is off
Nothing. The observer is a generic parameter with a
()implementation thatmonomorphizes away, and
timsseek_clireaches the dashboard through a two-armmod calib_dash_hookwhose no-op side has the same arity — no#[cfg]at thecall sites and no behavioural difference in the search path.
calib_dashisout of
default-members, so a plaincargo builddoes not compile ratatui.Changes outside the new crate
calibrtgrew aFitObserverhook so the fit can be recorded, and lost thetwo
bins²scans that had been computing deadFitEventpayload fields onthe hot path.
timsseek_cli's Phase 1 now returns the dashboard handle itcreated rather than taking one, which removes a
--calib-libpath where thedashboard could be attached to a different calibrant set than the one being
scored.
Notes for review
suppress_nonmaxseeds its row/column maxima at1.0, so a grid whose weightsare all below 1.0 suppresses everything and returns
NoPoints. That predatesthis branch and is left alone, but two tests here deliberately depend on it and
would need updating alongside any fix.
Conflicts with #81 on root
Cargo.tomlanddocs/development.md— mechanical,whichever merges second.