fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #97 from fiddlerwoaroof/wave/format-and-fit

Browse Source 
Wave M: number-format precision (711n), preflight under-count (x1ns), e8no declined
This commit is contained in:
Ed Langley
2026-09-21 10:55:57 -07:00
committed by GitHub
parent 730f39a514 c2bc939608
commit f5df19553e
9 changed files with 774 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/core.md
+53
View File
@@ -641,3 +641,56 @@ targets, which is what the column list shows anyway.
And the list is **not** filtered to visible items. Hidden items are still cycled,
because that is what the TUI has always done, and making the browser differ would
be the divergence rather than the fix. Changing it is a change to both, together.
## The number format — `format.rs` (improvise-711n)
A view's `number_format` is a **printf-style picture, and only that**. A `,`
anywhere turns on thousands grouping; a `.` introduces the precision, which is
the **leading digit run after the last `.`**; anything following those digits —
a conversion letter — is ignored. So `,.0`, `,.2`, `.2`, `,.2f` and `%.2f` all
name what they look like they name.
**Why the trailing `f` is accepted rather than refused.** It round-trips into
documents today and always has. `improv.pest` spells the line
`format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines }` — the
grammar takes the format string as an **opaque line**, so `format: ,.2f` is a
document the grammar accepts; `:set-number-format` stores whatever the user
typed verbatim (`cmd/effect_cmds.rs`), and `format_md` writes `view.number_format`
back out unchanged. Refusing the spelling at load would therefore reject
documents this repo's own grammar admits and its own `.improv` example spells,
to no benefit: by gotcha 8 the format is display-only, so misreading it costs a
wrong pixel and can never cost a wrong number. A refusal here buys nothing and
costs a document.
**Why printf and not Excel.** The two conventions disagree on exactly the
spelling that distinguishes them: `.00` is *precision zero* to printf and *two
places* to Excel, so a parser that tried to honour both would have to guess. One
is picked, and it is the one every spelling in this repository already uses.
Consequences worth stating rather than discovering: `0.00` reads as
`(false, 0)`, and an Excel picture such as `#,##0.00` reads as
`(true, 0)` — grouped, no decimals.
**Nothing is validated, deliberately.** A spelling the rule cannot read names no
precision and formats with none; there is no error channel and no `Result`,
because the reader is display-only and four call sites would have to thread one.
**What the bug was, and why six tests sat on top of it.** The precision was read
as `fmt[i+1..].parse::<u8>()` over the *whole* tail, so `"2f"` failed to parse
and fell through to `unwrap_or(0)` — `,.2f` displayed **zero** decimals, in the
TUI, in the browser projection and in CSV export. `",.2f"` and its `,.3f`/`,.4f`
siblings are what this workspace's own fixtures reach for, in about ten tests,
and none could see it: they assert the stored *string* round-trips or use it as
an opaque value to dirty a widget, or — in `export_csv_quotes_commas_per_rfc4180`
— they assert the rendered value `starts_with("1,200")`, which `1,200` and
`1,200.00` both satisfy. A prefix check over a value whose *suffix* is the thing
in question answers a neighbouring question, which is why that one is now an
exact equality.
**Only two of the three consumers are guarded.** Reverting the fix fails
`format.rs`'s own unit tests and `export_csv_quotes_commas_per_rfc4180`, and
nothing else in the workspace: `ui/project.rs::grid_of` (the browser projection)
and `ui/app.rs` both call `parse_number_format` and neither has a test that
renders a cell and asserts its digits. That gap is named rather than closed —
those files are `improvise-command`, which was another wave's ground when this
landed — and it is the more useful half of this entry, because it says where the
next regression would go unseen.
context/code/gp.md
+167 -7
View File
@@ -561,14 +561,79 @@ the fix (`Label`), because that is what the user actually wants.
side of the cost: a category with a distinct value per record allocates a
rows × rows one-hot *during* preprocessing, so the refusal arrives after the
memory and the O(rows²) work have been spent. It approximates `preprocess`'s
type inference rather than sharing it — a column whose present cells all parse
as a number is one feature (two with a missing indicator), anything else is
counted at its distinct-value count. Copying the predicate is the lesser evil;
type inference rather than sharing it. Copying the predicate is the lesser evil;
the alternative is exporting the inference and calling this a second
implementation of it. **The cost of the copy drifting is bounded in both
directions by the exact check** on `Dataset::feature_names.len()`:
over-counting costs a refusal the exact check would not have made,
under-counting only lets the exact check do its job.
implementation of it.
**The debt that copy carries is that the preflight owes EVERY branch
`preprocess` can take, and it paid two of three.** `preprocess` infers
`Numeric → Date → Categorical`, in that order, and the preflight had no date arm
at all: a column whose cells parse as neither numbers nor a forced category fell
through to the categorical count. So a **measure column holding dates** — one
distinct timestamp in every row — promised **1** and materialised **5**, because
`build_date` emits `epoch_day/year/month/day/dow`, and **7** once any cell
carries a time (improvise-x1ns, second instance, found by ChatGPT Codex on #97).
That is an under-count of four or six on a single column, against a budget of 64.
The two instances are the same defect one inference-branch apart, which is why
the shape is now structural rather than remembered. `inferred_kind` reproduces
`preprocess`'s match arm for arm and returns a `Forced`, and `encoded_width`
matches that **with no wildcard** — so a fourth `Forced` variant is a compile
error here instead of silently taking the categorical arm, which is exactly how
`Date` was missed. The `.missing` flag is added once outside the match, because
every branch owes it on the same terms.
The count each arm owes:
| `preprocess` builds | features |
|---|---|
| `build_numeric` | 1 |
| `build_date` | 5, or 7 if any present cell has a time |
| `build_categorical` | distinct present values, **+1** for the reserved `(missing)` |
plus `col.missing` wherever any cell is missing, and clamped to at least 1.
**The two directions of a drift do not cost the same, and this paragraph said
they did** (improvise-x1ns). It read: *the cost of the copy drifting is bounded
in both directions by the exact check* — over-counting costs a needless refusal,
under-counting "only lets the exact check do its job". That is wrong, and it is
wrong against the argument **two paragraphs above it**: the exact check runs on
`Dataset::feature_names.len()`, which exists only *after* `preprocess` has
materialised the matrix. So it bounds the **error message**, never the **cost** —
which is precisely the thing the preflight was introduced to bound. Over-counting
is cheap; under-counting spends the memory and the O(rows²) work the preflight
exists to prevent, and then reports it politely.
The live instance was the `(missing)` column: the categorical arm counted
`present` values only, so a forced category with N distinct present values and
one record lacking that coordinate promised N and materialised N+1. `has_missing`
was already computed and used in the numeric arm alone — **two arms of one match
disagreeing about what a column costs**, which is improvise-zv8t's forcing-by-name
/ budgeting-by-index defect one level in. That one was repaired by making the
ordering a data dependency and so a compile error; this one has no such shape
available, so the guard is a test that asks `preprocess` what it actually built
rather than asserting a number
(`the_preflight_counts_the_missing_category_a_forced_column_materialises`).
The previous wave's review saw a version of this and reasoned it away as *"read,
not injected"* and *"bounded by the exact check"*. Both halves of that are the
failure mode this file exists to catch: the first declined to run the experiment,
and the second repeated the sentence above instead of checking it. **An argument
that a cost is bounded is checkable, and this one was checkable the day it was
written**: `encoded_width` landed 2026-09-19 and the sentence 2026-09-20, so the
code it described was one commit old and one file away.
`encoded_width` remains **advisory** in the direction that is safe: it may
over-count, and a column whose cells are *entirely* missing is counted as one
where `preprocess` skips it altogether. It must never under-count.
**That sentence was written as a proposed `kb.pl` invariant while the tree held a
counter-example**, in the same change that fixed the categorical one — the date
branch was missing the whole time and nothing in the hand-back noticed. An
invariant proposed from the defect you just fixed generalises the *fix*, not the
code; the thing that would have caught it is the question "what else does
`preprocess` do that I have not mirrored", which is a question about the other
function and not about the bug.
`MAX_RECORDS = 20_000` bounds the *work*, which the width bound does not: a
one-feature table with a million records still runs cross-validation, the final
@@ -586,6 +651,101 @@ sequence, five-fold CV included. There is deliberately **no timing test**: a
wall-clock assertion is flaky on shared CI and would fail for a reason that is
not this code's. Re-measure after changing the numbers.
### Forcing a numeric category is a *representability* choice, not a budget one
improvise-zv8t made `fit_measure` force every document category to
`Forced::Categorical`, and improvise-e8no records the cost: a hundred-item
numeric category one-hots to a hundred columns, past `MAX_FEATURES`, and is
refused where inference used to read it as one numeric column and the fit
succeeded. **improvise-e8no is declined, and the refusal is correct.**
The ticket rules out making the forcing conditional on cardinality, and rightly:
that is inference by another name, and it makes the *encoding* depend on a budget
rather than on what the document says. But the stronger reason to decline is that
the alternatives are not merely distasteful — they are **unstorable**, and the
code already says so.
A coordinate becomes an `Expr` **only by passing through `IS`**: `is_expr` is
`identifier ~ sp+ ~ ^"IS" ~ sp+ ~ filter_value`, a single-value equality, and
there is no `IN` and no ordering over a category's items. (`where_clause` and
`inline_where` are coordinate predicates too, and also single-value equalities —
but they *gate* a formula or an aggregate rather than being an `Expr`, which is
gotcha 37(d)'s distinction and is why `IS` is the one that matters here.) So
whatever a tree says about a category, the emitted formula names its items **one
at a time**.
**A first draft of this paragraph said that made one-hot the only spellable
encoding. That is false**, and an adversarial review disproved it by running
`parse_formula` rather than reading the grammar: `is_expr` is a `primary`, so it
is an `Expr` yielding 1/0, and `cmp_op`'s operands are full `expr`s — so an
ordinal split *is* spellable, as a weighted indicator sum
(`IF(2020*(Year IS |2020|) + 2021*(Year IS |2021|) < 2020.5, …)`). The claim is
recorded here rather than quietly deleted because it is the shape this file keeps
catching: a grammar argument made by reading the rule that looked relevant
instead of running the parser.
So **ordinal is not ruled out by spellability, and is ruled out anyway**, for two
reasons that survive. It invents a **metric the document does not assert** — a
category's items carry insertion order, not magnitude, so `Year < 3.5` means "the
first four items as they happen to be ordered" and stops meaning that when
someone reorders them. And the width it saves does not stay saved: the *encoding*
would be one feature, but the *formula* still spells one `IS` per item, so it
trades a `MAX_FEATURES` problem for a formula-size problem while adding a false
order. What the current emitter does with it is narrower and is not the argument:
`spell()` maps `FeatureRole::Numeric` over a *category* column to
`Artefact::NumericCategory`, because a bare reference to the category's name
would resolve to a `_Measure` item of the same name (improvise-8xhy) — a fact
about the bare-ref spelling, not about ordinals in general. **Hashing** is ruled
out outright: a bucket has no name in the document, so it cannot even be refused
informatively.
And this is what makes the decline clean rather than resigned. The "fit that
previously succeeded" was one of two things, never a third. Either the tree
**never split** on the numeric category — in which case the column contributed
nothing to the rule, and marking it a `Label` gives a fit over the same remaining
features, inside the budget, which is exactly what the refusal already advises.
Or the tree **did** split on it, and `fitted_formula` refused the result as
`Artefact::NumericCategory` at storage time. So **no fit is lost that was both
storable and used the column**.
Two things that claim less than a first draft of this paragraph did, both worth
keeping. Marking the category a `Label` does **not** yield *the same fit*: the
feature count changes, so the RNG draw sequence in `random_tree`/`sample_thresh`
changes, so the champion for a given seed changes — gotcha 37(b)'s own rule,
applied to this paragraph. "The same fit" is a checkable cost claim and it is
false; what is true is a fit over the same remaining features. And the dichotomy
above is **argued**, from `spell()`'s exhaustive match over `FeatureRole`, not
asserted: what
`a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored` asserts is one
instance of its second branch, over one three-row fixture. That is the most a
test can do here, and saying so is the difference between evidence and a
flourish.
The test still had no predecessor. `NumericCategory` had no coverage before it —
**and it was one of three, not the only one**: grepping each `Artefact` variant
and each variant's `reason()` text, `MissingFlag` and `DatePart` are covered by
`the_missing_flag_and_the_date_sub_features_are_refused_too`, and
**`ValueOfAMeasure` and `AbsentCoordinate` have no test at all**, before this
change or after it. Two families remain uncovered; that is filed, not fixed
here.
What the decline does cost, stated rather than waved at: a user whose `Year`
category really is a quantity they want fitted **as a number** has no way to say
so through `fit_measure`. The document says `Year` is a category of items, and
that is what is honoured. The remedy is a document change — `Year` belongs in
`_Measure` if it is a measurement — and `crate::dataset` is `pub` and takes the
caller's `Overrides`, so a host that wants to offer the choice can (gotcha 37d).
Offering it *inside* `fit_measure` would be a new user-facing control, and
nobody has asked for one.
The refusal's wording was narrower than the rule and is now aligned in both
places that carry it: it said *a category with a distinct value per record
becomes one column per record*, which diagnoses the runaway shape and not
improvise-e8no's — a category of modest cardinality that is simply wider than
the budget. Both now say a category becomes one column per distinct item,
**whether its items read as numbers or not**, since that clause is the whole of
what changed under zv8t.
### `Quality` is an enum, not an `Option`
`cv_regress` pools no pairs below two folds, and scoring an empty pool gives an
context/test-evidence/core.md
+41
View File
@@ -611,3 +611,44 @@ narrow as it claims.
Fault injected by deleting either emission: the corresponding test fails with
`Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and
`the_two_front_ends_draw_the_same_column_header` fails with it.
### a_trailing_conversion_letter_does_not_erase_the_precision
improvise-711n. `parse_number_format` read the precision as
`fmt[i+1..].parse::<u8>().ok().unwrap_or(0)` over everything after the last `.`,
so any trailing conversion letter made the whole suffix unparseable and the
format fell back to **zero decimals, silently**. `format: ,.2f` — the spelling
`context/repo-map.md`'s own `.improv` example carries, and the printf/Python
convention a user reaches for first — displayed integers everywhere.
Fails before the fix with `left: (true, 0)`, `right: (true, 2)`.
The test also pins the reading against its nearest *wrong* neighbour. Two
implementations both fix `,.2f`: take the leading digit run, or trim the
trailing non-digits off the tail. They differ only on a tail with digits on both
sides of a letter, so `".2f3"` is in the assertion. Injecting
`s.trim_end_matches(|c: char| !c.is_ascii_digit())` in place of `leading_digits`
fails the test at `left: (false, 0)`, `right: (false, 2)` — so the test sees the
rule, not merely the bug.
### a_zero_padded_picture_string_is_read_as_printf_not_as_excel
The companion to the above, and the reason the fix is not "be generous". A
forgiving reader would have to answer `.00`, where printf says precision zero
and an Excel picture says two places. This pins the printf reading — `"0.00"` is
`(false, 0)` — so that "accept the trailing `f`" cannot quietly grow into
"accept anything and guess". It passes before the fix as well as after: it is
there to stop the *next* change, not to catch this one.
### a_precision_too_large_to_name_reads_as_none
`".300"` does not fit `u8`, so it reads as no precision rather than as 300
decimal places. Unchanged by improvise-711n and pinned so the fallback is a
stated rule rather than an artefact of the integer type.
### a_format_naming_no_precision_at_all_reads_as_zero
`",f"` has no `.`, and `".f"` has a `.` with no digits after it. Both are zero
decimals. Pinned because `leading_digits` returns `""` for the second, and an
empty string reaching `parse::<u8>()` is the one path in the function with no
digits to carry the answer.
context/test-evidence/formula-and-io.md
+9
View File
@@ -455,6 +455,15 @@ into two fields, **corrupting every row**. The test reads the file back with a
real CSV reader and asserts the field *count*, which is the thing a string
`contains` check cannot see.
**Strengthened by improvise-711n.** The assertion on the value was
`records[1][1].starts_with("1,200")`, and the fixture's format is `,.2f` — so
the check could not tell `1,200` from `1,200.00` and sat green while
`parse_number_format` threw the `.2` away. It is now an exact equality. Reverting
the `format.rs` fix fails it with `left: "1,200"`, `right: "1,200.00"`, which is
the cross-crate demonstration that the format bug reached a user's exported file
and not only the screen.
---
## `improvise-io/src/import/analyzer.rs`
context/test-evidence/gp.md
+162
View File
@@ -749,3 +749,165 @@ fixture really names one column twice, that the preflight's 41 really disagrees
with what forcing would build, and that `as_categories` itself refuses. A test
asserting only the error message would pass on either ordering, since the
message is identical -- the whole defect was in work done, not in the result.
### the_preflight_counts_the_missing_category_a_forced_column_materialises
improvise-x1ns, found by ChatGPT Codex reviewing PR #90 and live on `main` at
`ce895ff`. `encoded_width` computed `has_missing` and used it in the **numeric**
arm only:
```
true => 1 + usize::from(has_missing),
false => present.iter().collect::<HashSet<_>>().len().max(1),
```
`build_categorical` maps every missing cell to `MISSING_CAT` and includes it in
`distinct(&mapped)`, so a categorical column with N distinct *present* values and
at least one absent cell materialises **N+1** one-hots. The preflight promised N.
It under-reports by exactly one per such column, and it under-reports in the
direction that costs: the preflight runs *before* `preprocess`, the exact check
on `feature_names.len()` runs *after*, so the exact check bounds the error
message and not the allocation — which is the whole reason the preflight exists.
The previous wave's review recorded this shape as "read, not injected" and
"bounded by the exact check"; the second half is false and is now corrected in
`context/code/gp.md` §14.
**Fails before the fix** with
```
the preflight under-reported: it promised 5 features and `preprocess`
materialised 6 — ["Year=(missing)", "Year=2020", "Year=2021", "Year=2022",
"Year=2023", "Cost"]
```
The fixture is Codex's suggested case, and the reason it is the interesting one:
a **forced numeric category with a missing coordinate**. Forcing is what sends a
numeric column down the categorical branch, so only a *forced* column reaches the
arm that was wrong, and only a missing cell makes the arms disagree.
The test does not assert a number. It asks `preprocess` what it actually built
and requires `encoded_width >= data.feature_names.len()`, so it cannot pass by
agreeing with a stale constant, and it keeps holding if the encoding widens for
some other reason. Three preconditions are asserted first, because each would
make it vacuous: that exactly one record lacks the coordinate, that the present
cells really are numeric (otherwise the column would be categorical by inference
and forcing would prove nothing), and that a `Year=(missing)` one-hot is really
among the materialised features.
The `.max(1)` is deliberately outside the addition. A column whose cells are
*entirely* missing has no present values, and `preprocess` skips it with a
warning rather than encoding it; keeping the clamp on the sum leaves that case
counted as 1 exactly as before, so the fix moves only the case it names.
### a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored
The evidence improvise-e8no is declined on, and the only coverage
`Artefact::NumericCategory` has ever had. It was **one of three** families with
no test, not the only one: `MissingFlag` and `DatePart` are covered by
`the_missing_flag_and_the_date_sub_features_are_refused_too`, and
`ValueOfAMeasure` and `AbsentCoordinate` still have none — filed, not fixed
here.
improvise-e8no observes that forcing a document category to `Forced::Categorical`
(improvise-zv8t) widens the encoding, so a high-cardinality numeric category now
exceeds `MAX_FEATURES` where inference used to read it as one numeric column and
the fit succeeded. The decline turns on what that success was worth: a tree that
splits on a numeric column over a *category* column produces a formula
`fitted_formula` **refuses**, because a bare reference to the category's name
would resolve to a `_Measure` item of the same name (improvise-8xhy). So the
pre-zv8t success was either a fit that never touched the column — for which
marking it a `Label` gives a fit over the same remaining features, within budget
— or one that could not be stored. Note what this does *not* establish: an
ordinal encoding is spellable as a weighted indicator sum, so it is declined on
semantics and formula size rather than on spellability (`context/code/gp.md`).
The test asks `preprocess` with `Forced::Numeric`, which is the pre-zv8t reading
reproduced deliberately rather than described, and then asks `Features::of` and
`fitted_formula`. It does **not** use the `artefacts()` helper: that helper
fabricates a `Spelling`, which is the shape gotcha 37(d) warns about — it would
assert that the emitter refuses what the test told it to refuse. Here the
document and the encoder decide.
Injected fault: change `spell()`'s `FeatureRole::Numeric { .. }` arm from
`Spelling::Artefact(Artefact::NumericCategory)` to `Spelling::Measure(column.clone())`
— the "fix" someone would reach for on reading improvise-e8no, and the one that
silently reopens improvise-8xhy. Fails with
```
assertion `left == right` failed: a numeric column over a category column is unspellable
left: Measure("Year")
right: Artefact(NumericCategory)
```
### an_ordinal_split_on_a_category_is_spellable_as_an_indicator_sum
Not a bug fix: this pins a **grammar fact a prose argument rests on**, after that
argument got it wrong. improvise-e8no's decline was first written claiming that
one-hot is the only encoding of a category whose splits the formula language can
spell, and therefore that ordinal encoding was ruled out by spellability. An
adversarial review disproved it by running `parse_formula` instead of reading the
rule that looked relevant: `is_expr` is a `primary`, so it is an `Expr` yielding
1/0, and `cmp_op`'s operands are full `expr`s — so
`IF(2020*(Year IS |2020|) + 2021*(Year IS |2021|) < 2020.5, 1, 2)` parses.
The decline survives on other grounds (a category's items carry insertion order,
not magnitude; and the emitted formula still spells one `IS` per item, so the
width is not saved) — but the *stated reason* was false, and a decline is the one
artefact nothing checks. This test is the check: if the grammar ever narrows so
that an indicator sum stops parsing, the paragraph that says it does becomes
discoverable rather than quietly wrong.
The negative arm is the incidental fact that explains the emitter: `Year IS 2020`
does **not** parse, because `filter_value` is `string | pipe_quoted | bare_ident`
and a bare number is none of them. That is why `formula_text` pipe-quotes a
numeric item name through `quote_name`, and asserting it here keeps the positive
arm from passing for the trivial reason that everything parses.
### the_preflight_counts_the_parts_a_date_measure_expands_into
improvise-x1ns, **second instance** — found by ChatGPT Codex on PR #97, live at
`f128087`, i.e. in the very change that fixed the first one.
`preprocess` infers `Numeric → Date → Categorical` (`data.rs:406`), and
`encoded_width` had **no date arm**: numeric-or-categorical only. A date string
does not parse as `f64`, so a measure column holding dates took the categorical
arm and was counted by its distinct strings. `build_date` emits
`epoch_day/year/month/day/dow` — seven parts once any cell has a time
(`data.rs:326`) — so a column with **one** distinct timestamp promised 1 and
materialised 5.
Fails before the fix, on the plain-date case:
```
a plain date: the preflight under-reported: it promised 4 features and
`preprocess` materialised 8 — ["Region=East", "Region=North", "Region=South",
"When.epoch_day", "When.year", "When.month", "When.day", "When.dow"]
```
**The fixture carries a time case because of the rival fix, not because of the
bug.** Two implementations repair the plain-date failure: "a date column is 5
features", and "5, or 7 when any present cell carries a time". They differ only
on a column containing a time, so the second sub-case is what separates them.
Injected, `date_parts` hard-coded to 5:
```
a date with a time: the preflight under-reported: it promised 8 features and
`preprocess` materialised 10 — [… "When.hour", "When.minute"]
```
Note the plain-date sub-case **passes** under that injection — so without the
time fixture this test would have been satisfied by either reading, and would
have pinned the bug without pinning the rule.
Both arms compare against `data.feature_names.len()` rather than against a width
computed a second way, so the test cannot pass by agreeing with a stale constant
or by reimplementing the thing under test. Two preconditions guard vacuity:
every `When` cell really is the same string (so the categorical count really
would be 1), and the fixture's timestamp really does or does not carry a time,
asked of `parse_datetime` rather than of the literal's shape.
The column must be a **measure** — `as_categories` forces every document
category to `Forced::Categorical`, so a category of dates never reaches the date
branch at all. That is gotcha 37(d)'s "the date decline is narrower than it
read": this is the genuine date *measure*.
crates/improvise-core/src/format.rs
+52 -6
View File
@@ -10,14 +10,19 @@ pub fn format_value(v: Option<&CellValue>, comma: bool, decimals: u8) -> String
}
}
/// Parse a number format string like ",.0" into (use_commas, decimal_places).
/// The digits a precision is named by: everything up to the first non-digit.
fn leading_digits(s: &str) -> &str {
&s[..s.find(|c: char| !c.is_ascii_digit()).unwrap_or(s.len())]
}
/// Parse a printf-style number format like ",.2f" into (use_commas, decimals).
/// Which spellings are legal: `context/code/core.md`, "The number format".
pub fn parse_number_format(fmt: &str) -> (bool, u8) {
let comma = fmt.contains(',');
let decimals = fmt
.rfind('.')
.and_then(|i| fmt[i + 1..].parse::<u8>().ok())
.unwrap_or(0);
(comma, decimals)
let precision = fmt
.rsplit_once('.')
.map_or("", |(_, tail)| leading_digits(tail));
(comma, precision.parse::<u8>().unwrap_or(0))
}
/// Round half away from zero (the "normal" rounding people expect).
@@ -100,6 +105,47 @@ mod tests {
assert_eq!(parse_number_format(",.1.3"), (true, 3));
}
/// Bug improvise-711n: a trailing conversion letter made the whole suffix
/// unparseable. Evidence: `context/test-evidence/core.md`.
#[test]
fn a_trailing_conversion_letter_does_not_erase_the_precision() {
assert_eq!(parse_number_format(",.2f"), (true, 2));
assert_eq!(parse_number_format(".2f"), (false, 2));
assert_eq!(parse_number_format("%.2f"), (false, 2));
// The precision is the LEADING digit run, not the tail with its
// non-digits trimmed off — these two readings differ here.
assert_eq!(parse_number_format(".2f3"), (false, 2));
}
/// One convention, not two: `.00` is printf's "precision zero", never
/// Excel's "two places". Evidence: `context/test-evidence/core.md`.
#[test]
fn a_zero_padded_picture_string_is_read_as_printf_not_as_excel() {
assert_eq!(parse_number_format("0.00"), (false, 0));
assert_eq!(parse_number_format(",.0f"), (true, 0));
}
#[test]
fn a_precision_too_large_to_name_reads_as_none() {
assert_eq!(parse_number_format(".300"), (false, 0));
assert_eq!(parse_number_format(",.999f"), (true, 0));
}
/// `format:` is `rest_of_line` in the grammar, so the format string can hold
/// any text at all — including a multibyte char right after the dot.
#[test]
fn a_multibyte_character_after_the_dot_does_not_split_a_char_boundary() {
assert_eq!(parse_number_format(",.2\u{20ac}"), (true, 2));
assert_eq!(parse_number_format(".\u{20ac}"), (false, 0));
assert_eq!(parse_number_format(".\u{0662}"), (false, 0));
}
#[test]
fn a_format_naming_no_precision_at_all_reads_as_zero() {
assert_eq!(parse_number_format(",f"), (true, 0));
assert_eq!(parse_number_format(".f"), (false, 0));
}
// ── format_f64 basic ───────────────────────────────────────────────
#[test]
crates/improvise-gp/src/fit.rs
+223 -18
View File
@@ -1,7 +1,7 @@
//! One fit, end to end: a [`View`]'s records in, an improvise [`Formula`] and the
//! evidence for believing it out. It never prints — `context/code/gp.md` §14.
use gp_tree::data::{Forced, Overrides, Target, preprocess};
use gp_tree::data::{Forced, Overrides, Target, parse_datetime, preprocess};
use gp_tree::fit::{
FitConfig, RegressionQuality, cv_regress, denormalize, fit, importance_ranking, mse,
permutation_importance, regression_quality, standardize,
@@ -13,6 +13,31 @@ use improvise_formula::Formula;
use crate::dataset::{FeatureTable, feature_table};
/// Which branch `preprocess` would take for this column. It mirrors that function's
/// own `match`, arm for arm — the preflight owes every branch, `context/code/gp.md` §14.
fn inferred_kind(present: &[&str], is_document_category: bool) -> Forced {
match present.is_empty() {
// `preprocess` SKIPS an entirely-missing column. Counting it as one
// categorical over-counts by one, which is the safe direction.
true => Forced::Categorical,
false if is_document_category => Forced::Categorical,
false if present.iter().all(|c| c.parse::<f64>().is_ok()) => Forced::Numeric,
false if present.iter().all(|c| parse_datetime(c).is_some()) => Forced::Date,
false => Forced::Categorical,
}
}
/// `build_date` emits five parts, or seven once any cell carries a time.
fn date_parts(present: &[&str]) -> usize {
match present
.iter()
.any(|c| parse_datetime(c).is_some_and(|d| d.has_time))
{
true => 7,
false => 5,
}
}
/// A cheap upper bound on how wide `preprocess` would expand this table, computed
/// without expanding it. **Advisory, not authoritative** — `context/code/gp.md` §14.
fn encoded_width(table: &FeatureTable, category_columns: usize) -> usize {
@@ -29,20 +54,21 @@ fn encoded_width(table: &FeatureTable, category_columns: usize) -> usize {
.copied()
.filter(|c| !gp_tree::data::is_missing(c))
.collect();
let has_missing = present.len() < cells.len();
// A document category is forced categorical below, so the preflight
// counts it by cardinality too. Where the two still differ: gp.md §14.
let numeric = col >= category_columns
&& !present.is_empty()
&& present.iter().all(|c| c.parse::<f64>().is_ok());
match numeric {
// A numeric column is one feature, plus the `.missing` flag.
true => 1 + usize::from(has_missing),
// Everything else: one column per distinct value.
false => present
// Every branch adds the `.missing` flag on the same terms, so it is
// added once here rather than in each arm.
let missing = usize::from(present.len() < cells.len());
// No wildcard: a new `Forced` variant must be counted here rather
// than silently falling into the categorical arm.
match inferred_kind(&present, col < category_columns) {
Forced::Numeric => 1 + missing,
Forced::Date => date_parts(&present) + missing,
// One column per distinct value, plus the one `build_categorical`
// reserves for `(missing)` — gp.md §14.
Forced::Categorical => (present
.iter()
.collect::<std::collections::HashSet<_>>()
.len()
+ missing)
.max(1),
}
})
@@ -239,8 +265,9 @@ pub fn fit_measure(
anyhow::ensure!(
width <= MAX_FEATURES,
"fitting {target:?} would encode {width} features from {} records, which is too wide \
to search on the UI thread. A category with a distinct value per record becomes one \
column per record — mark such a category as a Label so it is not a feature",
to search on the UI thread. A category becomes one column per distinct item, whether \
its items read as numbers or not — mark such a category as a Label so it is not a \
feature",
table.rows.len(),
);
@@ -264,13 +291,13 @@ pub fn fit_measure(
cannot spell and a search cannot minimise. Correct or clear those cells first"
);
// The exact width, now that it is known. The preflight above is advisory, and
// this is the check that enforces the bound in both directions of a drift.
// The exact width, now that it is known. It bounds the error MESSAGE, not the
// cost: `preprocess` has already run — the preflight is the cost bound (§14).
anyhow::ensure!(
data.feature_names.len() <= MAX_FEATURES,
"fitting {target:?} encoded {} features, over the {MAX_FEATURES} this host will \
search. A category with many distinct values becomes one column per value — \
mark such a category as a Label so it is not a feature",
search. A category becomes one column per distinct item, whether its items read as \
numbers or not — mark such a category as a Label so it is not a feature",
data.feature_names.len(),
);
// Empty is a **panic**, not a poor fit: zero features makes `random_tree` draw
@@ -714,6 +741,184 @@ mod tests {
(model, view)
}
/// Bug improvise-x1ns (2nd): `preprocess` infers Date BEFORE Categorical and
/// `build_date` emits 5 or 7 parts. Evidence: `context/test-evidence/gp.md`.
#[test]
fn the_preflight_counts_the_parts_a_date_measure_expands_into() {
for (label, stamp, parts) in [
("a plain date", "2024-01-15", 5),
("a date with a time", "2024-01-15T09:30:00", 7),
] {
let (model, view) = a_date_valued_measure(stamp);
let table = feature_table(&model, &view, "Score").expect("the table builds");
let category_columns = model.regular_category_names().len();
// The fixture must really be the shape the bug needs: ONE distinct
// date, which the categorical arm would count as a single column.
let when: Vec<&str> = table
.rows
.iter()
.map(|r| r[category_columns].as_str())
.collect();
assert!(
when.iter().all(|c| *c == stamp),
"{label}: every When cell must be the same date: {when:?}"
);
assert!(
gp_tree::data::parse_datetime(stamp).is_some_and(|d| d.has_time == (parts == 7)),
"{label}: the fixture must really carry a time iff {parts} parts are expected"
);
let width = super::encoded_width(&table, category_columns);
let overrides =
super::as_categories(&table.header, category_columns).expect("the header is clean");
let data = preprocess(&table.header, &table.rows, &overrides);
assert_eq!(
data.feature_names
.iter()
.filter(|n| n.starts_with("When."))
.count(),
parts,
"{label}: the fixture must really expand into {parts} date parts — {:?}",
data.feature_names
);
assert!(
width >= data.feature_names.len(),
"{label}: the preflight under-reported: it promised {width} features \
and `preprocess` materialised {} — {:?}",
data.feature_names.len(),
data.feature_names,
);
}
}
/// One regular category and a `When` MEASURE whose every cell is the same
/// timestamp — the low-cardinality date column a distinct-count misses.
fn a_date_valued_measure(stamp: &str) -> (Model, View) {
let mut model = Model::new("sales");
model.add_category(REGION).expect("under the limit");
let measure = model.category_mut(MEASURE).expect("_Measure always exists");
for item in ["When", "Score"] {
measure.add_item(item);
}
for (i, region) in ["North", "South", "East"].iter().enumerate() {
model
.category_mut(REGION)
.expect("just added")
.add_item(*region);
model.set_cell(
CellKey::new(vec![
(REGION.to_string(), (*region).to_string()),
(MEASURE.to_string(), "When".to_string()),
]),
CellValue::Text(stamp.to_string()),
);
model.set_cell(
CellKey::new(vec![
(REGION.to_string(), (*region).to_string()),
(MEASURE.to_string(), "Score".to_string()),
]),
CellValue::Number(i as f64),
);
}
let mut view = View::new("Default");
for cat in model.category_names() {
view.on_category_added(cat);
}
view.set_axis(REGION, Axis::Row);
view.set_axis(MEASURE, Axis::Page);
(model, view)
}
/// Bug improvise-x1ns: the preflight counted a forced categorical by its
/// PRESENT values only. Evidence: `context/test-evidence/gp.md`.
#[test]
fn the_preflight_counts_the_missing_category_a_forced_column_materialises() {
let (model, view) = a_numeric_category_with_a_missing_coordinate();
let table = feature_table(&model, &view, "Score").expect("the table builds");
let category_columns = model.regular_category_names().len();
// The fixture must really carry the shape the bug needs: a forced
// category whose cells are numeric, one of them absent.
let year: Vec<&str> = table.rows.iter().map(|r| r[0].as_str()).collect();
assert_eq!(
year.iter().filter(|c| gp_tree::data::is_missing(c)).count(),
1,
"exactly one record must lack the Year coordinate: {year:?}"
);
assert!(
year.iter()
.filter(|c| !gp_tree::data::is_missing(c))
.all(|c| c.parse::<f64>().is_ok()),
"the present Year cells must all be numeric: {year:?}"
);
let width = super::encoded_width(&table, category_columns);
let overrides =
super::as_categories(&table.header, category_columns).expect("the header is clean");
let data = preprocess(&table.header, &table.rows, &overrides);
// `build_categorical` maps the absent cell to the reserved `(missing)`
// category, so it is one more one-hot column than the present values.
assert!(
data.feature_names
.iter()
.any(|n| n == &format!("Year={}", gp_tree::data::MISSING_CAT)),
"the forced column must really emit a (missing) one-hot: {:?}",
data.feature_names
);
assert!(
width >= data.feature_names.len(),
"the preflight under-reported: it promised {width} features and \
`preprocess` materialised {} — {:?}",
data.feature_names.len(),
data.feature_names,
);
}
/// A numeric `Year` category with one record that has no `Year` coordinate, so
/// forcing sends a numeric column down the categorical branch with a gap in it.
fn a_numeric_category_with_a_missing_coordinate() -> (Model, View) {
let mut model = Model::new("sales");
model.add_category("Year").expect("under the limit");
let measure = model.category_mut(MEASURE).expect("_Measure always exists");
for item in ["Cost", "Score"] {
measure.add_item(item);
}
for i in 0..4u32 {
let year = format!("{}", 2020 + i);
model
.category_mut("Year")
.expect("just added")
.add_item(year.clone());
for (m, v) in [("Cost", f64::from(i) * 10.0), ("Score", f64::from(i))] {
model.set_cell(
CellKey::new(vec![
("Year".to_string(), year.clone()),
(MEASURE.to_string(), m.to_string()),
]),
CellValue::Number(v),
);
}
}
// The record with no `Year` coordinate at all — an empty cell, which
// `is_missing` reads as absent rather than as a value.
for (m, v) in [("Cost", 99.0), ("Score", 9.0)] {
model.set_cell(
CellKey::new(vec![(MEASURE.to_string(), m.to_string())]),
CellValue::Number(v),
);
}
let mut view = View::new("Default");
for cat in model.category_names() {
view.on_category_added(cat);
}
view.set_axis("Year", Axis::Row);
view.set_axis(MEASURE, Axis::Page);
(model, view)
}
/// The review finding on improvise-zv8t: forcing is by NAME and the preflight
/// counts by INDEX. Evidence: `context/test-evidence/gp.md`.
#[test]
crates/improvise-gp/src/lib.rs
+64
View File
@@ -812,6 +812,70 @@ mod tests {
}
}
/// improvise-e8no declines ordinal encoding on semantics, NOT on
/// spellability — a first draft said otherwise. `context/code/gp.md`.
#[test]
fn an_ordinal_split_on_a_category_is_spellable_as_an_indicator_sum() {
// `is_expr` is a `primary`, so it is an `Expr` yielding 1/0, and
// `cmp_op`'s operands are full `expr`s.
for text in [
"Score = (Year IS |2020|) + (Year IS |2021|)",
"Score = IF(2020*(Year IS |2020|) + 2021*(Year IS |2021|) < 2020.5, 1, 2)",
] {
assert!(
parse_formula(text, MEASURE).is_ok(),
"the grammar the decline reasons about must really accept {text:?}"
);
}
// And the reason a numeric item name must be pipe-quoted: a bare
// number is not a `filter_value`.
assert!(
parse_formula("Score = (Year IS 2020)", MEASURE).is_err(),
"a bare number as a filter_value must stay a parse error"
);
}
/// improvise-e8no's decline rests on this: the pre-zv8t encoding of a
/// numeric category yields fits no document can store. Evidence: `gp.md`.
#[test]
fn a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored() {
use gp_tree::data::{Forced, Overrides, preprocess};
// The pre-improvise-zv8t reading: a category of numbers inferred as a
// numeric column. Asked of `preprocess`, never a fabricated spelling.
let header = vec!["Year".to_string(), TARGET.to_string()];
let rows: Vec<Vec<String>> = [("2020", "1"), ("2021", "2"), ("2022", "3")]
.iter()
.map(|(y, t)| vec![(*y).to_string(), (*t).to_string()])
.collect();
let overrides = Overrides {
force: [("Year".to_string(), Forced::Numeric)]
.into_iter()
.collect(),
..Overrides::default()
};
let data = preprocess(&header, &rows, &overrides);
let features = crate::Features::of(&data, &["Year".to_string()]);
assert_eq!(
data.feature_names,
vec!["Year".to_string()],
"the fixture must really encode Year as ONE numeric column"
);
assert_eq!(
features.0[0].spelling,
crate::Spelling::Artefact(Artefact::NumericCategory),
"a numeric column over a category column is unspellable"
);
let tree = split(0, 2020.5, leaf(1.0), leaf(2.0));
let err = fitted_formula(&tree, &features, TARGET, MEASURE, &document())
.expect_err("a split on a numeric category is not storable")
.to_string();
assert!(err.contains("Year"), "{err}");
assert!(err.contains("cannot spell"), "{err}");
}
/// The property the *lying* test could not state: if `fitted_formula` says
/// `Ok`, no cell of the target is a `ref:` error. See the evidence file.
#[test]
crates/improvise-io/src/persistence/export.rs
+3 -5
View File
@@ -139,11 +139,9 @@ mod tests {
assert_eq!(r.len(), 2, "comma split a field: {r:?}");
}
assert_eq!(&records[1][0], "Smith, Jr.");
assert!(
records[1][1].starts_with("1,200"),
"expected comma-grouped value, got: {:?}",
&records[1][1]
);
// Exact, not `starts_with`: the fixture asks for `.2f`, and a prefix
// check cannot tell two decimals from none (improvise-711n).
assert_eq!(&records[1][1], "1,200.00");
}
/// Bug improvise-iot6: `render_csv` built its layout with `GridLayout::new`,