fix(gp-tree): a deserialized Scaling goes through its one constructor (improvise-nzeu)
PR #111 made a non-finite affine map unrepresentable where it is DERIVED and left it constructible where it is READ BACK: TargetModel::Regress carried a raw { mean: f32, std: f32 } pair, so a hand-edited or corrupted model reintroduced the poisoned state on `fit_csv --predict`. The poison is ordinary JSON — `1e39` is well inside f64, over f32::MAX, and serde's f32 visitor takes it by `as f32`, which is infinity. Scaling is now #[serde(try_from = "RawScaling", into = "RawScaling")], so a saved model reaches Scaling::new like every other caller. NOT a check at the use site: that is the validated form of an invariant pv3g spent its whole change making unrepresentable, and it would leave two answers to "is this map usable". Regress carries #[serde(flatten)] scaling, so the saved shape is unchanged. Measured rather than reasoned: over a regression CSV at one seed the saved JSON, the fit report and the --predict output are byte-identical, the new code reads the old code's model identically, and the old code reads the new one's. This also gives Scaling::new's `mean` arm a reachable path and a test, which context/code/gp.md section 7 explicitly recorded as having neither. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -202,6 +202,32 @@ of the *type* (anyone may call `Scaling::new`), not a reachable path through
|
||||
that can see it. Saying so is the difference between a guard and a guard that
|
||||
is believed to do something it does not.
|
||||
|
||||
**Deserialization is the second caller, and it makes that contract
|
||||
load-bearing** (improvise-nzeu). pv3g made a poisoned map unconstructible where
|
||||
it is *derived* and left it constructible where it is *read back*:
|
||||
`TargetModel::Regress` carried a raw `{ mean: f32, std: f32 }` pair, so a
|
||||
hand-edited or corrupted model reintroduced on `--predict` exactly the state the
|
||||
type exists to forbid. `Scaling` is now `#[serde(try_from = "RawScaling", into =
|
||||
"RawScaling")]`, so a saved model reaches the one constructor like every other
|
||||
caller. **Not a check at the use site**: a use-site check is the validated form
|
||||
of the invariant this whole section spent its argument making unrepresentable,
|
||||
and it would leave two answers to "is this map usable" with nothing holding them
|
||||
in agreement.
|
||||
|
||||
Two things the fix had to preserve, and one it gains. `Regress` carries
|
||||
`#[serde(flatten)] scaling: Scaling`, so the **saved shape is unchanged** — the
|
||||
two numbers still sit beside `tree` — and an existing model file still loads.
|
||||
Measured both ways rather than reasoned: over a regression CSV at one seed the
|
||||
saved JSON is byte-identical, the fit report and the `--predict` output are
|
||||
byte-identical, the new code reads the old code's model identically, and the old
|
||||
code reads the new one's.
|
||||
The arithmetic is unchanged: the CLI's `eval(tree, &f) * std + mean` is now
|
||||
`scaling.restore(..)`, which is the same expression, and the `1e-6` floor is
|
||||
idempotent because the saved `std` came through the constructor already. What it
|
||||
gains is that the `mean` arm finally has a **reachable path and a test**: the
|
||||
poison arrives as a plain JSON number, `1e39`, well inside `f64` and over
|
||||
`f32::MAX`, which serde's `as f32` turns into infinity.
|
||||
|
||||
**One refusal does not make an entrypoint safe, and the CLI had a second abort
|
||||
one function further along.** At `1e18` the map is finite, the fit lands, and
|
||||
`report_importance` dies with a *capacity overflow*, because
|
||||
|
||||
@@ -1183,3 +1183,41 @@ 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.
|
||||
|
||||
### data::tests::a_deserialized_regression_model_cannot_carry_a_non_finite_map
|
||||
|
||||
improvise-nzeu, left open deliberately by PR #111 (improvise-pv3g), which made a
|
||||
non-finite affine map unrepresentable at the point it is **derived** and not at
|
||||
the point it is **deserialized**. `TargetModel::Regress` carried a raw
|
||||
`{ mean: f32, std: f32 }` pair, so a hand-edited or corrupted model file
|
||||
reintroduced the poisoned state on `fit_csv --predict`: `denormalize` computes
|
||||
`score * inf + mean`, and the NaN surfaced two crates away in `number_text`.
|
||||
Inbound-only, because `--save` can no longer write one — `standardize` refuses
|
||||
before a model is produced.
|
||||
|
||||
**The poison is ordinary JSON, which is what makes the hole real.** serde_json
|
||||
will not round-trip `inf` (it writes `null`, which then fails to read back as an
|
||||
`f32`), so it is tempting to conclude the state is unreachable across the wire.
|
||||
It is not: `1e39` is well inside `f64`, over `f32::MAX`, and serde's f32 visitor
|
||||
takes it by `as f32`, which is infinity. A hand editor can plausibly type it and
|
||||
every parser on the way accepts it.
|
||||
|
||||
The test pokes `json["target"]["Regress"][part]` for both `mean` and `std`, and
|
||||
its first assertion is that the position it pokes currently holds a **finite
|
||||
number** — which is simultaneously the "assert the mutated input is still
|
||||
well-formed" discipline and the guard on the saved shape. It asserts the error
|
||||
*text* names finiteness rather than merely that deserialization failed, because
|
||||
a rewrite that broke the JSON would fail too and prove nothing.
|
||||
|
||||
Three rivals, each caught at a different assertion:
|
||||
|
||||
- **Plain `derive(Deserialize)` on `Scaling`** — the state at HEAD — fails at
|
||||
`a non-finite mean was accepted`.
|
||||
- **A `try_from` that checks `std` only** fails the same way, on `mean`. This is
|
||||
the rival that matters for `context/code/gp.md` §7's own note that the `mean`
|
||||
arm of `Scaling::new` is a contract of the type with no reachable path:
|
||||
deserialization *is* that path, so the arm now has a test that fails when it is
|
||||
removed.
|
||||
- **Dropping `#[serde(flatten)]`** fails at the shape assertion with
|
||||
`"mean" ... : null`, since the map would then nest under a `scaling` key and
|
||||
every previously saved model would stop loading.
|
||||
|
||||
@@ -309,9 +309,9 @@ fn run_predict(model_path: &str, csv_path: &str) {
|
||||
for (i, row) in rows.iter().enumerate() {
|
||||
let f = encode_row(&model.columns, &header, row);
|
||||
match &model.target {
|
||||
TargetModel::Regress { mean, std, tree } => {
|
||||
TargetModel::Regress { scaling, tree } => {
|
||||
assert!(is_valid(tree, features), "corrupt model: bad feature index");
|
||||
println!("{i},{:.4}", eval(tree, &f) * std + mean);
|
||||
println!("{i},{:.4}", scaling.restore(eval(tree, &f)));
|
||||
}
|
||||
TargetModel::Classify { labels, trees } => {
|
||||
assert!(
|
||||
@@ -451,7 +451,7 @@ fn fit_and_report(
|
||||
);
|
||||
println!();
|
||||
report_importance(&names, &imp);
|
||||
TargetModel::Regress { mean, std, tree }
|
||||
TargetModel::Regress { scaling, tree }
|
||||
}
|
||||
Target::Classify {
|
||||
labels,
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
use serde::{Deserialize, Serialize};
|
||||
|
||||
use crate::Tree;
|
||||
use crate::fit::Row;
|
||||
use crate::fit::{Row, Scaling};
|
||||
|
||||
/// Suffixes for the numeric sub-features a date column expands into (the first
|
||||
/// five always; the last two only when the column has a time component).
|
||||
@@ -59,8 +59,10 @@ pub enum Target {
|
||||
#[derive(Serialize, Deserialize)]
|
||||
pub enum TargetModel {
|
||||
Regress {
|
||||
mean: f32,
|
||||
std: f32,
|
||||
/// Flattened, so the saved shape is unchanged: the two numbers sit beside
|
||||
/// `tree` on the wire and go through `Scaling::new` on the way in.
|
||||
#[serde(flatten)]
|
||||
scaling: Scaling,
|
||||
tree: Tree,
|
||||
},
|
||||
Classify {
|
||||
@@ -616,8 +618,7 @@ mod tests {
|
||||
"outlook=rainy".to_string(),
|
||||
],
|
||||
target: TargetModel::Regress {
|
||||
mean: 1.5,
|
||||
std: 0.5,
|
||||
scaling: Scaling::new(1.5, 0.5).expect("finite"),
|
||||
tree: Tree::Split {
|
||||
feat: 0,
|
||||
thresh: 2.0,
|
||||
@@ -628,6 +629,29 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_deserialized_regression_model_cannot_carry_a_non_finite_map() {
|
||||
for part in ["mean", "std"] {
|
||||
let mut json = serde_json::to_value(a_saved_model(None)).unwrap();
|
||||
let at = &mut json["target"]["Regress"][part];
|
||||
assert!(
|
||||
at.as_f64().is_some_and(f64::is_finite),
|
||||
"the saved shape must put {part:?} where this test pokes it, finite: {at}"
|
||||
);
|
||||
// Well within f64 and over f32::MAX, so serde's `as f32` yields
|
||||
// infinity from a number a hand editor can plausibly type.
|
||||
*at = serde_json::json!(1e39);
|
||||
let refused = match serde_json::from_value::<Model>(json) {
|
||||
Ok(_) => panic!("a non-finite {part} was accepted"),
|
||||
Err(e) => e.to_string(),
|
||||
};
|
||||
assert!(
|
||||
refused.contains("not finite"),
|
||||
"a non-finite {part} must be refused by Scaling::new, not accepted: {refused}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_saved_model_round_trips_with_its_seed() {
|
||||
let json = serde_json::to_string(&a_saved_model(Some(0xABCD_1234))).unwrap();
|
||||
|
||||
@@ -185,12 +185,43 @@ pub fn misclass_rate(trees: &[Tree], rows: &[Row<usize>]) -> f32 {
|
||||
|
||||
/// The affine map [`standardize`] folds out of a target, and folds back in. It
|
||||
/// cannot carry a non-finite part: `context/code/gp.md` §7.
|
||||
#[derive(Clone, Copy, Debug, PartialEq)]
|
||||
#[derive(Clone, Copy, Debug, PartialEq, serde::Serialize, serde::Deserialize)]
|
||||
#[serde(try_from = "RawScaling", into = "RawScaling")]
|
||||
pub struct Scaling {
|
||||
mean: f32,
|
||||
std: f32,
|
||||
}
|
||||
|
||||
/// The two numbers on the wire. Deserializing goes `RawScaling -> Scaling`, so a
|
||||
/// saved model reaches [`Scaling::new`] like every other caller — improvise-nzeu.
|
||||
#[derive(serde::Serialize, serde::Deserialize)]
|
||||
struct RawScaling {
|
||||
mean: f32,
|
||||
std: f32,
|
||||
}
|
||||
|
||||
impl From<Scaling> for RawScaling {
|
||||
fn from(s: Scaling) -> Self {
|
||||
Self {
|
||||
mean: s.mean,
|
||||
std: s.std,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl TryFrom<RawScaling> for Scaling {
|
||||
type Error = String;
|
||||
|
||||
fn try_from(raw: RawScaling) -> Result<Self, Self::Error> {
|
||||
Scaling::new(raw.mean, raw.std).ok_or_else(|| {
|
||||
format!(
|
||||
"the saved affine map is not finite (mean {}, std {}), so predictions from it would be NaN. Refit the model",
|
||||
raw.mean, raw.std
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
impl Scaling {
|
||||
/// The map, with `std` floored away from zero — or `None` when either part is
|
||||
/// not finite, which is what a target too large for `f32` arrives as.
|
||||
|
||||
Reference in New Issue
Block a user