improvise-x1ns (2nd): the preflight counts a date measure's parts
P1 from ChatGPT Codex on #97, live at f128087 — in the change that fixed the
first instance of this same defect.
`preprocess` infers Numeric -> Date -> Categorical, and `encoded_width` had no
date arm at all. A date string does not parse as f64, so a measure column holding
dates fell through to the categorical count. `build_date` emits
epoch_day/year/month/day/dow, and seven parts once any cell carries a time, so a
column with ONE distinct timestamp promised 1 and materialised 5. Against a
budget of 64 that is an under-count of four on a single column, in the direction
that spends the allocation the preflight exists to prevent.
The fix is structural rather than another remembered branch, because two
instances one branch apart is the tell. `inferred_kind` reproduces `preprocess`'s
own match arm for arm and returns a `Forced`; `encoded_width` matches that with
NO wildcard, so a fourth variant is a compile error here instead of silently
taking the categorical arm — which is exactly how Date was missed. The .missing
flag moves out of the arms, since every branch owes it on the same terms.
The fixture carries a time case because of the RIVAL FIX, not the bug: "a date
column is 5" and "5, or 7 when any cell has a time" both repair the plain-date
failure and differ only on a column containing a time. Injected, `date_parts`
hard-coded to 5, the plain-date case still PASSES and the time case fails at
"promised 8 ... materialised 10". Injecting the bug proves the test sees the bug;
injecting the rival proves it sees the rule. Both arms compare against
`data.feature_names.len()`, never against a width computed a second way.
Also recorded in gp.md: the "must never under-count" line was written as a
proposed kb.pl invariant while the tree already held this counter-example. An
invariant generalised from the defect just fixed generalises the fix, not the
code; the question that would have caught it is "what else does `preprocess` do
that I have not mirrored", which is about the other function and not the bug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
+37
-5
@@ -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
|
||||
|
||||
@@ -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*.
|
||||
|
||||
+129
-15
@@ -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::<f64>().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::<f64>().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::<std::collections::HashSet<_>>()
|
||||
.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]
|
||||
|
||||
Reference in New Issue
Block a user