diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index d0a2eaca..ad2dc12c 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -7,7 +7,6 @@ use improvise_protocol::header_level_widths; use crate::format::{FormatPlan, NumberFormat}; use crate::model::Model; -use crate::model::category::MEASURE_CATEGORY; use crate::model::cell::{CellKey, CellValue}; use crate::view::{AxisEntry, GridEval, GridLayout}; @@ -50,9 +49,7 @@ pub fn grid_body( // evaluated once (improvise-h6z1). The model does not change during it. 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 { + let totals: Vec> = match layout.draws_totals() { true => (0..n).map(|ci| eval.column_total(ci)).collect(), false => Vec::new(), }; diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index eeb6f71a..cea45e01 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -339,7 +339,7 @@ fn eval_is(coordinate: Option<&str>, filter: &Filter) -> f64 { /// Whether a grid hides dimensions a cell key does not name. The emptiness of /// the view's `Axis::None` list is the whole question — `context/code/core.md`. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum Aggregation { /// The key names every dimension, so the cell is read as stored. Exact, diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 37a0c133..d7187d22 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -991,6 +991,13 @@ impl GridLayout { Some(CellKey::new(coords)) } + /// Whether the grid draws a total under each column: not in records mode, + /// not when a column mixes measures (`_Measure` on the rows), not when empty. + pub fn draws_totals(&self) -> bool { + let measures_on_rows = self.row_cats.iter().any(|c| c == MEASURE_CATEGORY); + !self.is_records_mode() && !measures_on_rows && self.row_count() > 0 && self.col_count() > 0 + } + /// The coordinates a COLUMN answers for once its rows are aggregated away: /// the page/filter constraints and the column's own tuple (core.md §6). pub fn column_key(&self, col: usize) -> Option { diff --git a/crates/improvise-incremental/src/eager.rs b/crates/improvise-incremental/src/eager.rs index 7f3cf188..846c950d 100644 --- a/crates/improvise-incremental/src/eager.rs +++ b/crates/improvise-incremental/src/eager.rs @@ -23,6 +23,17 @@ impl Eager { } } +impl Eager { + /// Whether `item` names a group of `cat` that covers `held`, a stored item. + fn group_holds(&self, cat: &str, item: &str, held: Option<&str>) -> bool { + let Some(category) = self.model.category(cat) else { + return false; + }; + let members = category.items_in_group(item); + !category.items.contains_key(item) && held.is_some_and(|h| members.contains(&h)) + } +} + impl Derived for Eager { fn value(&self, key: &CellKey, agg: Aggregation) -> Option { self.model.evaluate_aggregated(key, agg) @@ -50,8 +61,16 @@ impl Derived for Eager { fn occupancy(&self, partial: &CellKey, reading: &Reading) -> usize { let drawn = |(cat, item): &(String, String)| reading.covers(cat, item); - let stored = self.model.data.matching_cells(&partial.0); - stored.iter().filter(|(k, _)| k.0.iter().all(drawn)).count() + let named_by = |stored: &CellKey| { + partial.0.iter().all(|(cat, item)| { + let held = stored.get(cat); + held == Some(item.as_str()) || self.group_holds(cat, item, held) + }) + }; + let cells = self.model.data.iter_cells(); + cells + .filter(|(k, _)| named_by(k) && k.0.iter().all(drawn)) + .count() } } diff --git a/crates/improvise-incremental/src/harness.rs b/crates/improvise-incremental/src/harness.rs index e266e41a..a77a4736 100644 --- a/crates/improvise-incremental/src/harness.rs +++ b/crates/improvise-incremental/src/harness.rs @@ -13,7 +13,7 @@ pub trait Maintained: Derived { } /// One question put to both backends. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Probe { Value(CellKey, Aggregation), RawFold(CellKey), @@ -81,14 +81,14 @@ pub const MAX_DROPPED: usize = 3; /// The questions asked of a backend, once each, in the order first added. #[derive(Default)] struct Asked { - seen: HashSet, + seen: HashSet, order: Vec, } impl Asked { fn extend(&mut self, probes: impl IntoIterator) { for p in probes { - if self.seen.insert(format!("{p:?}")) { + if self.seen.insert(p.clone()) { self.order.push(p); } } @@ -161,46 +161,63 @@ pub fn probes(model: &Model) -> Vec { /// Everything each of `views` draws of `model`: cells, column totals and /// occupancy. Why: `context/code/improvise-incremental.md`, step 4 groundwork. -pub fn drawn_probes(model: &Model, views: &[View]) -> Vec { +/// Only views whose None categories are `none_cats` are asked (the oracle's +/// formulas are recomputed under one set); records mode is skipped. +pub fn drawn_probes(model: &Model, views: &[View], none_cats: &[String]) -> Vec { + let sorted = |cats: &[String]| { + let mut cats = cats.to_vec(); + cats.sort(); + cats + }; + let oracle_set = sorted(none_cats); views .iter() .map(|view| (view, GridLayout::with_frozen_records(model, view, None))) .filter(|(_, layout)| !layout.is_records_mode()) - .flat_map(|(view, layout)| view_probes(view, &layout)) + .filter(|(_, layout)| sorted(&layout.none_cats) == oracle_set) + .flat_map(|(view, layout)| view_probes(model, view, &layout)) .collect() } -fn axis_partial(cats: &[String], tuple: Option<&[String]>) -> CellKey { - let named = tuple.unwrap_or_default().iter(); - CellKey::new(cats.iter().cloned().zip(named.cloned()).collect()) +/// A stored cell count is asked of a key only if something is stored under it. +fn occupancy_probe(model: &Model, key: CellKey, reading: &Reading) -> Option { + model + .formula_for(&key) + .is_none() + .then(|| Probe::Occupancy(key, reading.clone())) } -fn view_probes(view: &View, layout: &GridLayout) -> Vec { +fn view_probes(model: &Model, view: &View, layout: &GridLayout) -> Vec { let reading = Reading::of_view(view, &layout.none_cats); let cells = (0..layout.row_count()) .flat_map(|r| (0..layout.col_count()).map(move |c| (r, c))) .filter_map(|(r, c)| layout.cell_key(r, c)) .flat_map(|key| { let covered = Probe::Covered(key.clone(), reading.clone()); - let occupied = Probe::Occupancy(key.clone(), reading.clone()); - all_three(key).into_iter().chain([covered, occupied]) + let occupied = occupancy_probe(model, key.clone(), &reading); + all_three(key).into_iter().chain([covered]).chain(occupied) }); let columns = (0..layout.col_count()).flat_map(|c| { - let total = layout - .column_key(c) - .map(|key| Probe::Total(key, layout.row_cats.clone(), reading.clone())); - let line = axis_partial(&layout.col_cats, layout.col_tuple(c)); - total - .into_iter() - .chain([Probe::Occupancy(line, reading.clone())]) + let key = layout.column_key(c); + let total = key.clone().filter(|_| layout.draws_totals()); + let total = total.map(|k| Probe::Total(k, layout.row_cats.clone(), reading.clone())); + let line = key.and_then(|k| occupancy_probe(model, k, &reading)); + total.into_iter().chain(line) }); - let rows = (0..layout.row_count()).map(|r| { - let line = axis_partial(&layout.row_cats, layout.row_tuple(r)); - Probe::Occupancy(line, reading.clone()) + let rows = (0..layout.row_count()).flat_map(|r| { + let line = layout + .cell_key(r, 0) + .map(|k| without_all(k, &layout.col_cats)); + line.and_then(|k| occupancy_probe(model, k, &reading)) }); cells.chain(columns).chain(rows).collect() } +/// `key` less each of `cats`: a row's line is a cell without its column coordinates. +fn without_all(key: CellKey, cats: &[String]) -> CellKey { + cats.iter().fold(key, |k, cat| k.without(cat)) +} + /// What an edit names: its key, asked all three ways, after the edit. pub fn edit_probes(edit: &Edit) -> Vec { edit.key() @@ -252,13 +269,13 @@ pub fn differential_in( let mut eager = Eager::new(model.clone(), none_cats.to_vec()); let mut asked = Asked::default(); asked.extend(probes(eager.model())); - asked.extend(drawn_probes(eager.model(), views)); + asked.extend(drawn_probes(eager.model(), views, none_cats)); let mut probes_run = compare(0, &eager, &candidate, &asked)?; for (n, edit) in edits.iter().enumerate() { eager.apply(edit); candidate.apply(edit); asked.extend(probes(eager.model())); - asked.extend(drawn_probes(eager.model(), views)); + asked.extend(drawn_probes(eager.model(), views, none_cats)); asked.extend(edit_probes(edit)); probes_run += compare(n + 1, &eager, &candidate, &asked)?; } @@ -267,3 +284,41 @@ pub fn differential_in( probes: probes_run, }) } + +#[cfg(test)] +mod tests { + use super::*; + + fn old_mask_walk(key: &CellKey) -> Vec { + let n = key.0.len(); + (0..1u64 << n) + .filter(|mask| mask.count_ones() as usize <= MAX_DROPPED) + .map(|mask| { + let kept = (0..n).filter(|i| mask >> i & 1 == 0); + CellKey::new(kept.map(|i| key.0[i].clone()).collect()) + }) + .collect() + } + + fn key_of(n: usize) -> CellKey { + CellKey::new((0..n).map(|i| (format!("C{i:02}"), "a".into())).collect()) + } + + #[test] + fn sub_partials_are_the_keys_the_old_mask_walk_gave() { + for n in 0..=12 { + let mut now: Vec<_> = sub_partials(&key_of(n)).collect(); + let mut was = old_mask_walk(&key_of(n)); + now.sort(); + was.sort(); + assert_eq!(now, was, "{n} coordinates"); + } + } + + #[test] + fn sub_partials_of_sixty_four_coordinates_number_the_choices_of_at_most_three() { + let n = 64; + let choices = 1 + n + n * (n - 1) / 2 + n * (n - 1) * (n - 2) / 6; + assert_eq!(sub_partials(&key_of(n)).count(), choices); + } +} diff --git a/crates/improvise-incremental/src/reading.rs b/crates/improvise-incremental/src/reading.rs index d8e28a7a..9954b2b8 100644 --- a/crates/improvise-incremental/src/reading.rs +++ b/crates/improvise-incremental/src/reading.rs @@ -5,8 +5,9 @@ use improvise_core::view::View; use std::collections::BTreeSet; /// One view's way of reading: its aggregation and its hidden `(category, item)` -/// pairs, which a fold and an occupancy count both leave out. -#[derive(Debug, Clone, PartialEq, Eq)] +/// pairs. A fold over a group or a spanned category leaves them out; a read of a +/// key naming neither does not, and an occupancy count always does. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct Reading { pub agg: Aggregation, hidden: BTreeSet<(String, String)>, diff --git a/crates/improvise-incremental/tests/oracle.rs b/crates/improvise-incremental/tests/oracle.rs index 1b5d1dd5..aa362585 100644 --- a/crates/improvise-incremental/tests/oracle.rs +++ b/crates/improvise-incremental/tests/oracle.rs @@ -5,7 +5,7 @@ use improvise_core::formula::ast::AggFunc; use improvise_core::formula::parse_formula; use improvise_core::model::cell::{CellKey, CellValue}; use improvise_core::model::{Aggregation, Model}; -use improvise_core::view::{Axis, View}; +use improvise_core::view::{Axis, GridLayout, View}; use improvise_incremental::{ CellDelta, DeltaValue, Derived, Divergence, Eager, Edit, Maintained, Probe, Reading, cell_delta, differential, differential_in, drawn_probes, probes, same_value, @@ -220,7 +220,7 @@ fn eager_matches_the_evaluator_on_the_shipped_documents() { direct.recompute_formulas(&none); let mut asked = probes(eager.model()); assert!(!asked.is_empty(), "{name}: no probes"); - asked.extend(drawn_probes(eager.model(), &views)); + asked.extend(drawn_probes(eager.model(), &views, &none)); for probe in asked { let want = match &probe { Probe::Value(k, a) => direct.evaluate_aggregated(k, *a), @@ -232,8 +232,16 @@ fn eager_matches_the_evaluator_on_the_shipped_documents() { } Probe::Occupancy(p, r) => { let drawn = |(c, i): &(String, String)| r.covers(c, i); + let names = |k: &CellKey| { + p.0.iter().all(|(c, i)| { + let held = k.get(c); + let members = direct.category(c).map(|x| x.items_in_group(i)); + held == Some(i.as_str()) + || held.is_some_and(|h| members.is_some_and(|m| m.contains(&h))) + }) + }; let stored = direct.data.iter_cells().map(|(k, _)| k); - let n = stored.filter(|k| k.matches_partial(&p.0) && k.0.iter().all(drawn)); + let n = stored.filter(|k| names(k) && k.0.iter().all(drawn)); Some(CellValue::Number(n.count() as f64)) } }; @@ -758,6 +766,13 @@ fn wide_workbook() -> improvise_core::workbook::Workbook { wb } +/// The None categories of the active view, virtual ones included: the set an +/// `Eager` standing for that view is built with. +fn none_of(wb: &improvise_core::workbook::Workbook) -> Vec { + let none = wb.active_view().categories_on(Axis::None); + none.into_iter().map(String::from).collect() +} + fn wide_cell() -> CellKey { CellKey::new(vec![ ("C0".into(), "a".into()), @@ -801,7 +816,7 @@ impl Maintained for WrongAt { #[test] fn the_harness_checks_a_cell_a_view_draws_that_drops_four_coordinates() { let wb = wide_workbook(); - let none: Vec = ["C1", "C2", "C3", "C4"].map(String::from).to_vec(); + let none = none_of(&wb); let rival = WrongAt(Eager::new(wb.model.clone(), none.clone()), wide_cell()); let views: Vec<_> = wb.views.values().cloned().collect(); let d = differential_in(&wb.model, &views, &none, rival, &[]).unwrap_err(); @@ -888,16 +903,52 @@ impl Maintained for Blind { fn run_blind(fault: Option) -> Result { let wb = hidden_workbook(); - let none = vec!["_Measure".to_string()]; + let edit = Edit::SetCell(key("West", "A", "Revenue"), CellValue::Number(6.0)); + run_blind_on(&wb, fault, edit) +} + +fn run_blind_on( + wb: &improvise_core::workbook::Workbook, + fault: Option, + edit: Edit, +) -> Result { + let none = none_of(wb); let eager = Eager::new(wb.model.clone(), none.clone()); let views: Vec<_> = wb.views.values().cloned().collect(); - let edit = Edit::SetCell(key("West", "A", "Revenue"), CellValue::Number(6.0)); match fault { Some(f) => differential_in(&wb.model, &views, &none, Blind(eager, f), &[edit]), None => differential_in(&wb.model, &views, &none, eager, &[edit]), } } +/// Nothing is hidden and Margin is a ratio: the pooled total of Margin is +/// Revenue over Cost summed across the rows, which no single row's fold gives. +fn ratio_workbook() -> improvise_core::workbook::Workbook { + let mut wb = improvise_core::workbook::Workbook::new("ratio"); + for c in ["Region", "Product"] { + wb.add_category(c).unwrap(); + } + wb.add_item("Region", "East").unwrap(); + wb.add_item("Region", "West").unwrap(); + wb.add_item("Product", "A").unwrap(); + for m in ["Revenue", "Cost"] { + wb.add_item("_Measure", m).unwrap(); + } + for (r, rev, cost) in [("East", 10.0, 4.0), ("West", 5.0, 2.0)] { + wb.model + .set_cell(key(r, "A", "Revenue"), CellValue::Number(rev)); + wb.model + .set_cell(key(r, "A", "Cost"), CellValue::Number(cost)); + } + wb.model + .add_formula(parse_formula("Margin = Revenue / Cost", "_Measure").unwrap()); + let view = wb.active_view_mut(); + view.set_axis("Region", Axis::Row); + view.set_axis("_Measure", Axis::Column); + view.set_axis("Product", Axis::None); + wb +} + #[test] fn the_harness_passes_a_view_reader_that_is_right() { run_blind(None).unwrap(); @@ -918,8 +969,18 @@ fn the_harness_catches_a_total_that_counts_a_hidden_item() { #[test] fn the_harness_catches_a_total_that_does_not_span_its_rows() { - let d = run_blind(Some(Blindness::TotalsDoNotSpan)).unwrap_err(); + let edit = Edit::SetCell(key("East", "A", "Cost"), CellValue::Number(5.0)); + let run = |fault| run_blind_on(&ratio_workbook(), fault, edit.clone()); + run(None).unwrap(); + let d = run(Some(Blindness::TotalsDoNotSpan)).unwrap_err(); assert!(matches!(*d.probe, Probe::Total(..)), "{d}"); + assert_eq!(measure_of(&d.probe), Some("Margin"), "{d}"); + let pooled = 15.0 / 6.0; + assert_eq!( + (d.step, d.eager.clone(), d.candidate.clone()), + (0, num(pooled), None), + "{d}" + ); } #[test] @@ -932,7 +993,7 @@ fn the_harness_catches_an_occupancy_that_counts_a_hidden_item() { fn a_view_is_drawn_as_cells_totals_and_occupancy() { let wb = hidden_workbook(); let views: Vec<_> = wb.views.values().cloned().collect(); - let drawn = drawn_probes(&wb.model, &views); + let drawn = drawn_probes(&wb.model, &views, &none_of(&wb)); let kinds = [ drawn.iter().any(|p| matches!(p, Probe::Covered(..))), drawn.iter().any(|p| matches!(p, Probe::Total(..))), @@ -941,19 +1002,155 @@ fn a_view_is_drawn_as_cells_totals_and_occupancy() { assert_eq!(kinds, [true; 3]); } +fn coastal_line() -> CellKey { + CellKey::new(vec![("Region".into(), "Coastal".into())]) +} + #[test] -fn probing_a_key_of_twenty_six_coordinates_is_not_exponential() { - let coords = (0..26) - .map(|i| (format!("C{i:02}"), "a".to_string())) - .collect(); - let mut m = Model::new("deep"); - m.set_cell(CellKey::new(coords), CellValue::Number(1.0)); - let started = std::time::Instant::now(); - let n = probes(&m).len(); - assert!(n > 0); +fn a_collapsed_groups_occupancy_counts_its_visible_members() { + let wb = hidden_workbook(); + let eager = Eager::new(wb.model.clone(), none_of(&wb)); + let reading = Reading::of_view(wb.active_view(), &none_of(&wb)); + assert_eq!(eager.occupancy(&coastal_line(), &reading), 2); + let east = CellKey::new(vec![("Region".into(), "East".into())]); + assert_eq!(eager.occupancy(&east, &reading), 0, "East is hidden"); +} + +fn drawn_of(wb: &improvise_core::workbook::Workbook) -> Vec { + let views: Vec<_> = wb.views.values().cloned().collect(); + drawn_probes(&wb.model, &views, &none_of(wb)) +} + +fn occupancy_keys(probes: &[Probe]) -> Vec { + let keyed = |p: &Probe| match p { + Probe::Occupancy(k, _) => Some(k.clone()), + _ => None, + }; + probes.iter().filter_map(keyed).collect() +} + +fn line(coords: &[(&str, &str)]) -> CellKey { + CellKey::new( + coords + .iter() + .map(|(c, i)| (c.to_string(), i.to_string())) + .collect(), + ) +} + +/// What the grid's own evaluator reads for every cell and total each view draws. +#[test] +fn eager_reads_what_the_grid_evaluator_reads() { + let mut compared = 0; + for (name, model, none, views) in corpus() { + let eager = Eager::new(model, none.clone()); + for view in &views { + let layout = GridLayout::with_frozen_records(eager.model(), view, None); + let mut sorted = layout.none_cats.clone(); + sorted.sort(); + let mut mine = none.clone(); + mine.sort(); + if layout.is_records_mode() || sorted != mine { + continue; + } + let reading = Reading::of_view(view, &layout.none_cats); + let grid = layout.evaluator(eager.model()); + for (r, c) in + (0..layout.row_count()).flat_map(|r| (0..layout.col_count()).map(move |c| (r, c))) + { + let k = layout.cell_key(r, c).unwrap(); + assert_eq!(eager.covered(&k, &reading), grid.cell(&k), "{name}: {k:?}"); + compared += 1; + } + for c in (0..layout.col_count()).filter(|_| layout.draws_totals()) { + let k = layout.column_key(c).unwrap(); + let want = grid.column_total(c); + assert_eq!( + eager.total(&k, &layout.row_cats, &reading), + want, + "{name}: {k:?}" + ); + compared += 1; + } + } + } + assert!(compared > 100, "{compared}"); +} + +#[test] +fn a_page_selection_narrows_the_line_occupancy_it_is_asked_under() { + let mut wb = hidden_workbook(); + let view = wb.active_view_mut(); + view.set_axis("Region", Axis::Page); + view.set_axis("Product", Axis::Row); + view.set_page_selection("Region", "West"); + let keys = occupancy_keys(&drawn_of(&wb)); assert!( - started.elapsed().as_millis() < 250, - "{:?}", - started.elapsed() + keys.contains(&line(&[("Product", "A"), ("Region", "West")])), + "{keys:?}" + ); + assert!(!keys.contains(&line(&[("Product", "A")])), "{keys:?}"); +} + +#[test] +fn every_row_and_column_line_is_asked_its_occupancy() { + let keys = occupancy_keys(&drawn_of(&hidden_workbook())); + let rows = [ + line(&[("Region", "Inland")]), + line(&[("Region", "Coastal")]), + ]; + assert!(rows.iter().all(|k| keys.contains(k)), "{keys:?}"); + assert!(keys.contains(&line(&[("Product", "B")])), "{keys:?}"); +} + +#[test] +fn a_pruned_view_is_asked_only_about_the_lines_it_draws() { + let mut wb = hidden_workbook(); + wb.add_item("Region", "Empty").unwrap(); + let empty = line(&[("Region", "Empty")]); + assert!( + occupancy_keys(&drawn_of(&wb)).contains(&empty), + "drawn unpruned" + ); + wb.active_view_mut().prune_empty = true; + assert!( + !occupancy_keys(&drawn_of(&wb)).contains(&empty), + "pruned away" ); } + +#[test] +fn a_records_view_is_not_asked_about() { + let mut wb = hidden_workbook(); + let view = wb.active_view_mut(); + view.set_axis("_Index", Axis::Row); + view.set_axis("_Dim", Axis::Column); + for cat in ["Region", "Product", "_Measure"] { + view.set_axis(cat, Axis::None); + } + assert!(view.is_records_layout()); + assert!(drawn_of(&wb).is_empty()); +} + +#[test] +fn no_total_is_asked_where_the_grid_draws_none() { + let mut wb = ratio_workbook(); + let totals = |wb: &improvise_core::workbook::Workbook| { + drawn_of(wb) + .iter() + .filter(|p| matches!(p, Probe::Total(..))) + .count() + }; + assert!(totals(&wb) > 0, "totals are drawn here"); + let view = wb.active_view_mut(); + view.set_axis("_Measure", Axis::Row); + view.set_axis("Region", Axis::Column); + assert_eq!(totals(&wb), 0, "a column mixing measures has no total"); +} + +#[test] +fn a_view_under_another_none_set_is_not_asked() { + let wb = hidden_workbook(); + let views: Vec<_> = wb.views.values().cloned().collect(); + assert!(drawn_probes(&wb.model, &views, &["Region".to_string()]).is_empty()); +}