Merge pull request #54 from fiddlerwoaroof/wave/f-route-then-aggregate
CI / Test and lint (host) (push) Has been cancelled
CI / wasm32 check (browser bundle crates) (push) Has been cancelled
CI / Knowledge base consistency (push) Has been cancelled
CI / Browser harness (OPFS contract + shell) (push) Has been cancelled
CI / Browser bundle + static site (push) Has been cancelled
CI / Deploy to GitHub Pages (push) Has been cancelled

fit: route per record and aggregate, never route on the aggregate (improvise-4zl7)
This commit is contained in:
Ed Langley
2026-09-20 13:02:15 -07:00
committed by GitHub
16 changed files with 1510 additions and 406 deletions
+122
View File
@@ -464,6 +464,128 @@ renamed. Found in review, and the first recorded reason for declining it was
**wrong** — it claimed the injected behaviour was defensible by analogy with a
formula targeting a regular category, which is a different key entirely.
## 13. Route then aggregate: `SUM(<expr>)` 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, 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
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(<expr>)` 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.
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.
**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.
**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.
**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
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.
## `PageAxis` — what constrains, and what the user may turn (improvise-p1ym)
`GridLayout::page_coords` answers two questions that gotcha 27 says must not be
+28 -1
View File
@@ -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
+99 -67
View File
@@ -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
+132
View File
@@ -411,3 +411,135 @@ 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(<expr>)` 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.
### 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.
### 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.
+33
View File
@@ -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.
+70 -39
View File
@@ -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::<f64>() > 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.
+149 -33
View File
@@ -452,32 +452,55 @@ pub fn preprocess(header: &[String], rows: &[Vec<String>], 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<f32> {
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<FeatureRole> {
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<f32> {
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(
@@ -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");
@@ -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}");
}
}
+372 -100
View File
@@ -41,6 +41,16 @@ fn aggregate_cell_values(values: &[&CellValue], func: &AggFunc) -> Option<CellVa
})
}
/// The member records behind `partial`, in `DataStore` insertion order and
/// without repeats — the coordinate tuples minus `_Measure` (§13).
fn member_records(model: &Model, partial: &CellKey) -> Vec<CellKey> {
let mut seen: IndexSet<CellKey> = 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<f64, String>,
) -> Result<f64, String> {
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<f64> = model
.data
.matching_values(&partial.0)
.into_iter()
.filter_map(|v| v.as_f64())
.collect();
let values: Vec<f64> = 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(partial.clone(), |k, (cat, item)| k.with(cat, item));
eval_at(inner, &at)
})
.collect::<Result<Vec<f64>, String>>()?,
};
match func {
AggFunc::Sum => Ok(values.iter().sum()),
@@ -76,6 +101,105 @@ 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<f64, String> {
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<bool, String> {
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 {
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)]
@@ -519,7 +643,11 @@ impl Model {
func,
inner.as_ref(),
agg_filter.as_ref(),
// 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_) => {
let cv = eval_bool_cached(cond, context, model, target_category, none_cats)?;
if cv {
@@ -583,8 +711,6 @@ impl Model {
context: &CellKey,
depth: u8,
) -> Option<CellValue> {
use crate::formula::Expr;
// Check the WHERE filter first.
if let Some(filter) = &formula.filter {
let matches = context
@@ -596,97 +722,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<f64, String> {
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(),
),
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<bool, String> {
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,
@@ -740,7 +775,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));
@@ -2242,3 +2278,239 @@ 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 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]
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 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]
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);
}
/// 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]
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"
);
}
}
+3
View File
@@ -40,6 +40,9 @@ pub enum Expr {
UnaryMinus(Box<Expr>),
Agg(AggFunc, Box<Expr>, Option<Filter>),
If(Box<Expr>, Box<Expr>, Box<Expr>),
/// `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)]
@@ -47,9 +47,18 @@ 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.
// 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 ~ ")" }
// Aggregates with optional inline WHERE filter inside the parens.
+94 -1
View File
@@ -116,7 +116,7 @@ fn build_unary(pair: Pair<Rule>) -> Expr {
}
fn build_primary(pair: Pair<Rule>) -> 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<Rule>) -> 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,96 @@ 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
);
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 ─────────────────────────────────────────────────────
#[test]
+1 -1
View File
@@ -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`.
+63 -68
View File
@@ -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()
@@ -340,32 +334,15 @@ fn only_raw_measure_features(
> 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(),
"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<String> = 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 +494,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 +626,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 +660,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 +698,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 +916,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 +945,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
+252 -94
View File
@@ -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: `<target> = <nested IFs>`. 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<Feature>);
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: `<target> = SUM(<nested IFs>)`. 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<Formula> {
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<String> {
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<String> {
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<Tree> {
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<f32> = 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());
}
}
}