review: correct six prose claims, four of them false
An adversarial review of this branch found that the prose was worse than the diff — including, twice, in the exact genre the branch was congratulating itself for catching. Recorded rather than quietly rewritten, because that is the shape. Must-fix, all verified before correcting: - `fit.rs`'s comment on the exact check still read "this is the check that enforces the bound in both directions of a drift" — verbatim the sentence gp.md §14 now spends 25 lines refuting. Gotcha 48's third clause exactly: a fact stated twice is changed by grep, not by edit. I refuted the copy in the prose and left the copy in the code. - "false for eight months" was fabricated. `encoded_width` landed 2026-09-19, the sentence 2026-09-20; the repository itself is six months old. A paragraph whose point is that unchecked cost claims are checkable closed on one. - "NumericCategory was the one artefact family with no coverage" is false: it was one of three, and `ValueOfAMeasure` and `AbsentCoordinate` still have none. Corrected in both files that said it, and the remaining gap is named. The decline's stated reason was wrong, which matters most because a decline is the artefact nothing checks. It claimed one-hot is the only encoding of a category whose splits are spellable; `is_expr` is a `primary`, so an ordinal split IS spellable as a weighted indicator sum. Verified by running `parse_formula`, not by re-reading the grammar, and now pinned by `an_ordinal_split_on_a_category_is_spellable_as_an_indicator_sum` so the paragraph cannot go quietly wrong again. e8no stays declined on grounds that survive: a category's items carry insertion order rather than magnitude, and the emitted formula spells one IS per item either way, so the width is not saved. Also weakened two overclaims — "marking it a Label yields the same fit" (false: the feature count moves the RNG draw sequence, gotcha 37b) and "asserted rather than argued" (the dichotomy is argued from spell()'s exhaustive match; the test asserts one instance) — narrowed "the only coordinate predicate is is_expr" to what is true of it, corrected an uncheckable test count, named which two of the three consumers of parse_number_format are actually guarded, and moved an evidence block that had been inserted under the wrong module heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
+17
-6
@@ -677,9 +677,20 @@ 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
|
**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
|
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
|
and fell through to `unwrap_or(0)` — `,.2f` displayed **zero** decimals, in the
|
||||||
TUI, in the browser projection and in CSV export. `",.2f"` is the spelling six
|
TUI, in the browser projection and in CSV export. `",.2f"` and its `,.3f`/`,.4f`
|
||||||
tests in this workspace reach for, and none could see it: they assert the stored
|
siblings are what this workspace's own fixtures reach for, in about ten tests,
|
||||||
*string* round-trips, or — in `export_csv_quotes_commas_per_rfc4180` — that the
|
and none could see it: they assert the stored *string* round-trips or use it as
|
||||||
rendered value `starts_with("1,200")`, which `1,200` and `1,200.00` both satisfy.
|
an opaque value to dirty a widget, or — in `export_csv_quotes_commas_per_rfc4180`
|
||||||
A prefix check over a value whose *suffix* is the thing in question answers a
|
— they assert the rendered value `starts_with("1,200")`, which `1,200` and
|
||||||
neighbouring question, which is why that one is now an exact equality.
|
`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.
|
||||||
|
|||||||
+61
-24
@@ -595,7 +595,9 @@ 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
|
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,
|
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
|
and the second repeated the sentence above instead of checking it. **An argument
|
||||||
that a cost is bounded is checkable, and this one was false for eight months.**
|
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
|
`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
|
over-count, and a column whose cells are *entirely* missing is counted as one
|
||||||
@@ -631,34 +633,69 @@ rather than on what the document says. But the stronger reason to decline is tha
|
|||||||
the alternatives are not merely distasteful — they are **unstorable**, and the
|
the alternatives are not merely distasteful — they are **unstorable**, and the
|
||||||
code already says so.
|
code already says so.
|
||||||
|
|
||||||
The formula language's only coordinate predicate is `is_expr`, spelled
|
A coordinate becomes an `Expr` **only by passing through `IS`**: `is_expr` is
|
||||||
`identifier ~ sp+ ^"IS" ~ sp+ filter_value` in `formula.pest`: a **single-value
|
`identifier ~ sp+ ~ ^"IS" ~ sp+ ~ filter_value`, a single-value equality, and
|
||||||
equality**. There is no `IN`, no ordering over a category's items, and no way to
|
there is no `IN` and no ordering over a category's items. (`where_clause` and
|
||||||
put a coordinate on either side of a `cmp_op` — a coordinate becomes an `Expr`
|
`inline_where` are coordinate predicates too, and also single-value equalities —
|
||||||
only by passing through `IS`. So the set of predicates a fitted tree may state
|
but they *gate* a formula or an aggregate rather than being an `Expr`, which is
|
||||||
about a category is exactly the set of its items, one at a time, and **one-hot is
|
gotcha 37(d)'s distinction and is why `IS` is the one that matters here.) So
|
||||||
the only encoding whose splits are spellable**. The width is not an
|
whatever a tree says about a category, the emitted formula names its items **one
|
||||||
implementation's appetite; it is the size of the vocabulary.
|
at a time**.
|
||||||
|
|
||||||
That disposes of the two representations the ticket invites. **Ordinal** encoding
|
**A first draft of this paragraph said that made one-hot the only spellable
|
||||||
imposes an order the document does not assert, and a split on it is already a
|
encoding. That is false**, and an adversarial review disproved it by running
|
||||||
classified refusal: `spell()` maps `FeatureRole::Numeric` over a *category*
|
`parse_formula` rather than reading the grammar: `is_expr` is a `primary`, so it
|
||||||
column to `Artefact::NumericCategory`, whose reason is that a reference to the
|
is an `Expr` yielding 1/0, and `cmp_op`'s operands are full `expr`s — so an
|
||||||
category's name would resolve to a `_Measure` item of the same name — a different
|
ordinal split *is* spellable, as a weighted indicator sum
|
||||||
column (improvise-8xhy). **Hashing** is strictly worse: a bucket has no name in
|
(`IF(2020*(Year IS |2020|) + 2021*(Year IS |2021|) < 2020.5, …)`). The claim is
|
||||||
the document at all, so it cannot even be refused informatively.
|
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
|
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
|
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
|
**never split** on the numeric category — in which case the column contributed
|
||||||
nothing, and marking it a `Label` yields the same fit, honestly, inside the
|
nothing to the rule, and marking it a `Label` gives a fit over the same remaining
|
||||||
budget, which is exactly what the refusal already advises. Or the tree **did**
|
features, inside the budget, which is exactly what the refusal already advises.
|
||||||
split on it, and `fitted_formula` refused the result as `Artefact::NumericCategory`
|
Or the tree **did** split on it, and `fitted_formula` refused the result as
|
||||||
at storage time. **No fit is lost that was both storable and used the column**,
|
`Artefact::NumericCategory` at storage time. So **no fit is lost that was both
|
||||||
which is asserted rather than argued, by
|
storable and used the column**.
|
||||||
`a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored`. That test had
|
|
||||||
no predecessor: `NumericCategory` was the one artefact family with no coverage,
|
Two things that claim less than a first draft of this paragraph did, both worth
|
||||||
and the decline rests on it.
|
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`
|
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
|
category really is a quantity they want fitted **as a number** has no way to say
|
||||||
|
|||||||
@@ -455,10 +455,6 @@ 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
|
real CSV reader and asserts the field *count*, which is the thing a string
|
||||||
`contains` check cannot see.
|
`contains` check cannot see.
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## `improvise-io/src/import/analyzer.rs`
|
|
||||||
|
|
||||||
**Strengthened by improvise-711n.** The assertion on the value was
|
**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
|
`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
|
the check could not tell `1,200` from `1,200.00` and sat green while
|
||||||
@@ -467,6 +463,11 @@ 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
|
the cross-crate demonstration that the format bug reached a user's exported file
|
||||||
and not only the screen.
|
and not only the screen.
|
||||||
|
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## `improvise-io/src/import/analyzer.rs`
|
||||||
|
|
||||||
### detect_short_year_format
|
### detect_short_year_format
|
||||||
|
|
||||||
Two-digit years are ambiguous with the four-digit format, so `%m/%d/%Y` matches
|
Two-digit years are ambiguous with the four-digit format, so `%m/%d/%Y` matches
|
||||||
|
|||||||
@@ -803,19 +803,24 @@ 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
|
### a_split_on_a_numeric_category_read_as_a_number_cannot_be_stored
|
||||||
|
|
||||||
The evidence improvise-e8no is declined on, and the only coverage
|
The evidence improvise-e8no is declined on, and the only coverage
|
||||||
`Artefact::NumericCategory` has ever had — it was the one artefact family with no
|
`Artefact::NumericCategory` has ever had. It was **one of three** families with
|
||||||
test, which is why the decline needed writing before it could be made.
|
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-e8no observes that forcing a document category to `Forced::Categorical`
|
||||||
(improvise-zv8t) widens the encoding, so a high-cardinality numeric category now
|
(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
|
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
|
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
|
splits on a numeric column over a *category* column produces a formula
|
||||||
`fitted_formula` **refuses**, because a reference to the category's name would
|
`fitted_formula` **refuses**, because a bare reference to the category's name
|
||||||
resolve to a `_Measure` item of the same name (improvise-8xhy). So the
|
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
|
pre-zv8t success was either a fit that never touched the column — for which
|
||||||
marking it a `Label` is exactly equivalent and within budget — or one that could
|
marking it a `Label` gives a fit over the same remaining features, within budget
|
||||||
not be stored.
|
— 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
|
The test asks `preprocess` with `Forced::Numeric`, which is the pre-zv8t reading
|
||||||
reproduced deliberately rather than described, and then asks `Features::of` and
|
reproduced deliberately rather than described, and then asks `Features::of` and
|
||||||
@@ -834,3 +839,27 @@ assertion `left == right` failed: a numeric column over a category column is uns
|
|||||||
left: Measure("Year")
|
left: Measure("Year")
|
||||||
right: Artefact(NumericCategory)
|
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.
|
||||||
|
|||||||
@@ -267,8 +267,8 @@ pub fn fit_measure(
|
|||||||
cannot spell and a search cannot minimise. Correct or clear those cells first"
|
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
|
// The exact width, now that it is known. It bounds the error MESSAGE, not the
|
||||||
// this is the check that enforces the bound in both directions of a drift.
|
// cost: `preprocess` has already run — the preflight is the cost bound (§14).
|
||||||
anyhow::ensure!(
|
anyhow::ensure!(
|
||||||
data.feature_names.len() <= MAX_FEATURES,
|
data.feature_names.len() <= MAX_FEATURES,
|
||||||
"fitting {target:?} encoded {} features, over the {MAX_FEATURES} this host will \
|
"fitting {target:?} encoded {} features, over the {MAX_FEATURES} this host will \
|
||||||
|
|||||||
@@ -812,6 +812,29 @@ 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
|
/// improvise-e8no's decline rests on this: the pre-zv8t encoding of a
|
||||||
/// numeric category yields fits no document can store. Evidence: `gp.md`.
|
/// numeric category yields fits no document can store. Evidence: `gp.md`.
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in New Issue
Block a user