fix(incremental): a new aggregation reaches the stems that carry its measure; ordered evaluation re-runs the passes over a zero (improvise-shbc)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze

context/code/improvise-incremental.md

@@ -269,17 +269,24 @@ is followed in outline, and the places it is not are the reason for this section
    formulas' dependency graph by name. When it has no cycle and there are fewer formulas than
    passes, one evaluation per cell in dependency order reaches the fixed point eager's passes
    reach (a chain of fifteen links needs fifteen passes and a sixteenth to see no change). It
-   differs from the passes in one place: a cell holding NaN is never equal to itself, so eager's passes
+   differs from the passes in two places. A cell holding NaN is never equal to itself, so eager's
-   never see it settle and call it `circular`, and the cells that read it keep what they computed
+   passes never see it settle and call it `circular`, and the cells that read it keep what they
-   from the NaN. `mark_nan_circular` does the same afterwards. `Evaluation::Plain` always runs
+   computed from the NaN; `mark_nan_circular` does the same afterwards. And eager stops on `==`,
-   the passes; `tests/ordering.rs` compares the two and `tests/formulas.rs` compares both with
+   which calls `-0.0` equal to the `0.0` an early pass read from a stored cell, so a stem with a
-   eager.
+   zero cell can settle on a sign dependency order never sees (a stored `0.0` at `F1`, `F3 = F1`,
+   `F4 = F3 ^ -1`, then `F1 = -0`: eager's `F4` is `inf`). A stem whose ordered evaluation holds any
+   zero is therefore evaluated again by the passes (`holds_a_zero`). `Evaluation::Plain` always
+   runs the passes; `tests/ordering.rs` compares the two after every edit, zero signs included, and
+   `tests/formulas.rs` compares both with eager.
 6. **A formula edit rebuilds the stems.** Adding, removing or replacing a formula (the fingerprint
    of the formula list moves) rebuilds the stems from the store by one pass, clears the cache and
    evaluates every stem, because a formula has a cell at each. Clause 5 wants a formula delta;
    the index is not maintained across a formula edit. `Meter::rebuilds` counts it.
-7. **A new aggregation dirties every stem if a formula reads that measure.** `measure_agg` is not
+7. **A new aggregation dirties the stems it can reach.** `measure_agg` is not a cell, so no change
-   a cell, so no change reaches the readers; `reads_name` asks the formulas.
+   reaches the readers. A fold under a None category takes its aggregation from the `_Measure`
+   coordinate of the key, which for a formula targeting another category comes from the stem and
+   from no name the formula reads, so every live stem carrying that measure is dirtied
+   (`Stems::carrying`). Where a formula names the measure (it targets `_Measure`) `reads_name` dirties every stem.
 8. **A write of the same bits changes nothing; a write of `-0.0` over `0.0` does.** The cell delta
    treats the two as one value (clause 3), but a formula may not (`Units ^ -1` is `inf` and
    `-inf`), so the formula side reads the raw change log, not the canonical delta.

context/test-evidence/improvise-incremental.md

@@ -460,7 +460,6 @@ What each fault was and the test that names it:
   `a_cell_edit_evaluates_the_one_stem_that_reads_it_whatever_the_table_size`,
   `every_formula_in_the_pool_is_added_edited_around_and_removed_like_eager` and six others;
 - a stem that no cell gives keeps its cells: `a_new_record_adds_its_stem_and_a_cleared_one_takes_it_away`;
-- reads skip the formulas a name targets: `formulas::tests::what_a_formula_reads_includes_what_the_formulas_it_names_read`;
 - reads skip the filters' categories: `a_filter_on_a_category_the_stem_has_reads_the_cells_of_other_stems`;
 - reads skip the formula's own target name:
   `a_new_aggregation_for_the_measure_a_filtered_formula_falls_back_to_changes_it`;
@@ -488,7 +487,10 @@ What each fault was and the test that names it:
   the cache: `every_formula_in_the_pool_is_added_edited_around_and_removed_like_eager`,
   `incremental_matches_eager_on_a_pooled_total_of_a_ratio`.
 
-**Faults that cannot be seen, and why.** A born stem is found again by the lookup of the cell that
+**Faults that cannot be seen, and why.** Reads that skip the formulas a name targets change no answer
+either (only the unit test `formulas::tests::what_a_formula_reads_includes_what_the_formulas_it_names_read`
+fails): a stem is evaluated whole, so whatever the skipped formula would have added is
+re-evaluated because the formula itself dirties the stem. A born stem is found again by the lookup of the cell that
 gave it, since the cell agrees with the stem outside the target category, so skipping the extra
 "born means dirty" step changes nothing. The target category being in a formula's reads cannot
 matter either: stems are derived by removing it, so no stem has it to drop.
@@ -632,3 +634,24 @@ Both zeros in both orders. Fails on `Max` negative if any zero is.
 **Speed.** The corpus self-comparison and the maintained-against-fresh property moved to their own
 binaries (`tests/corpus_eager.rs`, `tests/maintained.rs`): `oracle.rs` ran 2.45 to 3.04 s in loaded full
 runs because its two heaviest tests shared it. The tests are unchanged.
+
+### Review of PR #351: aggregation edits, zero signs (improvise-shbc)
+
+### a_new_aggregation_reaches_the_stems_of_a_formula_that_targets_another_category
+
+`R0 = East + West` on Region, Time folded, `SetMeasureAgg Revenue Max`: eager reads 13 (the maximum of
+East plus the maximum of West), the first version kept 14. Fails when a new aggregation dirties only
+the stems of formulas that name the measure, and when `Stems::carrying` ignores the measure.
+
+### a_new_aggregation_reaches_a_product_targeted_filtered_sum
+
+The reviewer's second repro, a Product-targeted `SUM(F0 WHERE Region = East)`. It already agreed on this
+fixture and stays as the second shape of the same hazard.
+
+### ordered_evaluation_keeps_the_zero_eager_settles_on
+
+A stored `0.0` at `F1`, then `F3 = F1`, `F4 = F3 ^ -1`, then `F1 = -0`: eager settles on `F3 = 0.0` and `F4 = inf`
+because it calls `-0.0` equal to the `0.0` it first read; the first ordered evaluation gave `-inf`. Fails
+(only `Ordered`) when a stem with a zero is not re-evaluated by the passes. The ordering property's pool
+now holds `-0`, `Units * -1` and `-Cost`, and compares after every edit, but did not find this fault
+alone in 1,500 cases: this test is the pin.

crates/improvise-incremental/src/formulas.rs

@@ -3,6 +3,7 @@
 
 use improvise_core::formula::ast::Expr;
 use improvise_core::model::Model;
+use improvise_core::model::category::MEASURE_CATEGORY;
 use improvise_core::model::cell::{CellChange, CellKey};
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 
@@ -255,6 +256,15 @@ impl Stems {
         self.live.keys()
     }
 
+    /// The live stems whose own `_Measure` coordinate is `measure`: a fold under a None category
+    /// takes its aggregation from that coordinate, whether or not a formula names the measure.
+    pub fn carrying(&self, measure: &str) -> impl Iterator<Item = CellKey> + '_ {
+        let measure = measure.to_string();
+        self.live()
+            .filter(move |s| s.get(MEASURE_CATEGORY) == Some(measure.as_str()))
+            .cloned()
+    }
+
     /// Whether any formula reads a cell under `name`, directly or through another formula.
     pub fn reads_name(&self, name: &str) -> bool {
         self.reads.iter().any(|r| r.names.contains(name))

crates/improvise-incremental/src/incremental.rs

@@ -204,10 +204,11 @@ impl Incremental {
             self.clear_stem(stem);
         }
         let mut dirty = moved.dirty;
-        if let Edit::SetMeasureAgg { measure, .. } = edit
+        if let Edit::SetMeasureAgg { measure, .. } = edit {
-            && self.stems.reads_name(measure)
+            dirty.extend(self.stems.carrying(measure));
-        {
+            if self.stems.reads_name(measure) {
-            dirty.extend(self.stems.live().cloned());
+                dirty.extend(self.stems.live().cloned());
+            }
         }
         self.evaluate(dirty);
     }
@@ -253,7 +254,14 @@ impl Incremental {
                     let value = self.evaluate_cell(&keys[i]);
                     self.model.set_formula_cell(keys[i].clone(), value);
                 }
-                self.mark_nan_circular(&keys);
+                if self.holds_a_zero(&keys) {
+                    for key in &keys {
+                        self.model.set_formula_cell(key.clone(), None);
+                    }
+                    self.passes(&keys);
+                } else {
+                    self.mark_nan_circular(&keys);
+                }
             }
             _ => self.passes(&keys),
         }
@@ -286,6 +294,13 @@ impl Incremental {
         }
     }
 
+    /// Eager stops on `==`, which calls `-0.0` equal to the `0.0` an early pass read, so a stem
+    /// with a zero cell can settle on a sign the dependency order never sees: it runs the passes.
+    fn holds_a_zero(&self, keys: &[CellKey]) -> bool {
+        let zero = |k: &CellKey| matches!(self.model.formula_cell(k), Some(CellValue::Number(n)) if *n == 0.0);
+        keys.iter().any(zero)
+    }
+
     /// A NaN is not equal to itself, so eager's passes never see it settle and call it circular
     /// once they run out; the cells that read it keep what they computed from the NaN.
     fn mark_nan_circular(&mut self, keys: &[CellKey]) {

crates/improvise-incremental/tests/formulas.rs

@@ -292,3 +292,109 @@ fn a_new_record_adds_its_stem_and_a_cleared_one_takes_it_away() {
     ];
     same_model(&wb.model, &none, &edits, Evaluation::Ordered).unwrap();
 }
+
+/// Region {East, West}, a Time category folded away, Revenue by quarter: `R0 = East + West`
+/// targets Region, so its stems carry the measure but no formula names it.
+fn region_target_model() -> Model {
+    let mut model = Model::new("regions");
+    for (cat, items) in [("Region", vec!["East", "West"]), ("Time", vec!["Q1", "Q2"])] {
+        model.add_category(cat).unwrap();
+        for item in items {
+            model.category_mut(cat).unwrap().add_item(item);
+        }
+    }
+    model.category_mut("_Measure").unwrap().add_item("Revenue");
+    for (region, quarter, v) in [
+        ("East", "Q1", 1.0),
+        ("East", "Q2", 3.0),
+        ("West", "Q1", 10.0),
+    ] {
+        let key = CellKey::new(vec![
+            ("Region".into(), region.into()),
+            ("Time".into(), quarter.into()),
+            ("_Measure".into(), "Revenue".into()),
+        ]);
+        model.set_cell(key, CellValue::Number(v));
+    }
+    model.add_formula(parse_formula("R0 = East + West", "Region").unwrap());
+    model
+}
+
+#[test]
+fn a_new_aggregation_reaches_the_stems_of_a_formula_that_targets_another_category() {
+    let model = region_target_model();
+    let none = vec!["Time".to_string()];
+    let edits = [Edit::SetMeasureAgg {
+        measure: "Revenue".into(),
+        func: improvise_core::formula::ast::AggFunc::Max,
+    }];
+    for by in [Evaluation::Ordered, Evaluation::Plain] {
+        same_model(&model, &none, &edits, by).unwrap();
+    }
+    let mut backend = Incremental::new(model, none);
+    backend.apply(&edits[0]);
+    let at = CellKey::new(vec![
+        ("Region".into(), "R0".into()),
+        ("_Measure".into(), "Revenue".into()),
+    ]);
+    let want = Some(CellValue::Number(13.0));
+    assert_eq!(
+        backend.value(&at, Aggregation::OverHidden),
+        want,
+        "Max 3 + Max 10"
+    );
+}
+
+#[test]
+fn a_new_aggregation_reaches_a_product_targeted_filtered_sum() {
+    let mut model = Model::new("products");
+    for (cat, items) in [("Region", vec!["East", "West"]), ("Product", vec!["F0"])] {
+        model.add_category(cat).unwrap();
+        for item in items {
+            model.category_mut(cat).unwrap().add_item(item);
+        }
+    }
+    model.category_mut("_Measure").unwrap().add_item("Cost");
+    for (region, v) in [("East", 4.0), ("West", 9.0)] {
+        let key = CellKey::new(vec![
+            ("Region".into(), region.into()),
+            ("Product".into(), "F0".into()),
+            ("_Measure".into(), "Cost".into()),
+        ]);
+        model.set_cell(key, CellValue::Number(v));
+    }
+    model.add_formula(parse_formula("P0 = SUM(F0 WHERE Region = East)", "Product").unwrap());
+    let none = vec![];
+    let edits = [Edit::SetMeasureAgg {
+        measure: "Cost".into(),
+        func: improvise_core::formula::ast::AggFunc::Min,
+    }];
+    for by in [Evaluation::Ordered, Evaluation::Plain] {
+        same_model(&model, &none, &edits, by).unwrap();
+    }
+}
+
+/// A stored `0.0` at `F1`, then `F3 = F1`, `F4 = F3 ^ -1` and last `F1 = -0`: eager's first
+/// pass reads the stored zero, and its later passes call `-0.0` equal to it and stop.
+#[test]
+fn ordered_evaluation_keeps_the_zero_eager_settles_on() {
+    let mut model = Model::new("zeros");
+    model.add_category("Region").unwrap();
+    model.category_mut("Region").unwrap().add_item("East");
+    let key = CellKey::new(vec![
+        ("Region".into(), "East".into()),
+        ("_Measure".into(), "F1".into()),
+    ]);
+    model.set_cell(key, CellValue::Number(0.0));
+    let edits: Vec<Edit> = [
+        ("F3 = F1", "_Measure"),
+        ("F4 = F3 ^ -1", "_Measure"),
+        ("F1 = -0", "_Measure"),
+    ]
+    .iter()
+    .map(|(raw, cat)| Edit::AddFormula(parse_formula(raw, cat).unwrap()))
+    .collect();
+    for by in [Evaluation::Ordered, Evaluation::Plain] {
+        same_model(&model, &[], &edits, by).unwrap_or_else(|e| panic!("{by:?}: {e}"));
+    }
+}

crates/improvise-incremental/tests/support/mod.rs

@@ -238,7 +238,7 @@ pub fn hidden_counted_workbook() -> improvise_core::workbook::Workbook {
 
 /// Formulas the random tests add and remove, as `(text, target category)`: chains, a cycle,
 /// aggregates over expressions, filters, a ratio, and two that target Region.
-pub const FORMULA_POOL: [(&str, &str); 18] = [
+pub const FORMULA_POOL: [(&str, &str); 21] = [
     ("Profit = Revenue - Cost", "_Measure"),
     ("Margin = Profit / Revenue", "_Measure"),
     ("Weighted = SUM(Revenue * Units)", "_Measure"),
@@ -257,6 +257,9 @@ pub const FORMULA_POOL: [(&str, &str); 18] = [
     ("EastRev = Revenue", "Region"),
     ("Recip = Units ^ -1", "_Measure"),
     ("Pa = A + Revenue", "_Measure"),
+    ("NegZero = -0", "_Measure"),
+    ("Flip = Units * -1", "_Measure"),
+    ("Negate = -Cost", "_Measure"),
 ];
 
 pub fn formula_edit(index: usize) -> Edit {
@@ -371,23 +374,24 @@ pub fn any_formula_edit() -> impl Strategy<Value = Edit> {
     ]
 }
 
-/// The same edits told to a backend that evaluates in dependency order and to one that always
+/// The same edits told to a backend that evaluates in dependency order and to one that runs
-/// runs eager's passes: their models, formula cache included, must print alike.
+/// eager's passes: models, cache and zero signs must print alike after every edit.
 pub fn same_orders(model: &Model, none: &[String], edits: &[Edit]) -> Result<(), String> {
     let side = |by| Incremental::new(model.clone(), none.to_vec()).with_evaluation(by);
     let (mut ordered, mut plain) = (side(Evaluation::Ordered), side(Evaluation::Plain));
-    for edit in edits {
+    for (n, edit) in edits.iter().enumerate() {
         ordered.apply(edit);
         plain.apply(edit);
+        let (want, got) = (
+            format!("{:?}", plain.model()),
+            format!("{:?}", ordered.model()),
+        );
+        if want != got {
+            let told = &edits[..=n];
+            return Err(format!("after {told:?}:\n plain {want}\n ordered {got}"));
+        }
     }
-    let (want, got) = (
+    Ok(())
-        format!("{:?}", plain.model()),
-        format!("{:?}", ordered.model()),
-    );
-    match want == got {
-        true => Ok(()),
-        false => Err(format!("after {edits:?}:\n plain {want}\n ordered {got}")),
-    }
 }
 
 /// An `Eager` rebuilt from scratch on every edit: maintaining one in place