From 9f035f918e4cf4c0618bedaae9f7047d262049ee Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-backend)" Date: Sat, 10 Oct 2026 16:37:00 -0700 Subject: [PATCH 1/2] fix(core): Min/Max, NaN and signed zero follow the math, not the old fold (improvise-bw4p.20) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/improvise-incremental.md | 38 +++++- .../test-evidence/improvise-incremental.md | 31 ++++- crates/improvise-core/src/model/types.rs | 119 +++++++++++++++++- crates/improvise-incremental/src/group.rs | 23 ++-- .../improvise-incremental/src/incremental.rs | 30 +---- .../improvise-incremental/tests/formulas.rs | 47 +++---- crates/improvise-incremental/tests/random.rs | 3 +- .../tests/support/mod.rs | 3 +- crates/improvise-incremental/tests/zeros.rs | 22 +++- 9 files changed, 222 insertions(+), 94 deletions(-) diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index 47a8f139..ab795434 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -175,7 +175,7 @@ taken where clause 4 and clause 7 leave a choice; evidence is in `SetMeasureAgg` rebuilds nothing. A read of several groups (a collapsed group, a total) folds their union without merging them: `fold_groups` sums the counts and takes the smallest of the groups' minima, the largest of their maxima, or the one value they share. -4. **Eager's rules are reproduced, not improved.** An all-numbers group reduces; any other group +4. **Eager's rules are reproduced, not improved** (superseded for NaN and zeros by "Eager's math, fixed" below). An all-numbers group reduces; any other group is unanimous or `Error("mixed")`, so `Count` of a group holding text is that text. `Min` and `Max` skip NaN and are `+inf`/`-inf` over only NaN, as `fold(INFINITY, f64::min)` is. The multiset orders by the delta's total order, with NaN last, so the skip is a step past one @@ -265,7 +265,7 @@ is followed in outline, and the places it is not are the reason for this section cell under the stem changes, not adjusted for the one record. That needs the evaluator to accept a maintained aggregate in place of folding, which is a change to core's evaluator and to the order its records are visited in (insertion order, not `CellKey` order). It is a bead. -5. **Dependency order is an optimisation and is checked against the plain passes.** `Order` is the +5. **Dependency order is an optimisation and is checked against the plain passes** (the NaN and zero differences below are gone: "Eager's math, fixed"). `Order` is the 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 @@ -352,3 +352,37 @@ of that step, are gone; formula-owned keys still go to the model, whose cache st - The hidden overlay and kept totals (clause 7). - A group that does not pay for a sum it never reads (item 6). + +## Eager's math, fixed (user ruling 2026-10-10) + +"Don't reproduce math bugs you might find in the Eager mode." Every place the earlier steps matched +Eager's arithmetic "as eager does" was reviewed. The items below supersede the sentences in steps 4 and 5 +that say otherwise (step 4 items 4 and 10, step 5 item 5). Each is a user-visible numeric change. + +1. **Min and Max of both zeros** is `-0.0` and `+0.0`, whatever the order or the platform. Core's fold used + `f64::min` and `f64::max`, whose tie between the zeros is left to the platform; it now orders by + `total_cmp` over the non-NaN values. Incremental already returned this. (improvise-bw4p.20) +2. **Min and Max over only NaN** are NaN, where they were `+inf` and `-inf` (the seed of the old fold leaking + out). NaN among numbers is still skipped. +3. **A NaN formula result is a value, not a cycle.** The fixed point compared cached cells with `!=`, so a + cell that settled on NaN never settled and became `circular`, dragging the cells that read it. It now + compares with `model::types::settled`: numbers by bits, every NaN one value. A cell that really oscillates + is still `circular` after sixteen passes. `mark_nan_circular` is gone. +4. **A zero's sign is a change.** `settled` compares by bits, so `0.0` becoming `-0.0` between passes moves + the cell, and eager's fixed point and dependency order's agree. Dependency order is the unique fixed point + of an acyclic set under bit equality (each cell is a function of the final values it reads), so the + `holds_a_zero` fallback is dropped and `Ordered` needs no extra precondition. +5. **Empty `SUM`** is `+0.0` (step 6), listed here for completeness. + +### Left alone, with the reasoning + +- **`Count` of a group that holds text** returns the text (the unanimity branch of + `aggregate_cell_values`), not the number of cells. That is probably not what a user wants, but it is a + decision about what a `Count` measure means for non-numbers, not an arithmetic error, so it is filed + rather than changed. +- **The division and equality epsilon** (`FLOAT_EQ_EPSILON`, 1e-10) is shared and deliberate (gotcha 17). +- **A group holding NaN beside text** is `mixed`: NaN is a number and text is not, so no unanimity exists. +- **`Avg` and `Sum`** are already exact and correctly rounded (step 6). +- **Overflow of the exact sum to infinity** follows `ExactSum::value` (round to nearest, so an overflowing + total is infinite): correct IEEE behaviour. + diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index b2b4cf0e..2f444241 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -314,8 +314,8 @@ because they change nothing observable (below); every other is named by at least `group::tests::an_emptied_group_is_absent_and_keeps_no_entries`, `incremental_matches_eager_on_the_shipped_documents`, `incremental_equals_eager_over_random_edit_sequences`; - a group never counts its numbers: `a_group_read_comes_from_a_maintained_group`, - `group::tests::min_and_max_skip_nan_and_are_infinite_over_nothing_else` and eight differentials; -- `Max` takes the first key: `group::tests::min_and_max_skip_nan_and_are_infinite_over_nothing_else`, + `group::tests::min_and_max_skip_nan_and_are_nan_over_nothing_else` and eight differentials; +- `Max` takes the first key: `group::tests::min_and_max_skip_nan_and_are_nan_over_nothing_else`, `an_overwrite_costs_two_updates_per_shape_whatever_the_table_size` and five differentials; - `Count` counts one: `a_collapsed_group_reads_one_group_per_member_it_covers`, `group::tests::*`; - unanimity ignores disagreement: `group::tests::text_is_unanimous_or_mixed_across_groups`, @@ -437,7 +437,7 @@ A multiset that ignores a negative weight keeps the minimum it should have lost. After the last retraction the group reads as absent and its map holds no zero-weight entry. -### group::tests::min_and_max_skip_nan_and_are_infinite_over_nothing_else +### group::tests::min_and_max_skip_nan_and_are_nan_over_nothing_else NaN is skipped by `Min` and `Max`, a group of only NaN gives `+inf` and `-inf` and still counts. @@ -472,7 +472,7 @@ What each fault was and the test that names it: `a_formula_that_keeps_moving_for_sixteen_passes_is_circular`, `dependency_order_and_the_plain_passes_leave_the_same_cache`; - dependencies after dependents: `dependency_order_and_the_plain_passes_leave_the_same_cache` and five more; -- NaN is not marked circular: `a_formula_cell_holding_nan_is_circular_and_the_cells_that_read_it_keep_the_nan_they_read`; +- NaN is not marked circular: `a_formula_cell_holding_nan_is_nan_and_not_circular`; - fifteen passes / seventeen passes: `a_chain_added_last_link_first_is_circular_exactly_where_eager_says`; - the passes end without marking the cells still moving, or begin from the old cache: `a_chain_added_last_link_first_is_circular_exactly_where_eager_says`, @@ -557,7 +557,7 @@ the whole recompute. `IF(Revenue > 1000, B, Revenue)`: at the stem of a big product it reads the cache of the stem of B, which is evaluated after it. Fails when such a formula set is not wide. -### a_formula_cell_holding_nan_is_circular_and_the_cells_that_read_it_keep_the_nan_they_read +### a_formula_cell_holding_nan_is_nan_and_not_circular NaN is not equal to itself, so eager's passes never see it settle. Fails when the ordered path leaves it. @@ -643,7 +643,7 @@ runs because its two heaviest tests shared it. The tests are unchanged. 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 +### a_new_aggregation_reaches_a_filtered_sum_on_the_fallback_path 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. @@ -766,3 +766,22 @@ average of a rounded sum. Infinities and NaN read as `ExactSum::value` does, and a zero keeps its sign. +## Eager's math, fixed (follow-up to steps 4 to 6) + +Ruling (user, 2026-10-10): do not reproduce math bugs found in Eager; fix Eager. Eight faults were +injected (six in the table below, two more in the groups' NaN handling); `nan-never-settles` survived the +first pass because NaN produced by one evaluation always has the same bits, and is closed by a unit test of +`settled`. Each fix has a regression test named after the bug: +- `model::types::eager_math_tests::min_and_max_of_both_zeros_do_not_depend_on_their_order` and + `with_both_zeros_min_is_negative_and_max_is_positive_whatever_the_order`: a naive `f64::min` fold + passes on ARM and fails on other platforms, so the rival is a fold that keeps the first of two zeros; +- `model::types::eager_math_tests::min_and_max_over_only_nan_are_nan_not_infinite`, + `min_and_max_over_only_nan_are_nan_in_both_backends`, `group::tests::min_and_max_skip_nan_and_are_nan_over_nothing_else`; +- `model::types::eager_math_tests::a_formula_that_settles_on_nan_is_nan_and_not_circular`, + `a_formula_cell_holding_nan_is_nan_and_not_circular`, `settled_treats_every_nan_as_one_value_and_the_zeros_as_two`; +- `model::types::eager_math_tests::a_zero_that_changes_sign_between_passes_settles_on_the_new_sign` and + `ordered_evaluation_keeps_the_zero_eager_settles_on` (now with the sign settled by bits, no fallback); +- `model::types::eager_math_tests::a_formula_that_never_settles_is_still_circular`: the guard that a real + oscillation is still `circular`. +The random and differential suites compare numbers by bits and their pools hold both zeros again. + diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 83273f24..9c87fb1b 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -32,14 +32,32 @@ fn exact(values: &[f64]) -> numtower::ExactSum { sum } +/// The least of `values` under `order`, skipping NaN; NaN if nothing else is left. Zeros order +/// by sign (`-0.0` below `+0.0`), so the answer does not depend on the order or the platform. +fn extreme(values: &[f64], order: impl Fn(&f64, &f64) -> std::cmp::Ordering) -> f64 { + let numbers = values.iter().copied().filter(|v| !v.is_nan()); + numbers.min_by(order).unwrap_or(f64::NAN) +} + +/// Whether two cached cells are the same value for a fixed point: numbers by bits, so a zero +/// that changes sign is a change, except that every NaN is one value, so a NaN can settle. +pub fn settled(a: Option<&CellValue>, b: Option<&CellValue>) -> bool { + match (a, b) { + (Some(CellValue::Number(x)), Some(CellValue::Number(y))) => { + x.to_bits() == y.to_bits() || (x.is_nan() && y.is_nan()) + } + _ => a == b, + } +} + /// Reduce raw values. `None` on empty: an empty cell reads as **absent**, not /// as zero. Both aggregation paths share it (improvise-kdp, core.md §2). fn apply_agg(values: &[f64], func: &AggFunc) -> Option { (!values.is_empty()).then(|| match func { AggFunc::Sum => exact_sum(values), AggFunc::Avg => exact_mean(values), - AggFunc::Min => values.iter().copied().fold(f64::INFINITY, f64::min), - AggFunc::Max => values.iter().copied().fold(f64::NEG_INFINITY, f64::max), + AggFunc::Min => extreme(values, f64::total_cmp), + AggFunc::Max => extreme(values, |a, b| b.total_cmp(a)), AggFunc::Count => values.len() as f64, }) } @@ -1046,7 +1064,7 @@ impl Model { // whatever the cache already holds. let new_val = self.evaluate_formula_cell(&key, none_cats); let old_val = self.formula_cache.get(&key); - if old_val != new_val.as_ref() { + if !settled(old_val, new_val.as_ref()) { moving.push(key.clone()); if let Some(v) = new_val { self.formula_cache.insert(key, v); @@ -3709,3 +3727,98 @@ mod exact_sum_tests { assert_eq!(got.to_bits(), (1.0f64 / 3.0).to_bits()); } } + +/// Eager's math, fixed (ADR 009 follow-up): none of these depends on the order, the platform or +/// on NaN being unequal to itself. +#[cfg(test)] +mod eager_math_tests { + use super::*; + use crate::formula::parse_formula; + + fn bits(v: Option) -> Option { + v.map(f64::to_bits) + } + + #[test] + fn min_and_max_of_both_zeros_do_not_depend_on_their_order() { + for values in [[0.0, -0.0], [-0.0, 0.0]] { + let min = apply_agg(&values, &AggFunc::Min); + let max = apply_agg(&values, &AggFunc::Max); + assert_eq!(bits(min), Some((-0.0f64).to_bits()), "{values:?}"); + assert_eq!(bits(max), Some(0.0f64.to_bits()), "{values:?}"); + } + } + + #[test] + fn min_and_max_over_only_nan_are_nan_not_infinite() { + for func in [AggFunc::Min, AggFunc::Max] { + let got = apply_agg(&[f64::NAN, -f64::NAN], &func).unwrap(); + assert!(got.is_nan(), "{func:?} gave {got}"); + } + assert_eq!(apply_agg(&[f64::NAN, 2.0], &AggFunc::Min), Some(2.0)); + assert_eq!(apply_agg(&[f64::NAN, 2.0], &AggFunc::Max), Some(2.0)); + } + + fn model_with(cells: &[(&str, f64)], formulas: &[&str]) -> Model { + let mut m = Model::new("m"); + m.add_category("Region").unwrap(); + m.category_mut("Region").unwrap().add_item("East"); + for (measure, v) in cells { + m.category_mut("_Measure").unwrap().add_item(*measure); + let key = CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), (*measure).into()), + ]); + m.set_cell(key, CellValue::Number(*v)); + } + for raw in formulas { + m.add_formula(parse_formula(raw, "_Measure").unwrap()); + } + m.recompute_formulas(&[]); + m + } + + fn at(m: &Model, measure: &str) -> Option { + let key = CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), measure.into()), + ]); + m.evaluate_aggregated(&key, Aggregation::OverHidden) + } + + #[test] + fn a_formula_that_settles_on_nan_is_nan_and_not_circular() { + let m = model_with(&[("Revenue", f64::NAN)], &["Profit = Revenue * 1"]); + let Some(CellValue::Number(n)) = at(&m, "Profit") else { + panic!("a NaN, not {:?}", at(&m, "Profit")); + }; + assert!(n.is_nan()); + } + + #[test] + fn a_zero_that_changes_sign_between_passes_settles_on_the_new_sign() { + let m = model_with(&[("F1", 0.0)], &["F3 = F1", "F4 = F3 ^ -1", "F1 = -0"]); + let f4 = at(&m, "F4"); + assert_eq!(f4, Some(CellValue::Number(f64::NEG_INFINITY))); + let Some(CellValue::Number(f3)) = at(&m, "F3") else { + panic!("a number") + }; + assert_eq!(f3.to_bits(), (-0.0f64).to_bits()); + } + + #[test] + fn settled_treats_every_nan_as_one_value_and_the_zeros_as_two() { + let n = |x: f64| Some(CellValue::Number(x)); + assert!(settled(n(f64::NAN).as_ref(), n(-f64::NAN).as_ref())); + assert!(settled(n(1.5).as_ref(), n(1.5).as_ref())); + assert!(!settled(n(0.0).as_ref(), n(-0.0).as_ref())); + assert!(!settled(n(f64::NAN).as_ref(), n(1.0).as_ref())); + assert!(!settled(n(1.0).as_ref(), None)); + } + + #[test] + fn a_formula_that_never_settles_is_still_circular() { + let m = model_with(&[("X", 1.0)], &["X = X + 1"]); + assert_eq!(at(&m, "X"), Some(CellValue::Error("circular".into()))); + } +} diff --git a/crates/improvise-incremental/src/group.rs b/crates/improvise-incremental/src/group.rs index 7f2b6164..e1c2b6b3 100644 --- a/crates/improvise-incremental/src/group.rs +++ b/crates/improvise-incremental/src/group.rs @@ -119,7 +119,7 @@ pub fn fold_groups(groups: &[&Group], func: Reduce) -> Option { } } -/// `Min` and `Max` skip NaN, and are infinite when nothing else is left, as eager's folds are. +/// `Min` and `Max` skip NaN and are NaN when nothing else is left. fn reduce(groups: &[&Group], func: Reduce, cells: i64) -> f64 { match func { Reduce::Count => cells as f64, @@ -127,13 +127,13 @@ fn reduce(groups: &[&Group], func: Reduce, cells: i64) -> f64 { Reduce::Avg => total(groups).mean(cells as u64), Reduce::Min => { let smallest = groups.iter().filter_map(|g| g.smallest()); - let min = smallest.fold(f64::INFINITY, f64::min); + let min = smallest.reduce(f64::min).unwrap_or(f64::NAN); let negative: i64 = groups.iter().map(|g| g.negative_zeros).sum(); signed_zero(min, negative > 0) } Reduce::Max => { let largest = groups.iter().filter_map(|g| g.largest()); - let max = largest.fold(f64::NEG_INFINITY, f64::max); + let max = largest.reduce(f64::max).unwrap_or(f64::NAN); let (zeros, negative): (i64, i64) = ( groups.iter().map(|g| g.zeros()).sum(), groups.iter().map(|g| g.negative_zeros).sum(), @@ -202,19 +202,18 @@ mod tests { } #[test] - fn min_and_max_skip_nan_and_are_infinite_over_nothing_else() { + fn min_and_max_skip_nan_and_are_nan_over_nothing_else() { let some = group(&[num(f64::NAN), num(3.0), num(-1.0)]); assert_eq!(fold_groups(&[&some], Reduce::Min), Some(num(-1.0))); assert_eq!(fold_groups(&[&some], Reduce::Max), Some(num(3.0))); let only_nan = group(&[num(f64::NAN), num(-f64::NAN)]); - assert_eq!( - fold_groups(&[&only_nan], Reduce::Min), - Some(num(f64::INFINITY)) - ); - assert_eq!( - fold_groups(&[&only_nan], Reduce::Max), - Some(num(f64::NEG_INFINITY)) - ); + for func in [Reduce::Min, Reduce::Max] { + let got = fold_groups(&[&only_nan], func); + assert!( + matches!(got, Some(CellValue::Number(n)) if n.is_nan()), + "{func:?}" + ); + } assert_eq!(fold_groups(&[&only_nan], Reduce::Count), Some(num(2.0))); } diff --git a/crates/improvise-incremental/src/incremental.rs b/crates/improvise-incremental/src/incremental.rs index 4f22e943..65211405 100644 --- a/crates/improvise-incremental/src/incremental.rs +++ b/crates/improvise-incremental/src/incremental.rs @@ -9,7 +9,7 @@ use crate::{Derived, Edit, Maintained, Reading, cell_delta}; use improvise_core::formula::ast::AggFunc; use improvise_core::model::category::MEASURE_CATEGORY; use improvise_core::model::cell::{CellChange, CellKey, CellValue}; -use improvise_core::model::types::Covers; +use improvise_core::model::types::{Covers, settled}; use improvise_core::model::{Aggregation, Model}; use std::collections::BTreeMap; @@ -253,14 +253,6 @@ impl Incremental { let value = self.evaluate_cell(&keys[i]); self.model.set_formula_cell(keys[i].clone(), value); } - 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), } @@ -277,7 +269,7 @@ impl Incremental { moving.clear(); for key in keys { let new = self.evaluate_cell(key); - if self.model.formula_cell(key) != new.as_ref() { + if !settled(self.model.formula_cell(key), new.as_ref()) { moving.push(key); self.model .set_formula_cell(key.clone(), new.or_else(|| self.keep(key))); @@ -293,24 +285,6 @@ 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]) { - for key in keys { - if matches!(self.model.formula_cell(key), Some(CellValue::Number(n)) if n.is_nan()) { - let circular = CellValue::Error("circular".into()); - self.model.set_formula_cell(key.clone(), Some(circular)); - } - } - } - /// A cell whose new value is absent keeps the one it has. fn keep(&self, key: &CellKey) -> Option { self.model.formula_cell(key).cloned() diff --git a/crates/improvise-incremental/tests/formulas.rs b/crates/improvise-incremental/tests/formulas.rs index 03cabdd2..04a1fd30 100644 --- a/crates/improvise-incremental/tests/formulas.rs +++ b/crates/improvise-incremental/tests/formulas.rs @@ -236,7 +236,7 @@ fn a_name_in_another_category_reads_another_stems_cell() { } #[test] -fn a_formula_cell_holding_nan_is_circular_and_the_cells_that_read_it_keep_the_nan_they_read() { +fn a_formula_cell_holding_nan_is_nan_and_not_circular() { let wb = formula_workbook(); let none = vec!["Region".to_string()]; let nan = bump("East", "A", "Revenue", f64::NAN); @@ -248,15 +248,13 @@ fn a_formula_cell_holding_nan_is_circular_and_the_cells_that_read_it_keep_the_na ("_Measure".into(), m.into()), ]) }; - let circular = Some(CellValue::Error("circular".into())); - assert_eq!( - backend.value(&at("Profit"), Aggregation::OverHidden), - circular - ); - assert_eq!( - backend.value(&at("Margin"), Aggregation::OverHidden), - circular - ); + for measure in ["Profit", "Margin"] { + let got = backend.value(&at(measure), Aggregation::OverHidden); + assert!( + matches!(got, Some(CellValue::Number(n)) if n.is_nan()), + "{measure}: {got:?}" + ); + } for by in [Evaluation::Ordered, Evaluation::Plain] { same_model(&wb.model, &none, std::slice::from_ref(&nan), by).unwrap(); } @@ -346,36 +344,19 @@ fn a_new_aggregation_reaches_the_stems_of_a_formula_that_targets_another_categor } #[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![]; +fn a_new_aggregation_reaches_a_filtered_sum_on_the_fallback_path() { + let mut model = region_target_model(); + model.add_formula(parse_formula("P0 = SUM(Revenue) WHERE Region = East", "_Measure").unwrap()); + let none = vec!["Time".to_string()]; let edits = [Edit::SetMeasureAgg { - measure: "Cost".into(), - func: improvise_core::formula::ast::AggFunc::Min, + measure: "Revenue".into(), + func: improvise_core::formula::ast::AggFunc::Max, }]; 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"); diff --git a/crates/improvise-incremental/tests/random.rs b/crates/improvise-incremental/tests/random.rs index c9bc0ce1..f88a0ea9 100644 --- a/crates/improvise-incremental/tests/random.rs +++ b/crates/improvise-incremental/tests/random.rs @@ -16,8 +16,7 @@ use support::{ fn pool() -> impl Strategy { prop_oneof![ - // No +0.0: with both zeros in a group eager's Min and Max leave the sign to the platform. - prop_oneof![-3i32..0, 1i32..4].prop_map(|n| CellValue::Number(f64::from(n))), + (-3i32..4).prop_map(|n| CellValue::Number(f64::from(n))), Just(CellValue::Number(-0.0)), Just(CellValue::Number(f64::NAN)), Just(CellValue::Number(-f64::NAN)), diff --git a/crates/improvise-incremental/tests/support/mod.rs b/crates/improvise-incremental/tests/support/mod.rs index acadd571..0ba3d060 100644 --- a/crates/improvise-incremental/tests/support/mod.rs +++ b/crates/improvise-incremental/tests/support/mod.rs @@ -339,8 +339,7 @@ pub fn same_model( fn any_formula_value() -> impl Strategy { prop_oneof![ - // No +0.0 beside -0.0: eager's Min and Max of both zeros is platform-defined. - prop_oneof![-2i32..0, 1i32..4].prop_map(|n| CellValue::Number(f64::from(n))), + (-2i32..4).prop_map(|n| CellValue::Number(f64::from(n))), Just(CellValue::Number(-0.0)), Just(CellValue::Number(f64::NAN)), prop::sample::select(vec![0.1, 1e16, -1e16]).prop_map(CellValue::Number), diff --git a/crates/improvise-incremental/tests/zeros.rs b/crates/improvise-incremental/tests/zeros.rs index 8a1c671a..125d2677 100644 --- a/crates/improvise-incremental/tests/zeros.rs +++ b/crates/improvise-incremental/tests/zeros.rs @@ -83,12 +83,22 @@ fn overwriting_a_zero_with_the_other_zero_moves_the_sign_of_min_and_max() { #[test] fn with_both_zeros_min_is_negative_and_max_is_positive_whatever_the_order() { - let at = |values: &[f64], func| { - let backend = Incremental::new(model_of(values, func), vec![]); - bits(&backend) - }; for values in [[0.0, -0.0], [-0.0, 0.0]] { - assert_eq!(at(&values, AggFunc::Min), Some((-0.0f64).to_bits())); - assert_eq!(at(&values, AggFunc::Max), Some(0.0f64.to_bits())); + let min = Some((-0.0f64).to_bits()); + let max = Some(0.0f64.to_bits()); + assert_eq!(both(&values, AggFunc::Min), (min, min), "{values:?}"); + assert_eq!(both(&values, AggFunc::Max), (max, max), "{values:?}"); + } +} + +#[test] +fn min_and_max_over_only_nan_are_nan_in_both_backends() { + for func in [AggFunc::Min, AggFunc::Max] { + let (eager, incremental) = both(&[f64::NAN, -f64::NAN], func.clone()); + let nan = |b: Option| b.is_some_and(|b| f64::from_bits(b).is_nan()); + assert!( + nan(eager) && nan(incremental), + "{func:?}: {eager:?} {incremental:?}" + ); } } From 1a05107198411108e86fd42a7f513ea91e748a4c Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-backend)" Date: Sat, 10 Oct 2026 16:50:28 -0700 Subject: [PATCH 2/2] docs(context): correct the Eager-math evidence, restore the product-targeted test, record the cycle changes (improvise-bw4p.20) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/core.md | 3 +- context/code/improvise-incremental.md | 24 +++++++------ .../test-evidence/improvise-incremental.md | 36 +++++++++++++------ .../improvise-incremental/tests/formulas.rs | 28 +++++++++++++++ 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index 30e48d8c..5a322c11 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -94,7 +94,8 @@ target category and every `none_cat` stripped), and iterates `stem × formula target` until nothing changes, bounded by `MAX_EVAL_DEPTH = 16`. Chained formulas (`Margin = Profit / Revenue` over `Profit = Revenue - Cost`) converge naturally; circular references converge to `CellValue::Error("circular")` -rather than overflowing the stack. +rather than overflowing the stack. "Nothing changed" is `model::types::settled`: numbers by bits (a +zero that changes sign is a change) with every NaN one value, so a NaN can settle. **A cell still moving when the bound runs out has not converged, and the cached path had to be told so** (improvise-7iww). The direct evaluator gets this free: diff --git a/context/code/improvise-incremental.md b/context/code/improvise-incremental.md index ab795434..f3604067 100644 --- a/context/code/improvise-incremental.md +++ b/context/code/improvise-incremental.md @@ -218,8 +218,8 @@ taken where clause 4 and clause 7 leave a choice; evidence is in sums are fed. Eager's `f64::min` and `f64::max` leave the sign of a tie between the two zeros to the platform and the order (`fmin` on ARM returns `-0.0`; x86 may not). The backend reproduces the part that does not depend on either: all zeros negative gives `-0.0`, all positive gives `+0.0`. - With both, `Min` is `-0.0` and `Max` is `+0.0`, which is what ARM's `fmin`/`fmax` return; on other - platforms eager may differ there, and `tests/random.rs` keeps `+0.0` out of its pool for that reason. + With both, `Min` is `-0.0` and `Max` is `+0.0`; eager now returns the same (see "Eager's math, + fixed"). ### Not built in step 4 @@ -268,14 +268,8 @@ is followed in outline, and the places it is not are the reason for this section 5. **Dependency order is an optimisation and is checked against the plain passes** (the NaN and zero differences below are gone: "Eager's math, fixed"). `Order` is the 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 two places. A cell holding NaN is never equal to itself, so eager's - passes never see it settle and call it `circular`, and the cells that read it keep what they - computed from the NaN; `mark_nan_circular` does the same afterwards. And eager stops on `==`, - which calls `-0.0` equal to the `0.0` an early pass read from a stored cell, so a stem with a - 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 + reach (a chain of fifteen links needs fifteen passes and a sixteenth to see no change). It is the same fixed point, because Eager's convergence test compares by bits ("Eager's math, fixed"). + `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 @@ -370,9 +364,17 @@ that say otherwise (step 4 items 4 and 10, step 5 item 5). Each is a user-visibl is still `circular` after sixteen passes. `mark_nan_circular` is gone. 4. **A zero's sign is a change.** `settled` compares by bits, so `0.0` becoming `-0.0` between passes moves the cell, and eager's fixed point and dependency order's agree. Dependency order is the unique fixed point - of an acyclic set under bit equality (each cell is a function of the final values it reads), so the + of an acyclic set under bit equality, up to NaN payload and sign (`settled` merges NaNs): each cell is a + function of the final values it reads. So the `holds_a_zero` fallback is dropped and `Ordered` needs no extra precondition. 5. **Empty `SUM`** is `+0.0` (step 6), listed here for completeness. +6. **Cycles over zeros and NaN.** A cycle that only flips a zero's sign (`X = -X`, `X = X * -1`, + `IF(X < 1, -X, X)`, `A = -B` with `B = A` over zeros) used to settle, because `-0.0 == 0.0`; it is now + `circular`, as the same cycle over nonzero data always was. A cycle fed a NaN (`A = A + 1`, or `A = A` + with `A` NaN) used to be `circular` and now settles on NaN. +7. **`Min` and `Max` skip NaN while `Sum` and `Avg` propagate it.** Kept: it is IEEE's `minimumNumber` and + `maximumNumber` for the extremes and ordinary addition for the sums, and consistent with ADR 005. + Whether a spreadsheet user expects one rule for both is a product question (bead text in the hand-back). ### Left alone, with the reasoning diff --git a/context/test-evidence/improvise-incremental.md b/context/test-evidence/improvise-incremental.md index 2f444241..55683f27 100644 --- a/context/test-evidence/improvise-incremental.md +++ b/context/test-evidence/improvise-incremental.md @@ -439,7 +439,8 @@ After the last retraction the group reads as absent and its map holds no zero-we ### group::tests::min_and_max_skip_nan_and_are_nan_over_nothing_else -NaN is skipped by `Min` and `Max`, a group of only NaN gives `+inf` and `-inf` and still counts. +NaN is skipped by `Min` and `Max`; a group of only NaN gives NaN (it gave `+inf` and `-inf` before the +Eager-math follow-up) and still counts. ### group::tests::a_fold_over_several_groups_is_the_fold_of_their_union @@ -472,7 +473,6 @@ What each fault was and the test that names it: `a_formula_that_keeps_moving_for_sixteen_passes_is_circular`, `dependency_order_and_the_plain_passes_leave_the_same_cache`; - dependencies after dependents: `dependency_order_and_the_plain_passes_leave_the_same_cache` and five more; -- NaN is not marked circular: `a_formula_cell_holding_nan_is_nan_and_not_circular`; - fifteen passes / seventeen passes: `a_chain_added_last_link_first_is_circular_exactly_where_eager_says`; - the passes end without marking the cells still moving, or begin from the old cache: `a_chain_added_last_link_first_is_circular_exactly_where_eager_says`, @@ -559,7 +559,10 @@ which is evaluated after it. Fails when such a formula set is not wide. ### a_formula_cell_holding_nan_is_nan_and_not_circular -NaN is not equal to itself, so eager's passes never see it settle. Fails when the ordered path leaves it. +A NaN formula result is a value: `Profit` and `Margin` over a NaN cell read NaN under both evaluations and +eager. It used to be `circular` in eager, because NaN is not equal to itself and never settled; the +fixed point now compares with `settled` (follow-up, "Eager's math, fixed"). Fails when NaN is +marked `circular`. ### a_new_aggregation_for_the_measure_a_filtered_formula_falls_back_to_changes_it @@ -643,16 +646,23 @@ runs because its two heaviest tests shared it. The tests are unchanged. 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 + +A Product-targeted `SUM(F0 WHERE Region = East)` with no None category, and `SetMeasureAgg Cost Min`. +It agreed with eager before the fix too, so it pins the shape and not the fault. + ### a_new_aggregation_reaches_a_filtered_sum_on_the_fallback_path -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. +The reviewer's shape: `P0 = SUM(Revenue) WHERE Region = East` on `_Measure` with Time as the None +category, then `SetMeasureAgg Revenue Max`, so the fallback to the raw group at the formula's own target +reads the new aggregation. Fails when a new aggregation does not reach the stems carrying the measure. ### 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 +A stored `0.0` at `F1`, then `F3 = F1`, `F4 = F3 ^ -1`, then `F1 = -0`. Eager used to settle on `F4 = inf` +because it called `-0.0` equal to the `0.0` it first read, while the ordered evaluation gave `-inf`; with +zero signs settled by bits both give `F3 = -0.0` and `F4 = -inf`. Fails (only `Ordered`) when a stem +with a zero is not re-evaluated by the passes, and in core when zeros settle equal. 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. @@ -768,8 +778,9 @@ Infinities and NaN read as `ExactSum::value` does, and a zero keeps its sign. ## Eager's math, fixed (follow-up to steps 4 to 6) -Ruling (user, 2026-10-10): do not reproduce math bugs found in Eager; fix Eager. Eight faults were -injected (six in the table below, two more in the groups' NaN handling); `nan-never-settles` survived the +Ruling (user, 2026-10-10): do not reproduce math bugs found in Eager; fix Eager. Six faults were +injected into core and the groups (mixed zeros, Min over NaN, NaN never settling, zeros settling equal, and +the groups' Min and Max over NaN); `nan-never-settles` survived the first pass because NaN produced by one evaluation always has the same bits, and is closed by a unit test of `settled`. Each fix has a regression test named after the bug: - `model::types::eager_math_tests::min_and_max_of_both_zeros_do_not_depend_on_their_order` and @@ -785,3 +796,8 @@ first pass because NaN produced by one evaluation always has the same bits, and oscillation is still `circular`. The random and differential suites compare numbers by bits and their pools hold both zeros again. +User-visible changes this evidence also covers: a cycle that only flips a zero's sign (`X = -X`, +`X = X * -1`, `IF(X < 1, -X, X)`, `A = -B` with `B = A` over zeros) was `-0` and is now `circular`, as the +same cycle over nonzero data always was; a cycle fed a NaN (`A = A + 1`, `A = A` with `A` NaN) settles on +NaN instead of `circular`. + diff --git a/crates/improvise-incremental/tests/formulas.rs b/crates/improvise-incremental/tests/formulas.rs index 04a1fd30..6b7187b7 100644 --- a/crates/improvise-incremental/tests/formulas.rs +++ b/crates/improvise-incremental/tests/formulas.rs @@ -379,3 +379,31 @@ fn ordered_evaluation_keeps_the_zero_eager_settles_on() { same_model(&model, &[], &edits, by).unwrap_or_else(|e| panic!("{by:?}: {e}")); } } +#[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(); + } +}