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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
committed by
Edward Langley
parent
5645f8d24a
commit
9f035f918e
@@ -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.
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user