Merge pull request #139 from fiddlerwoaroof/fix/k844-importance-overflow

fix(gp-tree): a share of a large importance overflowed f32 twice over (improvise-k844)

context/code/gp.md

@@ -208,13 +208,24 @@ one function further along.** At `1e18` the map is finite, the fit lands, and
 `importance_ranking` computes `100.0 * imp[j] / total` — the multiplication
 first, so an importance over `f32::MAX / 100` is `inf` before the division that
 would have given 100.0 — and `"#".repeat((inf / 5.0).round() as usize)` is a
-saturating cast. The bar is clamped and the arithmetic is filed as
-improvise-k844, deliberately unfixed here: reordering it moves the bit patterns
-`misclass_rate_and_importance_are_bit_for_bit_what_they_were` pins, and this
-change's own evidence is that none moved. The lesson to carry is the one the
-*test* taught — a "just under the overflow" fixture at `1e18` aborted for an
-unrelated reason, and the fixture that means what it says sits at `1e9`, with
-`1e18` kept as its own guard against the second abort.
+saturating cast. The bar is clamped and the arithmetic was filed as
+improvise-k844, deliberately unfixed here.
+
+**The stated reason for deferring it was wrong, and is worth keeping as the
+correction rather than deleting.** This paragraph said reordering the arithmetic
+"moves the bit patterns
+`misclass_rate_and_importance_are_bit_for_bit_what_they_were` pins". It does
+not: that test pins `permutation_importance`'s **raw scores**, and
+`importance_ranking` consumes them afterwards, so no path it witnesses runs
+through the share. Measured when k844 was actually fixed — the whole file is
+byte-identical and not one literal moved. A cost predicted from a test's *name*
+rather than from what it calls is how a one-line fix stays filed; §8 carries the
+fix and what it really cost.
+
+The lesson to carry is the one the *test* taught — a "just under the overflow"
+fixture at `1e18` aborted for an unrelated reason, and the fixture that means
+what it says sits at `1e9`, with `1e18` kept as its own guard against the second
+abort.
 
 ## 8. `RegressionQuality` carries its own baseline
 
@@ -234,6 +245,31 @@ itself and reports **10%**, where the one feature the model uses carries all of
 it. Importances this small are ordinary, being error differences over a
 *standardized* target, so the shares have to sum to 100% at every scale.
 
+**"Every scale" includes the top of `f32`, and it did not** (improvise-k844).
+The sum and the ratio are computed in `f64` and only the resulting share — which
+is bounded by 100 and therefore always finite — is narrowed back to `f32`. Two
+distinct overflows made that necessary, and only the first is the one the ticket
+named. `100.0 * imp[j] / total` associates left, so a lone importance above
+`3.4e36` overflowed on the **multiply**, before the divide that would have
+brought it back into range, and the feature carrying everything was reported as
+`inf%`. Reassociating to `100.0 * (imp[j] / total)` fixes exactly that and
+nothing else — which is the trap, because the second overflow is in `total`
+itself: several large importances sum past `f32::MAX`, and a client of the
+reassociated version then divides by `inf` and reports a serene **0%** for every
+feature. `f64` closes both at once, since no sum of finite `f32`s can overflow
+it, and the guard that remains (`!total.is_finite()`) answers only the case
+where an importance arrives non-finite — nothing can be apportioned out of an
+infinite total, so the ranking is empty rather than `NaN`.
+
+Note where this is *reachable from*, because it is not where importances are
+small: both callers score permutation importance in the target's **own** units
+rather than the standardized ones, so an importance is an MSE difference and
+grows as the square of the target. That puts a live window between
+improvise-pv3g's `standardize` refusal and this overflow — a document whose
+target `standardize` accepts can still have a derived importance that `f32`
+cannot scale. As there, **every input was finite and the overflow was in the
+derived quantity**, which is why guarding inputs sees neither bug.
+
 ## 9. A fit takes two things to reproduce — and here, four
 
 `data::Model` records `seed: Option<u64>` **and** `config: Option<FitConfig>`,

context/test-evidence/gp.md

@@ -1098,3 +1098,88 @@ The column must be a **measure** — `as_categories` forces every document
 category to `Forced::Categorical`, so a category of dates never reaches the date
 branch at all. That is gotcha 37(d)'s "the date decline is narrower than it
 read": this is the genuine date *measure*.
+
+### fit::quality_tests::a_huge_lone_importance_is_still_the_whole_of_it
+
+improvise-k844, the arm the ticket's title names. `importance_ranking` computed
+`100.0 * imp[j] / total`, which in `f32` **multiplies first**: a lone importance
+of `3e37` makes `100.0 * 3e37` overflow to `inf` before the divide ever happens,
+and `inf / 3e37` is `inf`. The feature carrying all of the importance was
+reported as `inf%` of it. The all-zero early return could not see this — the
+total is large and positive, not zero — and neither could
+`a_tiny_lone_importance_is_still_the_whole_of_it`, whose whole concern is the
+opposite end of the range.
+
+The fault is in the *derived* quantity while every input is finite and ordinary,
+which is exactly improvise-pv3g's shape one function over. That is why the
+reproduction is `3e37` and not `f32::MAX`: the input is a plausible importance,
+not a sentinel.
+
+### fit::quality_tests::shares_are_apportioned_when_the_total_overflows_f32
+
+The silent twin, and the reason the ticket's own proposed fix is not the fix.
+Here **no single importance overflows** — two features at `2e38` are each well
+inside `f32` — but their `f32` **sum** is `inf`, so every share came back `NaN`
+(`100.0 * 2e38` is `inf`, and `inf / inf` is `NaN`). Nothing panicked and
+nothing was refused; a user's fit report simply carried `NaN%` against every
+feature. This is the same shape as `cv_regress` pooling nothing for a fold it
+could not standardize: an overflow that corrupts a reported number with no crash
+to announce it.
+
+**The injected rival is the ticket's own title.** Reassociating to
+`100.0 * (imp[j] / total)` in `f32` — "multiplies first", fixed by dividing
+first — passes `a_huge_lone_importance_is_still_the_whole_of_it` and
+`the_shares_sum_to_a_hundred_at_any_scale`, and fails this test and its
+non-finite sibling: with `total` already `inf`, `2e38 / inf` is `0.0`, so the
+rival turns `NaN%` into a silent **`0%` for every feature**, which is worse,
+being readable. Measured: 58 passed, 2 failed under the rival. That separation is
+the whole argument for accumulating in `f64` rather than reassociating in `f32` —
+`f64` cannot overflow on any sum of finite `f32`s, so the *arithmetic* stops
+being able to produce a non-finite share instead of the call site checking for
+one afterwards (`doctrine(an_invariant_is_made_unrepresentable_not_validated)`).
+
+### fit::quality_tests::a_non_finite_importance_ranks_nothing
+
+The one case `f64` accumulation cannot absorb: an importance that is *already*
+`inf` or `NaN` when it arrives. Summing in `f64` keeps it non-finite, and
+apportioning shares of an infinite total is meaningless, so the guard is
+`!total.is_finite()` beside the existing `total <= 0.0` and the ranking comes
+back empty — nothing to report, rather than `NaN%` against every feature. It
+returns data and prints nothing (gotcha 37a). The `NaN` arm is not redundant
+with the `inf` arm: `NaN <= 0.0` is `false`, so the pre-existing early return
+passed `NaN` straight through.
+
+### fit::quality_tests::the_shares_sum_to_a_hundred_at_any_scale
+
+Extended for improvise-k844 from four scales topping out at `1e6` to six topping
+out at `8e37`. `context/code/gp.md` §8 states the contract as "the shares have to
+sum to 100% at every scale", and the test stopped eleven orders of magnitude
+short of where `f32` actually breaks — so the prose was already right and only
+the test was narrow. The top two scales fail before the fix.
+
+Note what this test **cannot** see, which is why the two tests above exist: at
+`[8e37, 2.4e38]` the total is finite, so it separates "multiplies first" from
+"divides first" and is satisfied by either the rival or the fix.
+
+### fit::tests::a_large_measures_importance_shares_reach_the_report_finite
+
+improvise-k844 end to end, in `improvise-gp`. The reachability is the part the
+ticket did not state: both callers measure permutation importance in the
+document's **own units** — `fit_csv` through `real_unit_mse`, `fit_measure`
+through `mse(&champion, &rows)` against the *unnormalized* rows — so importance
+is an MSE difference, which is **squared**. A target near `2e18` therefore yields
+an importance near `9e36`, and `100.0 *` that overflows.
+
+That leaves a real window between improvise-pv3g's guard and this one:
+`standardize` refuses once `sum((t - mean)^2)` passes `f32::MAX`, around a spread
+of `5e18` at twelve rows, while `100.0 * imp` overflows from about `3.4e36`, i.e.
+a spread near `1.8e18`. `document_with_score_scaled_by(2e15)` sits inside it —
+the fit lands, every value is finite, and before the fix the report read
+`Cost reported inf% of the importance: [("Cost", inf)]`. The scale is load-bearing
+in both directions: at `4e15` `standardize` refuses and the test fails on
+`expect("the fit lands")` instead, proving nothing about shares.
+
+The assertion that the ranking is **non-empty** is there so the test cannot pass
+by reporting nothing at all — which is precisely what the non-finite guard makes
+`importance_ranking` do, one layer down, if the overflow is ever reintroduced
+upstream of it.

crates/gp-tree/src/fit.rs

@@ -294,8 +294,8 @@ pub fn regression_quality(pooled: &[(f32, f32)]) -> RegressionQuality {
 /// [`permutation_importance`] scores as `(feature index, percentage share)`, most
 /// important first, unused features dropped. No divisor floor — `context/code/gp.md` §8.
 pub fn importance_ranking(imp: &[f32]) -> Vec<(usize, f32)> {
-    let total: f32 = imp.iter().sum();
-    if total <= 0.0 {
+    let total: f64 = imp.iter().copied().map(f64::from).sum();
+    if !total.is_finite() || total <= 0.0 {
         return Vec::new();
     }
     let mut order: Vec<usize> = (0..imp.len()).collect();
@@ -307,7 +307,7 @@ pub fn importance_ranking(imp: &[f32]) -> Vec<(usize, f32)> {
     order
         .into_iter()
         .filter(|&j| imp[j] > 0.0)
-        .map(|j| (j, 100.0 * imp[j] / total))
+        .map(|j| (j, (100.0 * f64::from(imp[j]) / total) as f32))
         .collect()
 }
 
@@ -519,10 +519,11 @@ mod quality_tests {
         assert!((ranked[0].1 - 100.0).abs() < 1e-3, "{ranked:?}");
     }
 
-    /// The general form of the same claim, across four orders of magnitude.
+    /// The general form of the same claim, over f32's whole positive range — the
+    /// top two scales are improvise-k844. `context/test-evidence/gp.md`.
     #[test]
     fn the_shares_sum_to_a_hundred_at_any_scale() {
-        for scale in [1e-12f32, 1e-6, 1.0, 1e6] {
+        for scale in [1e-12f32, 1e-6, 1.0, 1e6, 1e30, 8e37] {
             let total: f32 = importance_ranking(&[scale, 3.0 * scale])
                 .iter()
                 .map(|&(_, share)| share)
@@ -531,6 +532,34 @@ mod quality_tests {
         }
     }
 
+    /// improvise-k844, the arm the ticket names: one feature carries everything, and
+    /// `100.0 * imp` overflowed before the divide. `context/test-evidence/gp.md`.
+    #[test]
+    fn a_huge_lone_importance_is_still_the_whole_of_it() {
+        let ranked = importance_ranking(&[3e37]);
+        assert_eq!(ranked.len(), 1);
+        assert!((ranked[0].1 - 100.0).abs() < 1e-3, "{ranked:?}");
+    }
+
+    /// improvise-k844's silent twin: no single importance overflows, their *total*
+    /// does. Dividing first alone reports 0% here. `context/test-evidence/gp.md`.
+    #[test]
+    fn shares_are_apportioned_when_the_total_overflows_f32() {
+        let ranked = importance_ranking(&[2e38, 2e38]);
+        assert_eq!(ranked.len(), 2, "{ranked:?}");
+        for &(_, share) in &ranked {
+            assert!((share - 50.0).abs() < 1e-3, "{ranked:?}");
+        }
+    }
+
+    /// A non-finite importance has no share of anything, so it ranks nothing rather
+    /// than apportioning `NaN`. improvise-k844.
+    #[test]
+    fn a_non_finite_importance_ranks_nothing() {
+        assert!(importance_ranking(&[f32::INFINITY, 1.0]).is_empty());
+        assert!(importance_ranking(&[f32::NAN, 1.0]).is_empty());
+    }
+
     /// Eight rows at `±scale`, so the *spread* is what grows rather than the count.
     fn rows_at_scale(scale: f32) -> Vec<Row<f32>> {
         (0..8)

crates/improvise-gp/src/fit.rs

@@ -1695,4 +1695,39 @@ mod tests {
             "the seed is what reproduces the fit: {line}"
         );
     }
+
+    /// `document()` with `Score` multiplied out: every value and the target's spread
+    /// stay inside f32, so the fit lands and only the derived importance is large.
+    fn document_with_score_scaled_by(scale: f64) -> (Model, View) {
+        let (mut model, view) = document();
+        for i in 0..12u32 {
+            let key = cell(&format!("r{i}"), "Score");
+            let base = match model.evaluate(&key) {
+                Some(CellValue::Number(v)) => v,
+                other => panic!("r{i} Score came back as {other:?}"),
+            };
+            model.set_cell(key, CellValue::Number(base * scale));
+        }
+        (model, view)
+    }
+
+    /// improvise-k844 end to end: importance is measured in the document's OWN units,
+    /// so a large-but-finite measure reported `inf%`. `context/test-evidence/gp.md`.
+    #[test]
+    fn a_large_measures_importance_shares_reach_the_report_finite() {
+        let (model, view) = document_with_score_scaled_by(2e15);
+        let outcome =
+            fit_measure(&model, &view, "Score", FIT_INTO, &request()).expect("the fit lands");
+        assert!(
+            !outcome.importance.is_empty(),
+            "the fit uses a feature, so something carries the importance"
+        );
+        for (name, share) in &outcome.importance {
+            assert!(
+                share.is_finite(),
+                "{name} reported {share}% of the importance: {:?}",
+                outcome.importance
+            );
+        }
+    }
 }