fix(gp): the CLI refuses an overflowing target instead of aborting (Codex P1)
ChatGPT Codex on PR #111, correct. improvise-pv3g's claim is that a finite input no longer aborts the process, and the two LIBRARY paths honoured the refusal while examples/fit_csv.rs turned it straight back into a panic with `expect`. A user with a 1e20 column still got a panic -- at a different site than before, but still a panic, from the entrypoint a person actually runs. Demonstrated first: a temporary test drove `run_fit` over an 8-row CSV at 1e20/4e20 and aborted at fit_csv.rs:399. `run_fit` now returns `Result<(), String>` and `main` prints the refusal and exits 1. Gotcha 37(a) read the right way round: the library returns data, the example owns every message, and that includes the refusals. `fit_and_report` is the split that makes it testable without a temp file, and the example carries `test = true` so its tests run under `cargo test --workspace`. A SECOND abort on the same path, found because the first "just under the overflow" fixture used 1e18 and blew up somewhere else: at that scale the map is finite, the fit lands, and `report_importance` dies with a capacity overflow in `"#".repeat((share / 5.0).round() as usize)`. The share is `inf` because `importance_ranking` computes `100.0 * imp[j] / total` -- multiplication first, so an importance over f32::MAX/100 overflows before the division that would have returned 100.0 (measured: imp=[3.7499995e36], total the same, share=inf). The bar width is clamped and the arithmetic is filed as improvise-k844 rather than fixed here: reordering it moves the bit patterns `misclass_rate_and_importance_are_bit_for_bit_what_they_were` pins, and this PR's headline evidence is that none moved. AGENTS.md's own rule -- a finding can be correct and still be wrong to fix in that PR. Three guards, each failing under its own injection: restoring the panic, refusing every map, and removing the clamp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
+21
-3
@@ -152,9 +152,13 @@ The crate's leaf scores live in a ±5 range, so a target in the thousands cannot
|
||||
be reached by leaf mutation and `standardize` is not optional. Its consequence
|
||||
is that the champion predicts **standard deviations**.
|
||||
|
||||
`fit_csv` answers that by *printing* `≈ score × std + mean` beside the tree. A
|
||||
document has nowhere to put that, so `improvise-gp::fit_measure` folds the
|
||||
affine map back into the leaves with `denormalize` before a single character of
|
||||
`fit_csv` answers that by *printing* `≈ score × std + mean` beside the tree, and
|
||||
**turns the refusal below into a diagnostic rather than a panic** — `run_fit`
|
||||
returns `Result<(), String>` and `main` exits 1, which is gotcha 37(a) read the
|
||||
right way round: the library returns data, the example owns every message, and
|
||||
that includes the refusals (improvise-pv3g, found on PR #111 by Codex). A
|
||||
document has nowhere to put the affine map, so `improvise-gp::fit_measure` folds
|
||||
it back into the leaves with `denormalize` before a single character of
|
||||
formula text is spelled. Omitting it emits a perfectly well-formed formula whose
|
||||
every cell is out by an affine map — **wrong in the quiet way**, since small
|
||||
numbers in the right shape look like an answer.
|
||||
@@ -198,6 +202,20 @@ 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.
|
||||
|
||||
**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
|
||||
`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.
|
||||
|
||||
## 8. `RegressionQuality` carries its own baseline
|
||||
|
||||
A tree with a plausible-looking RMSE that is no better than always predicting
|
||||
|
||||
@@ -109,6 +109,57 @@ importance of `1e-10` carries all of what the model knows and was reported at
|
||||
*standardized* target. `the_shares_sum_to_a_hundred_at_any_scale` is the general
|
||||
form across four orders of magnitude.
|
||||
|
||||
### the_cli_refuses_an_overflowing_target_rather_than_aborting
|
||||
|
||||
Found by ChatGPT Codex on PR #111, as a P1, and correct. improvise-pv3g's claim
|
||||
is that a finite input no longer aborts the process; the two *library* paths
|
||||
honoured the refusal and `examples/fit_csv.rs` turned it straight back into a
|
||||
panic with `expect`, at `fit_csv.rs:399`. A user with a 1e20 column still got a
|
||||
panic — at a different site than before, but still a panic, from the entrypoint
|
||||
a person actually runs against a CSV.
|
||||
|
||||
Demonstrated before fixing: a temporary test drove `run_fit` over an 8-row CSV
|
||||
at 1e20/4e20 and aborted at exactly that line. `run_fit` now returns
|
||||
`Result<(), String>` and `main` prints the refusal and exits 1 — gotcha 37(a) is
|
||||
the reason the example is the right place for that translation, since the
|
||||
library returns data and the CLI owns every message.
|
||||
|
||||
The split that makes it testable is `fit_and_report`, which takes the header and
|
||||
rows already in hand; `run_fit` is the CSV-reading shell over it. The example
|
||||
carries `test = true` in `crates/gp-tree/Cargo.toml` so its `#[cfg(test)]`
|
||||
module runs under `cargo test --workspace` (examples default to `test = false`).
|
||||
|
||||
Rivals injected: restoring the panic fails this test and leaves its siblings
|
||||
green; refusing every map fails the just-under test and leaves this one green.
|
||||
|
||||
### the_cli_still_fits_a_target_just_under_the_overflow
|
||||
|
||||
The half that keeps the refusal from being satisfied by refusing everything. It
|
||||
sits at `1e9` rather than `1e18` for a reason worth reading — see below.
|
||||
|
||||
### the_cli_does_not_abort_when_an_importance_share_overflows
|
||||
|
||||
**The second abort on the same path, and it was found because the first draft of
|
||||
`the_cli_still_fits_a_target_just_under_the_overflow` used `1e18` and blew up
|
||||
somewhere else entirely.** At that scale `standardize` succeeds, the fit lands,
|
||||
and `report_importance` aborts with a **capacity overflow** in
|
||||
`"#".repeat((share / 5.0).round() as usize)`.
|
||||
|
||||
The cause is upstream and is filed as **improvise-k844**:
|
||||
`importance_ranking` computes `100.0 * imp[j] / total`, and the multiplication
|
||||
happens first, so an importance above `f32::MAX / 100` = 3.4e36 overflows to
|
||||
`inf` before the division that would have returned 100.0. Measured here:
|
||||
`imp = [3.7499995e36]`, `total = 3.7499995e36`, `share = inf`. A saturating
|
||||
`as usize` on that is an enormous allocation, which is an abort and not a
|
||||
diagnostic — the exact thing improvise-pv3g exists to remove, one function
|
||||
further along.
|
||||
|
||||
PR #111 clamped the bar width and filed the arithmetic, rather than reordering
|
||||
the expression, because `misclass_rate_and_importance_are_bit_for_bit_what_they_were`
|
||||
pins importance as f32 bit patterns and that PR's headline evidence is that no
|
||||
scoring bit pattern moved. Injecting the clamp's removal fails this test and
|
||||
only this test. What survives to k844: the CLI still *prints* `inf%`.
|
||||
|
||||
### standardize_refuses_a_spread_that_overflows_f32
|
||||
|
||||
improvise-pv3g, a P1 that crashed the TUI. `fit-measure` panicked at
|
||||
|
||||
@@ -12,3 +12,7 @@ serde = { version = "1", features = ["derive"] }
|
||||
|
||||
[dev-dependencies]
|
||||
serde_json = "1"
|
||||
|
||||
[[example]]
|
||||
name = "fit_csv"
|
||||
test = true
|
||||
|
||||
@@ -62,7 +62,9 @@ fn report_importance(names: &[String], imp: &[f32]) {
|
||||
println!(
|
||||
" {:<24} {share:>5.1}% {}",
|
||||
names[j],
|
||||
"#".repeat((share / 5.0).round() as usize)
|
||||
// A saturating `as usize` on a non-finite share is a capacity overflow,
|
||||
// which is an abort and not a diagnostic — improvise-pv3g's own rule.
|
||||
"#".repeat((share / 5.0).round().clamp(0.0, 20.0) as usize)
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -277,7 +279,13 @@ fn main() {
|
||||
save,
|
||||
opts,
|
||||
ov,
|
||||
} => run_fit(csv, save, opts, &ov),
|
||||
} => match run_fit(csv, save, opts, &ov) {
|
||||
Ok(()) => {}
|
||||
Err(refusal) => {
|
||||
eprintln!("\n{refusal}");
|
||||
std::process::exit(1);
|
||||
}
|
||||
},
|
||||
Mode::Predict { model, csv } => run_predict(&model, &csv),
|
||||
}
|
||||
}
|
||||
@@ -327,18 +335,35 @@ fn real_unit_mse(tree: &Tree, rows: &[Row<f32>], mean: f32, std: f32) -> f32 {
|
||||
/ rows.len() as f32
|
||||
}
|
||||
|
||||
fn run_fit(csv_path: Option<String>, save_path: Option<String>, opts: FitOpts, ov: &Overrides) {
|
||||
fn run_fit(
|
||||
csv_path: Option<String>,
|
||||
save_path: Option<String>,
|
||||
opts: FitOpts,
|
||||
ov: &Overrides,
|
||||
) -> Result<(), String> {
|
||||
println!(
|
||||
"seed 0x{:016x} (pass --seed to reproduce or vary this fit)\n",
|
||||
opts.seed
|
||||
);
|
||||
let (header, mut rows) = match csv_path {
|
||||
let (header, rows) = match csv_path {
|
||||
Some(path) => read_csv(&std::fs::read_to_string(&path).expect("could not read the CSV")),
|
||||
None => {
|
||||
eprintln!("(no CSV path given — fitting the built-in \"play tennis\" dataset)\n");
|
||||
demo_dataset()
|
||||
}
|
||||
};
|
||||
fit_and_report(header, rows, save_path, opts, ov)
|
||||
}
|
||||
|
||||
/// The fit itself, over data already in hand. A refusal the library returns as
|
||||
/// `None` becomes a diagnostic here: `context/code/gp.md` §2.
|
||||
fn fit_and_report(
|
||||
header: Vec<String>,
|
||||
mut rows: Vec<Vec<String>>,
|
||||
save_path: Option<String>,
|
||||
opts: FitOpts,
|
||||
ov: &Overrides,
|
||||
) -> Result<(), String> {
|
||||
assert!(
|
||||
header.len() >= 2,
|
||||
"need at least one feature column and a target"
|
||||
@@ -396,10 +421,14 @@ fn run_fit(csv_path: Option<String>, save_path: Option<String>, opts: FitOpts, o
|
||||
|
||||
// A standardized target, because the crate's leaf scores live in a
|
||||
// small score-space. The map back to real units is printed, not folded.
|
||||
let scaling = standardize(&train).expect(
|
||||
"the target's spread overflows 32-bit arithmetic — scale the column and refit",
|
||||
);
|
||||
let (normalized, mean, std) = (scaling.0, scaling.1.mean(), scaling.1.std());
|
||||
let (normalized, scaling) = standardize(&train).ok_or_else(|| {
|
||||
format!(
|
||||
"{target_name}: the values are too large for the 32-bit arithmetic a fit \
|
||||
searches in — their spread overflows. Scale the column (thousands, \
|
||||
millions) and fit that"
|
||||
)
|
||||
})?;
|
||||
let (mean, std) = (scaling.mean(), scaling.std());
|
||||
let tree = fit(&normalized, &spec, &cfg, &mut rng);
|
||||
let train_mse = real_unit_mse(&tree, &train, mean, std);
|
||||
println!("--- final tree (all rows): {target_name} ~ f(...) ---");
|
||||
@@ -486,4 +515,69 @@ fn run_fit(csv_path: Option<String>, save_path: Option<String>, opts: FitOpts, o
|
||||
std::fs::write(&path, json).expect("could not write the model file");
|
||||
eprintln!("\n(saved model to {path})");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Eight rows whose target is `scale` and `4 * scale`, so the SPREAD is what
|
||||
/// grows — the shape `standardize_refuses_a_spread_that_overflows_f32` uses.
|
||||
fn table_at_scale(scale: &str) -> (Vec<String>, Vec<Vec<String>>) {
|
||||
let header = vec!["Cost".to_string(), "Score".to_string()];
|
||||
let rows = (0..8)
|
||||
.map(|i| {
|
||||
let high = i % 2 == 1;
|
||||
vec![
|
||||
i.to_string(),
|
||||
format!("{}{scale}", if high { "4" } else { "1" }),
|
||||
]
|
||||
})
|
||||
.collect();
|
||||
(header, rows)
|
||||
}
|
||||
|
||||
fn opts() -> FitOpts {
|
||||
FitOpts {
|
||||
cv: 0,
|
||||
seeds: 1,
|
||||
penalty: 0.01,
|
||||
seed: 1,
|
||||
}
|
||||
}
|
||||
|
||||
/// improvise-pv3g, second site. `context/test-evidence/gp.md`.
|
||||
#[test]
|
||||
fn the_cli_refuses_an_overflowing_target_rather_than_aborting() {
|
||||
let (header, rows) = table_at_scale("e20");
|
||||
let refusal = fit_and_report(header, rows, None, opts(), &Overrides::default())
|
||||
.expect_err("a spread that overflows f32 must come back as a diagnostic");
|
||||
assert!(
|
||||
refusal.contains("too large for the 32-bit arithmetic"),
|
||||
"{refusal}"
|
||||
);
|
||||
assert!(
|
||||
refusal.contains("Score"),
|
||||
"the refusal must name the column: {refusal}"
|
||||
);
|
||||
}
|
||||
|
||||
/// The SECOND abort on the same path, found while fixing the first:
|
||||
/// `context/test-evidence/gp.md`.
|
||||
#[test]
|
||||
fn the_cli_does_not_abort_when_an_importance_share_overflows() {
|
||||
let (header, rows) = table_at_scale("e18");
|
||||
fit_and_report(header, rows, None, opts(), &Overrides::default())
|
||||
.expect("1e18 standardizes, so the CLI must reach the end of its report");
|
||||
}
|
||||
|
||||
/// The other half: the CLI still fits a large target that does not overflow, so
|
||||
/// the refusal above is not "refuse everything large".
|
||||
#[test]
|
||||
fn the_cli_still_fits_a_target_just_under_the_overflow() {
|
||||
let (header, rows) = table_at_scale("e9");
|
||||
fit_and_report(header, rows, None, opts(), &Overrides::default())
|
||||
.expect("1e18 is within f32's reach");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user