From 6b3577a09e6cdb9e03b0a0b26f038919af01be09 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 16:57:06 -0700 Subject: [PATCH 1/3] fold memo owns its agg and covers; grid_body's sharing is guarded (pntu, 0jw5) improvise-pntu: FoldMemo was keyed by (key, span) while the aggregation and covers were call arguments, so one memo across two covers answered the first (5.0 where 2.0 was right). FoldMemo::new(agg, covers) now owns both and the _in functions read them off it; GridLayout::fold_memo builds the grid's. The mismatched call no longer compiles. improvise-0jw5: a test in improvise-command counts fold evaluations across one grid_body call (18) against a fresh-memo control (90). A per-cell or per-total fresh memo fails it. The counter is exposed through a new test-support feature on improvise-core, enabled only as a dev-dependency. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/core.md | 11 +++ context/test-evidence/command.md | 34 ++++++++++ context/test-evidence/core.md | 35 ++++++++++ crates/improvise-command/Cargo.toml | 1 + crates/improvise-command/src/ui/viewport.rs | 75 ++++++++++++++++++++- crates/improvise-core/Cargo.toml | 5 ++ crates/improvise-core/src/model/types.rs | 71 ++++++++++--------- crates/improvise-core/src/view/layout.rs | 64 ++++++++++++++++-- 8 files changed, 258 insertions(+), 38 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index 6ab014a7..0628459e 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -941,6 +941,17 @@ every member. Five decisions: always names the row categories and a total's never does. It is kept because `evaluate_spanning_in` is public, and a caller sharing one memo across two spans would otherwise read the wrong fold. No test can see it today. + **The aggregation and the covers are the memo's, not the call's** + (improvise-pntu). They are the same argument as the span, but they were + left as parameters. So one memo passed with covers=all and then with Feb + hidden returned the first fold's answer (5.0 where 2.0 was right). Widening + the key instead would have needed a closure's identity, and a closure has + none worth hashing. So `FoldMemo::new(agg, covers)` owns both, and + `evaluate_covering_in`/`evaluate_spanning_in` read them off the memo. A + mismatched pair can no longer be passed. `GridLayout::fold_memo` builds the + one a grid shares. What stays open is a memo built for one layout and + passed to another's `evaluate_in`. That hands over a consistent pair, only + for a different grid. - **A fold covers the members the grid draws** (improvise-f2oo). Hidden members are not in it. So a fold is exactly the rows it replaces, the total of a column's total does not move on `z`, and a group whose every member is hidden diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index b81f783d..43b90213 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -6118,3 +6118,37 @@ four sites. It reverts each `effect::refuse(` to `effect::set_status(` alone and runs `cargo test --workspace --no-fail-fast`. Four of four fail this test, and nothing else fails. None of the four needs an abort: each is the only effect its command emits, and `p`/`i` are single bindings, not sequences. + +### one_grid_build_evaluates_each_fold_formula_once_for_cells_and_totals + +**What was unguarded (improvise-0jw5).** PR #247's whole effect is +`viewport::grid_body` making one fold memo and passing it to every cell and +every total. Replacing the per-cell memo with a fresh one passed the whole +workspace. `a_fold_row_evaluates_each_formula_once_across_the_row` calls +`GridLayout::evaluate_in` with a memo of its own. So it pins the layout-level +API, not the build that is supposed to use it. The totals' memo had no guard +at all. + +**The test.** It uses a folded F0..F8 chain, as in the layout test. It counts +fold-formula evaluations across **one `grid_body` call** and asserts **18**: +F0..F8 once for the Q1 fold row and once for the totals, whose keys omit +Month. A **setup** assertion first counts the same cells and totals, each on a +fresh memo, and requires **90** (45 and 45). That makes 18 an observed saving +and not a coincidence of the fixture. + +**The setup assertion paid for itself on the first run.** The counter lives in +improvise-core and was `#[cfg(test)]`, which is false when core is compiled as +a dependency. The first run read **0** for both counts. A test comparing the two runs, +"shared <= separate", would have passed as 0 <= 0 under every rival, forever. +The absolute setup value is what refused it. The counter is now `pub` under `cfg(any(test, feature = +"test-support"))`, and the increment is gated the same way. improvise-core's +new `test-support` feature is enabled only by improvise-command's +dev-dependency, following improvise-26dm's precedent. It is off in every +shipped build. + +**Rivals, each a source edit run against this test:** +- `cell_value` handed `&layout.fold_memo()` per cell: fails. +- The totals handed `&layout.fold_memo()` per column: fails. + +Each rival re-derives one half's chain, so the count moves off 18. The file +was restored after each and compared byte for byte. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index a91b2fb5..2e1d4f8e 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1525,3 +1525,38 @@ had collapsed into one column could not pass the 9 by accident. Rival: `evaluate_covering_in` ignoring the memo it is given and making a fresh one. Injected into a full workspace run, it fails this test (45 against 9) and nothing else. + +### a_memo_answers_for_the_covers_it_was_built_with + +**What broke (improvise-pntu).** Found reviewing PR #247, and it was also +ChatGPT Codex's P2 there. `FoldMemo` was keyed by (fold key, span categories), +while `evaluate_covering_in` took the aggregation and the covers as separate +arguments. With `margin_book(["R = Revenue / Cost"])`, key +`{Month=Q1, _Measure=R}` and `Exact`, one memo was passed first with covers=all +and then with Feb hidden. The second call returned **5.0** (40 / 8, the first +fold's answer) where a fresh memo gives **2.0** (10 / 5, Jan alone). + +**Red first.** The test was first written against the old API, exactly as the +reviewer gave the repro, and it failed `left: Some(Number(5.0)), right: +Some(Number(2.0))`. The fix moves agg and covers into the memo +(`FoldMemo::new(agg, covers)`), so that shape **no longer compiles**. The +signature is the guard. The test now builds one memo per covers and asserts +three things. Each memo gives its own fold's value. The Feb-hidden memo agrees +with a fresh `evaluate_covering`. The covers=all memo still says 5.0 afterwards. + +**Rival:** `FoldMemo::new` storing `|_, _| true` in place of the covers it was +given. This test fails, because Jan alone reads 5.0. + +### a_memo_answers_for_the_aggregation_it_was_built_with + +pntu's other half. `{_Measure=Revenue}` names no month. So under an `Exact` +memo it is not a stored cell (`None`), and under an `OverHidden` memo it sums +Jan and Feb (`40.0`). + +**Rivals:** +- `evaluate_covering_in` passing `Aggregation::Exact` instead of the memo's + agg: this test fails. +- The same fault in `evaluate_spanning_in`: caught by two existing tests, + `a_max_measures_total_does_not_move_when_a_row_group_collapses` and + `an_average_measures_total_is_the_mean_over_its_records`. So it needed no + new test. diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index acc61a1f..bc1ef6e9 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -23,5 +23,6 @@ unicode-width = "^0.2" web-time = "1" [dev-dependencies] +improvise-core = { path = "../improvise-core", features = ["test-support"] } improvise-protocol = { path = "../improvise-protocol", features = ["test-support"] } tempfile = "3" diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index 4c8c48d6..edf28307 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -49,7 +49,7 @@ pub fn grid_body( let mut widths = header_level_seed(layout, n); // One memo for the whole build: a fold every cell and total reaches is // evaluated once (improvise-h6z1). The model does not change during it. - let memo = FoldMemo::default(); + let memo = layout.fold_memo(); // A column mixing measures has no total to show: `context/code/command.md`. let measures_on_rows = layout.row_cats.iter().any(|c| c == MEASURE_CATEGORY); let totalled = !records && !measures_on_rows && layout.row_count() > 0 && n > 0; @@ -190,3 +190,76 @@ pub fn compute_visible_cols( } count.max(1) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::model::types::FOLD_FORMULA_EVALUATIONS; + use crate::view::Axis; + use crate::workbook::Workbook; + + /// Q1 = {Jan, Feb} collapsed, Q2 = {Apr}; F0 = Revenue and F(i) = F(i-1) twice. + fn folded_chain() -> Workbook { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_item_in_group("Month", "Apr", "Q2").unwrap(); + wb.add_item_in_group("Month", "Jan", "Q1").unwrap(); + wb.add_item_in_group("Month", "Feb", "Q1").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + for (month, revenue) in [("Jan", 10.0), ("Feb", 30.0)] { + let key = CellKey::new(vec![ + ("Month".into(), month.into()), + ("_Measure".into(), "Revenue".into()), + ]); + wb.model.set_cell(key, CellValue::Number(revenue)); + } + wb.model + .add_formula(crate::formula::parse_formula("F0 = Revenue", "_Measure").unwrap()); + for i in 1..=8 { + let text = format!("F{i} = F{p} + F{p}", p = i - 1); + wb.model + .add_formula(crate::formula::parse_formula(&text, "_Measure").unwrap()); + } + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().toggle_group_collapse("Month", "Q1"); + wb.model.recompute_formulas(&wb.active_view().none_cats()); + wb + } + + fn evaluations(run: impl FnOnce()) -> usize { + FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0)); + run(); + FOLD_FORMULA_EVALUATIONS.with(|n| n.get()) + } + + /// improvise-0jw5: context/test-evidence/command.md. + #[test] + fn one_grid_build_evaluates_each_fold_formula_once_for_cells_and_totals() { + let wb = folded_chain(); + let view = wb.active_view(); + let layout = GridLayout::new(&wb.model, view); + let plan = FormatPlan::of(&wb.model, view); + let (rows, cols) = (0..layout.row_count(), 0..layout.col_count()); + let apart = evaluations(|| { + for (r, c) in rows.clone().flat_map(|r| cols.clone().map(move |c| (r, c))) { + let key = layout.cell_key(r, c).unwrap(); + layout.evaluate(&wb.model, &key); + } + for c in cols.clone() { + layout.column_total(&wb.model, c, &layout.fold_memo()); + } + }); + let together = evaluations(|| { + grid_body(&wb.model, &layout, &plan, rows.clone(), cols.clone()); + }); + assert_eq!( + apart, 90, + "setup: 45 for the Q1 row's cells, 45 for the totals" + ); + assert_eq!( + together, 18, + "F0..F8 once for the Q1 row, once for the totals" + ); + } +} diff --git a/crates/improvise-core/Cargo.toml b/crates/improvise-core/Cargo.toml index 790fb647..383fb843 100644 --- a/crates/improvise-core/Cargo.toml +++ b/crates/improvise-core/Cargo.toml @@ -17,6 +17,11 @@ serde = { version = "1", features = ["derive"] } # wasm-clean. siphasher = "1" +[features] +# Exposes the fold-evaluation counter to other crates' tests (improvise-0jw5). +# Off in every shipped build; enabled only by improvise-command's dev-dependency. +test-support = [] + [dev-dependencies] proptest = "1" serde_json = "1" diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index cdb39642..f9cf3577 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -138,28 +138,42 @@ fn agg_values( } // How many formulas a fold has evaluated: what improvise-o1y0's regression counts. -#[cfg(test)] +#[cfg(any(test, feature = "test-support"))] thread_local! { - pub(crate) static FOLD_FORMULA_EVALUATIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; + pub static FOLD_FORMULA_EVALUATIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; } -/// Each (fold key, span) evaluated once across one grid build (improvise-o1y0, -/// improvise-h6z1). Valid for one model state only: `context/code/core.md` §14. -#[derive(Default)] -pub struct FoldMemo(std::cell::RefCell), MemoEntry>>); +/// Each (fold key, span) evaluated once per grid build, under the agg and covers it +/// owns (improvise-o1y0, h6z1, pntu). One model state only: `context/code/core.md` §14. +pub struct FoldMemo<'a> { + agg: Aggregation, + covers: OwnedCovers<'a>, + seen: std::cell::RefCell), MemoEntry>>, +} + +/// The members a memo's folds cover, owned so a caller cannot pass another set. +type OwnedCovers<'a> = Box bool + 'a>; /// `None` while in progress, which is how a cycle is seen (improvise-o1y0). type MemoEntry = Option>; -impl FoldMemo { +impl<'a> FoldMemo<'a> { + pub fn new(agg: Aggregation, covers: impl Fn(&str, &str) -> bool + 'a) -> Self { + Self { + agg, + covers: Box::new(covers), + seen: Default::default(), + } + } + fn get(&self, key: &CellKey, span: Span) -> Option { let at = (key.clone(), span.over.to_vec()); - self.0.borrow().get(&at).cloned() + self.seen.borrow().get(&at).cloned() } fn put(&self, key: &CellKey, span: Span, entry: MemoEntry) { let at = (key.clone(), span.over.to_vec()); - self.0.borrow_mut().insert(at, entry); + self.seen.borrow_mut().insert(at, entry); } } @@ -183,7 +197,7 @@ enum Reach<'a> { Fold { agg: Aggregation, span: Span<'a>, - memo: &'a FoldMemo, + memo: &'a FoldMemo<'a>, }, } @@ -635,21 +649,17 @@ impl Model { agg: Aggregation, covers: Covers, ) -> Option { - let span = Span { covers, over: &[] }; - self.evaluate_fold(key, agg, span, &FoldMemo::default()) + self.evaluate_covering_in(key, &FoldMemo::new(agg, covers)) } - /// [`Self::evaluate_covering`] sharing `memo` with every other evaluation of one - /// grid build, which must not outlive the model state it read (improvise-h6z1). - pub fn evaluate_covering_in( - &self, - key: &CellKey, - agg: Aggregation, - covers: Covers, - memo: &FoldMemo, - ) -> Option { - let span = Span { covers, over: &[] }; - self.evaluate_fold(key, agg, span, memo) + /// [`Self::evaluate_covering`] under `memo`'s aggregation and covers, sharing it + /// with one grid build; it must not outlive the model state (improvise-h6z1, pntu). + pub fn evaluate_covering_in(&self, key: &CellKey, memo: &FoldMemo) -> Option { + let span = Span { + covers: &*memo.covers, + over: &[], + }; + self.evaluate_fold(key, memo.agg, span, memo) } /// The value `key` has as ONE fold over every covered item of the `over` @@ -661,20 +671,21 @@ impl Model { agg: Aggregation, covers: Covers, ) -> Option { - self.evaluate_spanning_in(key, over, agg, covers, &FoldMemo::default()) + self.evaluate_spanning_in(key, over, &FoldMemo::new(agg, covers)) } - /// [`Self::evaluate_spanning`] sharing one grid build's `memo` (improvise-h6z1). + /// [`Self::evaluate_spanning`] under one grid build's `memo` (improvise-h6z1, pntu). pub fn evaluate_spanning_in( &self, key: &CellKey, over: &[String], - agg: Aggregation, - covers: Covers, memo: &FoldMemo, ) -> Option { - let span = Span { covers, over }; - self.evaluate_fold(key, agg, span, memo) + let span = Span { + covers: &*memo.covers, + over, + }; + self.evaluate_fold(key, memo.agg, span, memo) } /// A key naming a group folds; any other key reads as before. A formula is @@ -711,7 +722,7 @@ impl Model { Some(None) => Some(CellValue::Error("circular".into())), None => { memo.put(key, span, None); - #[cfg(test)] + #[cfg(any(test, feature = "test-support"))] FOLD_FORMULA_EVALUATIONS.with(|n| n.set(n.get() + 1)); let reach = Reach::Fold { agg, span, memo }; let value = self.eval_formula_folded(formula, key, members, agg, reach); diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 3e03f924..dd44d721 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -658,22 +658,27 @@ impl GridLayout { /// The value of a pivot key as THIS grid reads it: aggregated over its None /// axis, and a fold over the members it draws (core.md §14). One evaluation. pub fn evaluate(&self, model: &Model, key: &CellKey) -> Option { - self.evaluate_in(model, key, &FoldMemo::default()) + self.evaluate_in(model, key, &self.fold_memo()) + } + + /// A memo holding this grid's aggregation and the members it draws: the one + /// a build shares across its cells and totals (improvise-h6z1, pntu). + pub fn fold_memo(&self) -> FoldMemo<'_> { + FoldMemo::new(Aggregation::of(&self.none_cats), |cat, item| { + !self.hidden.get(cat).is_some_and(|h| h.contains(item)) + }) } /// [`Self::evaluate`] sharing one grid build's `memo` (improvise-h6z1). pub fn evaluate_in(&self, model: &Model, key: &CellKey, memo: &FoldMemo) -> Option { - let drawn = |cat: &str, item: &str| !self.hidden.get(cat).is_some_and(|h| h.contains(item)); - model.evaluate_covering_in(key, Aggregation::of(&self.none_cats), &drawn, memo) + model.evaluate_covering_in(key, memo) } /// A column's total: its cell as if every drawn row were ONE fold, so a /// formula is evaluated over the pooled rows (improvise-wm20, core.md §14). pub fn column_total(&self, model: &Model, col: usize, memo: &FoldMemo) -> Option { let key = self.column_key(col)?; - let drawn = |cat: &str, item: &str| !self.hidden.get(cat).is_some_and(|h| h.contains(item)); - let agg = Aggregation::of(&self.none_cats); - model.evaluate_spanning_in(&key, &self.row_cats, agg, &drawn, memo) + model.evaluate_spanning_in(&key, &self.row_cats, memo) } /// Display text for (row, col) in either mode. @@ -909,6 +914,7 @@ mod tests { AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, data_items, prune_axis_items, synthetic_record_info, }; + use crate::model::Aggregation; use crate::model::cell::{CellKey, CellValue}; use crate::model::types::{FOLD_FORMULA_EVALUATIONS, FoldMemo}; use crate::view::Axis; @@ -1867,7 +1873,7 @@ mod tests { .filter_map(|c| layout.cell_key(q1, c)) .collect(); let evaluations = |shared: bool| { - let memo = FoldMemo::default(); + let memo = layout.fold_memo(); FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0)); let values: Vec<_> = row_keys .iter() @@ -1888,6 +1894,50 @@ mod tests { assert_eq!(together, 9, "F0..F8 once each across the whole row"); } + /// improvise-pntu: context/test-evidence/core.md. + #[test] + fn a_memo_answers_for_the_covers_it_was_built_with() { + let wb = margin_book(&["R = Revenue / Cost"]); + let key = CellKey::new(vec![ + ("Month".into(), "Q1".into()), + ("_Measure".into(), "R".into()), + ]); + let all = FoldMemo::new(Aggregation::Exact, |_, _| true); + let no_feb = FoldMemo::new(Aggregation::Exact, |_, item| item != "Feb"); + let pooled = wb.model.evaluate_covering_in(&key, &all); + let jan_only = wb.model.evaluate_covering_in(&key, &no_feb); + assert_eq!( + pooled, + Some(CellValue::Number(5.0)), + "40 / 8 over Jan and Feb" + ); + assert_eq!( + jan_only, + Some(CellValue::Number(2.0)), + "10 / 5 over Jan alone" + ); + let fresh = wb + .model + .evaluate_covering(&key, Aggregation::Exact, &|_, item| item != "Feb"); + assert_eq!(jan_only, fresh, "a memo agrees with a fresh evaluation"); + assert_eq!(wb.model.evaluate_covering_in(&key, &all), pooled); + } + + /// improvise-pntu's other half: the aggregation is the memo's too. + #[test] + fn a_memo_answers_for_the_aggregation_it_was_built_with() { + let wb = margin_book(&[]); + let key = CellKey::new(vec![("_Measure".into(), "Revenue".into())]); + let exact = FoldMemo::new(Aggregation::Exact, |_, _| true); + let over = FoldMemo::new(Aggregation::OverHidden, |_, _| true); + assert_eq!(wb.model.evaluate_covering_in(&key, &exact), None); + assert_eq!( + wb.model.evaluate_covering_in(&key, &over), + Some(CellValue::Number(40.0)), + "Jan and Feb summed" + ); + } + /// improvise-eccl — see context/test-evidence/core.md. #[test] fn a_folds_ratio_formula_is_the_ratio_of_its_folded_inputs() { From fd221c6c6ca137a6fee0f4b7f7a0b5313c702c07 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:24:34 -0700 Subject: [PATCH 2/3] prune_empty shares one fold memo per build (metf) prune_empty evaluated every cell with a fresh memo, h6z1's shape on the same keypress path. Measured on an M4 Max: a pruned build of the folded chain went 705-900 ms -> ~350 ms at 18k and 6.4-6.6 s -> 3.0-5.0 s at 90k; the dense grid is unchanged within noise. A new bench line times the pruned build. The test counts 9 evaluations where a fresh memo per cell counts 45, and checks the pruned rows are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- benches/grid_projection.rs | 21 ++++++++- context/test-evidence/core.md | 33 ++++++++++++++ crates/improvise-core/src/view/layout.rs | 57 +++++++++++++++++++++--- 3 files changed, 103 insertions(+), 8 deletions(-) diff --git a/benches/grid_projection.rs b/benches/grid_projection.rs index 132a7c31..ac1c19e9 100644 --- a/benches/grid_projection.rs +++ b/benches/grid_projection.rs @@ -5,7 +5,7 @@ use std::time::{Duration, Instant}; use improvise::format::FormatPlan; use improvise::model::cell::{CellKey, CellValue}; -use improvise::view::Axis; +use improvise::view::{Axis, GridLayout}; use improvise::workbook::Workbook; use improvise_command::ui::app::App; use improvise_command::ui::project::{self, Viewport}; @@ -180,6 +180,21 @@ windowed {windowed:>11.3?} ratio {ratio:>4.1}x col_widths {widths:>11.3?} ({sh ); } +/// The layout build with `prune_empty` on: every cell evaluated to find the empty +/// rows and columns, on the same keypress path as `grid_of` (improvise-metf). +fn report_prune(label: &str, mut app: App, iters: u32) { + app.model_state.workbook.active_view_mut().prune_empty = true; + let model = &app.model_state.workbook.model; + let view = app.model_state.workbook.active_view(); + let mut best = Duration::MAX; + for _ in 0..iters.max(3) { + let start = Instant::now(); + std::hint::black_box(GridLayout::with_frozen_records(model, view, None)); + best = best.min(start.elapsed()); + } + println!("{label:<22} layout build with prune_empty {best:>11.3?}"); +} + fn main() { println!("project::grid_of, one call = one TUI frame's grid cache"); println!("a 60fps frame budget is 16.7ms; the TUI redraws on each keypress\n"); @@ -190,4 +205,8 @@ fn main() { println!("\nfolded rows and a five-formula chain (improvise-h6z1)\n"); report("folded chain ~18k", chain_app(1000, 30), 5); report("folded chain ~90k", chain_app(5000, 30), 3); + println!("\nthe same, pruned (improvise-metf)\n"); + report_prune("dense 18k", dense_app(150, 20, 6), 5); + report_prune("folded chain ~18k", chain_app(1000, 30), 5); + report_prune("folded chain ~90k", chain_app(5000, 30), 3); } diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 2e1d4f8e..858f8927 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1560,3 +1560,36 @@ Jan and Feb (`40.0`). `a_max_measures_total_does_not_move_when_a_row_group_collapses` and `an_average_measures_total_is_the_mean_over_its_records`. So it needed no new test. + +### pruning_evaluates_each_fold_formula_once_and_keeps_the_same_rows + +**What was slow (improvise-metf).** `GridLayout::prune_empty` evaluated every +cell with its own fresh fold memo. That is h6z1's shape, and it sits on the same +keypress path, since `with_frozen_records` prunes whenever the view asks. Each +column of a folded chain re-derived the chain below it. + +**Measured first**, on an Apple M4 Max (64 GB) running other agents' builds. +The bench is `cargo bench --bench grid_projection`, whose new "pruned" lines time +a `with_frozen_records` build with `prune_empty` on. There were two back-to-back +pairs, and the second put the old line back for the "before" half: + +| Case | Before (fresh memo per cell) | After (one memo per build) | +|---|---|---| +| dense 18k | 90 ms, 88 ms | 82 ms, 100 ms | +| folded chain ~18k | 900 ms, 705 ms | 355 ms, 351 ms | +| folded chain ~90k | 6.40 s, 6.62 s | 3.00 s, 4.96 s | + +It pays on the folded chain: about 2.0× to 2.5× at 18k, and 1.3× to 2.1× at 90k. +The dense grid has no folds and moves only within noise. So `prune_empty` now +shares one memo per build, taken from `GridLayout::fold_memo` and dropped before +the axes are pruned. + +**The test.** It uses `margin_book`'s F0..F8 chain with Q1 collapsed. It counts +fold-formula evaluations across one pruned `with_frozen_records` build and +asserts **9**. It also asserts that the pruned rows are exactly the rows a fresh +`evaluate` per cell would keep, so sharing the memo cannot change what is +pruned. And it asserts that the Q1 fold row survives, so the count is not +measured over an empty grid. + +**Rival:** the old line, `self.evaluate(model, &k)` per cell. This test fails: +`left: 45, right: 9`. diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index dd44d721..91e39265 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -611,14 +611,18 @@ impl GridLayout { return; } - // Build a row×col grid of "has content?" + // Build a row×col grid of "has content?", one fold memo for the whole + // walk (improvise-metf). It is dropped before the axes are pruned. let mut has_value = vec![vec![false; cc]; rc]; - for (ri, row) in has_value.iter_mut().enumerate() { - for (ci, cell) in row.iter_mut().enumerate() { - *cell = self - .cell_key(ri, ci) - .and_then(|k| self.evaluate(model, &k)) - .is_some(); + { + let memo = self.fold_memo(); + for (ri, row) in has_value.iter_mut().enumerate() { + for (ci, cell) in row.iter_mut().enumerate() { + *cell = self + .cell_key(ri, ci) + .and_then(|k| self.evaluate_in(model, &k, &memo)) + .is_some(); + } } } @@ -1854,6 +1858,45 @@ mod tests { .and_then(|v| v.as_f64()) } + /// improvise-metf: pruning evaluates each formula a fold row reaches once, and + /// keeps what a fresh evaluation per cell keeps. context/test-evidence/core.md. + #[test] + fn pruning_evaluates_each_fold_formula_once_and_keeps_the_same_rows() { + let chain: Vec = std::iter::once("F0 = Revenue".to_string()) + .chain((1..=8).map(|i| format!("F{i} = F{p} + F{p}", p = i - 1))) + .collect(); + let chain: Vec<&str> = chain.iter().map(String::as_str).collect(); + let mut wb = margin_book(&chain); + wb.active_view_mut().toggle_group_collapse("Month", "Q1"); + wb.model.recompute_formulas(&wb.active_view().none_cats()); + let unpruned = GridLayout::new(&wb.model, wb.active_view()); + let kept_by_fresh_evaluation: Vec = (0..unpruned.row_count()) + .filter(|&r| { + (0..unpruned.col_count()).any(|c| { + let k = unpruned.cell_key(r, c).unwrap(); + unpruned.evaluate(&wb.model, &k).is_some() + }) + }) + .map(|r| unpruned.row_label(r)) + .collect(); + wb.active_view_mut().prune_empty = true; + FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0)); + let pruned = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + let evaluations = FOLD_FORMULA_EVALUATIONS.with(|n| n.get()); + let rows: Vec = (0..pruned.row_count()) + .map(|r| pruned.row_label(r)) + .collect(); + assert_eq!( + rows, kept_by_fresh_evaluation, + "sharing the memo changed what is pruned" + ); + assert!( + rows.iter().any(|r| r == "Q1"), + "setup: the fold row survives pruning" + ); + assert_eq!(evaluations, 9, "F0..F8 once each for the fold row, not 45"); + } + /// improvise-h6z1: one grid build evaluates each formula a fold row reaches /// once, not once per column. context/test-evidence/core.md. #[test] From d7dceef54933f1f24e3542719e3ff4c2706d6680 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 17:30:41 -0700 Subject: [PATCH 3/3] a grid evaluates through its own evaluator, never another's memo (opbo) GridLayout::evaluate_in and column_total took any &FoldMemo, so a memo from a layout showing Feb, handed to one hiding it, gave 5.0 where 2.0 was right. GridLayout::evaluator(model) now returns a GridEval that owns a memo its layout built, with cell() and column_total(); the memo-taking methods and the public fold_memo are gone. grid_body and prune_empty use it, and the earlier rivals still fail when restated for the new API. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/core.md | 21 +++-- context/test-evidence/core.md | 35 ++++++++ crates/improvise-command/src/ui/viewport.rs | 22 ++--- crates/improvise-core/src/view/layout.rs | 95 +++++++++++++++------ crates/improvise-core/src/view/mod.rs | 4 +- 5 files changed, 129 insertions(+), 48 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index 0628459e..758e20ad 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -931,9 +931,9 @@ every member. Five decisions: what gave the wrong ratios. The remaining ~6× is each column's cell re-deriving the chain below it (45 evaluations per row where 9 would do). **The memo is now shared across one grid build** (improvise-h6z1). - `viewport::grid_body` makes one `FoldMemo` and passes it through - `GridLayout::evaluate_in` and `column_total` for every cell and total, so a - fold row's chain is evaluated 9 times, not 45. A memo is valid only for the + `viewport::grid_body` takes one `GridLayout::evaluator` and asks it for + every cell and total, so a fold row's chain is evaluated 9 times, not 45. + `prune_empty` does the same (improvise-metf). A memo is valid only for the model state it read. That is why it is made per build and never stored on `GridLayout`, which outlives cell writes. The memo is keyed by the fold key **and** the span's categories. Within one @@ -948,10 +948,17 @@ every member. Five decisions: the key instead would have needed a closure's identity, and a closure has none worth hashing. So `FoldMemo::new(agg, covers)` owns both, and `evaluate_covering_in`/`evaluate_spanning_in` read them off the memo. A - mismatched pair can no longer be passed. `GridLayout::fold_memo` builds the - one a grid shares. What stays open is a memo built for one layout and - passed to another's `evaluate_in`. That hands over a consistent pair, only - for a different grid. + mismatched pair can no longer be passed. + **A grid's evaluations cannot be handed another grid's memo either** + (improvise-opbo). `GridLayout::evaluate_in` and `column_total` used to take + any `&FoldMemo`. A memo built from a layout showing Feb, passed to a layout + hiding it, gave 5.0 where 2.0 was right: a consistent pair, only for a + different grid. `GridLayout::evaluator(model)` now returns a `GridEval` that + owns a memo its own layout built, with `cell` and `column_total` on it, and + the memo-taking methods are gone. `Model::evaluate_covering_in` and + `evaluate_spanning_in` still take a `&FoldMemo`, but a memo there carries + its own agg and covers, so the only mismatch left is a memo over a + different *model state*. That is the per-build rule above, unchanged. - **A fold covers the members the grid draws** (improvise-f2oo). Hidden members are not in it. So a fold is exactly the rows it replaces, the total of a column's total does not move on `z`, and a group whose every member is hidden diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 858f8927..8115d3ba 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1593,3 +1593,38 @@ measured over an empty grid. **Rival:** the old line, `self.evaluate(model, &k)` per cell. This test fails: `left: 45, right: 9`. + +### a_grid_evaluates_under_its_own_covers_not_another_grids + +**What was open (improvise-opbo, P3).** It confirms ChatGPT Codex's P2 on #258. +`GridLayout::evaluate_in` and `column_total` took any `&FoldMemo`. With +`margin_book(["R = Revenue / Cost"])` and Q1 collapsed, there are two layouts +over one model: one draws Jan and Feb, the other hides Feb. The first layout's +memo, handed to the second layout's `evaluate_in`, gave **5.0** for +`{Month=Q1, _Measure=R}` where the second grid reads **2.0**. Nothing in +production reached it, because each build made its own memo. + +**Red first.** The test was first written against `evaluate_in` with a foreign +memo. It failed `left: Some(Number(5.0)), right: Some(Number(2.0))`. + +**Fix: the review's proposal.** `GridLayout::evaluator(model)` returns a +`GridEval` that borrows the layout and owns a memo the layout built itself. It +has `cell(key)` and `column_total(col)`. `evaluate_in`, the memo-taking +`column_total`, and `fold_memo` are gone. So the shape the test first used no +longer compiles, and the signature is the guard. The test now asserts two +things. Each layout's evaluator answers for its own covers: 5.0 and 2.0. And +asking one does not move the other. + +**Rival:** the evaluator's covers ignoring the layout's hidden items. This test +fails `left: Some(Number(5.0)), right: Some(Number(2.0))`. + +**The earlier rivals, restated for the new API, still fail:** +- `pruning_evaluates_each_fold_formula_once_and_keeps_the_same_rows` with a + fresh evaluator per cell: `left: 45, right: 9`. +- `one_grid_build_evaluates_each_fold_formula_once_for_cells_and_totals` with + a fresh evaluator per cell: `left: 54, right: 18`. +- The same test with a fresh evaluator per total: `left: 54, right: 18`. + +The entries above for h6z1, 0jw5 and metf name `evaluate_in` and `fold_memo` +because those were the API when they were written. This entry supersedes the +names, not the arguments. diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index edf28307..bd1a6906 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -9,8 +9,7 @@ use crate::format::{FormatPlan, NumberFormat}; use crate::model::Model; use crate::model::category::MEASURE_CATEGORY; use crate::model::cell::{CellKey, CellValue}; -use crate::model::types::FoldMemo; -use crate::view::{AxisEntry, GridLayout}; +use crate::view::{AxisEntry, GridEval, GridLayout}; /// Minimum column width — enough for short numbers/labels + 1 char gap. pub const MIN_COL_WIDTH: u16 = 5; @@ -47,16 +46,14 @@ pub fn grid_body( let n = layout.col_count(); let records = layout.is_records_mode(); let mut widths = header_level_seed(layout, n); - // One memo for the whole build: a fold every cell and total reaches is + // One evaluator for the whole build: a fold every cell and total reaches is // evaluated once (improvise-h6z1). The model does not change during it. - let memo = layout.fold_memo(); + let eval = layout.evaluator(model); // A column mixing measures has no total to show: `context/code/command.md`. let measures_on_rows = layout.row_cats.iter().any(|c| c == MEASURE_CATEGORY); let totalled = !records && !measures_on_rows && layout.row_count() > 0 && n > 0; let totals: Vec> = match totalled { - true => (0..n) - .map(|ci| layout.column_total(model, ci, &memo)) - .collect(), + true => (0..n).map(|ci| eval.column_total(ci)).collect(), false => Vec::new(), }; let mut window: Vec>> = Vec::with_capacity(rows.len()); @@ -73,7 +70,7 @@ pub fn grid_body( false => Vec::new(), }; for ci in 0..n { - let (key, value) = cell_value(model, layout, records, ri, ci, &memo); + let (key, value) = cell_value(layout, &eval, records, ri, ci); let fmt = plan.at(key.as_ref()); let text = match records { true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(), @@ -106,20 +103,17 @@ pub fn grid_body( /// The one evaluation. Records mode reads its wide row; pivot mode aggregates. fn cell_value( - model: &Model, layout: &GridLayout, + eval: &GridEval, records: bool, row: usize, col: usize, - memo: &FoldMemo, ) -> (Option, Option) { match records { true => (None, layout.records_value(row, col)), false => { let key = layout.cell_key(row, col); - let value = key - .as_ref() - .and_then(|k| layout.evaluate_in(model, k, memo)); + let value = key.as_ref().and_then(|k| eval.cell(k)); (key, value) } } @@ -247,7 +241,7 @@ mod tests { layout.evaluate(&wb.model, &key); } for c in cols.clone() { - layout.column_total(&wb.model, c, &layout.fold_memo()); + layout.evaluator(&wb.model).column_total(c); } }); let together = evaluations(|| { diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 91e39265..675a978e 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -615,13 +615,10 @@ impl GridLayout { // walk (improvise-metf). It is dropped before the axes are pruned. let mut has_value = vec![vec![false; cc]; rc]; { - let memo = self.fold_memo(); + let eval = self.evaluator(model); for (ri, row) in has_value.iter_mut().enumerate() { for (ci, cell) in row.iter_mut().enumerate() { - *cell = self - .cell_key(ri, ci) - .and_then(|k| self.evaluate_in(model, &k, &memo)) - .is_some(); + *cell = self.cell_key(ri, ci).and_then(|k| eval.cell(&k)).is_some(); } } } @@ -662,27 +659,20 @@ impl GridLayout { /// The value of a pivot key as THIS grid reads it: aggregated over its None /// axis, and a fold over the members it draws (core.md §14). One evaluation. pub fn evaluate(&self, model: &Model, key: &CellKey) -> Option { - self.evaluate_in(model, key, &self.fold_memo()) + self.evaluator(model).cell(key) } - /// A memo holding this grid's aggregation and the members it draws: the one - /// a build shares across its cells and totals (improvise-h6z1, pntu). - pub fn fold_memo(&self) -> FoldMemo<'_> { - FoldMemo::new(Aggregation::of(&self.none_cats), |cat, item| { + /// One grid build's evaluator, whose fold memo this layout built, so no other + /// grid's memo can reach its cells or totals (improvise-h6z1, pntu, opbo). + pub fn evaluator<'a>(&'a self, model: &'a Model) -> GridEval<'a> { + let memo = FoldMemo::new(Aggregation::of(&self.none_cats), |cat, item| { !self.hidden.get(cat).is_some_and(|h| h.contains(item)) - }) - } - - /// [`Self::evaluate`] sharing one grid build's `memo` (improvise-h6z1). - pub fn evaluate_in(&self, model: &Model, key: &CellKey, memo: &FoldMemo) -> Option { - model.evaluate_covering_in(key, memo) - } - - /// A column's total: its cell as if every drawn row were ONE fold, so a - /// formula is evaluated over the pooled rows (improvise-wm20, core.md §14). - pub fn column_total(&self, model: &Model, col: usize, memo: &FoldMemo) -> Option { - let key = self.column_key(col)?; - model.evaluate_spanning_in(&key, &self.row_cats, memo) + }); + GridEval { + layout: self, + model, + memo, + } } /// Display text for (row, col) in either mode. @@ -912,6 +902,29 @@ fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec result } +/// One grid build's evaluations under one fold memo its own layout built, valid +/// for one model state (improvise-h6z1, opbo). `context/code/core.md` §14. +pub struct GridEval<'a> { + layout: &'a GridLayout, + model: &'a Model, + memo: FoldMemo<'a>, +} + +impl GridEval<'_> { + /// A pivot key's value as this grid reads it: [`GridLayout::evaluate`], shared. + pub fn cell(&self, key: &CellKey) -> Option { + self.model.evaluate_covering_in(key, &self.memo) + } + + /// A column's total: its cell as if every drawn row were ONE fold, so a + /// formula is evaluated over the pooled rows (improvise-wm20, core.md §14). + pub fn column_total(&self, col: usize) -> Option { + let key = self.layout.column_key(col)?; + self.model + .evaluate_spanning_in(&key, &self.layout.row_cats, &self.memo) + } +} + #[cfg(test)] mod tests { use super::{ @@ -1916,12 +1929,12 @@ mod tests { .filter_map(|c| layout.cell_key(q1, c)) .collect(); let evaluations = |shared: bool| { - let memo = layout.fold_memo(); + let eval = layout.evaluator(&wb.model); FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0)); let values: Vec<_> = row_keys .iter() .map(|k| match shared { - true => layout.evaluate_in(&wb.model, k, &memo), + true => eval.cell(k), false => layout.evaluate(&wb.model, k), }) .collect(); @@ -1981,6 +1994,38 @@ mod tests { ); } + /// improvise-opbo: a grid's evaluation cannot be handed another grid's memo. + #[test] + fn a_grid_evaluates_under_its_own_covers_not_another_grids() { + let mut wb = margin_book(&["R = Revenue / Cost"]); + wb.active_view_mut().toggle_group_collapse("Month", "Q1"); + let all = GridLayout::new(&wb.model, wb.active_view()); + let mut no_feb_view = wb.active_view().clone(); + no_feb_view.hide_item("Month", "Feb"); + let no_feb = GridLayout::new(&wb.model, &no_feb_view); + let key = CellKey::new(vec![ + ("Month".into(), "Q1".into()), + ("_Measure".into(), "R".into()), + ]); + let on_all = all.evaluator(&wb.model); + let on_no_feb = no_feb.evaluator(&wb.model); + assert_eq!( + on_all.cell(&key), + Some(CellValue::Number(5.0)), + "Jan and Feb" + ); + assert_eq!( + on_no_feb.cell(&key), + Some(CellValue::Number(2.0)), + "the grid hiding Feb reads Jan alone" + ); + assert_eq!( + on_all.cell(&key), + Some(CellValue::Number(5.0)), + "and neither moved the other" + ); + } + /// improvise-eccl — see context/test-evidence/core.md. #[test] fn a_folds_ratio_formula_is_the_ratio_of_its_folded_inputs() { diff --git a/crates/improvise-core/src/view/mod.rs b/crates/improvise-core/src/view/mod.rs index e567d3c6..3bdea6ce 100644 --- a/crates/improvise-core/src/view/mod.rs +++ b/crates/improvise-core/src/view/mod.rs @@ -4,8 +4,8 @@ pub mod types; pub use axis::{Axis, AxisColor}; pub use layout::{ - AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, ViewStep, - drill_constraint, drill_snapshot, drill_steps, records_spine, records_steps, + AxisEntry, GridEval, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, + ViewStep, drill_constraint, drill_snapshot, drill_steps, records_spine, records_steps, synthetic_record_info, }; pub use types::{