From 022e12f9b4ef5ac1d8672d34bac81dcff3bc79d8 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 11:52:27 -0700 Subject: [PATCH 1/4] fit: route per record and aggregate, never route on the aggregate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-4zl7. A tree is fitted on individual records; an improvise formula is evaluated at a CellKey, and an aggregating cell used to aggregate each Expr::Ref first and evaluate the IF once on the totals. That is the same answer for a linear formula and a different one for anything that routes, so every fitted tree meant one thing on a record row and something else one row up. The user ruled: aggregate the rule over the members. The rule needed no new aggregate vocabulary. SUM(e) already means "fold e over the members" and simply did not: eval_agg specialised Expr::Ref and, for every other inner expression, summed the raw matching cells and ignored the expression entirely. It now folds — one entry per member record, the expression evaluated at that record's own coordinates. eval_agg is shared by eval_expr and eval_expr_cached, so the fold has exactly one home; each evaluator passes its own recursion in. Expr::Is(Filter) is the new arm in both evaluators: `Cat IS Item` is 1.0 when the cell's own Cat coordinate is Item, keeping the evaluator f64-only and reusing Formula::filter's own context.get/unwrap_or(false). A grammar production in primary, before ref_expr, with both operands through improvise_formula::quote's one home; IS joins the keyword guard. The emitter reads feature index -> role off preprocess's plan rather than reconstructing it: gp_tree::data::feature_roles is the one statement of the expansion order and encode_row now drives off it too. improvise_gp::Features carries one Spelling per feature — Measure, Coordinate or Artefact — decided by role and column, never by the name's shape. A one-hot over a real category is emitted as `Region IS North` and accepted; col.missing and the date parts stay Artefact and stay refused (improvise-wd6n). fit-measure on a document a category explains now lands instead of refusing, and the refusal no longer depends on which features the search happened to pick. Evidence: eight hand-computed tests in improvise-core over a four-record fixture whose Cost straddles the threshold inside each region, so route-then-aggregate and aggregate-then-route differ at every level. Two faults injected and watched to fail — reverting the fold fails 7 of 8, and a constant-false eval_is fails exactly the 3 coordinate tests. Reordering feature_roles fails both it and encode_row_replays_the_training_expansion. The fold is O(members) per evaluation per fixed-point pass and unbenchmarked: improvise-9iyv. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/core.md | 68 ++++ context/code/formula-and-io.md | 29 +- context/code/gp.md | 166 +++++---- context/test-evidence/core.md | 66 ++++ context/test-evidence/gp.md | 109 ++++-- crates/gp-tree/src/data.rs | 182 +++++++-- .../improvise-command/src/command/cmd/fit.rs | 4 +- crates/improvise-core/src/model/types.rs | 223 ++++++++++- crates/improvise-formula/src/ast.rs | 3 + crates/improvise-formula/src/formula.pest | 5 + crates/improvise-formula/src/parser.rs | 72 +++- crates/improvise-formula/src/quote.rs | 2 +- crates/improvise-gp/src/fit.rs | 128 +++---- crates/improvise-gp/src/lib.rs | 346 +++++++++++++----- 14 files changed, 1090 insertions(+), 313 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index ece70251..02ba24a3 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -433,3 +433,71 @@ Registration is guaranteed for every model category: `Workbook::new`, `axis_of`. Use `try_axis_of` when the name comes from anywhere else — `cycle_axis` does, and no-ops on an unknown category to match `set_axis` (improvise-t8s). + +## 13. Route then aggregate: `SUM()` folds over the member records + +improvise-4zl7, and the one place the evaluator reaches past the cell it was +asked about. + +**The problem.** A formula is evaluated at a `CellKey`. In an aggregating cell +`evaluate_aggregated` aggregates each `Expr::Ref` *first* and evaluates the +expression once on the totals. That is exactly right for a linear formula — +`SUM(Revenue) - SUM(Cost)` and `SUM(Revenue - Cost)` are the same number — and +exactly wrong for anything that **routes**. `IF(Cost < 50, 1, 10)` over records +costing 10 and 100 answers `1 + 10 = 11` per record and `10` on the total, +because *aggregate-then-route is not route-then-aggregate*. Every fitted tree +(improvise-8xhy, improvise-vx3) is such a formula, so it meant one thing on a +record row and something else one row up. The user's ruling was to **aggregate +the rule over the members**. + +**The mechanism, and why it needed no new vocabulary.** `SUM(e)` already means +"fold `e` over the members". It simply did not: `eval_agg` specialised +`Expr::Ref` and, for every other inner expression, summed the raw cells matching +the partial key and *ignored the expression entirely*. It now folds — one entry +per member record, the expression evaluated at that record's own coordinates: + +``` +partial = context.without(target_category) (plus any inline WHERE) +records = distinct(matching_cells(partial).map(key.without("_Measure"))) +values = records.map(|r| eval(inner, context.with(every coord of r))) +``` + +Three consequences worth stating. + +The rule is **written once**. `eval_agg` is shared by `eval_expr` and +`eval_expr_cached`, so the fold has one home; the two evaluators pass their own +recursion in as `eval_at`. Only `Expr::Is` needed an arm in each, and the two +arms are one line apiece. + +The `Expr::Ref` fast path **stays**, and it is the same fold specialised: a +record's `Ref` is that record's own cell, so summing them is summing the +matching values. It differs in exactly one place — a record with no such cell is +*skipped* there and an **error** in the fold. That is deliberate: a bare +`SUM(Revenue)` asks for the values that exist, while `SUM()` asks the +expression of each member, and an expression that cannot be evaluated is an +error rather than a zero. Silently dropping members is how a routed sum would +quietly answer for a subset. + +Records come out in `DataStore` insertion order, deduplicated by an `IndexSet`, +for §8's reason — hash order here would make a fitted cell drift in its last +bits between processes. + +**Interaction with the fixed point (§3) and `formula_cache`.** None of this is +new state. The fold reads *raw* data through the same `evaluate_depth` / +`aggregate_raw` the non-folding path uses, so a formula cell still depends only +on raw cells and on other formula cells' cached values; `recompute_formulas` +iterates the same stems to the same fixed point, and `formula_cache` still holds +exactly the aggregated formula cells it held before. The fold does not cache and +does not memoise: it is O(members) per evaluation per pass, so a routed formula +over a large document is the one place this layer got materially more +expensive, and it is unbenchmarked. + +**`Expr::Is(Filter)` — a coordinate predicate as a number.** `Cat IS Item` is +`1.0` when the cell's own `Cat` coordinate is `Item` and `0.0` otherwise. It +keeps the evaluator `f64`-only — no string comparison enters `eval_expr` — and +it reuses the primitive `Formula::filter` already had, `context.get(&cat)`, +including its `unwrap_or(false)`: a cell with **no** `Cat` coordinate is not +that item. Under route-then-aggregate that arm is reached only where a record +genuinely lacks the coordinate, because the fold hands the expression a record +key. Evaluated bare, outside a `SUM`, it degrades exactly as `Formula::filter` +does — which is why a fitted tree is always emitted wrapped. diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index c3d08b8a..36375ee8 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -43,12 +43,18 @@ Three entry points, because the callers do not want the same thing: An item literally named `SUM` is written `|SUM|`. `is_bare_name` is deliberately **stricter than the grammar strictly requires**: bare keywords -(`WHERE SUM AVG MIN MAX COUNT IF`) *do* parse today, but only because +(`WHERE SUM AVG MIN MAX COUNT IF IS`) *do* parse today, but only because `agg_call` needs a `(` and PEG backtracking then falls through to `ref_expr` — an emergent property of **rule ordering**, not a stated rule. A pipe-quoted name is an identifier in every position, with nothing to fall through, so a name colliding with a keyword is quoted and stops depending on that ordering. +`IS` joined the list with `is_expr` (improvise-4zl7), and it is the one keyword +whose omission would have been *silently* wrong rather than merely fragile: a +category literally named `IS` written bare turns `IS IS East` into a predicate +on a category called `East`. Adding a production to `formula.pest` means adding +its keyword here — the two are one change. + The cost is worth paying because a `## Formulas` line carries formula text **verbatim**: a name `format_md` writes is later read by `parse_formula`. One namespace, one spelling rule. @@ -65,6 +71,27 @@ io returned it and `parser.rs` panicked; the survivor returns it. --- +## `Cat IS Item`: a coordinate predicate in the expression grammar + +`is_expr = { identifier ~ ^"IS" ~ filter_value }` sits in `primary` **before** +`ref_expr`, which would otherwise consume the category name and leave `IS Item` +unparseable. A bare name still parses as a reference: `is_expr` needs the +keyword, fails without it, and PEG backtracks into `ref_expr`. + +It reuses `where_clause`'s two operands — an `identifier` for the category and a +`filter_value` for the item — and therefore `build_filter`, which already reads +exactly that pair. The `^"IS"` literal is a string in the grammar rather than a +rule, so it contributes no pair and `into_inner()` yields the two operands and +nothing else. One builder, one quoting rule, both halves through +`improvise_formula::quote`. + +The grammar-walking generator needs no new case: it reads `formula.pest` through +`pest_meta` at test time, so a production added to `primary` is generated and +round-tripped from the day it exists. + +What the *evaluator* does with it, and why a fitted tree is always wrapped in a +`SUM`, is `context/code/core.md` §13. + ## A formula target is unquoted, exactly as a reference is `parse_formula` strips the pipes off the target the same way it does off an diff --git a/context/code/gp.md b/context/code/gp.md index 336e0bba..a3409c94 100644 --- a/context/code/gp.md +++ b/context/code/gp.md @@ -209,14 +209,22 @@ A regression tree *is* a nested conditional, and improvise formulas already have an ordinary formula with nothing GP-specific left in the file: ```text -Score = IF(humidity < 77.5, 1.25, IF(|outlook=rainy| < 0.5, -0.5, 2)) +Score = SUM(IF(humidity < 77.5, 1.25, IF(outlook IS rainy < 0.5, -0.5, 2))) ``` -(That sample used to sit in `improvise-gp/src/lib.rs`'s module doc inside a -```text fence. rustdoc does not execute a `text` fence, so it was prose wearing -a fence rather than a doctest, and the two-line cap applies to it like anything -else. Note it is also an example of a formula `fitted_formula` would **refuse**, -`outlook=rainy` being an encoding artefact — see §11.) +(An earlier form of that sample sat in `improvise-gp/src/lib.rs`'s module doc +inside a ```text fence. rustdoc does not execute a `text` fence, so it was prose +wearing a fence rather than a doctest, and the two-line cap applies to it like +anything else.) + +Two things in it are improvise-4zl7's, and both are load-bearing. The outer +`SUM` is the **route-then-aggregate rule made syntax**: a tree is fitted on +records, and `SUM(e)` is improvise's existing word for "fold `e` over the member +records" (`context/code/core.md` §13). Without it the formula routes on the +totals one row up and means something else there; with it, a record row is a +one-member fold and answers the rule itself. `outlook IS rainy` is the one-hot +`outlook=rainy` said as a **coordinate predicate** rather than as a reference to +an item no document has — see §11 for which features get which spelling. Two properties the emitter owes its caller: @@ -258,58 +266,74 @@ items of the document**, and `Expr::Ref` resolves through `Model::find_item_category` — so a tree splitting on one emitted a perfectly well-formed formula whose every cell then evaluated to `Error("ref:Region=North")`, with nothing between the fit and the file to say so -(improvise-8xhy). +(improvise-8xhy). improvise-4zl7 closed the categorical third of that by giving +the emitter a vocabulary for a coordinate, and by giving the *evaluator* a rule +that makes such a formula mean the same thing at every level of aggregation +(`context/code/core.md` §13). -`fitted_formula` therefore takes the `&Model` and refuses, listing what it could -not resolve. `formula_text` still spells it, being the tree's own text. +### `Features`: one entry per feature index, read off the plan -**The check asks the document, never the name's shape.** A measure literally -named `Region=North` is a legal document and is accepted; the two are -indistinguishable as strings, and only the document can tell them apart. Asking -the real precondition also covers `.missing`, the date parts, and every future -sub-feature naming scheme with no second copy of `preprocess`'s expansion rules -to drift from. +`Features::of(&Dataset, categories)` walks `gp_tree::data::feature_roles` — the +**one** statement of `preprocess`'s expansion order, which `encode_row` drives +off too — and pairs each role with the name `preprocess` gave it. A tree's +`feat` indexes `Features` directly, so nothing anywhere reconstructs +index → role from the feature *names*. `Spec::names` is `&[&str]` and could not +have carried this; the earlier check matched names against the raw plan instead +and said in this section that it was avoiding a second implementation of the +expansion. Now there is no second implementation to avoid. -**This is an interim, and deliberately the smallest of three remedies.** -Materialising the one-hots as real `_Measure` items pollutes the document with -encoding artefacts the user never asked for. Teaching the emitter improvise's -`WHERE` vocabulary needs a *scalar coordinate predicate* the formula language -does not have — `Filter` gates a formula or an aggregate, it is not an `Expr` — -so it needs a new `Expr` variant, a grammar production, and an answer to what -the predicate means in a cell that aggregates over the category; and it would -close only the categorical third, leaving `.missing` and the date parts exactly -here. That is improvise-4zl7. +Each feature gets one of three `Spelling`s, decided **by role and by column**, +never by the shape of the name: -### The hole a name cannot close +| role | column | spelling | +|---|---|---| +| `Numeric` | a measure | `Expr::Ref(measure)` | +| `OneHot { value }` | a category, `value != "(missing)"` | `Cat IS Item` | +| anything else | | `Artefact` | -`fitted_formula`'s question is "can this be spelled", and that is the right -question for *that* check and wrong as the only check. A numeric category called -`Cost` produces a feature called `Cost`; if a `_Measure` item is also called -`Cost`, the emitted `Expr::Ref("Cost")` resolves cleanly — **to the measure**, -which is not the column the tree was fitted on. +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 +`Cat IS Item` asks about coordinates. `(missing)` is the *absence* of a +coordinate, which `Expr::Is` reads as `0` by §13's `unwrap_or(false)`, so the +one spelling that would be wrong is exactly the one omitted. A `Numeric` role +over a **category** column is the trap the previous version of this section +called "the hole a name cannot close": a category whose items all parse as +numbers produces a feature named after the category, and `Expr::Ref` would +resolve it to a `_Measure` item of the same name — a different column. Both +halves now fall out of `is_category`, which is a lookup in the header's own +first `category_columns` entries. -`fit::only_raw_measure_features` closes it by asking `preprocess`'s **plan** -instead, and **by name, never by position**. `Dataset::columns` is *not* -index-aligned with the header: `preprocess` `continue`s past an entirely-missing -column without pushing a `ColumnSpec`, so one empty category shifts every later -spec left and a positional `i >= category_columns` test reads a numeric measure -as a category and refuses a perfectly good fit. +Duplicate header names are still refused outright, by `no_ambiguous_column_names` +and before anything else: `is_category` decides by name, so a name that means two +columns is a spelling with two meanings. -The header that `feature_table` built is the reliable statement of role: its -first `category_columns` entries are `Model::regular_category_names`, the rest up -to the target are measures. A used feature is accepted only when its name is a -**measure** column of the header *and* the plan gives that column kind -`Numeric` — which rules out every one-hot, every `.missing` flag, every date -part and every category coordinate, by role and not by spelling. Duplicate -header names are refused outright first, since a name that means two columns -cannot be resolved by either check. +`Dataset::columns` is **not** index-aligned with the header — `preprocess` +`continue`s past an entirely-missing column without pushing a `ColumnSpec`, so +one empty category shifts every later spec left. `FeatureRole::column()` indexes +`columns`, not the header, so that shift is carried rather than tripped over. -It deliberately does **not** reconstruct the expansion order index by index: -that is improvise-4zl7's job and its ticket warns against a second -implementation of it. Matching a used feature's name against the raw plan is -sound in the *refusing* direction — `preprocess` names a plain numeric column -exactly `col` and gives every expanded feature a suffix or an infix — and a -false refusal costs a fit, where a false acceptance costs a wrong rule. +### What `fitted_formula` still asks the document + +Two refusals, in order. An `Artefact` is refused with no reference to the +document at all — nothing could make `Cost.missing` spellable. Everything else +is asked **of the document**: a `Measure` must be `find_item_category`-resolvable +and a `Coordinate` must name a category the model has. A measure literally named +`Region=North` is a legal document and is accepted, because the check never looks +at the name's shape — and that is also why the date parts and the `.missing` +flags stay covered without a second copy of `preprocess`'s naming rules. + +`formula_text` still spells whatever it is handed, being the tree's own text; an +`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 + +`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. ## 12. A fit is persisted as its formula, and nothing else (improvise-7na) @@ -496,21 +520,25 @@ bar. Flattening only the success path left the failure path able to split it. A related trap with no mechanism guarding it: a `rustfmt` pass over a multi-line string literal leaves runs of ten spaces mid-sentence in a message a user actually sees — invisible in the source and in every test that only asks whether -a substring is present. `a_fit_that_needs_a_category_is_refused_naming_the_feature` -asserts no `\n` and no double space for that reason. +a substring is present. `a_name_containing_a_newline_cannot_split_the_status_line` +asserts the one-line rule for that reason. ## 15. Known gaps, filed rather than fixed -**Aggregate-then-route is not route-then-aggregate (improvise-4zl7).** A tree is -fitted on individual **records**. An improvise formula is evaluated at a -`CellKey`, and in any cell that aggregates over a hidden category -`evaluate_aggregated` aggregates each `Expr::Ref` *first* and then evaluates the -`IF`. For a linear formula that is the same answer — `SUM(Revenue) - SUM(Cost)` -is `SUM(Revenue - Cost)` — and for a tree it is not. **So the stored rule means -one thing on a record cell and a different thing one row up.** The user has -ruled that the fix is to aggregate the rule over the members; until then -`a_fitted_rule_at_an_aggregated_cell_routes_on_the_aggregate_not_the_records` -characterises it rather than asserting it is right. +**Closed: aggregate-then-route is not route-then-aggregate (improvise-4zl7).** +Kept here because the shape recurs. A tree is fitted on individual **records**, +and an improvise formula evaluated at an aggregating `CellKey` used to aggregate +each `Expr::Ref` first and evaluate the `IF` once on the totals — the same answer +for a linear formula, a different one for anything that routes, so the stored +rule meant one thing on a record cell and another one row up. The emitter now +wraps every tree in `SUM(…)` and `eval_agg` folds a non-`Ref` inner expression +over the member records: `context/code/core.md` §13. What remains of it is a +**cost**, not a defect — the fold is O(members) per evaluation per fixed-point +pass and is unbenchmarked (improvise-9iyv). + +**Two artefact families are still unspellable (improvise-wd6n).** `col.missing` +and the date sub-features stay `Spelling::Artefact` and are still refused. §11 +says what each would need; they are independent of each other. **A mean-imputed feature is read raw by the formula.** `preprocess` fills a missing numeric cell with the column mean, so a champion may split on a value @@ -544,12 +572,16 @@ measures too** — the only shape `fit_measure` currently stores — so "the cas that stores leaks nothing" is wrong. Closing it means building the ranges and the preprocessing plan per training fold and encoding each validation fold with *that* plan, which `cv_regress` cannot express: it takes one spec and rows that -are already encoded. That is an API change in `gp-tree`, and it is the same -change improvise-4zl7 will want, so the two are filed to land together. +are already encoded. That is an API change in `gp-tree`, and it is +independent of improvise-4zl7, which turned out to need no change to how +`cv_regress` is fed. -**Which features a champion picks is a property of the search**, so on a -document with several categories the same command refuses or succeeds depending -on the seed. +**Which features a champion picks is a property of the search**, which used to +mean the same command refused or succeeded depending on the seed. For the +categorical family that is gone: a one-hot over a real category is spellable +whichever way the search goes. It survives for `col.missing` and the date parts +(improvise-wd6n), and it is the reason those are worth closing rather than +living with — a refusal that depends on a seed is not a contract. ## 16. `preprocess`: what each column kind expands into diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index a5767852..5916c6f1 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -411,3 +411,69 @@ with the entry above this pins the rotation without enumerating it twice. Distinct axes must be distinguishable at a glance; `color()` is the only reason `AxisColor` exists, so a collision defeats it. + +### a_routing_formula_over_a_group_sums_the_members_answers + +improvise-4zl7, and the hand-computed half of its evidence. Every number in +`route_then_aggregate_tests` is written out rather than obtained from a second +call into the evaluator, because the bug being guarded is *the evaluator +answering consistently and wrongly*. + +The fixture is four records — `Cost` of 10 and 100 in North, 20 and 200 in +South — and the formula is `Pred = SUM(IF(Cost < 50, 1, 10))`. `Cost` straddles +the threshold **inside each region**, which is the whole design of the fixture: + +| cell | route-then-aggregate | aggregate-then-route | +|---|---|---| +| one record (`Cost` 10) | 1 | 1 | +| North (`Tag` away) | 1 + 10 = **11** | `110 < 50` → **10** | +| everything | 1+10+1+10 = **22** | `330 < 50` → **10** | + +A fixture whose records fall on the same side of the split would give the same +answer either way at the record level and differ only by a factor of the record +count, which is the kind of agreement that reads as correctness. + +Two faults were injected and both were watched to fail. Restoring the old +`eval_agg` — sum the matching raw values and ignore the inner expression — +fails **7 of the 8** tests with 330 for 22, 110 for 11 and 10 for 1; the one +survivor is `is_is_one_when_the_cells_own_coordinate_matches_and_zero_otherwise`, +which has no `SUM` and so never reaches the fold. Making `eval_is` return `0.0` +unconditionally fails exactly the **3** coordinate tests, with 400 for 214 and +200 for 14. The two faults failing disjoint-but-overlapping sets is itself the +evidence that the tests separate the fold from the predicate. + +### a_coordinate_split_survives_aggregating_over_that_very_category + +The case the ruling exists for, and the one the ticket's own text called the +product question. With `Region` aggregated away the cell has no `Region` +coordinate of its own, so a formula that read the *cell's* coordinate would see +nothing and take `Expr::Is`'s `unwrap_or(false)` branch — answering 100 once. +The fold hands the expression a **record** key, so each of the four records +answers for its own coordinate: 7 + 7 + 100 + 100 = 214. + +### a_member_whose_input_is_missing_errors_rather_than_being_dropped + +The asymmetry between the `Expr::Ref` fast path and the fold, asserted rather +than left to be discovered. `SUM(Revenue)` skips a record with no `Revenue` +cell; `SUM()` cannot, because the expression is what is being asked and an +expression that will not evaluate is an error. The fixture clears one record's +`Cost` and writes a different measure on the same record, so the record still +exists and still has a member cell to be found by — otherwise the record would +simply vanish from `matching_cells` and the test would prove nothing. + +### feature_roles_line_up_with_the_names_preprocess_emitted + +Lives in `gp-tree`, recorded here because the rule it guards is improvise-4zl7's. +`feature_roles` is the one statement of `preprocess`'s expansion order and +`encode_row` drives off it, so a drift is silent in both directions: the emitter +would spell the wrong feature and a re-encoded row would land in the wrong +column. The assertion rebuilds each feature's *name* from its role and compares +it with the name `preprocess` emitted, which is the only thing that ties the two +walks together. + +The last four assertions exist because the loop is vacuous over a fixture that +produces only one kind of feature. The fixture has a numeric column with a +missing cell, a categorical column and a date column, so all four roles appear. +Fault injected: swapping the `Numeric` and `Missing` pushes in `feature_roles` +fails this test **and** `encode_row_replays_the_training_expansion`, which is +the pair that would have disagreed. diff --git a/context/test-evidence/gp.md b/context/test-evidence/gp.md index 62f74858..74bf7c89 100644 --- a/context/test-evidence/gp.md +++ b/context/test-evidence/gp.md @@ -271,13 +271,27 @@ equals the threshold routes `hi` under `eval` and would route `lo` through a formula carrying the rounded threshold. The test asserts the trap is live — `"0.7".parse::() > thresh as f64` — before relying on it. -### a_split_on_a_one_hot_feature_is_refused +### a_split_on_a_one_hot_feature_is_spelled_as_a_coordinate -improvise-8xhy, the flip of the characterization test it replaces. `preprocess` +improvise-8xhy's defect, and improvise-4zl7's answer to it. `preprocess` one-hots a categorical column into features named `Region=North`, an encoding -artefact no document has an item for, so `find_item_category` returns `None` and -every cell of the fitted measure came back as `Error("ref:Region=North")` — -silently, with the emitter having said nothing. +artefact no document has an item for, so `find_item_category` returned `None` +and every cell of the fitted measure came back as `Error("ref:Region=North")` — +silently, with the emitter having said nothing. 8xhy shipped a refusal; 4zl7 +replaced the refusal with a spelling, `Region IS North`. + +The assertion is the emitted **text**, exactly, and not merely that the call is +`Ok`: the whole failure mode here was a well-formed formula meaning the wrong +thing, so "it parsed" is the assertion that missed it the first time. The +fault to inject is `Spelling::Coordinate` rendering as `quote_name(name)` — +byte-identical for every measure, and wrong for exactly this family. + +### a_coordinate_naming_no_category_of_this_document_is_refused + +The negative arm of the same rule, and it is not redundant with the artefact +refusal: a `Coordinate` is *spellable* and may still be unresolvable. Without +it, `resolves_in`'s `Coordinate` arm could return `true` unconditionally and +every test above would still pass. ### a_measure_that_merely_looks_like_a_one_hot_is_accepted @@ -419,12 +433,21 @@ the header once anything is dropped. A positional `i >= category_columns` role test then read the surviving numeric measure as a category and refused a perfectly good fit. The test asserts the fixture really drops a column first. -### a_category_coordinate_is_refused_even_when_an_item_shares_its_name +### a_coordinate_is_spelled_as_one_even_when_an_item_shares_the_one_hots_name -The other half of the same rule, and the one a *name* cannot catch. The fixture -adds a `_Measure` item literally named `Region=North` and asserts -`find_item_category` resolves it, so the older check would have passed — which -is what makes this test prove something the previous one does not. +The collision a *name* cannot resolve, now closed by construction rather than by +a refusal. The fixture adds a `_Measure` item literally named `Region=North` and +asserts `find_item_category` resolves it — so a check that matched the one-hot's +feature name against the document would have found the wrong column. + +Under improvise-4zl7 the one-hot is never spelled by its feature name at all: +`Features::of` reads the **role** off `preprocess`'s plan and emits +`Region IS North`, which cannot collide with any measure however the document is +named. So the assertion is the *absence* of `|Region=North|` in the emitted text +as much as the presence of `Region IS`. Injecting the fault means having +`spell` fall through to `Spelling::Measure` for a `OneHot` role: the fit then +succeeds, the formula reads `|Region=North|`, and it silently evaluates against +the wrong column — which is the original bug wearing the new API. ### a_high_cardinality_numeric_measure_is_one_feature_not_many @@ -484,18 +507,23 @@ reaches the search. `.improv` pipe quoting makes a newline legal inside a name, so a document can hold a measure whose name would split the TUI status bar in two. -### a_fit_that_needs_a_category_is_refused_naming_the_feature +### a_fit_that_needs_a_category_now_spells_it_as_a_coordinate -improvise-8xhy reaching a real user, and the reason improvise-4zl7 exists. The -fixture clears **both** measures on every North record: clearing only `Cost` -would leave North carrying both target values, so the one-hot would explain -nothing and the champion would have no reason to split on it. +improvise-8xhy reaching a real user, and what improvise-4zl7 bought: the same +fixture, the same champion, and a formula instead of a refusal. The fixture +clears **both** measures on every North record: clearing only `Cost` would leave +North carrying both target values, so the one-hot would explain nothing and the +champion would have no reason to split on it. -It also asserts the refusal contains no `\n` and no double space. A `rustfmt` -pass over the multi-line string literal that spells the message had left runs of -ten spaces mid-sentence — invisible in the source and in every test that only -asked whether a substring was present, and visible to precisely the user the -message exists for. +The second assertion is the one that earns its place. Checking only that the +text says `Region IS` would pass for a rule that routed both groups to the same +leaf — which is what a tree that could not see the coordinate would produce. The +fixture's two groups are 1000 and 4000, so requiring one leaf under 2000 and one +over 3000 separates "spelled it" from "spelled it and meant it". + +The `\n`-and-double-space assertions moved with the refusal they guarded; the +one-line rule itself is unchanged and is still asserted by +`a_name_containing_a_newline_cannot_split_the_status_line`. ### the_fold_count_moves_the_champion_and_so_is_recorded @@ -524,25 +552,28 @@ item installed cleanly and then made every later formula referring to it read the category coordinate. `a_later_formula_referring_to_the_fit_reads_the_fit` is the property that refusal protects, asserted against a real document. -### a_fitted_rule_at_an_aggregated_cell_routes_on_the_aggregate_not_the_records +### a_fitted_rule_at_an_aggregated_cell_routes_per_record_and_sums -**The most important finding of PR #25, and a product question rather than a -defect.** A tree is fitted on individual records; an improvise formula is -evaluated at a `CellKey`, and in any cell that aggregates over a hidden category -`evaluate_aggregated` aggregates each `Expr::Ref` *first* and then evaluates the -`IF`. Linear formulas are unaffected — `SUM(Revenue) - SUM(Cost)` is -`SUM(Revenue - Cost)` — and a tree is not. So the stored rule means one thing on -a record cell and a different thing one row up. +The characterization PR #25 left here, now replaced by the rule it was waiting +for. A tree is fitted on individual records; an improvise formula is evaluated +at a `CellKey`, and `evaluate_aggregated` used to aggregate each `Expr::Ref` +*first* and evaluate the `IF` once on the totals. Linear formulas are unaffected +— `SUM(Revenue) - SUM(Cost)` is `SUM(Revenue - Cost)` — and a tree is not, so +the stored rule meant one thing on a record cell and a different thing one row +up. improvise-4zl7's ruling was to aggregate the rule over the members. -This **characterises** rather than asserts correctness: what to do about it is -improvise-4zl7, now re-scoped, and the user has ruled that the fix is to -aggregate the rule over the members. +The old test asserted the **negative** and carried a message telling whoever +broke it to replace it with the rule. It fired on the first run of the fold. -Two things about the assertion. It asserts the **negative** — that the -aggregated cell does *not* agree with the sum of the per-record predictions — -which covers both outcomes and fails the day aggregation becomes record-wise, -which is the day the characterization should be replaced by the rule. And on -this document the aggregated cell is in fact `Error("ref:Cost")` rather than a -wrongly-routed number, which is the loud failure this repo prefers and is **not -something to rely on**: it is what this shape happens to produce, not a rule -anything states. +What this one asserts is the property the ticket names: the aggregated cell +equals the sum of the per-record predictions. That compares two *different* +code paths — a fully-specified key evaluates one member, an aggregating key +folds twelve — rather than one path against itself, but it is still an +end-to-end agreement rather than a hand-computed number. **The hand-computed +evidence is in `improvise-core`**, under +`route_then_aggregate_tests`, on a four-record fixture whose every expected +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. diff --git a/crates/gp-tree/src/data.rs b/crates/gp-tree/src/data.rs index 949efc6e..7fb7d9c5 100644 --- a/crates/gp-tree/src/data.rs +++ b/crates/gp-tree/src/data.rs @@ -452,32 +452,55 @@ pub fn preprocess(header: &[String], rows: &[Vec], ov: &Overrides) -> Da } } -/// Re-encode one raw row into the layout `columns` expects — the inverse of -/// [`preprocess`]'s expansion, driven by the saved plan rather than re-inferring. -pub fn encode_row(columns: &[ColumnSpec], header: &[String], row: &[String]) -> Vec { - let mut out = Vec::new(); - for col in columns { - let j = header - .iter() - .position(|h| h == &col.name) - .unwrap_or_else(|| panic!("the row is missing column {:?}", col.name)); - let cell = &row[j]; - let missing = is_missing(cell); +/// One expanded feature's job: the raw column it reads, and what it reads off that +/// column's cell. Roles carry their own imputation — `context/code/gp.md` §16. +#[derive(Clone, Debug, PartialEq)] +pub enum FeatureRole { + Numeric { + column: usize, + impute: f32, + }, + DatePart { + column: usize, + part: usize, + impute: f32, + }, + Missing { + column: usize, + }, + OneHot { + column: usize, + value: String, + }, +} + +impl FeatureRole { + /// Which entry of the `columns` plan this feature was expanded from. + pub fn column(&self) -> usize { + match self { + Self::Numeric { column, .. } + | Self::DatePart { column, .. } + | Self::Missing { column } + | Self::OneHot { column, .. } => *column, + } + } +} + +/// Every feature a saved plan expands to, in the order [`preprocess`] emitted them. +/// The **one** statement of that order: [`encode_row`] drives off it too. +pub fn feature_roles(columns: &[ColumnSpec]) -> Vec { + let mut roles = Vec::new(); + for (column, col) in columns.iter().enumerate() { match &col.kind { ColumnKind::Numeric { impute, has_missing, } => { - out.push(if missing { - *impute - } else { - cell.parse().unwrap_or_else(|_| { - panic!("column {:?}: {cell:?} is not numeric", col.name) - }) + roles.push(FeatureRole::Numeric { + column, + impute: *impute, }); - if *has_missing { - out.push(missing as i32 as f32); - } + roles.extend(has_missing.then_some(FeatureRole::Missing { column })); } ColumnKind::Date { has_time, @@ -485,24 +508,67 @@ pub fn encode_row(columns: &[ColumnSpec], header: &[String], row: &[String]) -> has_missing, } => { let width = if *has_time { 7 } else { 5 }; - if missing { - out.extend_from_slice(&impute[..width]); - } else { - let dt = parse_datetime(cell) - .unwrap_or_else(|| panic!("column {:?}: {cell:?} is not a date", col.name)); - out.extend_from_slice(&date_parts(&dt)[..width]); - } - if *has_missing { - out.push(missing as i32 as f32); - } + roles.extend((0..width).map(|part| FeatureRole::DatePart { + column, + part, + impute: impute[part], + })); + roles.extend(has_missing.then_some(FeatureRole::Missing { column })); } ColumnKind::Categorical { values } => { - let key = if missing { MISSING_CAT } else { cell.as_str() }; - out.extend(values.iter().map(|v| (key == v) as i32 as f32)); + roles.extend(values.iter().map(|value| FeatureRole::OneHot { + column, + value: value.clone(), + })); } } } - out + roles +} + +/// One feature's value for one raw cell. The role carries everything the reading +/// needs, so there is no arm here a plan cannot reach. +fn encode_feature(role: &FeatureRole, cell: &str, name: &str) -> f32 { + let missing = is_missing(cell); + match role { + FeatureRole::Numeric { impute, .. } => match missing { + true => *impute, + false => cell + .parse() + .unwrap_or_else(|_| panic!("column {name:?}: {cell:?} is not numeric")), + }, + FeatureRole::DatePart { part, impute, .. } => match missing { + true => *impute, + false => { + let dt = parse_datetime(cell) + .unwrap_or_else(|| panic!("column {name:?}: {cell:?} is not a date")); + date_parts(&dt)[*part] + } + }, + FeatureRole::Missing { .. } => missing as i32 as f32, + FeatureRole::OneHot { value, .. } => { + (if missing { MISSING_CAT } else { cell } == value.as_str()) as i32 as f32 + } + } +} + +/// Re-encode one raw row into the layout `columns` expects — the inverse of +/// [`preprocess`]'s expansion, driven by the saved plan rather than re-inferring. +pub fn encode_row(columns: &[ColumnSpec], header: &[String], row: &[String]) -> Vec { + let cells: Vec<&str> = columns + .iter() + .map(|col| { + let j = header + .iter() + .position(|h| h == &col.name) + .unwrap_or_else(|| panic!("the row is missing column {:?}", col.name)); + row[j].as_str() + }) + .collect(); + feature_roles(columns) + .iter() + .map(|role| encode_feature(role, cells[role.column()], &columns[role.column()].name)) + .collect() } #[cfg(test)] @@ -831,6 +897,56 @@ mod tests { ); } + /// `feature_roles` is the *one* statement of the expansion order (improvise-4zl7), + /// so it must line up with the names `preprocess` emitted, entry for entry. + #[test] + fn feature_roles_line_up_with_the_names_preprocess_emitted() { + let (h, rows) = table( + &["size", "region", "when", "price"], + &[ + &["1", "east", "2024-01-02", "10"], + &["", "west", "2024-06-30", "20"], + &["3", "east", "", "30"], + ], + ); + let d = preprocess(&h, &rows, &Overrides::default()); + let roles = feature_roles(&d.columns); + assert_eq!(roles.len(), d.feature_names.len(), "{:?}", d.feature_names); + // Each role must name the column its feature name is built from, and say + // which of the four kinds of feature it is. + for (role, name) in roles.iter().zip(&d.feature_names) { + let column = &d.columns[role.column()].name; + let expected = match role { + FeatureRole::Numeric { .. } => column.clone(), + FeatureRole::Missing { .. } => format!("{column}.missing"), + FeatureRole::DatePart { part, .. } => format!("{column}.{}", DATE_PARTS[*part]), + FeatureRole::OneHot { value, .. } => format!("{column}={value}"), + }; + assert_eq!(&expected, name, "{roles:?}"); + } + // And the fixture really exercises all four, or the loop proves little. + assert!( + roles + .iter() + .any(|r| matches!(r, FeatureRole::Numeric { .. })) + ); + assert!( + roles + .iter() + .any(|r| matches!(r, FeatureRole::Missing { .. })) + ); + assert!( + roles + .iter() + .any(|r| matches!(r, FeatureRole::DatePart { .. })) + ); + assert!( + roles + .iter() + .any(|r| matches!(r, FeatureRole::OneHot { .. })) + ); + } + #[test] fn a_missing_categorical_cell_becomes_its_own_level() { let (h, rows) = table( diff --git a/crates/improvise-command/src/command/cmd/fit.rs b/crates/improvise-command/src/command/cmd/fit.rs index bc62222d..a45c76ef 100644 --- a/crates/improvise-command/src/command/cmd/fit.rs +++ b/crates/improvise-command/src/command/cmd/fit.rs @@ -100,8 +100,8 @@ mod tests { const REGION: &str = "Region"; const TAG: &str = "Tag"; - /// A document a tree can learn exactly. **Every category is a `Label`**, or the - /// champion can reach an unresolvable feature — see the evidence. + /// A document a tree can learn exactly. **Every category is a `Label`**, so + /// `Cost` is the only thing the search can reach — see the evidence. fn workbook() -> crate::workbook::Workbook { let mut wb = crate::workbook::Workbook::new("sales"); wb.add_category(REGION).expect("first category"); diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 3acb9428..6b156936 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -41,6 +41,16 @@ fn aggregate_cell_values(values: &[&CellValue], func: &AggFunc) -> Option Vec { + let mut seen: IndexSet = IndexSet::new(); + for (key, _) in model.data.matching_cells(&partial.0) { + seen.insert(key.without(MEASURE_CATEGORY)); + } + seen.into_iter().collect() +} + /// A formula-level aggregate over the data matching `context` minus the /// target. Deliberately *not* [`apply_agg`]'s emptiness rule — §2. fn eval_agg( @@ -50,6 +60,7 @@ fn eval_agg( func: &AggFunc, inner: &Expr, agg_filter: Option<&Filter>, + eval_at: &dyn Fn(&Expr, &CellKey) -> Result, ) -> Result { let mut partial = context.without(target_category); if let Expr::Ref(item_name) = inner @@ -60,12 +71,26 @@ fn eval_agg( if let Some(f) = agg_filter { partial = partial.with(&f.category, &f.item); } - let values: Vec = model - .data - .matching_values(&partial.0) - .into_iter() - .filter_map(|v| v.as_f64()) - .collect(); + let values: Vec = match inner { + // A bare ref asks for the values that exist; anything else asks the + // expression of each member record and folds the answers — §13. + Expr::Ref(_) => model + .data + .matching_values(&partial.0) + .into_iter() + .filter_map(|v| v.as_f64()) + .collect(), + _ => member_records(model, &partial) + .into_iter() + .map(|record| { + let at = record + .0 + .iter() + .fold(context.clone(), |k, (cat, item)| k.with(cat, item)); + eval_at(inner, &at) + }) + .collect::, String>>()?, + }; match func { AggFunc::Sum => Ok(values.iter().sum()), @@ -76,6 +101,14 @@ fn eval_agg( } } +/// `Cat IS Item` as a number. A cell with no `Cat` coordinate is **not** that +/// item, matching `Formula::filter`'s own `unwrap_or(false)` — §13. +fn eval_is(context: &CellKey, filter: &Filter) -> f64 { + context + .get(&filter.category) + .is_some_and(|v| v == filter.item.as_str()) as i32 as f64 +} + /// Pure-data document model: categories, cells, formulas. It knows nothing /// about views — see [`crate::workbook::Workbook`] and core.md §10. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -521,7 +554,9 @@ impl Model { func, inner.as_ref(), agg_filter.as_ref(), + &|e, at| eval_expr_cached(e, at, model, target_category, none_cats), ), + Expr::Is(filter) => Ok(eval_is(context, filter)), Expr::If(cond, then, else_) => { let cv = eval_bool_cached(cond, context, model, target_category, none_cats)?; if cv { @@ -649,7 +684,9 @@ impl Model { func, inner.as_ref(), agg_filter.as_ref(), + &|e, at| eval_expr(e, at, model, target_category, depth), ), + Expr::Is(filter) => Ok(eval_is(context, filter)), Expr::If(cond, then, else_) => { let cv = eval_bool(cond, context, model, target_category, depth)?; if cv { @@ -742,7 +779,8 @@ mod agg_helper_tests { let model = Model::new("T"); let missing = Expr::Ref("Nonexistent".to_string()); let ctx = CellKey::new(vec![]); - let run = |func| eval_agg(&model, &ctx, "_Measure", &func, &missing, None); + let never = |_: &Expr, _: &CellKey| unreachable!("a bare Ref never folds per record"); + let run = |func| eval_agg(&model, &ctx, "_Measure", &func, &missing, None, &never); assert_eq!(run(AggFunc::Sum), Ok(0.0)); assert_eq!(run(AggFunc::Count), Ok(0.0)); @@ -2244,3 +2282,174 @@ mod prop_tests { } } } + +/// improvise-4zl7: a routing formula is applied to each member record and the +/// answers aggregated. Every number here is hand-computed — see the evidence file. +#[cfg(test)] +mod route_then_aggregate_tests { + use super::*; + use crate::formula::parse_formula; + use crate::model::cell::{CellKey, CellValue}; + + const MEASURE: &str = "_Measure"; + + fn key(pairs: &[(&str, &str)]) -> CellKey { + CellKey::new( + pairs + .iter() + .map(|(c, i)| ((*c).to_string(), (*i).to_string())) + .collect(), + ) + } + + /// Four records over two regions, with `Cost` straddling 50 **inside** each + /// region: 10 and 100 in North, 20 and 200 in South. + fn four_records(formula: &str) -> Model { + let mut m = Model::new("fit"); + for (cat, items) in [("Region", ["North", "South"]), ("Tag", ["a", "b"])] { + m.add_category(cat).expect("under the limit"); + for item in items { + m.category_mut(cat).expect("just added").add_item(item); + } + } + m.category_mut(MEASURE) + .expect("_Measure is always present") + .add_item("Cost"); + for (tag, region, cost) in [ + ("a", "North", 10.0), + ("b", "North", 100.0), + ("a", "South", 20.0), + ("b", "South", 200.0), + ] { + m.set_cell( + key(&[("Tag", tag), ("Region", region), (MEASURE, "Cost")]), + CellValue::Number(cost), + ); + } + m.add_formula(parse_formula(formula, MEASURE).expect("the fixture parses")); + m + } + + fn number_at(m: &Model, pairs: &[(&str, &str)]) -> f64 { + match m.evaluate(&key(pairs)) { + Some(CellValue::Number(n)) => n, + other => panic!("{pairs:?} evaluated to {other:?}"), + } + } + + /// A record row: one member, so the fold is the rule itself. 10 < 50 and + /// 100 is not, so the two North records answer 1 and 10. + #[test] + fn a_routing_formula_at_a_record_is_the_rule_itself() { + let m = four_records("Pred = SUM(IF(Cost < 50, 1, 10))"); + assert_eq!( + number_at(&m, &[("Tag", "a"), ("Region", "North"), (MEASURE, "Pred")]), + 1.0 + ); + assert_eq!( + number_at(&m, &[("Tag", "b"), ("Region", "North"), (MEASURE, "Pred")]), + 10.0 + ); + } + + /// One row up, `Tag` aggregated away: North is 1 + 10 = **11**. Routing on the + /// aggregate would ask `110 < 50` once and answer **10**. + #[test] + fn a_routing_formula_over_a_group_sums_the_members_answers() { + let m = four_records("Pred = SUM(IF(Cost < 50, 1, 10))"); + assert_eq!( + number_at(&m, &[("Region", "North"), (MEASURE, "Pred")]), + 11.0 + ); + assert_eq!( + number_at(&m, &[("Region", "South"), (MEASURE, "Pred")]), + 11.0 + ); + } + + /// Everything aggregated away: 1 + 10 + 1 + 10 = **22**, where routing on the + /// total would ask `330 < 50` once and answer **10**. + #[test] + fn a_routing_formula_over_the_whole_document_sums_every_member() { + let m = four_records("Pred = SUM(IF(Cost < 50, 1, 10))"); + assert_eq!(number_at(&m, &[(MEASURE, "Pred")]), 22.0); + } + + /// The cached evaluator must answer the same. `recompute_formulas` is the + /// fixed point of gotcha 9, and it caches the *aggregated* stems. + #[test] + fn the_cached_evaluator_routes_per_record_too() { + let mut m = four_records("Pred = SUM(IF(Cost < 50, 1, 10))"); + let none_cats = ["Tag".to_string()]; + m.recompute_formulas(&none_cats); + for region in ["North", "South"] { + assert_eq!( + m.evaluate_aggregated(&key(&[("Region", region), (MEASURE, "Pred")]), &none_cats), + Some(CellValue::Number(11.0)), + "{region}" + ); + } + } + + /// A coordinate split: `Region IS North` is 1 on North's two records and 0 on + /// South's, so `IF(… < 0.5, 100, 7)` gives 7+7 = **14** and 100+100 = **200**. + #[test] + fn a_coordinate_split_reads_each_records_own_coordinate() { + let m = four_records("Pred = SUM(IF(Region IS North < 0.5, 100, 7))"); + assert_eq!( + number_at(&m, &[("Region", "North"), (MEASURE, "Pred")]), + 14.0 + ); + assert_eq!( + number_at(&m, &[("Region", "South"), (MEASURE, "Pred")]), + 200.0 + ); + } + + /// The case the whole ruling exists for: with `Region` aggregated away the + /// cell has no `Region` coordinate of its own, yet the answer is 14 + 200. + #[test] + fn a_coordinate_split_survives_aggregating_over_that_very_category() { + let m = four_records("Pred = SUM(IF(Region IS North < 0.5, 100, 7))"); + assert_eq!(number_at(&m, &[(MEASURE, "Pred")]), 214.0); + } + + /// `Expr::Is` on its own: 1, 0, and 0 for a cell with no such coordinate — + /// the same `unwrap_or(false)` a `WHERE` filter makes. + #[test] + fn is_is_one_when_the_cells_own_coordinate_matches_and_zero_otherwise() { + let m = four_records("Pred = Region IS North"); + assert_eq!( + number_at(&m, &[("Tag", "a"), ("Region", "North"), (MEASURE, "Pred")]), + 1.0 + ); + assert_eq!( + number_at(&m, &[("Tag", "a"), ("Region", "South"), (MEASURE, "Pred")]), + 0.0 + ); + assert_eq!(number_at(&m, &[("Tag", "a"), (MEASURE, "Pred")]), 0.0); + } + + /// The fold asks the expression of each member, so a record whose input is + /// missing is an error rather than a silently dropped row. + #[test] + fn a_member_whose_input_is_missing_errors_rather_than_being_dropped() { + let mut m = four_records("Pred = SUM(IF(Cost < 50, 1, 10))"); + m.set_cell( + key(&[("Tag", "a"), ("Region", "North"), (MEASURE, "Other")]), + CellValue::Number(1.0), + ); + m.clear_cell(&key(&[ + ("Tag", "a"), + ("Region", "North"), + (MEASURE, "Cost"), + ])); + assert!( + matches!( + m.evaluate(&key(&[("Region", "North"), (MEASURE, "Pred")])), + Some(CellValue::Error(_)) + ), + "a member with no Cost must surface, not vanish" + ); + } +} diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index aaf8341a..90482721 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -40,6 +40,9 @@ pub enum Expr { UnaryMinus(Box), Agg(AggFunc, Box, Option), If(Box, Box, Box), + /// `Cat IS Item` — 1.0 when the cell's own `Cat` coordinate is `Item`, else + /// 0.0. A coordinate predicate as a number; `context/code/core.md` §13. + Is(Filter), } #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/crates/improvise-formula/src/formula.pest b/crates/improvise-formula/src/formula.pest index cf91971d..9c14a1b1 100644 --- a/crates/improvise-formula/src/formula.pest +++ b/crates/improvise-formula/src/formula.pest @@ -47,9 +47,14 @@ primary = { | agg_call | if_expr | paren_expr + | is_expr | ref_expr } +// `Cat IS Item` — a coordinate predicate as 1/0. It must precede `ref_expr`, +// which would otherwise consume the category name and leave `IS Item` behind. +is_expr = { identifier ~ ^"IS" ~ filter_value } + paren_expr = { "(" ~ expr ~ ")" } // Aggregates with optional inline WHERE filter inside the parens. diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index 9e68dda9..49706fbc 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -116,7 +116,7 @@ fn build_unary(pair: Pair) -> Expr { } fn build_primary(pair: Pair) -> Expr { - // primary = { number | agg_call | if_expr | paren_expr | ref_expr } + // primary = { number | agg_call | if_expr | paren_expr | is_expr | ref_expr } let inner = first_inner(pair); let rule = inner.as_rule(); if rule == Rule::number { @@ -127,6 +127,9 @@ fn build_primary(pair: Pair) -> Expr { build_if_expr(inner) } else if rule == Rule::paren_expr { build_expr(first_inner(inner)) + } else if rule == Rule::is_expr { + // Same shape as a WHERE clause minus the keyword: identifier ~ filter_value. + Expr::Is(build_filter(inner)) } else { // ref_expr is the only remaining alternative. Expr::Ref(identifier_to_string(first_inner(inner))) @@ -460,6 +463,73 @@ mod tests { assert_eq!(filter.item, "East"); } + // ── IS, the coordinate predicate (improvise-4zl7) ─────────────────── + + #[test] + fn parse_is_expression() { + let f = parse_formula("X = Region IS East", "Cat").unwrap(); + let Expr::Is(filter) = &f.expr else { + panic!("expected an IS, got {:?}", f.expr); + }; + assert_eq!( + (filter.category.as_str(), filter.item.as_str()), + ("Region", "East") + ); + } + + /// The shape the emitter actually writes: a coordinate predicate as the left + /// side of an `IF`'s comparison. + #[test] + fn parse_is_inside_an_if_condition() { + let f = parse_formula("X = IF(Region IS East < 0.5, 1, 2)", "Cat").unwrap(); + let Expr::If(cond, ..) = &f.expr else { + panic!("expected an IF, got {:?}", f.expr); + }; + let Expr::BinOp(BinOp::Lt, lhs, _) = &**cond else { + panic!("expected a `<`, got {cond:?}"); + }; + assert!(matches!(&**lhs, Expr::Is(_)), "got {lhs:?}"); + } + + /// Both halves quote through `improvise_formula::quote`'s one home, so a name + /// with spaces, a pipe or a keyword spelling survives (gotcha 34). + #[test] + fn parse_is_with_awkward_names() { + for (cat, item) in [ + ("Region Name", "East Coast"), + ("IS", "IF"), + ("a|b", "c\\d"), + ("SUM", "9lives"), + ] { + let text = format!( + "X = {} IS {}", + crate::quote::quote_name(cat), + crate::quote::quote_name(item) + ); + let f = parse_formula(&text, "Cat") + .unwrap_or_else(|e| panic!("{text:?} did not parse: {e}")); + let Expr::Is(filter) = &f.expr else { + panic!("{text:?} is not an IS: {:?}", f.expr); + }; + assert_eq!( + (filter.category.as_str(), filter.item.as_str()), + (cat, item) + ); + } + } + + /// `ref_expr` must not win the race: a bare name still parses as a reference, + /// and only the `IS` keyword turns it into a predicate. + #[test] + fn a_bare_name_is_still_a_reference() { + let f = parse_formula("X = Region", "Cat").unwrap(); + assert!( + matches!(f.expr, Expr::Ref(ref n) if n == "Region"), + "{:?}", + f.expr + ); + } + // ── Error paths ───────────────────────────────────────────────────── #[test] diff --git a/crates/improvise-formula/src/quote.rs b/crates/improvise-formula/src/quote.rs index 968080b1..81b49eda 100644 --- a/crates/improvise-formula/src/quote.rs +++ b/crates/improvise-formula/src/quote.rs @@ -3,7 +3,7 @@ /// Words the formula grammar gives a meaning of their own. A name spelled like /// one is pipe-quoted even though it is otherwise a legal bare identifier. -const KEYWORDS: [&str; 7] = ["WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF"]; +const KEYWORDS: [&str; 8] = ["WHERE", "SUM", "AVG", "MIN", "MAX", "COUNT", "IF", "IS"]; /// Bare-writable: matches `[A-Za-z_][A-Za-z0-9_-]*` and is not a [`KEYWORDS`] /// entry. The keyword clause is canonical — `context/code/formula-and-io.md`. diff --git a/crates/improvise-gp/src/fit.rs b/crates/improvise-gp/src/fit.rs index a2be64c7..9ca259f4 100644 --- a/crates/improvise-gp/src/fit.rs +++ b/crates/improvise-gp/src/fit.rs @@ -229,6 +229,8 @@ pub fn fit_measure( let header = table.header.clone(); let mut data = preprocess(&table.header, &table.rows, &Overrides::default()); data.warnings.splice(0..0, table.warnings); + no_ambiguous_column_names(&header)?; + let features = crate::Features::of(&data, &header[..category_columns]); let rows = regression_rows(data.target, target)?; // Non-finite data is a **panic** too, and further down: `standardize` makes NaN @@ -299,12 +301,10 @@ pub fn fit_measure( &mut rng, ); - only_raw_measure_features(&champion, &spec, &header, category_columns, &data.columns)?; - Ok(FitOutcome { formula: crate::fitted_formula( &champion, - &spec, + &features, into, improvise_core::model::category::MEASURE_CATEGORY, model, @@ -319,15 +319,9 @@ pub fn fit_measure( }) } -/// Refuse a champion that splits on anything but a **raw numeric measure** column, -/// asked of `preprocess`'s plan and by name, never by position. gp.md §11. -fn only_raw_measure_features( - champion: &gp_tree::Tree, - spec: &Spec, - header: &[String], - category_columns: usize, - columns: &[gp_tree::data::ColumnSpec], -) -> anyhow::Result<()> { +/// Refuse a table that names one column twice. `Features::of` decides category or +/// measure by **name**, so an ambiguous name is a spelling with two meanings. +fn no_ambiguous_column_names(header: &[String]) -> anyhow::Result<()> { let feature_header = &header[..header.len().saturating_sub(1)]; let mut ambiguous: Vec<&str> = feature_header .iter() @@ -346,26 +340,6 @@ fn only_raw_measure_features( "this document names {ambiguous:?} both as a category and as a measure, so a fitted \ formula could not say which one it meant. Rename one of them" ); - let raw_measure_names: Vec<&str> = feature_header - .iter() - .skip(category_columns) - .map(String::as_str) - .filter(|name| { - columns.iter().any(|c| { - c.name == *name && matches!(c.kind, gp_tree::data::ColumnKind::Numeric { .. }) - }) - }) - .collect(); - let not_measures: Vec = crate::used_features(champion, spec) - .into_iter() - .filter(|name| !raw_measure_names.contains(&name.as_str())) - .collect(); - anyhow::ensure!( - not_measures.is_empty(), - "the fitted tree splits on {not_measures:?}, which is not a numeric measure of this \ - document — a category coordinate or an encoding artefact, and a formula has no way \ - to spell either. Fit over numeric measures for now" - ); Ok(()) } @@ -517,7 +491,10 @@ mod tests { Expr::If(c, t, e) => [c, t, e].iter().flat_map(|e| leaf_numbers(e)).collect(), Expr::BinOp(_, l, r) => [l, r].iter().flat_map(|e| leaf_numbers(e)).collect(), Expr::UnaryMinus(e) => leaf_numbers(e), - Expr::Ref(_) | Expr::Agg(..) => Vec::new(), + // The whole tree now sits inside the route-then-aggregate `SUM`, so + // stopping at `Agg` would make this walk return nothing at all. + Expr::Agg(_, inner, _) => leaf_numbers(inner), + Expr::Ref(_) | Expr::Is(_) => Vec::new(), } } @@ -646,10 +623,10 @@ mod tests { .expect("distinct numeric values are not distinct categories"); } - /// The other half of the same rule, and the one a *name* cannot catch: a category - /// coordinate is refused **by role**, even where an item shares its spelling. + /// The spelling of a coordinate and the spelling of a measure cannot collide: + /// `Region IS North` is not `|Region=North|` however the document is named. #[test] - fn a_category_coordinate_is_refused_even_when_an_item_shares_its_name() { + fn a_coordinate_is_spelled_as_one_even_when_an_item_shares_the_one_hots_name() { let (mut model, view) = document(); model.category_mut(REGION).expect("Region exists").kind = CategoryKind::Regular; model @@ -680,15 +657,24 @@ mod tests { "the fixture must make the one-hot's name resolvable, or this test \ proves nothing the previous one does not" ); - let err = fit_measure(&model, &view, "Score", FIT_INTO, &request()) - .expect_err("a one-hot is not a measure however it is spelled"); - assert!(err.to_string().contains("not a numeric measure"), "{err}"); + let outcome = fit_measure(&model, &view, "Score", FIT_INTO, &request()) + .expect("a one-hot over a real category is now spellable"); + assert!( + outcome.formula.raw.contains("Region IS "), + "{}", + outcome.formula.raw + ); + assert!( + !outcome.formula.raw.contains("|Region=North|"), + "the coordinate must not be emitted as the measure that shares its name: {}", + outcome.formula.raw + ); } - /// **The finding that matters most in this whole PR, and a product question - /// rather than a defect** (improvise-4zl7). See `context/test-evidence/gp.md`. + /// improvise-4zl7's ruling, end to end: the aggregated cell is the sum of the + /// per-record predictions. The hand-computed form is in `improvise-core`. #[test] - fn a_fitted_rule_at_an_aggregated_cell_routes_on_the_aggregate_not_the_records() { + fn a_fitted_rule_at_an_aggregated_cell_routes_per_record_and_sums() { let (mut model, view) = document(); let outcome = fit_measure(&model, &view, "Score", FIT_INTO, &request()).expect("the fit lands"); @@ -709,20 +695,27 @@ mod tests { .collect(); assert_eq!(per_record.len(), 12, "every record must evaluate"); - // One row up — the whole of `Tag` aggregated away — the formula sees - // the aggregate of `Cost` over every record and routes once on that. - let aggregated = model.evaluate(&CellKey::new(vec![ - (REGION.to_string(), "North".to_string()), - (MEASURE.to_string(), FIT_INTO.to_string()), - ])); + // One row up — the whole of `Tag` aggregated away — the rule is applied + // to each record and the answers summed, not applied once to the total. + let aggregated = model.evaluate(&CellKey::new(vec![( + MEASURE.to_string(), + FIT_INTO.to_string(), + )])); let sum_of_records: f64 = per_record.iter().sum(); - // Asserting the negative covers both outcomes and fails the day aggregation - // becomes record-wise — which is when this characterization is replaced. assert!( - !matches!(aggregated, Some(CellValue::Number(n)) if (n - sum_of_records).abs() <= 1.0), - "the aggregated cell now agrees with the per-record predictions \ - ({aggregated:?} vs {sum_of_records}). improvise-4zl7's question \ - has been answered — replace this characterization with the rule." + matches!(aggregated, Some(CellValue::Number(n)) if (n - sum_of_records).abs() <= 1e-6), + "the aggregated cell must be the sum of the per-record predictions \ + ({aggregated:?} vs {sum_of_records})" + ); + // And the two really are different code paths: routing once on the total + // would land on a single leaf, which cannot be their sum over 12 records. + let leaves = leaf_numbers(&outcome.formula.expr); + assert!( + !leaves + .iter() + .any(|leaf| (leaf - sum_of_records).abs() <= 1e-6), + "the fixture must not make the right answer equal a single leaf, \ + or this test cannot tell the two rules apart: {leaves:?}" ); } @@ -920,10 +913,10 @@ mod tests { } } - /// improvise-8xhy reaching a real user, and the reason improvise-4zl7 exists. - /// What it costs in practice, and the fixture: `context/test-evidence/gp.md`. + /// improvise-8xhy reaching a real user, and what improvise-4zl7 bought: the + /// fit a category explains now lands. Fixture: `context/test-evidence/gp.md`. #[test] - fn a_fit_that_needs_a_category_is_refused_naming_the_feature() { + fn a_fit_that_needs_a_category_now_spells_it_as_a_coordinate() { let (mut model, view) = document(); // `document()` makes every category a Label; this test is about what // happens when one is not. @@ -949,21 +942,20 @@ mod tests { CellValue::Number(if region == "North" { 1000.0 } else { 4000.0 }), ); } - let err = fit_measure(&model, &view, "Score", FIT_INTO, &request()) - .expect_err("the champion can only split on a one-hot"); - let msg = err.to_string(); + let outcome = fit_measure(&model, &view, "Score", FIT_INTO, &request()) + .expect("the champion splits on a one-hot, which is now spellable"); + let raw = &outcome.formula.raw; assert!( - msg.contains("Region=North") || msg.contains("Region=South"), - "{msg}" + raw.contains("Region IS North") || raw.contains("Region IS South"), + "{raw}" ); + // And it means what it says: the fixture's two groups are 1000 and 4000, + // so the two leaves must straddle them rather than sit on one average. + let leaves = leaf_numbers(&outcome.formula.expr); assert!( - msg.contains("not a numeric measure"), - "the refusal must say what kind of thing the feature is: {msg}" + leaves.iter().any(|n| *n < 2000.0) && leaves.iter().any(|n| *n > 3000.0), + "the rule must separate the two groups, got {leaves:?}" ); - // The refusal is read off a one-line status bar, so it has to *be* one line - // and carry no rustfmt-inserted space runs. See `context/code/gp.md` §14. - assert!(!msg.contains('\n'), "{msg:?}"); - assert!(!msg.contains(" "), "{msg:?}"); } /// A text-valued target is a *classification* — k trees and an argmax, not diff --git a/crates/improvise-gp/src/lib.rs b/crates/improvise-gp/src/lib.rs index 1fb1955a..10986aea 100644 --- a/crates/improvise-gp/src/lib.rs +++ b/crates/improvise-gp/src/lib.rs @@ -7,7 +7,8 @@ pub mod fit; pub use dataset::{FeatureTable, dataset, feature_table}; pub use fit::{FitOutcome, FitRequest, Quality, fit_measure, fitted_measure_name}; -use gp_tree::{Spec, Tree}; +use gp_tree::Tree; +use gp_tree::data::{ColumnSpec, Dataset, FeatureRole, MISSING_CAT, feature_roles}; use improvise_core::model::Model; use improvise_formula::{Formula, parse_formula}; @@ -15,59 +16,152 @@ use improvise_formula::{Formula, parse_formula}; // that defines it. Re-exported because it was part of this crate's public surface. pub use improvise_formula::quote_name; -/// The formula text for a fitted tree: ` = `. Preconditions: the -/// tree is [`gp_tree::is_valid`] for `spec.features()`, thresholds and leaves finite. -pub fn formula_text(tree: &Tree, spec: &Spec, target: &str) -> String { - format!("{} = {}", quote_name(target), expr_text(tree, spec)) +/// How one fitted feature is said in improvise's own vocabulary. +/// `context/code/gp.md` §11 carries why each arm asks the document what it asks. +#[derive(Clone, Debug, PartialEq)] +pub enum Spelling { + /// `Expr::Ref` — a measure the document must name as an item. + 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, +} + +/// One fitted feature: what `preprocess` called it, and how improvise says it. +#[derive(Clone, Debug, PartialEq)] +pub struct Feature { + pub name: String, + pub spelling: Spelling, +} + +/// The fitted features in spec order — one entry per feature index, so a tree's +/// `feat` indexes this directly. Built from the plan, never from the names. +#[derive(Clone, Debug, PartialEq)] +pub struct Features(Vec); + +impl Features { + /// Read `preprocess`'s own plan. `categories` names the header columns that are + /// document *categories*; everything else is a measure column. + pub fn of(data: &Dataset, categories: &[String]) -> Self { + let features = feature_roles(&data.columns) + .into_iter() + .zip(&data.feature_names) + .map(|(role, name)| Feature { + name: name.clone(), + spelling: spell(&role, &data.columns, categories), + }) + .collect(); + Self(features) + } + + pub fn len(&self) -> usize { + self.0.len() + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn get(&self, feat: usize) -> &Feature { + self.0 + .get(feat) + .expect("tree indexes a feature the spec does not name") + } +} + +/// A role as improvise says it. A one-hot over a *measure* column names no +/// coordinate, and `(missing)` is an absent coordinate — both are artefacts. +fn spell(role: &FeatureRole, columns: &[ColumnSpec], categories: &[String]) -> Spelling { + let column = &columns[role.column()].name; + let is_category = categories.iter().any(|c| c == column); + match role { + FeatureRole::Numeric { .. } if !is_category => Spelling::Measure(column.clone()), + FeatureRole::OneHot { value, .. } if is_category && value != MISSING_CAT => { + Spelling::Coordinate { + category: column.clone(), + item: value.clone(), + } + } + _ => Spelling::Artefact, + } +} + +/// The formula text for a fitted tree: ` = SUM()`. The `SUM` is +/// the route-then-aggregate rule made syntax — `context/code/core.md` §13. +pub fn formula_text(tree: &Tree, features: &Features, target: &str) -> String { + format!( + "{} = SUM({})", + quote_name(target), + expr_text(tree, features) + ) } /// [`formula_text`], parsed **and** checked against the document it is to be stored -/// in — the form improvise stores. Why, and the interim: `context/code/gp.md` §11. +/// in — the form improvise stores. Why, and what is left: `context/code/gp.md` §11. pub fn fitted_formula( tree: &Tree, - spec: &Spec, + features: &Features, target: &str, target_category: &str, model: &Model, ) -> anyhow::Result { - let unresolvable = unresolvable_features(tree, spec, model); + 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(); anyhow::ensure!( - unresolvable.is_empty(), + 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", + ); + let unresolvable: Vec<&str> = used + .iter() + .filter(|f| !resolves_in(&f.spelling, model)) + .map(|f| f.name.as_str()) + .collect(); + anyhow::ensure!( + unresolvable.is_empty(), "the fitted tree splits on {unresolvable:?}, which {} not {} of this document \ — a formula refers to its inputs by name, so every cell of {target:?} would be \ - a reference error. Encoding artefacts (`Cat=Item` one-hots, `col.missing` flags, \ - date parts) cannot be written as a formula: fit over numeric measures for now", + a reference error", if unresolvable.len() == 1 { "is" } else { "are" }, if unresolvable.len() == 1 { - "an item" + "an item or category" } else { - "items" + "items or categories" }, ); - parse_formula(&formula_text(tree, spec, target), target_category) + parse_formula(&formula_text(tree, features, target), target_category) } -/// Every feature the tree splits on that `Expr::Ref` could not resolve, in the -/// order the spec names them and without repeats. -fn unresolvable_features(tree: &Tree, spec: &Spec, model: &Model) -> Vec { - used_features(tree, spec) - .into_iter() - .filter(|name| model.find_item_category(name).is_none()) - .collect() +/// Whether the document can answer the spelling. The question is asked **of the +/// document**, never of the name's shape — `context/code/gp.md` §11. +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, + } } /// Every feature the tree splits on, in spec order and without repeats. **One** walk, /// because two questions are asked of the same set — `context/code/gp.md` §11. -pub(crate) fn used_features(tree: &Tree, spec: &Spec) -> Vec { - let mut used = vec![false; spec.names.len()]; +pub(crate) fn used_features<'a>(tree: &Tree, features: &'a Features) -> Vec<&'a Feature> { + let mut used = vec![false; features.len()]; mark_features(tree, &mut used); - spec.names + features + .0 .iter() .zip(&used) .filter(|(_, u)| **u) - .map(|(name, _)| (*name).to_string()) + .map(|(feature, _)| feature) .collect() } @@ -83,7 +177,7 @@ fn mark_features(tree: &Tree, used: &mut [bool]) { } } -fn expr_text(tree: &Tree, spec: &Spec) -> String { +fn expr_text(tree: &Tree, features: &Features) -> String { match tree { Tree::Leaf(score) => number_text(*score), Tree::Split { @@ -91,18 +185,26 @@ fn expr_text(tree: &Tree, spec: &Spec) -> String { thresh, lo, hi, - } => { - let name = spec - .names - .get(*feat) - .expect("tree indexes a feature the spec does not name"); - format!( - "IF({} < {}, {}, {})", - quote_name(name), - number_text(*thresh), - expr_text(lo, spec), - expr_text(hi, spec) - ) + } => format!( + "IF({} < {}, {}, {})", + spelling_text(&features.get(*feat).spelling), + number_text(*thresh), + expr_text(lo, features), + expr_text(hi, features) + ), + } +} + +/// A spelling as formula text. `Artefact` is a precondition failure here and a +/// refusal in [`fitted_formula`] — the same split `number_text` makes. +fn spelling_text(spelling: &Spelling) -> String { + match spelling { + Spelling::Measure(name) => quote_name(name), + Spelling::Coordinate { category, item } => { + format!("{} IS {}", quote_name(category), quote_name(item)) + } + Spelling::Artefact => { + panic!("expr_text wants a feature the formula language can spell") } } } @@ -127,7 +229,7 @@ fn number_text(v: f32) -> String { mod tests { use super::*; use gp_tree::fit::{Fit, FitConfig, Row, fit_detailed}; - use gp_tree::{Rng, SplitMix64, eval, random_tree}; + use gp_tree::{Rng, Spec, SplitMix64, eval, random_tree}; use improvise_core::formula::{Expr, parse_formula}; use improvise_core::model::cell::{CellKey, CellValue}; use improvise_core::model::types::Model; @@ -139,6 +241,50 @@ mod tests { Spec { ranges, names } } + /// Features that spell themselves — exactly what `Features::of` builds for a + /// numeric measure column, and all most of these tests need. + fn measures(names: &[&str]) -> Features { + Features( + names + .iter() + .map(|n| Feature { + name: (*n).to_string(), + spelling: Spelling::Measure((*n).to_string()), + }) + .collect(), + ) + } + + /// One-hot features: what `Features::of` builds for a categorical *category* + /// column, named the way `preprocess` names them. + fn coordinates(pairs: &[(&str, &str)]) -> Features { + Features( + pairs + .iter() + .map(|(cat, item)| Feature { + name: format!("{cat}={item}"), + spelling: Spelling::Coordinate { + category: (*cat).to_string(), + item: (*item).to_string(), + }, + }) + .collect(), + ) + } + + /// Features no formula can spell: a `col.missing` flag or a date part. + fn artefacts(names: &[&str]) -> Features { + Features( + names + .iter() + .map(|n| Feature { + name: (*n).to_string(), + spelling: Spelling::Artefact, + }) + .collect(), + ) + } + fn leaf(v: f32) -> Box { Box::new(Tree::Leaf(v)) } @@ -183,31 +329,33 @@ mod tests { #[test] fn a_leaf_emits_its_score_and_parses() { - let spec = spec_of(&[(0.0, 1.0)], &["x"]); - let text = formula_text(&Tree::Leaf(-1.5), &spec, TARGET); - assert_eq!(text, "Score = -1.5"); + let text = formula_text(&Tree::Leaf(-1.5), &measures(&["x"]), TARGET); + assert_eq!(text, "Score = SUM(-1.5)"); assert!(parse_formula(&text, MEASURE).is_ok(), "{text}"); } #[test] fn a_nested_tree_emits_nested_ifs_and_parses() { - let spec = spec_of(&[(0.0, 100.0), (0.0, 1.0)], &["humidity", "windy"]); + let features = measures(&["humidity", "windy"]); let tree = split( 0, 77.5, Box::new(split(1, 0.5, leaf(-0.5), leaf(2.0))), leaf(1.25), ); - let text = formula_text(&tree, &spec, TARGET); + let text = formula_text(&tree, &features, TARGET); assert_eq!( text, - "Score = IF(humidity < 77.5, IF(windy < 0.5, -0.5, 2), 1.25)" + "Score = SUM(IF(humidity < 77.5, IF(windy < 0.5, -0.5, 2), 1.25))" ); let parsed = parse_formula(&text, MEASURE).expect("emitted formula parses"); assert_eq!(parsed.target, TARGET); - // Nesting survives: the outer IF's `then` branch is itself an IF. - let Expr::If(_, then_branch, _) = &parsed.expr else { - panic!("expected an IF at the root, got {:?}", parsed.expr); + // The route-then-aggregate SUM wraps the tree; nesting survives under it. + let Expr::Agg(_, routed, _) = &parsed.expr else { + panic!("expected a SUM at the root, got {:?}", parsed.expr); + }; + let Expr::If(_, then_branch, _) = &**routed else { + panic!("expected an IF under the SUM, got {routed:?}"); }; assert!( matches!(**then_branch, Expr::If(..)), @@ -234,15 +382,15 @@ mod tests { "", // empty ]; for (i, name) in names.iter().enumerate() { - let ranges = [(0.0f32, 1.0f32)]; - let name_slice = [*name]; - let spec = spec_of(&ranges, &name_slice); let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); - let text = formula_text(&tree, &spec, TARGET); + let text = formula_text(&tree, &measures(&[*name]), TARGET); let parsed = parse_formula(&text, MEASURE) .unwrap_or_else(|e| panic!("name {name:?} (#{i}) emitted {text:?}: {e}")); - let Expr::If(cond, ..) = &parsed.expr else { - panic!("expected an IF, got {:?}", parsed.expr); + let Expr::Agg(_, routed, _) = &parsed.expr else { + panic!("expected a SUM, got {:?}", parsed.expr); + }; + let Expr::If(cond, ..) = &**routed else { + panic!("expected an IF, got {routed:?}"); }; let Expr::BinOp(_, lhs, _) = &**cond else { panic!("expected a comparison, got {cond:?}"); @@ -258,9 +406,8 @@ mod tests { /// correct. See `context/test-evidence/gp.md`. #[test] fn an_awkward_target_name_is_quoted_and_round_trips() { - let spec = spec_of(&[(0.0, 1.0)], &["x"]); - let text = formula_text(&Tree::Leaf(1.0), &spec, "Total Revenue"); - assert_eq!(text, "|Total Revenue| = 1"); + let text = formula_text(&Tree::Leaf(1.0), &measures(&["x"]), "Total Revenue"); + assert_eq!(text, "|Total Revenue| = SUM(1)"); let parsed = parse_formula(&text, MEASURE).expect("emitted formula parses"); assert_eq!(parsed.target, "Total Revenue"); } @@ -269,10 +416,15 @@ mod tests { /// `add_formula` puts the target into `_Measure`, where the user can see it. #[test] fn an_awkward_target_names_the_users_measure_item() { - let spec = spec_of(&[(0.0, 1.0)], &["x"]); let mut model = Model::new("fit"); - let formula = fitted_formula(&Tree::Leaf(1.0), &spec, "Total Revenue", MEASURE, &model) - .expect("emitted formula parses"); + let formula = fitted_formula( + &Tree::Leaf(1.0), + &measures(&["x"]), + "Total Revenue", + MEASURE, + &model, + ) + .expect("emitted formula parses"); model.add_formula(formula); assert!( model @@ -317,7 +469,6 @@ mod tests { /// Evidence: `context/test-evidence/gp.md`. #[test] fn a_value_sitting_on_the_threshold_falls_the_same_side() { - let spec = spec_of(&[(0.0, 1.0)], &["x"]); let thresh = 0.7f32; // The trap, spelled out: the short rendering is not the same number. assert!( @@ -325,11 +476,11 @@ mod tests { "this test is only meaningful while 0.7 rounds upward" ); let tree = split(0, thresh, leaf(-1.0), leaf(1.0)); - let text = formula_text(&tree, &spec, TARGET); + let text = formula_text(&tree, &measures(&["x"]), TARGET); assert_eq!(eval(&tree, &[thresh]), 1.0, "f32 routes an equal value hi"); assert_eq!(improvise_eval(&text, &["x"], &[thresh]), 1.0); assert_ne!( - text, "Score = IF(x < 0.7, -1, 1)", + text, "Score = SUM(IF(x < 0.7, -1, 1))", "0.7 is not the f32's exact value — emitting it is the bug" ); } @@ -342,10 +493,11 @@ mod tests { // Deliberately awkward: a space, a keyword, an operator, a one-hot label. const NAMES: [&str; 4] = ["Net Revenue", "MAX", "Cost + Tax", "outlook=rainy"]; let spec = spec_of(&RANGES, &NAMES); + let features = measures(&NAMES); let mut rng = SplitMix64::new(0x0F17_C0FF_EE00_D00D); for _ in 0..60 { let tree = random_tree(&spec, 5, &mut rng); - let text = formula_text(&tree, &spec, TARGET); + let text = formula_text(&tree, &features, TARGET); for _ in 0..10 { let f: Vec = RANGES .iter() @@ -362,14 +514,14 @@ mod tests { #[test] fn fitted_formula_hands_back_a_parsed_formula() { - let spec = spec_of(&[(0.0, 1.0)], &["Net Revenue"]); let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); let mut model = Model::new("fit"); model .category_mut(MEASURE) .expect("_Measure is always present") .add_item("Net Revenue"); - let formula = fitted_formula(&tree, &spec, TARGET, MEASURE, &model).expect("parses"); + let formula = fitted_formula(&tree, &measures(&["Net Revenue"]), TARGET, MEASURE, &model) + .expect("parses"); assert_eq!(formula.target, TARGET); assert_eq!(formula.target_category, MEASURE); } @@ -417,8 +569,9 @@ mod tests { fn the_same_seed_fits_the_same_formula() { let spec = spec_of(&FIT_RANGES, &FIT_NAMES); let rows = fit_rows(); - let first = formula_text(&fit(&spec, &rows, 0xABCD_1234), &spec, TARGET); - let second = formula_text(&fit(&spec, &rows, 0xABCD_1234), &spec, TARGET); + let features = measures(&FIT_NAMES); + let first = formula_text(&fit(&spec, &rows, 0xABCD_1234), &features, TARGET); + let second = formula_text(&fit(&spec, &rows, 0xABCD_1234), &features, TARGET); assert_eq!(first, second); } @@ -428,8 +581,9 @@ mod tests { fn a_different_seed_fits_a_different_formula() { let spec = spec_of(&FIT_RANGES, &FIT_NAMES); let rows = fit_rows(); - let first = formula_text(&fit(&spec, &rows, 0xABCD_1234), &spec, TARGET); - let other = formula_text(&fit(&spec, &rows, 0x9999_0001), &spec, TARGET); + let features = measures(&FIT_NAMES); + let first = formula_text(&fit(&spec, &rows, 0xABCD_1234), &features, TARGET); + let other = formula_text(&fit(&spec, &rows, 0x9999_0001), &features, TARGET); assert_ne!(first, other); } @@ -452,7 +606,7 @@ mod tests { /// What a fitted formula does inside a user's *document*, as opposed to inside /// `improvise_eval`, which fabricates an item per feature name (improvise-7na). mod evaluability_in_a_real_document { - use super::{MEASURE, TARGET, leaf, spec_of, split}; + use super::{MEASURE, TARGET, artefacts, coordinates, leaf, measures, split}; use crate::{fitted_formula, formula_text}; use improvise_core::formula::parse_formula; use improvise_core::model::cell::{CellKey, CellValue}; @@ -495,11 +649,11 @@ mod tests { /// needs no saved encoding plan. See `context/code/gp.md` §12. #[test] fn a_record_added_after_the_fit_is_scored_by_the_formula_alone() { - let spec = spec_of(&[(0.0, 200.0)], &["Cost"]); let tree = split(0, 50.0, leaf(1.0), leaf(2.0)); let mut model = document(); model.add_formula( - parse_formula(&formula_text(&tree, &spec, TARGET), MEASURE).expect("parses"), + parse_formula(&formula_text(&tree, &measures(&["Cost"]), TARGET), MEASURE) + .expect("parses"), ); // A region the fit never saw, added afterwards. model @@ -518,9 +672,8 @@ mod tests { /// fit story works in today. #[test] fn a_split_on_a_measure_evaluates() { - let spec = spec_of(&[(0.0, 200.0)], &["Cost"]); let tree = split(0, 50.0, leaf(1.0), leaf(2.0)); - let text = formula_text(&tree, &spec, TARGET); + let text = formula_text(&tree, &measures(&["Cost"]), TARGET); let mut model = document(); assert_eq!( score_at(&mut model, &text, "North"), @@ -532,21 +685,29 @@ mod tests { ); } - /// improvise-8xhy, the flip of the characterization test this replaces: a - /// one-hot name is an encoding artefact no document has an item for. + /// improvise-4zl7 flips improvise-8xhy's refusal for this family: a one-hot + /// over a real category is a coordinate predicate the grammar now spells. #[test] - fn a_split_on_a_one_hot_feature_is_refused() { - let spec = spec_of(&[(0.0, 1.0)], &["Region=North"]); + fn a_split_on_a_one_hot_feature_is_spelled_as_a_coordinate() { + let features = coordinates(&[(REGION, "North")]); let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); assert_eq!( - formula_text(&tree, &spec, TARGET), - "Score = IF(|Region=North| < 0.5, 1, 2)" + formula_text(&tree, &features, TARGET), + "Score = SUM(IF(Region IS North < 0.5, 1, 2))" ); - let err = fitted_formula(&tree, &spec, TARGET, MEASURE, &document()) - .expect_err("a one-hot feature is not an item of the document") + assert!(fitted_formula(&tree, &features, TARGET, MEASURE, &document()).is_ok()); + } + + /// The other half of the same rule: the document must really have the + /// category, and the question is asked of the document. + #[test] + fn a_coordinate_naming_no_category_of_this_document_is_refused() { + let features = coordinates(&[("Continent", "Antarctica")]); + let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); + let err = fitted_formula(&tree, &features, TARGET, MEASURE, &document()) + .expect_err("Continent is not a category of this document") .to_string(); - assert!(err.contains("Region=North"), "{err}"); - assert!(err.contains("numeric measures"), "{err}"); + assert!(err.contains("Continent=Antarctica"), "{err}"); } /// improvise-hkt6. The same precondition in two places answered differently, @@ -554,9 +715,8 @@ mod tests { #[test] #[should_panic(expected = "tree indexes a feature the spec does not name")] fn a_tree_indexing_past_the_spec_says_so() { - let spec = spec_of(&[(0.0, 200.0)], &["Cost"]); let tree = split(7, 50.0, leaf(1.0), leaf(2.0)); - let _ = fitted_formula(&tree, &spec, TARGET, MEASURE, &document()); + let _ = fitted_formula(&tree, &measures(&["Cost"]), TARGET, MEASURE, &document()); } /// The same refusal for the other two artefact families. Nothing here @@ -564,13 +724,12 @@ mod tests { #[test] fn the_missing_flag_and_the_date_sub_features_are_refused_too() { for name in ["Cost.missing", "when.year", "when.month"] { - let names = [name]; - let spec = spec_of(&[(0.0, 1.0)], &names); let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); - let err = fitted_formula(&tree, &spec, TARGET, MEASURE, &document()) + let err = fitted_formula(&tree, &artefacts(&[name]), TARGET, MEASURE, &document()) .expect_err(name) .to_string(); assert!(err.contains(name), "{err}"); + assert!(err.contains("cannot spell"), "{err}"); } } @@ -578,10 +737,9 @@ mod tests { /// `Ok`, no cell of the target is a `ref:` error. See the evidence file. #[test] fn a_formula_the_emitter_accepted_evaluates_everywhere() { - let spec = spec_of(&[(0.0, 200.0)], &["Cost"]); let tree = split(0, 50.0, leaf(1.0), leaf(2.0)); let mut model = document(); - let formula = fitted_formula(&tree, &spec, TARGET, MEASURE, &model) + let formula = fitted_formula(&tree, &measures(&["Cost"]), TARGET, MEASURE, &model) .expect("a split on a real measure is accepted"); model.add_formula(formula); model.recompute_formulas(&[]); @@ -597,14 +755,14 @@ mod tests { /// accepted — which is why the check asks the document, not the name's shape. #[test] fn a_measure_that_merely_looks_like_a_one_hot_is_accepted() { - let spec = spec_of(&[(0.0, 1.0)], &["Region=North"]); let tree = split(0, 0.5, leaf(1.0), leaf(2.0)); let mut model = document(); model .category_mut(MEASURE) .expect("_Measure is always present") .add_item("Region=North"); - assert!(fitted_formula(&tree, &spec, TARGET, MEASURE, &model).is_ok()); + let features = measures(&["Region=North"]); + assert!(fitted_formula(&tree, &features, TARGET, MEASURE, &model).is_ok()); } } } From 0edd26b1b5155fe2d86c5474619dd26646b5af21 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 11:54:29 -0700 Subject: [PATCH 2/4] fit: fold the member record onto `partial`, not onto `context` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the commit before it. The record key the fold evaluates at was built by folding the record's coordinates onto `context`, which still carries the formula's own target coordinate. Harmless for a `_Measure` target, because `Expr::Ref` overwrites `_Measure` on the way in — and not harmless for a formula targeting any other category: the record key keeps `Levy=Tax`, `evaluate_depth` matches the same formula again at that key, and the cell converges to `Error("circular")`. A formula silently self-referencing through its own aggregate. `partial` is `context.without(target_category)` plus any inline WHERE, so folding onto it drops the target coordinate by construction. Every member satisfies `partial` by definition — that is what selected them — so nothing constraining is lost. `a_formula_targeting_another_category_still_folds_over_its_records` is the guard; restoring the `context` fold fails it with `Error("circular")` against `11.0` and leaves the other eight passing. Evidence in `context/test-evidence/core.md`. Co-Authored-By: Claude Opus 5 (1M context) --- context/test-evidence/core.md | 19 +++++++++++++++++++ crates/improvise-core/src/model/types.rs | 18 +++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 5916c6f1..6af1c0a8 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -477,3 +477,22 @@ missing cell, a categorical column and a date column, so all four roles appear. Fault injected: swapping the `Numeric` and `Missing` pushes in `feature_roles` fails this test **and** `encode_row_replays_the_training_expansion`, which is the pair that would have disagreed. + +### a_formula_targeting_another_category_still_folds_over_its_records + +Found by self-review, not by the ticket. The fold evaluates the inner expression +at each member record's key, and the first version built that key by folding the +record's coordinates onto **`context`** — which still carries the formula's own +target coordinate. For the ordinary `_Measure` target that is harmless, because +`Expr::Ref` overwrites `_Measure` on the way in. For a formula targeting any +other category it is not: the record key keeps `Levy=Tax`, so `evaluate_depth` +matches the formula again at that key and the whole cell converges to +`Error("circular")` — a formula quietly self-referencing through its own +aggregate. + +The key is now folded onto `partial` (`context.without(target_category)`, plus +any inline `WHERE`), which drops the target coordinate by construction rather +than by every caller remembering to. Fault injected by restoring the `context` +fold: this test fails with `Error("circular")` against `11.0` and the other +eight pass, which is why it is a separate test and not an extra assertion on one +of them. diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 6b156936..19d2a79c 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -86,7 +86,7 @@ fn eval_agg( let at = record .0 .iter() - .fold(context.clone(), |k, (cat, item)| k.with(cat, item)); + .fold(partial.clone(), |k, (cat, item)| k.with(cat, item)); eval_at(inner, &at) }) .collect::, String>>()?, @@ -2430,6 +2430,22 @@ mod route_then_aggregate_tests { assert_eq!(number_at(&m, &[("Tag", "a"), (MEASURE, "Pred")]), 0.0); } + /// A formula may target a category other than `_Measure`, and that + /// coordinate must not travel onto the record keys the fold evaluates at. + #[test] + fn a_formula_targeting_another_category_still_folds_over_its_records() { + let mut m = four_records("Tax = SUM(IF(Cost < 50, 1, 10))"); + m.add_category("Levy").expect("under the limit"); + m.category_mut("Levy").expect("just added").add_item("Tax"); + m.add_formula( + parse_formula("Tax = SUM(IF(Cost < 50, 1, 10))", "Levy").expect("the fixture parses"), + ); + assert_eq!( + m.evaluate(&key(&[("Region", "North"), ("Levy", "Tax")])), + Some(CellValue::Number(11.0)) + ); + } + /// The fold asks the expression of each member, so a record whose input is /// missing is an error rather than a silently dropped row. #[test] From 5fbcb1718abf957ac3dc8b8a374aa66908bb20e4 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 12:20:53 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fit:=20one=20evaluator=20for=20the=20fold?= =?UTF-8?q?=20=E2=80=94=20which=20was=20also=20the=20stall?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #54 found the fold reintroducing exactly the divergence the ruling exists to remove, and the same line turned out to be the 244 ms stall. One defect, two symptoms. `eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at `context.with(cat, name)`. Inside the fold `context` is a MEMBER-RECORD key carrying the none_cat coordinates, while `formula_cache` is keyed by stems with none_cats STRIPPED — so the lookup could never hit, fell through to `aggregate_raw`, found no raw cell behind a formula target, and answered Err("ref:…"). A routing formula over a derived measure gave Number(11.0) uncached and Error("ref:Margin") cached, and the cached path is the one a pivot grid runs whenever any category sits on Axis::None. `eval_expr`/`eval_bool` are hoisted to module level and BOTH evaluators now hand the fold the same record-level `eval_expr`. The two agree by construction rather than by a test. Note the repair not taken: teaching the fold to hit the cache by stripping none_cats off the member key returns the GROUP's value to every member — aggregate-then-route through a cache lookup, and silent. That same per-member `aggregate_raw` was one poorly-narrowed scan per member, an O(n^2) shape. Measured with the new `examples/fold_bench.rs`, five runs a point, at 18,050 cells: 51.1-53.3 ms before, 18.1-19.2 ms after, against 7.1-9.0 ms for a linear formula. Scaling went from superlinear (45x cells, 85x time) to linear (45x cells, 45x time). Debug 2.75 s -> 132.9-135.1 ms. The fixed point runs exactly 2 passes, measured by counting, so hoisting the member set has a 2x ceiling and is declined on improvise-9iyv — a memo keyed by less than the whole `partial` is a second answer to what a cell depends on. `Expr::Is` has an arm in each evaluator and the cached one was untested: the earlier fault was injected into the shared `eval_is`, which both arms call. Replacing the cached arm alone left the suite green. The first replacement test ALSO stayed green, which is the interesting part — once the fold routes through `eval_expr`, an `Is` inside a SUM never reaches the cached arm, so that arm is reachable only by a bare `Is` outside an aggregate. That is what the new test uses, and injecting into that arm alone now fails it. `IS` gained a word boundary: `Region ISLAND` parsed as Is(Region, "LAND"), silently wrong where it was a parse error. `is_expr` is compound-atomic with `sp+` written out, because a negative lookahead in an ordinary rule tests the character AFTER pest's auto-inserted whitespace and fails backwards. The lookahead was then redundant and removed — deleting it left the suite green, while relaxing `sp+` to `sp*` fails the test, so `sp+` is the mechanism. `WHERE` has the same hole, is far less exposed, and is filed as improvise-r08u. Also: `ambiguous.sort_unstable()` before `dedup`, which only removes consecutive duplicates, so the refusal could name a column twice. Every fault re-injected after the refactor: no-fold 10 failures, shared eval_is 5, record-level Is arm 4, cached Is arm 1, feature_roles order 2, context-vs- partial 1, the new cached-fold divergence 1, `sp+` relaxed 1. Baseline 0. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/core.md | 54 ++++- context/test-evidence/core.md | 47 ++++ context/test-evidence/formula-and-io.md | 33 +++ crates/improvise-core/examples/fold_bench.rs | 81 +++++++ crates/improvise-core/src/model/types.rs | 239 +++++++++++-------- crates/improvise-formula/src/formula.pest | 6 +- crates/improvise-formula/src/parser.rs | 23 ++ crates/improvise-gp/src/fit.rs | 3 + 8 files changed, 382 insertions(+), 104 deletions(-) create mode 100644 crates/improvise-core/examples/fold_bench.rs diff --git a/context/code/core.md b/context/code/core.md index 02ba24a3..dcad37c5 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -464,10 +464,27 @@ values = records.map(|r| eval(inner, context.with(every coord of r))) Three consequences worth stating. -The rule is **written once**. `eval_agg` is shared by `eval_expr` and -`eval_expr_cached`, so the fold has one home; the two evaluators pass their own -recursion in as `eval_at`. Only `Expr::Is` needed an arm in each, and the two -arms are one line apiece. +The rule is **written once, and so is the evaluator it folds with.** `eval_agg` +is shared by `eval_expr` and `eval_expr_cached`, and **both** hand it the same +`eval_at`: the record-level `eval_expr`. That is not a convenience, it is the +correctness. A member key is a *record*, and `formula_cache` is keyed by +**stems with `none_cats` stripped** — so the cached evaluator's `Expr::Ref` can +never hit the cache at a member key, falls through to `aggregate_raw`, finds no +raw cell behind a formula target, and answers `Err("ref:…")`. A first version +did exactly that, and the two evaluators then disagreed on any routing formula +reading a *derived* measure: `Number(4.0)` uncached, `Error("ref:Profit")` +cached, which is the very divergence this section exists to remove (§3: "a rule +stated in one and not the other is a live bug"). + +**And making the lookup "work" would have been worse than the error.** The only +way to hit the cache from a member key is to strip `none_cats` off it — which +returns the *group's* value for that formula and hands it to every member. That +is aggregate-then-route again, laundered through a cache lookup, and it would +have been silent. The cache cannot answer a per-record question, so the fold +does not ask it. Only `Expr::Is` needed an arm in each evaluator, and since the +fold routes through `eval_expr`, the cached arm is now reached **only by a bare +`Is` outside a `SUM`** — which is what +`the_cached_evaluator_has_its_own_working_coordinate_predicate` pins. The `Expr::Ref` fast path **stays**, and it is the same fold specialised: a record's `Ref` is that record's own cell, so summing them is summing the @@ -478,6 +495,13 @@ expression of each member, and an expression that cannot be evaluated is an error rather than a zero. Silently dropping members is how a routed sum would quietly answer for a subset. +The asymmetry's **blast radius** is wider than it sounds, and worth stating: +gotcha 7 makes a pipe-quoted text cell a first-class `.improv` value, so **one** +text cell anywhere in a member set turns the whole aggregated fitted cell into +`Error("ref:Cost")`, where the bare-ref path would have dropped it and returned +a number. That is the loud failure this repo prefers to a quietly wrong sum, but +it is a real behaviour difference and not a detail. + Records come out in `DataStore` insertion order, deduplicated by an `IndexSet`, for §8's reason — hash order here would make a fitted cell drift in its last bits between processes. @@ -488,9 +512,25 @@ new state. The fold reads *raw* data through the same `evaluate_depth` / on raw cells and on other formula cells' cached values; `recompute_formulas` iterates the same stems to the same fixed point, and `formula_cache` still holds exactly the aggregated formula cells it held before. The fold does not cache and -does not memoise: it is O(members) per evaluation per pass, so a routed formula -over a large document is the one place this layer got materially more -expensive, and it is unbenchmarked. +does not memoise: it is O(members) per evaluation per pass. + +**Measured**, because the cost was a blocker and not a footnote +(improvise-9iyv; `crates/improvise-core/examples/fold_bench.rs`, five runs per +point, best–worst). At 18,050 cells with one routing formula and `Tag` on +`Axis::None`, `recompute_formulas` takes **18.1–19.2 ms** release and +132.9–135.1 ms debug, against 7.1–9.0 ms / 71.7–74.6 ms for a linear +`Rev - Cost`. That is **2.3×** a linear formula and **linear in cells**: 400 → +0.4 ms, 2,000 → 2.0 ms, 18,050 → 18.1 ms, so 45× the cells costs 45× the time. + +It was not always linear. Routing the fold through `eval_expr_cached` — the +first version, above — made every member's `Ref` miss the cache and fall into +`aggregate_raw`, one poorly-narrowed scan **per member**: 51.1–53.3 ms at the +same 18,050 cells and superlinear with it (45× the cells cost 85× the time). +So the divergence and the stall were **one defect**, and fixing the first fixed +the second. The fixed point runs exactly **2** passes (measured, not assumed), +so hoisting the member set out of the loop has a 2× ceiling on the fold's share +and was declined: a memo is state, and a memo keyed by anything less than the +whole `partial` is a second answer to "what does this cell depend on". **`Expr::Is(Filter)` — a coordinate predicate as a number.** `Cat IS Item` is `1.0` when the cell's own `Cat` coordinate is `Item` and `0.0` otherwise. It diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 6af1c0a8..d203bb31 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -496,3 +496,50 @@ than by every caller remembering to. Fault injected by restoring the `context` fold: this test fails with `Error("circular")` against `11.0` and the other eight pass, which is why it is a separate test and not an extra assertion on one of them. + +### the_two_evaluators_agree_when_the_fold_reads_a_derived_measure + +Adversarial review of PR #54, and the sharpest finding of the round: the first +version of the fold reintroduced **exactly the divergence the ruling exists to +remove**, through the cached path. + +`eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at +`context.with(cat, name)`. Inside the fold, `context` is a **member-record key** +carrying the `none_cats` coordinates, while `formula_cache` is keyed by stems +with `none_cats` **stripped**. The lookup could never hit, fell through to +`aggregate_raw`, found no raw cell behind a formula target, and answered +`Err("ref:…")`. So a routing formula reading a *derived* measure gave +`Number(11.0)` uncached and `Error("ref:Margin")` cached — and the cached path +is the one a pivot grid runs whenever any category sits on `Axis::None`. + +The fixture is the four-record one plus `Margin = Cost * 2`, so the fold's input +is a formula rather than raw data. The test asserts the uncached value **and** +that the two agree; asserting only agreement would pass if both broke. + +The fix makes them agree by construction rather than by this test: `eval_expr` +is hoisted to module level and **both** evaluators hand it to `eval_agg` as +`eval_at`. Note the repair that was *not* taken — teaching the fold to hit the +cache by stripping `none_cats` off the member key returns the **group's** value +to every member, which is aggregate-then-route through a cache lookup, silent. +Fault injected by restoring `eval_expr_cached` as the fold's evaluator: this +test fails with `Error("ref:Margin")` and the other eleven pass. + +### the_cached_evaluator_has_its_own_working_coordinate_predicate + +The same review's second finding, and a lesson about where to inject a fault. +`Expr::Is` has an arm in **each** evaluator. The original evidence injected the +fault into the shared `eval_is`, which both arms call — proving the tests see +the predicate, not that they see the cached *copy*. Replacing only the cached +arm with `Ok(0.0)` left the suite at 1330 passing. + +The first attempt to close it — a coordinate split run through +`recompute_formulas` + `evaluate_aggregated` — **also** stayed green, and that +is the interesting part: once the fold routes through `eval_expr`, an `Is` +inside a `SUM` never reaches the cached arm at all. The cached arm is now +reachable only by a **bare** `Is` outside an aggregate, which is what this test +uses. Fault injected into that arm alone: it fails, `Number(0.0)` against +`Number(1.0)`. + +The general shape: an injected fault proves coverage only of the code path it is +injected into. Two arms of the same rule need two injections, and if one of them +turns out to be unreachable, that is a fact about the design worth writing down. diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 3242bb6c..d16a5963 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -617,3 +617,36 @@ improvise-5j9: skipping a record for a missing category value used to be silent, so a 1000-row import could drop rows with no user-visible sign. The warning must name the count, the total, and the offending field(s) — a bare "some records were skipped" is not actionable. + +### is_does_not_swallow_the_start_of_a_longer_word + +Adversarial review of PR #54, low severity and fixed anyway. `is_expr` was +`identifier ~ ^"IS" ~ filter_value`, so `Region ISLAND` parsed as +`Is(Region, "LAND")` and `Region isotope` as `Is(Region, "otope")` — inputs that +were **parse errors** before the production existed, now silently-wrong +formulas. Gotcha 34's whole theme is that a silently wrong parse costs more than +a refusal. + +The repair went through two wrong shapes before the right one, and both are +worth keeping. A negative lookahead `^"IS" ~ !(ASCII_ALPHANUMERIC | …)` in an +ordinary rule **cannot work**: pest auto-inserts `WHITESPACE` between sequence +elements, so it tested the character *after* the space — rejecting +`Region IS East` while still accepting `Region ISLAND`, i.e. exactly backwards. +Making `is_expr` compound-atomic (`$`) stops the auto-insertion and forces the +separator to be written out as `sp+`. + +At which point **the lookahead was redundant and was removed**, which only +re-injecting the fault revealed: deleting `!bare_char` left the suite green, +because `sp+` after `IS` already refuses `ISLAND` — `IS` matches, then `sp+` +demands whitespace and finds `L`. A guard nobody has seen fail is +indistinguishable from one that always passes +(`doctrine(check_the_checker)`), so it went. **`sp+` is now the mechanism**, and +relaxing it to `sp*` — the obvious "let people write `Region IS|East|`" change — +reopens the hole; that substitution is what this test was finally proved +against. The negative arm is paired with a positive one, or "reject everything" +would pass. + +`^"WHERE"` has the identical hole and was deliberately **not** fixed here: +pre-existing, far less exposed (it sits in one fixed position after an +expression, where `IS` follows any identifier in expression position), and a +grammar change with its own round-trip risk. Filed as improvise-r08u. diff --git a/crates/improvise-core/examples/fold_bench.rs b/crates/improvise-core/examples/fold_bench.rs new file mode 100644 index 00000000..692527fb --- /dev/null +++ b/crates/improvise-core/examples/fold_bench.rs @@ -0,0 +1,81 @@ +//! The cost of a routing formula against a linear one, at `gotcha 41`'s own +//! scale. improvise-9iyv; the argument is `context/code/core.md` §13. + +use std::time::Instant; + +use improvise_core::formula::parse_formula; +use improvise_core::model::cell::{CellKey, CellValue}; +use improvise_core::model::types::Model; + +const MEASURE: &str = "_Measure"; + +/// `regions × tags` records carrying `Cost` and `Rev`, plus the formula under +/// test. Cost straddles the threshold so the routing really routes. +fn document(formula: &str, regions: usize, tags: usize) -> (Model, usize) { + let mut m = Model::new("bench"); + for cat in ["Region", "Tag"] { + m.add_category(cat).expect("under the limit"); + } + for r in 0..regions { + m.category_mut("Region") + .expect("just added") + .add_item(format!("r{r}")); + } + for t in 0..tags { + m.category_mut("Tag") + .expect("just added") + .add_item(format!("t{t}")); + } + let measure = m.category_mut(MEASURE).expect("_Measure always exists"); + for name in ["Cost", "Rev"] { + measure.add_item(name); + } + let mut cells = 0; + for r in 0..regions { + for t in 0..tags { + for (name, value) in [("Cost", (t % 100) as f64), ("Rev", (t % 7) as f64)] { + m.set_cell( + CellKey::new(vec![ + ("Region".to_string(), format!("r{r}")), + ("Tag".to_string(), format!("t{t}")), + (MEASURE.to_string(), name.to_string()), + ]), + CellValue::Number(value), + ); + cells += 1; + } + } + } + m.add_formula(parse_formula(formula, MEASURE).expect("the fixture parses")); + (m, cells) +} + +/// Milliseconds for `runs` recomputes, as (best, worst) — a single figure has +/// misled this wave twice. +fn time(m: &mut Model, none_cats: &[String], runs: usize) -> (f64, f64) { + let mut best = f64::INFINITY; + let mut worst: f64 = 0.0; + for _ in 0..runs { + let t = Instant::now(); + m.recompute_formulas(none_cats); + let ms = t.elapsed().as_secs_f64() * 1000.0; + best = best.min(ms); + worst = worst.max(ms); + } + (best, worst) +} + +fn main() { + let none_cats = ["Tag".to_string()]; + println!( + "{:>7} {:>26} {:>20}", + "cells", "routing SUM(IF(..))", "linear Rev-Cost" + ); + for (regions, tags) in [(5, 40), (5, 200), (25, 361)] { + let (mut routing, cells) = document("Pred = SUM(IF(Cost < 50, 1, 10))", regions, tags); + let (mut linear, _) = document("Pred = Rev - Cost", regions, tags); + let (rb, rw) = time(&mut routing, &none_cats, 5); + let (lb, lw) = time(&mut linear, &none_cats, 5); + println!("{cells:>7} {rb:>11.1}-{rw:<13.1} {lb:>8.1}-{lw:<10.1}"); + } +} diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 19d2a79c..426cfccc 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -101,6 +101,97 @@ fn eval_agg( } } +/// The **record-level** evaluator: no cache, because a member key is not a stem +/// the cache holds. Errors propagate immediately — §3, §13. +fn eval_expr( + expr: &Expr, + context: &CellKey, + model: &Model, + target_category: &str, + depth: u8, +) -> Result { + use crate::formula::BinOp; + match expr { + Expr::Number(n) => Ok(*n), + Expr::Ref(name) => { + let cat = model + .find_item_category(name) + .ok_or_else(|| format!("ref:{name}"))?; + let new_key = context.clone().with(cat, name); + match model.evaluate_depth(&new_key, depth) { + Some(CellValue::Number(n)) => Ok(n), + Some(CellValue::Error(e)) => Err(e), + _ => Err(format!("ref:{name}")), + } + } + Expr::BinOp(op, l, r) => { + let lv = eval_expr(l, context, model, target_category, depth)?; + let rv = eval_expr(r, context, model, target_category, depth)?; + match op { + BinOp::Add => Ok(lv + rv), + BinOp::Sub => Ok(lv - rv), + BinOp::Mul => Ok(lv * rv), + BinOp::Div => match rv.abs() < FLOAT_EQ_EPSILON { + true => Err("div/0".into()), + false => Ok(lv / rv), + }, + BinOp::Pow => Ok(lv.powf(rv)), + BinOp::Eq | BinOp::Ne | BinOp::Lt | BinOp::Gt | BinOp::Le | BinOp::Ge => { + Err("type".into()) + } + } + } + Expr::UnaryMinus(e) => Ok(-eval_expr(e, context, model, target_category, depth)?), + Expr::Agg(func, inner, agg_filter) => eval_agg( + model, + context, + target_category, + func, + inner.as_ref(), + agg_filter.as_ref(), + &|e, at| eval_expr(e, at, model, target_category, depth), + ), + Expr::Is(filter) => Ok(eval_is(context, filter)), + Expr::If(cond, then, else_) => { + let cv = eval_bool(cond, context, model, target_category, depth)?; + match cv { + true => eval_expr(then, context, model, target_category, depth), + false => eval_expr(else_, context, model, target_category, depth), + } + } + } +} + +/// The comparison half of [`eval_expr`]. Only an `IF` condition reaches it, so +/// anything but a comparison is a type error. +fn eval_bool( + expr: &Expr, + context: &CellKey, + model: &Model, + target_category: &str, + depth: u8, +) -> Result { + use crate::formula::BinOp; + match expr { + Expr::BinOp(op, l, r) => { + let lv = eval_expr(l, context, model, target_category, depth)?; + let rv = eval_expr(r, context, model, target_category, depth)?; + match op { + BinOp::Eq => Ok((lv - rv).abs() < FLOAT_EQ_EPSILON), + BinOp::Ne => Ok((lv - rv).abs() >= FLOAT_EQ_EPSILON), + BinOp::Lt => Ok(lv < rv), + BinOp::Gt => Ok(lv > rv), + BinOp::Le => Ok(lv <= rv), + BinOp::Ge => Ok(lv >= rv), + BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Pow => { + Err("type".into()) + } + } + } + _ => Err("type".into()), + } +} + /// `Cat IS Item` as a number. A cell with no `Cat` coordinate is **not** that /// item, matching `Formula::filter`'s own `unwrap_or(false)` — §13. fn eval_is(context: &CellKey, filter: &Filter) -> f64 { @@ -554,7 +645,9 @@ impl Model { func, inner.as_ref(), agg_filter.as_ref(), - &|e, at| eval_expr_cached(e, at, model, target_category, none_cats), + // The fold evaluates at a MEMBER key, which is not a stem the + // cache holds — so the record-level evaluator, in both paths. + &|e, at| eval_expr(e, at, model, target_category, Model::MAX_EVAL_DEPTH), ), Expr::Is(filter) => Ok(eval_is(context, filter)), Expr::If(cond, then, else_) => { @@ -620,8 +713,6 @@ impl Model { context: &CellKey, depth: u8, ) -> Option { - use crate::formula::Expr; - // Check the WHERE filter first. if let Some(filter) = &formula.filter { let matches = context @@ -633,99 +724,6 @@ impl Model { } } - /// Errors propagate immediately: a circular reference anywhere - /// short-circuits the whole formula. - fn eval_expr( - expr: &Expr, - context: &CellKey, - model: &Model, - target_category: &str, - depth: u8, - ) -> Result { - match expr { - Expr::Number(n) => Ok(*n), - Expr::Ref(name) => { - let cat = model - .find_item_category(name) - .ok_or_else(|| format!("ref:{name}"))?; - let new_key = context.clone().with(cat, name); - match model.evaluate_depth(&new_key, depth) { - Some(CellValue::Number(n)) => Ok(n), - Some(CellValue::Error(e)) => Err(e), - _ => Err(format!("ref:{name}")), - } - } - Expr::BinOp(op, l, r) => { - use crate::formula::BinOp; - let lv = eval_expr(l, context, model, target_category, depth)?; - let rv = eval_expr(r, context, model, target_category, depth)?; - match op { - BinOp::Add => Ok(lv + rv), - BinOp::Sub => Ok(lv - rv), - BinOp::Mul => Ok(lv * rv), - BinOp::Div => { - if rv.abs() < FLOAT_EQ_EPSILON { - Err("div/0".into()) - } else { - Ok(lv / rv) - } - } - BinOp::Pow => Ok(lv.powf(rv)), - BinOp::Eq | BinOp::Ne | BinOp::Lt | BinOp::Gt | BinOp::Le | BinOp::Ge => { - Err("type".into()) - } - } - } - Expr::UnaryMinus(e) => Ok(-eval_expr(e, context, model, target_category, depth)?), - Expr::Agg(func, inner, agg_filter) => eval_agg( - model, - context, - target_category, - func, - inner.as_ref(), - agg_filter.as_ref(), - &|e, at| eval_expr(e, at, model, target_category, depth), - ), - Expr::Is(filter) => Ok(eval_is(context, filter)), - Expr::If(cond, then, else_) => { - let cv = eval_bool(cond, context, model, target_category, depth)?; - if cv { - eval_expr(then, context, model, target_category, depth) - } else { - eval_expr(else_, context, model, target_category, depth) - } - } - } - } - - fn eval_bool( - expr: &Expr, - context: &CellKey, - model: &Model, - target_category: &str, - depth: u8, - ) -> Result { - use crate::formula::BinOp; - match expr { - Expr::BinOp(op, l, r) => { - let lv = eval_expr(l, context, model, target_category, depth)?; - let rv = eval_expr(r, context, model, target_category, depth)?; - match op { - BinOp::Eq => Ok((lv - rv).abs() < FLOAT_EQ_EPSILON), - BinOp::Ne => Ok((lv - rv).abs() >= FLOAT_EQ_EPSILON), - BinOp::Lt => Ok(lv < rv), - BinOp::Gt => Ok(lv > rv), - BinOp::Le => Ok(lv <= rv), - BinOp::Ge => Ok(lv >= rv), - BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Pow => { - Err("type".into()) - } - } - } - _ => Err("type".into()), - } - } - match eval_expr( &formula.expr, context, @@ -2375,6 +2373,23 @@ mod route_then_aggregate_tests { assert_eq!(number_at(&m, &[(MEASURE, "Pred")]), 22.0); } + /// The fold must answer the same in both evaluators when a member's input is + /// itself a formula. `Margin` doubles `Cost`: 20, 200, 40, 400. + #[test] + fn the_two_evaluators_agree_when_the_fold_reads_a_derived_measure() { + let mut m = four_records("Pred = SUM(IF(Margin < 100, 1, 10))"); + m.add_formula(parse_formula("Margin = Cost * 2", MEASURE).expect("the fixture parses")); + let none_cats = ["Tag".to_string()]; + let north = key(&[("Region", "North"), (MEASURE, "Pred")]); + + let uncached = m.evaluate(&north); + m.recompute_formulas(&none_cats); + let cached = m.evaluate_aggregated(&north, &none_cats); + + assert_eq!(uncached, Some(CellValue::Number(11.0))); + assert_eq!(cached, uncached, "the two evaluators must not diverge"); + } + /// The cached evaluator must answer the same. `recompute_formulas` is the /// fixed point of gotcha 9, and it caches the *aggregated* stems. #[test] @@ -2406,6 +2421,38 @@ mod route_then_aggregate_tests { ); } + /// The cached evaluator's own `Is` arm, which only a **bare** `Is` reaches: + /// inside a `SUM` the fold hands the predicate to the record-level evaluator. + #[test] + fn the_cached_evaluator_has_its_own_working_coordinate_predicate() { + let mut m = four_records("Pred = Region IS North"); + let none_cats = ["Tag".to_string()]; + m.recompute_formulas(&none_cats); + for (region, expected) in [("North", 1.0), ("South", 0.0)] { + assert_eq!( + m.evaluate_aggregated(&key(&[("Region", region), (MEASURE, "Pred")]), &none_cats), + Some(CellValue::Number(expected)), + "{region}" + ); + } + } + + /// A coordinate split through the **cached** evaluator — the one a real pivot + /// view runs, and the one a fitted `Cat IS Item` actually reaches. + #[test] + fn the_cached_evaluator_reads_each_records_coordinate_too() { + let mut m = four_records("Pred = SUM(IF(Region IS North < 0.5, 100, 7))"); + let none_cats = ["Tag".to_string()]; + m.recompute_formulas(&none_cats); + for (region, expected) in [("North", 14.0), ("South", 200.0)] { + assert_eq!( + m.evaluate_aggregated(&key(&[("Region", region), (MEASURE, "Pred")]), &none_cats), + Some(CellValue::Number(expected)), + "{region}" + ); + } + } + /// The case the whole ruling exists for: with `Region` aggregated away the /// cell has no `Region` coordinate of its own, yet the answer is 14 + 200. #[test] diff --git a/crates/improvise-formula/src/formula.pest b/crates/improvise-formula/src/formula.pest index 9c14a1b1..cffcdecc 100644 --- a/crates/improvise-formula/src/formula.pest +++ b/crates/improvise-formula/src/formula.pest @@ -53,7 +53,11 @@ primary = { // `Cat IS Item` — a coordinate predicate as 1/0. It must precede `ref_expr`, // which would otherwise consume the category name and leave `IS Item` behind. -is_expr = { identifier ~ ^"IS" ~ filter_value } +// Compound-atomic, so WHITESPACE is NOT auto-inserted and `sp+` is the real +// separator. `sp+` after `IS` is load-bearing — relax it and `Region ISLAND` +// reads as `Is(Region, "LAND")`, silently wrong where it was a parse error. +is_expr = ${ identifier ~ sp+ ~ ^"IS" ~ sp+ ~ filter_value } +sp = _{ " " | "\t" } paren_expr = { "(" ~ expr ~ ")" } diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index 49706fbc..1f56e2b4 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -528,6 +528,29 @@ mod tests { "{:?}", f.expr ); + let f = parse_formula("X = IS", "Cat").unwrap(); + assert!( + matches!(f.expr, Expr::Ref(ref n) if n == "IS"), + "{:?}", + f.expr + ); + } + + /// `IS` needs a trailing word boundary, or `Region ISLAND` reads as + /// `Is(Region, "LAND")` — silently wrong where it used to be a parse error. + #[test] + fn is_does_not_swallow_the_start_of_a_longer_word() { + for text in ["X = Region ISLAND", "X = Region isotope", "X = Region IS_A"] { + assert!( + parse_formula(text, "Cat").is_err(), + "{text:?} must not parse as a coordinate predicate" + ); + } + // …and the boundary does not reject the real thing. + for text in ["X = Region IS East", "X = Region IS |East Coast|"] { + let f = parse_formula(text, "Cat").unwrap_or_else(|e| panic!("{text:?}: {e}")); + assert!(matches!(f.expr, Expr::Is(_)), "{text:?}: {:?}", f.expr); + } } // ── Error paths ───────────────────────────────────────────────────── diff --git a/crates/improvise-gp/src/fit.rs b/crates/improvise-gp/src/fit.rs index 9ca259f4..d40e8715 100644 --- a/crates/improvise-gp/src/fit.rs +++ b/crates/improvise-gp/src/fit.rs @@ -334,6 +334,9 @@ fn no_ambiguous_column_names(header: &[String]) -> anyhow::Result<()> { > 1 }) .collect(); + // `dedup` only removes *consecutive* duplicates, and a header is not sorted, + // so without the sort the refusal can name the same column twice. + ambiguous.sort_unstable(); ambiguous.dedup(); anyhow::ensure!( ambiguous.is_empty(), From ee8d4b3709036d02ed113f0cd90bb456cbe96516 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 12:30:25 -0700 Subject: [PATCH 4/4] docs(core): the shape-dependence is what pins the cause, not the number The review re-ran fold_bench across two shapes at the SAME 18,050 cells: the old code cost 51 ms at 25x361 and 244 ms at 95x95, and after the fix the two are indistinguishable. A 4.8x spread from group count alone is what a per-member scan predicts and a constant factor does not, so the spread vanishing is better evidence for the diagnosis than the absolute number. Also records the depth asymmetry between the two Agg arms, which bounds a chain of folded formulas and must not be tidied into one rule. Co-Authored-By: Claude Opus 5 (1M context) --- context/code/core.md | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index 9118ab32..7af54b48 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -527,10 +527,25 @@ first version, above — made every member's `Ref` miss the cache and fall into `aggregate_raw`, one poorly-narrowed scan **per member**: 51.1–53.3 ms at the same 18,050 cells and superlinear with it (45× the cells cost 85× the time). So the divergence and the stall were **one defect**, and fixing the first fixed -the second. The fixed point runs exactly **2** passes (measured, not assumed), -so hoisting the member set out of the loop has a 2× ceiling on the fold's share -and was declined: a memo is state, and a memo keyed by anything less than the -whole `partial` is a second answer to "what does this cell depend on". +the second. + +**And "18,050 cells" was not a sufficient description of that workload.** At the +same cell count the old code cost 51 ms at a 25×361 shape and 244 ms at 95×95 — +a 4.8× spread from group *count* alone, which is what a per-member scan predicts +and a constant factor does not. After the fix the two shapes are 16.9–17.2 ms +and 16.4–17.5 ms: indistinguishable. **The shape-dependence being gone is the +confirmation of the cause**, more than the absolute number is. Re-measuring this +means stating the shape, not just the cells. + +The fixed point runs exactly **2** passes (measured, not assumed), so hoisting +the member set out of the loop has a 2× ceiling on the fold's share and was +declined: a memo is state, and a memo keyed by anything less than the whole +`partial` is a second answer to "what does this cell depend on". + +One asymmetry not to tidy: the cached path hands `eval_expr` a fresh +`MAX_EVAL_DEPTH` at each `Agg`, while the uncached `Agg` threads its own `depth` +through. The second is what bounds a chain of folded formulas; making them agree +by resetting both would unbound it. **`Expr::Is(Filter)` — a coordinate predicate as a number.** `Cat IS Item` is `1.0` when the cell's own `Cat` coordinate is `Item` and `0.0` otherwise. It