From 173d9820a6714d901b4a956c9418a780165f11f4 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:51:17 -0700 Subject: [PATCH] feat(gp): decline the artefact families separately, in their own words MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-wd6n. Spelling::Artefact was one variant standing for five distinct families, the last five reached through a `_ =>` wildcard in spell(). The one refusal they fed said the tree "splits on a col.missing flag or a date part", so three of the five were declined in words naming something they are not. Artefact now names which family, spell() is exhaustive (a sixth FeatureRole is a compile error rather than a sixth thing that message misdescribes), and the refusal gives one clause per family. The two families the ticket asks about are declined in writing in context/code/gp.md §11, separately, because each needs different vocabulary: col.missing needs a word for absence that reaches apply_agg and eval_agg, and the date parts need a date type improvise does not have anywhere. Nothing new is emitted, so improvise-4zl7's route-then-aggregate rule is untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/gp.md | 55 ++++++++-- context/test-evidence/gp.md | 30 ++++++ crates/improvise-gp/src/lib.rs | 185 +++++++++++++++++++++++++++++---- 3 files changed, 242 insertions(+), 28 deletions(-) diff --git a/context/code/gp.md b/context/code/gp.md index a3409c94..703779fd 100644 --- a/context/code/gp.md +++ b/context/code/gp.md @@ -289,7 +289,19 @@ never by the shape of the name: |---|---|---| | `Numeric` | a measure | `Expr::Ref(measure)` | | `OneHot { value }` | a category, `value != "(missing)"` | `Cat IS Item` | -| anything else | | `Artefact` | +| `Numeric` | a category | `Artefact(NumericCategory)` | +| `DatePart` | | `Artefact(DatePart)` | +| `Missing` | | `Artefact(MissingFlag)` | +| `OneHot` | a measure | `Artefact(ValueOfAMeasure)` | +| `OneHot { "(missing)" }` | a category | `Artefact(AbsentCoordinate)` | + +The last five arms were **one** `_ => Artefact` until improvise-wd6n, and the +wildcard was not merely untidy. The refusal it fed read "a `col.missing` flag or +a date part is an encoding artefact", so three of the five families were refused +in words naming something they are not — and a future `FeatureRole` variant +would have joined them silently, declined under whichever family the message +happened to mention. The match is now exhaustive, so a sixth role is a compile +error rather than a sixth thing that message misdescribes. The two exclusions are not fussiness. A one-hot over a **measure** column names no coordinate at all — the record's value for that measure is text, and @@ -326,14 +338,41 @@ flags stay covered without a second copy of `preprocess`'s naming rules. `Artefact` reaching it is a precondition failure and panics, the same split `number_text` makes for a non-finite value. -### The two thirds this still does not close +### The two families improvise-wd6n declines, separately -`col.missing` means "this record has no value for measure X" — a different -vocabulary, and one the formula language has no word for at all. The date -sub-features (`when.year`, `when.month`) need date arithmetic improvise does not -have. Both stay `Artefact`, both stay refused, and both are filed as -improvise-wd6n rather than smuggled in — separately from each other, because -they need different vocabulary. +Both stay `Artefact` and both stay refused. The decline is written here rather +than left open, because each needs a **formula-language** change and neither +needs an emitter change — and they are answered separately because the +vocabulary each wants is different. What changed with the decline is that the +refusal now says which, in that family's own words. + +**`col.missing` is declined because the language has no word for absence.** The +flag means "this record has no value for measure X". `Expr::Ref` over an absent +cell is an `Error`, not a zero, so there is no existing spelling to reuse and no +way to phrase the question as an expression. Closing it means deciding what an +absence *is* in a language whose aggregates already treat it as "no rows" — and +that decision reaches `apply_agg`, `eval_agg` and the text-unanimity rule, not +just this emitter. It is not worth that for a split a fit happens to like. + +**The date sub-features are declined because improvise has no dates.** +`when.epoch_day`, `.year`, `.month`, `.day`, `.dow`, and with a time component +`.hour`, `.minute` — `gp_tree::data::build_date` emits them. A `.improv` cell +holding a date is `CellValue::Text`; nothing in `eval_expr` reads one, and there +is no date type, no date literal and no date arithmetic anywhere in the crate. +Closing this is a type addition to the value language, and the sub-features are +the *smallest* part of it: a user who could split on `when.year` would +immediately want to filter and aggregate by it. + +Both are visible to the user only as a refusal naming the feature and the +reason, which is the right interim: a fit that cannot be stored says so, rather +than storing a formula whose every cell is a reference error (improvise-8xhy). +`improvise-4zl7`'s route-then-aggregate rule (`context/code/core.md` §13) is +untouched — nothing new is emitted, so there is nothing new for it to hold over. + +The other three families are declined permanently rather than pending: a one-hot +over a measure and the `(missing)` pseudo-item name no coordinate *by +construction*, and a numeric category column is a name collision the document +itself creates. No vocabulary closes those; they are refusals, not gaps. ## 12. A fit is persisted as its formula, and nothing else (improvise-7na) diff --git a/context/test-evidence/gp.md b/context/test-evidence/gp.md index 74bf7c89..4be5232e 100644 --- a/context/test-evidence/gp.md +++ b/context/test-evidence/gp.md @@ -577,3 +577,33 @@ value is written out in the test name or the assertion. The third assertion is what stops this one being satisfiable by accident: routing once on the total lands on a single leaf, so the test refuses a fixture in which a leaf happens to equal the right answer. + +### a_missing_flag_a_date_part_and_a_measures_value_are_refused_differently + +improvise-wd6n. `Spelling::Artefact` was one variant standing for **five** +distinct families — `Missing`, `DatePart`, a one-hot over a measure column, the +`(missing)` pseudo-item, and a `Numeric` role over a category column — the last +five reached through a `_ =>` wildcard in `spell`. The single refusal they all +fed said the tree "splits on a `col.missing` flag or a date part", so three of +the five were declined in words naming something they are not. A user fitting +over a text-valued measure column was told about missing-value flags. + +The test drives `Features::of` over a real `preprocess` output rather than +fabricating `Spelling`s — which is the point, since what is under test is +`spell`'s classification, and the helper `artefacts()` beside it fabricates +exactly what `spell` decides. That helper now takes the family as an argument +so a fabricated spelling at least names one on purpose; `Features::of` is still +what really decides, and this test is the one that asks it. + +Each family's assertion has two clauses, and the second is what earns it: the +error must contain that family's reason **and not the other two's**. The first +clause alone passes against one message naming every family at once, which is +precisely the state the ticket describes. Fault injected: the five arms of +`Artefact::reason` collapsed to one shared string — the test fails on the first +family, and with the injection removed all three arms pass. + +The fixture reaches three families from one `preprocess` call (a text measure +column, a measure with a hole in it, a column of ISO dates). The remaining two — +`AbsentCoordinate` and `NumericCategory` — are not reachable from it and are not +asserted here; they are refusals rather than pending gaps, argued in +`context/code/gp.md` §11. diff --git a/crates/improvise-gp/src/lib.rs b/crates/improvise-gp/src/lib.rs index 10986aea..9440ce92 100644 --- a/crates/improvise-gp/src/lib.rs +++ b/crates/improvise-gp/src/lib.rs @@ -24,9 +24,54 @@ pub enum Spelling { Measure(String), /// `Cat IS Item` — a coordinate predicate; the document must have `Cat`. Coordinate { category: String, item: String }, - /// An encoding artefact: a `col.missing` flag or a date part, which the - /// formula language cannot spell at all. - Artefact, + /// An encoding artefact the formula language cannot spell at all. The arm + /// names *which* family, because they are declined for different reasons. + Artefact(Artefact), +} + +/// Why a feature is unspellable. Five families, declined separately — each needs +/// different vocabulary to close. `context/code/gp.md` §11 carries the arguments. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Artefact { + /// `col.missing`: "this record has no value for X" (improvise-wd6n). + MissingFlag, + /// `when.year` and its siblings: date arithmetic (improvise-wd6n). + DatePart, + /// A one-hot over a *measure* column — the record's value, not a coordinate. + ValueOfAMeasure, + /// The `(missing)` pseudo-item: the absence of a coordinate, not an item. + AbsentCoordinate, + /// A category column whose items all parse as numbers; a reference to its + /// name would resolve to a `_Measure` item, which is a different column. + NumericCategory, +} + +impl Artefact { + /// The refusal's own clause — read off a status bar, so one line each. + fn reason(self) -> &'static str { + match self { + Self::MissingFlag => { + "says this record has no value for that measure, which the formula language \ + has no word for: a reference to an absent cell is an error, not a zero" + } + Self::DatePart => { + "is a part of a date, and date arithmetic is something improvise does not have \ + anywhere: a cell holding a date is text, and nothing in eval reads one" + } + Self::ValueOfAMeasure => { + "is a value of a measure rather than a coordinate, so `IS` cannot ask about it \ + — the record's value for that column is text" + } + Self::AbsentCoordinate => { + "is the absence of a coordinate rather than an item, which `IS` reads as false \ + rather than as a question" + } + Self::NumericCategory => { + "names a category whose items all parse as numbers, so a reference to it would \ + resolve to a `_Measure` item of the same name — a different column" + } + } + } } /// One fitted feature: what `preprocess` called it, and how improvise says it. @@ -84,7 +129,13 @@ fn spell(role: &FeatureRole, columns: &[ColumnSpec], categories: &[String]) -> S item: value.clone(), } } - _ => Spelling::Artefact, + // No wildcard: a new `FeatureRole` must be classified rather than + // silently declining as whichever family the message happened to name. + FeatureRole::Numeric { .. } => Spelling::Artefact(Artefact::NumericCategory), + FeatureRole::DatePart { .. } => Spelling::Artefact(Artefact::DatePart), + FeatureRole::Missing { .. } => Spelling::Artefact(Artefact::MissingFlag), + FeatureRole::OneHot { .. } if !is_category => Spelling::Artefact(Artefact::ValueOfAMeasure), + FeatureRole::OneHot { .. } => Spelling::Artefact(Artefact::AbsentCoordinate), } } @@ -98,6 +149,31 @@ pub fn formula_text(tree: &Tree, features: &Features, target: &str) -> String { ) } +/// The refused features grouped by family, each family's names in tree order. +/// Grouping is what lets the refusal give one reason per family, not per feature. +fn artefacts_by_family<'a>(used: &[&'a Feature]) -> Vec<(Artefact, Vec<&'a str>)> { + let mut families: Vec<(Artefact, Vec<&str>)> = Vec::new(); + for feature in used { + let Spelling::Artefact(family) = feature.spelling else { + continue; + }; + match families.iter_mut().find(|(f, _)| *f == family) { + Some((_, names)) => names.push(feature.name.as_str()), + None => families.push((family, vec![feature.name.as_str()])), + } + } + families +} + +/// One clause per family: the names it refused, then that family's own reason. +fn artefact_clauses(families: &[(Artefact, Vec<&str>)]) -> String { + families + .iter() + .map(|(family, names)| format!("{names:?} {}", family.reason())) + .collect::>() + .join("; and ") +} + /// [`formula_text`], parsed **and** checked against the document it is to be stored /// in — the form improvise stores. Why, and what is left: `context/code/gp.md` §11. pub fn fitted_formula( @@ -108,18 +184,14 @@ pub fn fitted_formula( model: &Model, ) -> anyhow::Result { let used = used_features(tree, features); - let artefacts: Vec<&str> = used - .iter() - .filter(|f| f.spelling == Spelling::Artefact) - .map(|f| f.name.as_str()) - .collect(); + let artefacts = artefacts_by_family(&used); anyhow::ensure!( artefacts.is_empty(), // One line, because it is read off a status bar: a `rustfmt` pass over a // multi-line literal leaves runs of ten spaces mid-sentence. - "the fitted tree splits on {artefacts:?}, which the formula language cannot spell \ - at all — a `col.missing` flag or a date part is an encoding artefact rather than \ - anything {target:?} could refer to. Fit over measures and categories for now", + "the fitted tree splits on features the formula language cannot spell at all, so \ + nothing {target:?} could refer to: {}. Fit over measures and categories for now", + artefact_clauses(&artefacts), ); let unresolvable: Vec<&str> = used .iter() @@ -147,7 +219,7 @@ fn resolves_in(spelling: &Spelling, model: &Model) -> bool { match spelling { Spelling::Measure(name) => model.find_item_category(name).is_some(), Spelling::Coordinate { category, .. } => model.category(category).is_some(), - Spelling::Artefact => false, + Spelling::Artefact(_) => false, } } @@ -203,7 +275,7 @@ fn spelling_text(spelling: &Spelling) -> String { Spelling::Coordinate { category, item } => { format!("{} IS {}", quote_name(category), quote_name(item)) } - Spelling::Artefact => { + Spelling::Artefact(_) => { panic!("expr_text wants a feature the formula language can spell") } } @@ -272,14 +344,15 @@ mod tests { ) } - /// Features no formula can spell: a `col.missing` flag or a date part. - fn artefacts(names: &[&str]) -> Features { + /// Features no formula can spell. The family is named rather than assumed: + /// these spellings are fabricated, and `Features::of` is what really decides. + fn artefacts(family: Artefact, names: &[&str]) -> Features { Features( names .iter() .map(|n| Feature { name: (*n).to_string(), - spelling: Spelling::Artefact, + spelling: Spelling::Artefact(family), }) .collect(), ) @@ -607,7 +680,7 @@ mod tests { /// `improvise_eval`, which fabricates an item per feature name (improvise-7na). mod evaluability_in_a_real_document { use super::{MEASURE, TARGET, artefacts, coordinates, leaf, measures, split}; - use crate::{fitted_formula, formula_text}; + use crate::{Artefact, fitted_formula, formula_text}; use improvise_core::formula::parse_formula; use improvise_core::model::cell::{CellKey, CellValue}; use improvise_core::model::types::Model; @@ -723,9 +796,14 @@ mod tests { /// enumerates them: the check asks the precondition `Expr::Ref` has. #[test] fn the_missing_flag_and_the_date_sub_features_are_refused_too() { - for name in ["Cost.missing", "when.year", "when.month"] { + for (family, name) in [ + (Artefact::MissingFlag, "Cost.missing"), + (Artefact::DatePart, "when.year"), + (Artefact::DatePart, "when.month"), + ] { let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); - let err = fitted_formula(&tree, &artefacts(&[name]), TARGET, MEASURE, &document()) + let features = artefacts(family, &[name]); + let err = fitted_formula(&tree, &features, TARGET, MEASURE, &document()) .expect_err(name) .to_string(); assert!(err.contains(name), "{err}"); @@ -765,4 +843,71 @@ mod tests { assert!(fitted_formula(&tree, &features, TARGET, MEASURE, &model).is_ok()); } } + /// improvise-wd6n. The artefact families are refused *separately*, because + /// they are declined for different reasons. Evidence: `context/test-evidence/gp.md`. + mod each_artefact_family_is_refused_in_its_own_words { + use super::{MEASURE, TARGET, leaf, split}; + use crate::{Features, fitted_formula}; + use gp_tree::data::{Overrides, preprocess}; + use improvise_core::model::types::Model; + + /// One document category (`Region`), a text-valued measure, a measure with a + /// hole in it, and a date column — one row per artefact family. + fn features() -> Features { + let header: Vec = ["Region", "Grade", "Cost", "When", "Score"] + .iter() + .map(|s| s.to_string()) + .collect(); + let rows: Vec> = [ + ["North", "A", "40", "2024-01-02", "1"], + ["South", "B", "", "2024-06-03", "2"], + ["North", "A", "70", "2024-09-04", "3"], + ["South", "B", "10", "2024-11-05", "4"], + ] + .iter() + .map(|r| r.iter().map(|s| s.to_string()).collect()) + .collect(); + let data = preprocess(&header, &rows, &Overrides::default()); + Features::of(&data, &header[..1]) + } + + fn refusal_for(features: &Features, name: &str) -> String { + let feat = (0..features.len()) + .find(|i| features.get(*i).name == name) + .unwrap_or_else(|| { + panic!( + "preprocess emitted no {name:?}; it emitted {:?}", + (0..features.len()) + .map(|i| features.get(i).name.as_str()) + .collect::>() + ) + }); + let tree = split(feat, 0.5, leaf(1.0), leaf(2.0)); + fitted_formula(&tree, features, TARGET, MEASURE, &Model::new("d")) + .expect_err(name) + .to_string() + } + + /// Each family's own reason, and **none of the others'** — one message + /// naming every family at once would pass the first clause alone. + #[test] + fn a_missing_flag_a_date_part_and_a_measures_value_are_refused_differently() { + let features = features(); + let missing = "has no value for"; + let date = "date arithmetic"; + let text = "a value of a measure"; + + let err = refusal_for(&features, "Cost.missing"); + assert!(err.contains(missing), "{err}"); + assert!(!err.contains(date) && !err.contains(text), "{err}"); + + let err = refusal_for(&features, "When.year"); + assert!(err.contains(date), "{err}"); + assert!(!err.contains(missing) && !err.contains(text), "{err}"); + + let err = refusal_for(&features, "Grade=A"); + assert!(err.contains(text), "{err}"); + assert!(!err.contains(missing) && !err.contains(date), "{err}"); + } + } }