improvise-x1ns: the preflight counts the (missing) one-hot; improvise-e8no declined
improvise-x1ns (found by ChatGPT Codex on PR #90, live on main at ce895ff).
`encoded_width` computed `has_missing` and used it in the numeric arm only, while
`build_categorical` maps every absent cell to MISSING_CAT and includes it in
`distinct(&mapped)`. A forced category with N distinct present values and one
record lacking that coordinate promised N features and materialised N+1.
That under-reports in the direction that costs. The preflight runs before
`preprocess`; the exact check on `feature_names.len()` runs after, so it bounds
the error message and not the allocation the preflight exists to prevent. gp.md
§14 claimed drift was "bounded in both directions by the exact check" — wrong,
and wrong against the argument two paragraphs above it in the same section. That
sentence is corrected rather than softened; the previous wave read this shape and
reasoned it away with it.
The clamp stays outside the addition, so an entirely-missing column — which
`preprocess` skips — is still counted as one and the fix moves only its own case.
improvise-e8no is DECLINED, on evidence rather than on appetite. `formula.pest`'s
only coordinate predicate is `is_expr`, a single-value equality: no IN, no
ordering over items, and a coordinate reaches an `Expr` only through IS. So the
predicates a tree may state about a category are exactly its items, one at a
time, and one-hot is the only encoding whose splits are spellable — the width is
the size of the vocabulary, not an implementation's appetite. Ordinal encoding is
already the classified refusal `Artefact::NumericCategory`, and hashing has no
document name at all. The pre-zv8t "fit that succeeded" therefore either never
split on the column, in which case Label is exactly equivalent and in budget, or
was refused at storage time: no fit is lost that was both storable and used the
column. Asserted by a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored,
which is also the first coverage NumericCategory has had.
Both refusal messages said "a category with a distinct value per record becomes
one column per record", which diagnoses the runaway shape and not e8no's. They
now say a category becomes one column per distinct item, whether its items read
as numbers or not — that clause being the whole of what zv8t changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
+96
-5
@@ -563,12 +563,43 @@ rows × rows one-hot *during* preprocessing, so the refusal arrives after the
|
||||
memory and the O(rows²) work have been spent. It approximates `preprocess`'s
|
||||
type inference rather than sharing it — a column whose present cells all parse
|
||||
as a number is one feature (two with a missing indicator), anything else is
|
||||
counted at its distinct-value count. Copying the predicate is the lesser evil;
|
||||
counted at its distinct-value count **plus one where any cell is missing**,
|
||||
because `build_categorical` maps an absent cell to the reserved `(missing)`
|
||||
category and emits a one-hot for it. Copying the predicate is the lesser evil;
|
||||
the alternative is exporting the inference and calling this a second
|
||||
implementation of it. **The cost of the copy drifting is bounded in both
|
||||
directions by the exact check** on `Dataset::feature_names.len()`:
|
||||
over-counting costs a refusal the exact check would not have made,
|
||||
under-counting only lets the exact check do its job.
|
||||
implementation of it.
|
||||
|
||||
**The two directions of a drift do not cost the same, and this paragraph said
|
||||
they did** (improvise-x1ns). It read: *the cost of the copy drifting is bounded
|
||||
in both directions by the exact check* — over-counting costs a needless refusal,
|
||||
under-counting "only lets the exact check do its job". That is wrong, and it is
|
||||
wrong against the argument **two paragraphs above it**: the exact check runs on
|
||||
`Dataset::feature_names.len()`, which exists only *after* `preprocess` has
|
||||
materialised the matrix. So it bounds the **error message**, never the **cost** —
|
||||
which is precisely the thing the preflight was introduced to bound. Over-counting
|
||||
is cheap; under-counting spends the memory and the O(rows²) work the preflight
|
||||
exists to prevent, and then reports it politely.
|
||||
|
||||
The live instance was the `(missing)` column: the categorical arm counted
|
||||
`present` values only, so a forced category with N distinct present values and
|
||||
one record lacking that coordinate promised N and materialised N+1. `has_missing`
|
||||
was already computed and used in the numeric arm alone — **two arms of one match
|
||||
disagreeing about what a column costs**, which is improvise-zv8t's forcing-by-name
|
||||
/ budgeting-by-index defect one level in. That one was repaired by making the
|
||||
ordering a data dependency and so a compile error; this one has no such shape
|
||||
available, so the guard is a test that asks `preprocess` what it actually built
|
||||
rather than asserting a number
|
||||
(`the_preflight_counts_the_missing_category_a_forced_column_materialises`).
|
||||
|
||||
The previous wave's review saw a version of this and reasoned it away as *"read,
|
||||
not injected"* and *"bounded by the exact check"*. Both halves of that are the
|
||||
failure mode this file exists to catch: the first declined to run the experiment,
|
||||
and the second repeated the sentence above instead of checking it. **An argument
|
||||
that a cost is bounded is checkable, and this one was false for eight months.**
|
||||
|
||||
`encoded_width` remains **advisory** in the direction that is safe: it may
|
||||
over-count, and a column whose cells are *entirely* missing is counted as one
|
||||
where `preprocess` skips it altogether. It must never under-count.
|
||||
|
||||
`MAX_RECORDS = 20_000` bounds the *work*, which the width bound does not: a
|
||||
one-feature table with a million records still runs cross-validation, the final
|
||||
@@ -586,6 +617,66 @@ sequence, five-fold CV included. There is deliberately **no timing test**: a
|
||||
wall-clock assertion is flaky on shared CI and would fail for a reason that is
|
||||
not this code's. Re-measure after changing the numbers.
|
||||
|
||||
### Forcing a numeric category is a *representability* choice, not a budget one
|
||||
|
||||
improvise-zv8t made `fit_measure` force every document category to
|
||||
`Forced::Categorical`, and improvise-e8no records the cost: a hundred-item
|
||||
numeric category one-hots to a hundred columns, past `MAX_FEATURES`, and is
|
||||
refused where inference used to read it as one numeric column and the fit
|
||||
succeeded. **improvise-e8no is declined, and the refusal is correct.**
|
||||
|
||||
The ticket rules out making the forcing conditional on cardinality, and rightly:
|
||||
that is inference by another name, and it makes the *encoding* depend on a budget
|
||||
rather than on what the document says. But the stronger reason to decline is that
|
||||
the alternatives are not merely distasteful — they are **unstorable**, and the
|
||||
code already says so.
|
||||
|
||||
The formula language's only coordinate predicate is `is_expr`, spelled
|
||||
`identifier ~ sp+ ^"IS" ~ sp+ filter_value` in `formula.pest`: a **single-value
|
||||
equality**. There is no `IN`, no ordering over a category's items, and no way to
|
||||
put a coordinate on either side of a `cmp_op` — a coordinate becomes an `Expr`
|
||||
only by passing through `IS`. So the set of predicates a fitted tree may state
|
||||
about a category is exactly the set of its items, one at a time, and **one-hot is
|
||||
the only encoding whose splits are spellable**. The width is not an
|
||||
implementation's appetite; it is the size of the vocabulary.
|
||||
|
||||
That disposes of the two representations the ticket invites. **Ordinal** encoding
|
||||
imposes an order the document does not assert, and a split on it is already a
|
||||
classified refusal: `spell()` maps `FeatureRole::Numeric` over a *category*
|
||||
column to `Artefact::NumericCategory`, whose reason is that a reference to the
|
||||
category's name would resolve to a `_Measure` item of the same name — a different
|
||||
column (improvise-8xhy). **Hashing** is strictly worse: a bucket has no name in
|
||||
the document at all, so it cannot even be refused informatively.
|
||||
|
||||
And this is what makes the decline clean rather than resigned. The "fit that
|
||||
previously succeeded" was one of two things, never a third. Either the tree
|
||||
**never split** on the numeric category — in which case the column contributed
|
||||
nothing, and marking it a `Label` yields the same fit, honestly, inside the
|
||||
budget, which is exactly what the refusal already advises. Or the tree **did**
|
||||
split on it, and `fitted_formula` refused the result as `Artefact::NumericCategory`
|
||||
at storage time. **No fit is lost that was both storable and used the column**,
|
||||
which is asserted rather than argued, by
|
||||
`a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored`. That test had
|
||||
no predecessor: `NumericCategory` was the one artefact family with no coverage,
|
||||
and the decline rests on it.
|
||||
|
||||
What the decline does cost, stated rather than waved at: a user whose `Year`
|
||||
category really is a quantity they want fitted **as a number** has no way to say
|
||||
so through `fit_measure`. The document says `Year` is a category of items, and
|
||||
that is what is honoured. The remedy is a document change — `Year` belongs in
|
||||
`_Measure` if it is a measurement — and `crate::dataset` is `pub` and takes the
|
||||
caller's `Overrides`, so a host that wants to offer the choice can (gotcha 37d).
|
||||
Offering it *inside* `fit_measure` would be a new user-facing control, and
|
||||
nobody has asked for one.
|
||||
|
||||
The refusal's wording was narrower than the rule and is now aligned in both
|
||||
places that carry it: it said *a category with a distinct value per record
|
||||
becomes one column per record*, which diagnoses the runaway shape and not
|
||||
improvise-e8no's — a category of modest cardinality that is simply wider than
|
||||
the budget. Both now say a category becomes one column per distinct item,
|
||||
**whether its items read as numbers or not**, since that clause is the whole of
|
||||
what changed under zv8t.
|
||||
|
||||
### `Quality` is an enum, not an `Option`
|
||||
|
||||
`cv_regress` pools no pairs below two folds, and scoring an empty pool gives an
|
||||
|
||||
@@ -749,3 +749,88 @@ fixture really names one column twice, that the preflight's 41 really disagrees
|
||||
with what forcing would build, and that `as_categories` itself refuses. A test
|
||||
asserting only the error message would pass on either ordering, since the
|
||||
message is identical -- the whole defect was in work done, not in the result.
|
||||
|
||||
### the_preflight_counts_the_missing_category_a_forced_column_materialises
|
||||
|
||||
improvise-x1ns, found by ChatGPT Codex reviewing PR #90 and live on `main` at
|
||||
`ce895ff`. `encoded_width` computed `has_missing` and used it in the **numeric**
|
||||
arm only:
|
||||
|
||||
```
|
||||
true => 1 + usize::from(has_missing),
|
||||
false => present.iter().collect::<HashSet<_>>().len().max(1),
|
||||
```
|
||||
|
||||
`build_categorical` maps every missing cell to `MISSING_CAT` and includes it in
|
||||
`distinct(&mapped)`, so a categorical column with N distinct *present* values and
|
||||
at least one absent cell materialises **N+1** one-hots. The preflight promised N.
|
||||
|
||||
It under-reports by exactly one per such column, and it under-reports in the
|
||||
direction that costs: the preflight runs *before* `preprocess`, the exact check
|
||||
on `feature_names.len()` runs *after*, so the exact check bounds the error
|
||||
message and not the allocation — which is the whole reason the preflight exists.
|
||||
The previous wave's review recorded this shape as "read, not injected" and
|
||||
"bounded by the exact check"; the second half is false and is now corrected in
|
||||
`context/code/gp.md` §14.
|
||||
|
||||
**Fails before the fix** with
|
||||
|
||||
```
|
||||
the preflight under-reported: it promised 5 features and `preprocess`
|
||||
materialised 6 — ["Year=(missing)", "Year=2020", "Year=2021", "Year=2022",
|
||||
"Year=2023", "Cost"]
|
||||
```
|
||||
|
||||
The fixture is Codex's suggested case, and the reason it is the interesting one:
|
||||
a **forced numeric category with a missing coordinate**. Forcing is what sends a
|
||||
numeric column down the categorical branch, so only a *forced* column reaches the
|
||||
arm that was wrong, and only a missing cell makes the arms disagree.
|
||||
|
||||
The test does not assert a number. It asks `preprocess` what it actually built
|
||||
and requires `encoded_width >= data.feature_names.len()`, so it cannot pass by
|
||||
agreeing with a stale constant, and it keeps holding if the encoding widens for
|
||||
some other reason. Three preconditions are asserted first, because each would
|
||||
make it vacuous: that exactly one record lacks the coordinate, that the present
|
||||
cells really are numeric (otherwise the column would be categorical by inference
|
||||
and forcing would prove nothing), and that a `Year=(missing)` one-hot is really
|
||||
among the materialised features.
|
||||
|
||||
The `.max(1)` is deliberately outside the addition. A column whose cells are
|
||||
*entirely* missing has no present values, and `preprocess` skips it with a
|
||||
warning rather than encoding it; keeping the clamp on the sum leaves that case
|
||||
counted as 1 exactly as before, so the fix moves only the case it names.
|
||||
|
||||
### a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored
|
||||
|
||||
The evidence improvise-e8no is declined on, and the only coverage
|
||||
`Artefact::NumericCategory` has ever had — it was the one artefact family with no
|
||||
test, which is why the decline needed writing before it could be made.
|
||||
|
||||
improvise-e8no observes that forcing a document category to `Forced::Categorical`
|
||||
(improvise-zv8t) widens the encoding, so a high-cardinality numeric category now
|
||||
exceeds `MAX_FEATURES` where inference used to read it as one numeric column and
|
||||
the fit succeeded. The decline turns on what that success was worth: a tree that
|
||||
splits on a numeric column over a *category* column produces a formula
|
||||
`fitted_formula` **refuses**, because a reference to the category's name would
|
||||
resolve to a `_Measure` item of the same name (improvise-8xhy). So the
|
||||
pre-zv8t success was either a fit that never touched the column — for which
|
||||
marking it a `Label` is exactly equivalent and within budget — or one that could
|
||||
not be stored.
|
||||
|
||||
The test asks `preprocess` with `Forced::Numeric`, which is the pre-zv8t reading
|
||||
reproduced deliberately rather than described, and then asks `Features::of` and
|
||||
`fitted_formula`. It does **not** use the `artefacts()` helper: that helper
|
||||
fabricates a `Spelling`, which is the shape gotcha 37(d) warns about — it would
|
||||
assert that the emitter refuses what the test told it to refuse. Here the
|
||||
document and the encoder decide.
|
||||
|
||||
Injected fault: change `spell()`'s `FeatureRole::Numeric { .. }` arm from
|
||||
`Spelling::Artefact(Artefact::NumericCategory)` to `Spelling::Measure(column.clone())`
|
||||
— the "fix" someone would reach for on reading improvise-e8no, and the one that
|
||||
silently reopens improvise-8xhy. Fails with
|
||||
|
||||
```
|
||||
assertion `left == right` failed: a numeric column over a category column is unspellable
|
||||
left: Measure("Year")
|
||||
right: Artefact(NumericCategory)
|
||||
```
|
||||
|
||||
@@ -19,7 +19,9 @@ fn leading_digits(s: &str) -> &str {
|
||||
/// Which spellings are legal: `context/code/core.md`, "The number format".
|
||||
pub fn parse_number_format(fmt: &str) -> (bool, u8) {
|
||||
let comma = fmt.contains(',');
|
||||
let precision = fmt.rsplit_once('.').map_or("", |(_, tail)| leading_digits(tail));
|
||||
let precision = fmt
|
||||
.rsplit_once('.')
|
||||
.map_or("", |(_, tail)| leading_digits(tail));
|
||||
(comma, precision.parse::<u8>().unwrap_or(0))
|
||||
}
|
||||
|
||||
|
||||
@@ -38,12 +38,14 @@ fn encoded_width(table: &FeatureTable, category_columns: usize) -> usize {
|
||||
match numeric {
|
||||
// A numeric column is one feature, plus the `.missing` flag.
|
||||
true => 1 + usize::from(has_missing),
|
||||
// Everything else: one column per distinct value.
|
||||
false => present
|
||||
// Everything else: one column per distinct value, plus the one
|
||||
// `build_categorical` reserves for `(missing)` — gp.md §14.
|
||||
false => (present
|
||||
.iter()
|
||||
.collect::<std::collections::HashSet<_>>()
|
||||
.len()
|
||||
.max(1),
|
||||
+ usize::from(has_missing))
|
||||
.max(1),
|
||||
}
|
||||
})
|
||||
.sum()
|
||||
@@ -239,8 +241,9 @@ pub fn fit_measure(
|
||||
anyhow::ensure!(
|
||||
width <= MAX_FEATURES,
|
||||
"fitting {target:?} would encode {width} features from {} records, which is too wide \
|
||||
to search on the UI thread. A category with a distinct value per record becomes one \
|
||||
column per record — mark such a category as a Label so it is not a feature",
|
||||
to search on the UI thread. A category becomes one column per distinct item, whether \
|
||||
its items read as numbers or not — mark such a category as a Label so it is not a \
|
||||
feature",
|
||||
table.rows.len(),
|
||||
);
|
||||
|
||||
@@ -269,8 +272,8 @@ pub fn fit_measure(
|
||||
anyhow::ensure!(
|
||||
data.feature_names.len() <= MAX_FEATURES,
|
||||
"fitting {target:?} encoded {} features, over the {MAX_FEATURES} this host will \
|
||||
search. A category with many distinct values becomes one column per value — \
|
||||
mark such a category as a Label so it is not a feature",
|
||||
search. A category becomes one column per distinct item, whether its items read as \
|
||||
numbers or not — mark such a category as a Label so it is not a feature",
|
||||
data.feature_names.len(),
|
||||
);
|
||||
// Empty is a **panic**, not a poor fit: zero features makes `random_tree` draw
|
||||
@@ -714,6 +717,94 @@ mod tests {
|
||||
(model, view)
|
||||
}
|
||||
|
||||
/// Bug improvise-x1ns: the preflight counted a forced categorical by its
|
||||
/// PRESENT values only. Evidence: `context/test-evidence/gp.md`.
|
||||
#[test]
|
||||
fn the_preflight_counts_the_missing_category_a_forced_column_materialises() {
|
||||
let (model, view) = a_numeric_category_with_a_missing_coordinate();
|
||||
let table = feature_table(&model, &view, "Score").expect("the table builds");
|
||||
let category_columns = model.regular_category_names().len();
|
||||
|
||||
// The fixture must really carry the shape the bug needs: a forced
|
||||
// category whose cells are numeric, one of them absent.
|
||||
let year: Vec<&str> = table.rows.iter().map(|r| r[0].as_str()).collect();
|
||||
assert_eq!(
|
||||
year.iter().filter(|c| gp_tree::data::is_missing(c)).count(),
|
||||
1,
|
||||
"exactly one record must lack the Year coordinate: {year:?}"
|
||||
);
|
||||
assert!(
|
||||
year.iter()
|
||||
.filter(|c| !gp_tree::data::is_missing(c))
|
||||
.all(|c| c.parse::<f64>().is_ok()),
|
||||
"the present Year cells must all be numeric: {year:?}"
|
||||
);
|
||||
|
||||
let width = super::encoded_width(&table, category_columns);
|
||||
let overrides =
|
||||
super::as_categories(&table.header, category_columns).expect("the header is clean");
|
||||
let data = preprocess(&table.header, &table.rows, &overrides);
|
||||
|
||||
// `build_categorical` maps the absent cell to the reserved `(missing)`
|
||||
// category, so it is one more one-hot column than the present values.
|
||||
assert!(
|
||||
data.feature_names
|
||||
.iter()
|
||||
.any(|n| n == &format!("Year={}", gp_tree::data::MISSING_CAT)),
|
||||
"the forced column must really emit a (missing) one-hot: {:?}",
|
||||
data.feature_names
|
||||
);
|
||||
assert!(
|
||||
width >= data.feature_names.len(),
|
||||
"the preflight under-reported: it promised {width} features and \
|
||||
`preprocess` materialised {} — {:?}",
|
||||
data.feature_names.len(),
|
||||
data.feature_names,
|
||||
);
|
||||
}
|
||||
|
||||
/// A numeric `Year` category with one record that has no `Year` coordinate, so
|
||||
/// forcing sends a numeric column down the categorical branch with a gap in it.
|
||||
fn a_numeric_category_with_a_missing_coordinate() -> (Model, View) {
|
||||
let mut model = Model::new("sales");
|
||||
model.add_category("Year").expect("under the limit");
|
||||
let measure = model.category_mut(MEASURE).expect("_Measure always exists");
|
||||
for item in ["Cost", "Score"] {
|
||||
measure.add_item(item);
|
||||
}
|
||||
for i in 0..4u32 {
|
||||
let year = format!("{}", 2020 + i);
|
||||
model
|
||||
.category_mut("Year")
|
||||
.expect("just added")
|
||||
.add_item(year.clone());
|
||||
for (m, v) in [("Cost", f64::from(i) * 10.0), ("Score", f64::from(i))] {
|
||||
model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Year".to_string(), year.clone()),
|
||||
(MEASURE.to_string(), m.to_string()),
|
||||
]),
|
||||
CellValue::Number(v),
|
||||
);
|
||||
}
|
||||
}
|
||||
// The record with no `Year` coordinate at all — an empty cell, which
|
||||
// `is_missing` reads as absent rather than as a value.
|
||||
for (m, v) in [("Cost", 99.0), ("Score", 9.0)] {
|
||||
model.set_cell(
|
||||
CellKey::new(vec![(MEASURE.to_string(), m.to_string())]),
|
||||
CellValue::Number(v),
|
||||
);
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
}
|
||||
view.set_axis("Year", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
(model, view)
|
||||
}
|
||||
|
||||
/// The review finding on improvise-zv8t: forcing is by NAME and the preflight
|
||||
/// counts by INDEX. Evidence: `context/test-evidence/gp.md`.
|
||||
#[test]
|
||||
|
||||
@@ -812,6 +812,47 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// improvise-e8no's decline rests on this: the pre-zv8t encoding of a
|
||||
/// numeric category yields fits no document can store. Evidence: `gp.md`.
|
||||
#[test]
|
||||
fn a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored() {
|
||||
use gp_tree::data::{Forced, Overrides, preprocess};
|
||||
|
||||
// The pre-improvise-zv8t reading: a category of numbers inferred as a
|
||||
// numeric column. Asked of `preprocess`, never a fabricated spelling.
|
||||
let header = vec!["Year".to_string(), TARGET.to_string()];
|
||||
let rows: Vec<Vec<String>> = [("2020", "1"), ("2021", "2"), ("2022", "3")]
|
||||
.iter()
|
||||
.map(|(y, t)| vec![(*y).to_string(), (*t).to_string()])
|
||||
.collect();
|
||||
let overrides = Overrides {
|
||||
force: [("Year".to_string(), Forced::Numeric)]
|
||||
.into_iter()
|
||||
.collect(),
|
||||
..Overrides::default()
|
||||
};
|
||||
let data = preprocess(&header, &rows, &overrides);
|
||||
let features = crate::Features::of(&data, &["Year".to_string()]);
|
||||
|
||||
assert_eq!(
|
||||
data.feature_names,
|
||||
vec!["Year".to_string()],
|
||||
"the fixture must really encode Year as ONE numeric column"
|
||||
);
|
||||
assert_eq!(
|
||||
features.0[0].spelling,
|
||||
crate::Spelling::Artefact(Artefact::NumericCategory),
|
||||
"a numeric column over a category column is unspellable"
|
||||
);
|
||||
|
||||
let tree = split(0, 2020.5, leaf(1.0), leaf(2.0));
|
||||
let err = fitted_formula(&tree, &features, TARGET, MEASURE, &document())
|
||||
.expect_err("a split on a numeric category is not storable")
|
||||
.to_string();
|
||||
assert!(err.contains("Year"), "{err}");
|
||||
assert!(err.contains("cannot spell"), "{err}");
|
||||
}
|
||||
|
||||
/// The property the *lying* test could not state: if `fitted_formula` says
|
||||
/// `Ok`, no cell of the target is a `ref:` error. See the evidence file.
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user