diff --git a/context/code/gp.md b/context/code/gp.md index 65e7667d..cb8fa596 100644 --- a/context/code/gp.md +++ b/context/code/gp.md @@ -561,14 +561,38 @@ the fix (`Label`), because that is what the user actually wants. side of the cost: a category with a distinct value per record allocates a 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 **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; +type inference rather than sharing it. Copying the predicate is the lesser evil; the alternative is exporting the inference and calling this a second implementation of it. +**The debt that copy carries is that the preflight owes EVERY branch +`preprocess` can take, and it paid two of three.** `preprocess` infers +`Numeric → Date → Categorical`, in that order, and the preflight had no date arm +at all: a column whose cells parse as neither numbers nor a forced category fell +through to the categorical count. So a **measure column holding dates** — one +distinct timestamp in every row — promised **1** and materialised **5**, because +`build_date` emits `epoch_day/year/month/day/dow`, and **7** once any cell +carries a time (improvise-x1ns, second instance, found by ChatGPT Codex on #97). +That is an under-count of four or six on a single column, against a budget of 64. + +The two instances are the same defect one inference-branch apart, which is why +the shape is now structural rather than remembered. `inferred_kind` reproduces +`preprocess`'s match arm for arm and returns a `Forced`, and `encoded_width` +matches that **with no wildcard** — so a fourth `Forced` variant is a compile +error here instead of silently taking the categorical arm, which is exactly how +`Date` was missed. The `.missing` flag is added once outside the match, because +every branch owes it on the same terms. + +The count each arm owes: + +| `preprocess` builds | features | +|---|---| +| `build_numeric` | 1 | +| `build_date` | 5, or 7 if any present cell has a time | +| `build_categorical` | distinct present values, **+1** for the reserved `(missing)` | + +plus `col.missing` wherever any cell is missing, and clamped to at least 1. + **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, @@ -603,6 +627,14 @@ code it described was one commit old and one file away. over-count, and a column whose cells are *entirely* missing is counted as one where `preprocess` skips it altogether. It must never under-count. +**That sentence was written as a proposed `kb.pl` invariant while the tree held a +counter-example**, in the same change that fixed the categorical one — the date +branch was missing the whole time and nothing in the hand-back noticed. An +invariant proposed from the defect you just fixed generalises the *fix*, not the +code; the thing that would have caught it is the question "what else does +`preprocess` do that I have not mirrored", which is a question about the other +function and not about the bug. + `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 search and permutation importance over all of them. Measured at 105 ms for 2,000 diff --git a/context/test-evidence/gp.md b/context/test-evidence/gp.md index beaa3c77..aa741972 100644 --- a/context/test-evidence/gp.md +++ b/context/test-evidence/gp.md @@ -863,3 +863,51 @@ does **not** parse, because `filter_value` is `string | pipe_quoted | bare_ident and a bare number is none of them. That is why `formula_text` pipe-quotes a numeric item name through `quote_name`, and asserting it here keeps the positive arm from passing for the trivial reason that everything parses. + +### the_preflight_counts_the_parts_a_date_measure_expands_into + +improvise-x1ns, **second instance** — found by ChatGPT Codex on PR #97, live at +`f128087`, i.e. in the very change that fixed the first one. + +`preprocess` infers `Numeric → Date → Categorical` (`data.rs:406`), and +`encoded_width` had **no date arm**: numeric-or-categorical only. A date string +does not parse as `f64`, so a measure column holding dates took the categorical +arm and was counted by its distinct strings. `build_date` emits +`epoch_day/year/month/day/dow` — seven parts once any cell has a time +(`data.rs:326`) — so a column with **one** distinct timestamp promised 1 and +materialised 5. + +Fails before the fix, on the plain-date case: + +``` +a plain date: the preflight under-reported: it promised 4 features and +`preprocess` materialised 8 — ["Region=East", "Region=North", "Region=South", +"When.epoch_day", "When.year", "When.month", "When.day", "When.dow"] +``` + +**The fixture carries a time case because of the rival fix, not because of the +bug.** Two implementations repair the plain-date failure: "a date column is 5 +features", and "5, or 7 when any present cell carries a time". They differ only +on a column containing a time, so the second sub-case is what separates them. +Injected, `date_parts` hard-coded to 5: + +``` +a date with a time: the preflight under-reported: it promised 8 features and +`preprocess` materialised 10 — [… "When.hour", "When.minute"] +``` + +Note the plain-date sub-case **passes** under that injection — so without the +time fixture this test would have been satisfied by either reading, and would +have pinned the bug without pinning the rule. + +Both arms compare against `data.feature_names.len()` rather than against a width +computed a second way, so the test cannot pass by agreeing with a stale constant +or by reimplementing the thing under test. Two preconditions guard vacuity: +every `When` cell really is the same string (so the categorical count really +would be 1), and the fixture's timestamp really does or does not carry a time, +asked of `parse_datetime` rather than of the literal's shape. + +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*. diff --git a/crates/improvise-gp/src/fit.rs b/crates/improvise-gp/src/fit.rs index b00d8fa8..00facbad 100644 --- a/crates/improvise-gp/src/fit.rs +++ b/crates/improvise-gp/src/fit.rs @@ -1,7 +1,7 @@ //! One fit, end to end: a [`View`]'s records in, an improvise [`Formula`] and the //! evidence for believing it out. It never prints — `context/code/gp.md` §14. -use gp_tree::data::{Forced, Overrides, Target, preprocess}; +use gp_tree::data::{Forced, Overrides, Target, parse_datetime, preprocess}; use gp_tree::fit::{ FitConfig, RegressionQuality, cv_regress, denormalize, fit, importance_ranking, mse, permutation_importance, regression_quality, standardize, @@ -13,6 +13,31 @@ use improvise_formula::Formula; use crate::dataset::{FeatureTable, feature_table}; +/// Which branch `preprocess` would take for this column. It mirrors that function's +/// own `match`, arm for arm — the preflight owes every branch, `context/code/gp.md` §14. +fn inferred_kind(present: &[&str], is_document_category: bool) -> Forced { + match present.is_empty() { + // `preprocess` SKIPS an entirely-missing column. Counting it as one + // categorical over-counts by one, which is the safe direction. + true => Forced::Categorical, + false if is_document_category => Forced::Categorical, + false if present.iter().all(|c| c.parse::().is_ok()) => Forced::Numeric, + false if present.iter().all(|c| parse_datetime(c).is_some()) => Forced::Date, + false => Forced::Categorical, + } +} + +/// `build_date` emits five parts, or seven once any cell carries a time. +fn date_parts(present: &[&str]) -> usize { + match present + .iter() + .any(|c| parse_datetime(c).is_some_and(|d| d.has_time)) + { + true => 7, + false => 5, + } +} + /// A cheap upper bound on how wide `preprocess` would expand this table, computed /// without expanding it. **Advisory, not authoritative** — `context/code/gp.md` §14. fn encoded_width(table: &FeatureTable, category_columns: usize) -> usize { @@ -29,23 +54,22 @@ fn encoded_width(table: &FeatureTable, category_columns: usize) -> usize { .copied() .filter(|c| !gp_tree::data::is_missing(c)) .collect(); - let has_missing = present.len() < cells.len(); - // A document category is forced categorical below, so the preflight - // counts it by cardinality too. Where the two still differ: gp.md §14. - let numeric = col >= category_columns - && !present.is_empty() - && present.iter().all(|c| c.parse::().is_ok()); - 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, plus the one - // `build_categorical` reserves for `(missing)` — gp.md §14. - false => (present + // Every branch adds the `.missing` flag on the same terms, so it is + // added once here rather than in each arm. + let missing = usize::from(present.len() < cells.len()); + // No wildcard: a new `Forced` variant must be counted here rather + // than silently falling into the categorical arm. + match inferred_kind(&present, col < category_columns) { + Forced::Numeric => 1 + missing, + Forced::Date => date_parts(&present) + missing, + // One column per distinct value, plus the one `build_categorical` + // reserves for `(missing)` — gp.md §14. + Forced::Categorical => (present .iter() .collect::>() .len() - + usize::from(has_missing)) - .max(1), + + missing) + .max(1), } }) .sum() @@ -717,6 +741,96 @@ mod tests { (model, view) } + /// Bug improvise-x1ns (2nd): `preprocess` infers Date BEFORE Categorical and + /// `build_date` emits 5 or 7 parts. Evidence: `context/test-evidence/gp.md`. + #[test] + fn the_preflight_counts_the_parts_a_date_measure_expands_into() { + for (label, stamp, parts) in [ + ("a plain date", "2024-01-15", 5), + ("a date with a time", "2024-01-15T09:30:00", 7), + ] { + let (model, view) = a_date_valued_measure(stamp); + let table = feature_table(&model, &view, "Score").expect("the table builds"); + let category_columns = model.regular_category_names().len(); + + // The fixture must really be the shape the bug needs: ONE distinct + // date, which the categorical arm would count as a single column. + let when: Vec<&str> = table + .rows + .iter() + .map(|r| r[category_columns].as_str()) + .collect(); + assert!( + when.iter().all(|c| *c == stamp), + "{label}: every When cell must be the same date: {when:?}" + ); + assert!( + gp_tree::data::parse_datetime(stamp).is_some_and(|d| d.has_time == (parts == 7)), + "{label}: the fixture must really carry a time iff {parts} parts are expected" + ); + + 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); + + assert_eq!( + data.feature_names + .iter() + .filter(|n| n.starts_with("When.")) + .count(), + parts, + "{label}: the fixture must really expand into {parts} date parts — {:?}", + data.feature_names + ); + assert!( + width >= data.feature_names.len(), + "{label}: the preflight under-reported: it promised {width} features \ + and `preprocess` materialised {} — {:?}", + data.feature_names.len(), + data.feature_names, + ); + } + } + + /// One regular category and a `When` MEASURE whose every cell is the same + /// timestamp — the low-cardinality date column a distinct-count misses. + fn a_date_valued_measure(stamp: &str) -> (Model, View) { + let mut model = Model::new("sales"); + model.add_category(REGION).expect("under the limit"); + let measure = model.category_mut(MEASURE).expect("_Measure always exists"); + for item in ["When", "Score"] { + measure.add_item(item); + } + for (i, region) in ["North", "South", "East"].iter().enumerate() { + model + .category_mut(REGION) + .expect("just added") + .add_item(*region); + model.set_cell( + CellKey::new(vec![ + (REGION.to_string(), (*region).to_string()), + (MEASURE.to_string(), "When".to_string()), + ]), + CellValue::Text(stamp.to_string()), + ); + model.set_cell( + CellKey::new(vec![ + (REGION.to_string(), (*region).to_string()), + (MEASURE.to_string(), "Score".to_string()), + ]), + CellValue::Number(i as f64), + ); + } + let mut view = View::new("Default"); + for cat in model.category_names() { + view.on_category_added(cat); + } + view.set_axis(REGION, Axis::Row); + view.set_axis(MEASURE, Axis::Page); + (model, view) + } + /// Bug improvise-x1ns: the preflight counted a forced categorical by its /// PRESENT values only. Evidence: `context/test-evidence/gp.md`. #[test]