08a1b108d9674c4dc9c00e3b18bc87aceecfb396
40 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Edward Langley
|
17a582ae4d |
test(gp): pin the FitOutcome::warnings order the doc comment and gp.md both state (improvise-ipqu)
`fit_measure` concatenates three warning tiers with two `splice(0..0, ..)` calls. The resulting order — the budget's notices, then the feature table's, then preprocess's — is stated in the field's doc comment and in context/code/gp.md section 14, joined into a user-facing status line, and was checked nowhere: swapping the two statements left all 74 improvise-gp tests green. No pre-existing fixture produced more than one tier, which is why nothing caught it. `all_three_warning_tiers_at_once` adds two things to the wide-category fixture: a `_Measure` item with no cell anywhere (a header column entirely missing in every row, which preprocess warns about) and a `Cost` cell at an unused coordinate (one visible record with no `Score`, which feature_table skips). Neither disturbs the budget arithmetic. The assertion is on the three positions, not on the concatenation containing them, and the helper panics on an absent tier so a tier that stops speaking fails loudly rather than making the ordering trivially true. Two rivals, each moving a different boundary, each caught only by this test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
bbfdf992cd |
fix(gp-tree): a share of a large importance overflowed f32 twice over (improvise-k844)
`importance_ranking` computed `100.0 * imp[j] / total` in f32, which associates
left: a lone importance above 3.4e36 overflowed on the multiply, before the
divide that would have brought it back into range, and the feature carrying all
of the importance was reported as `inf%`.
Dividing first — the fix the ticket's title proposes — is not enough, and that
is the half worth recording. `total` overflows too: several large importances
sum past f32::MAX, and the reassociated version then divides by `inf` and
reports a serene 0% for every feature, where the original reported NaN%. That is
improvise-pv3g's `cv_regress` shape again: an overflow that corrupts a reported
number with no panic to announce it.
So the sum and the ratio move to f64 — no sum of finite f32s can overflow it —
and only the resulting share, bounded by 100 and therefore always finite, is
narrowed back. The arithmetic stops being able to produce a non-finite share
rather than a call site checking for one afterwards; `fit_csv`'s existing clamp
stays as the bar-length bound it also is. The one remaining guard,
`!total.is_finite()`, answers only an importance that arrives non-finite: an
infinite total apportions nothing, so the ranking is empty rather than NaN.
Reachable end to end: both callers score importance in the target's own units,
so an importance is an MSE difference and grows as the square of the target.
That leaves a live window between pv3g's `standardize` refusal and this
overflow, and `a_large_measures_importance_shares_reach_the_report_finite` sits
inside it — before the fix, `fit_measure` reported `[("Cost", inf)]`.
The scoring bit patterns in tests/scoring_known_answers.rs are unchanged, which
is the evidence the fix is confined to the non-finite path: `importance_ranking`
is not on any path that file pins. gp-tree still prints nothing.
Evidence in context/test-evidence/gp.md; the argument in context/code/gp.md §8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
|
||
|
Edward Langley
|
c80da1be36 |
fix: seat a category by its kind, not by its name
improvise-cj4u (P2). View::on_category_added took a name and asked CategoryKind::of_name, which reads Label as Regular by design. Workbook::add_label_category papered over that with a set_axis(None) straight afterwards, so views existing at that moment were right; Workbook::create_view had only names to hand and had no such correction, so every view created after a label pivoted it onto Row — and on an already-pivoted document onto Page, filtering the whole document down to one description. on_category_added now takes the CategoryKind, and the two set_axis corrections are deleted: one answer to "where does this category go" rather than two, of which only one caller knew to write the second. The signature change is deliberate — it is what made the compiler name every call site. Every site outside Workbook is test code and passes of_name(name), which preserves what those fixtures meant. Shares a theme with improvise-dy3j — a Label kind that cannot cross a boundary carrying only a name — but not a mechanism: dy3j is the file format, this is a function signature. Fixed separately for that reason. The control tests carry as much as the fix: a blanket `unseated = true` satisfies every label assertion in the repository, so the direction the bug cannot supply is asserted too, and a label is shown not to consume the Row slot the next regular category wants. Reasoning in context/code/core.md §9; evidence in context/test-evidence/core.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
d00cea2477 |
fix: show decimals by default, and let a formula name its own format
improvise-e0ex. `Margin = Profit / Revenue` drew as `1`: `View::new` seeded
`,.0` — zero decimals — so a 0.66 ratio rounded half-away-from-zero. The cause
is the format, not the arithmetic; exact decimal math would not change the
rendering by one character, so nothing here touches the evaluator and gotcha 8
is unchanged.
Two halves, per the user's ruling:
* `View::new` seeds `,.2`. Revenue draws `18,400.00` and Margin `0.66`. The
noise on whole numbers is the cheaper error — a currency column with two
redundant zeroes is legible, a ratio with none is wrong — and a format cannot
be guessed from a value.
* A formula may carry its own format, which wins for its target; a raw measure
keeps the view's. `formula.pest` gains
`format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }`, with `sp+` for `WHERE`'s
reason (a predicate is invisible to the grammar-walking generator) and
`FORMAT` added to `quote::KEYWORDS`.
`improv.pest` is deliberately unchanged: `formula_raw` already carries the
`## Formulas` line verbatim, so the format rides in the formula text with
nothing to add — and the generator, which reads `improv.pest`, sees no new
spelling to violate.
Resolution has one home. `FormatPlan::of(model, view)` replaces the
`(comma, decimals)` pair every render path threaded — `GridLayout::display_text`,
`viewport::grid_body`/`compute_col_widths`, `project::grid_in` and
`persistence/export.rs::render_csv` — rather than being added beside it, which
would be two answers to "what format is this cell". It is asked of the cell's
`CellKey`, so a cell aggregating over `_Measure` keeps the view's format by
construction. `GridBody::col_formats` carries the answer to the totals strip,
which is formatted on its own path.
Persistence is untouched except for metadata: `format_f64` is display,
`persistence/format.rs::format_number` writes round-trip precision, and the two
have no call between them. Newly created views write `format: ,.2`; existing
documents are unaffected, because `format_md` has always written the line
explicitly. `web/demo.improv` moves to `,.2` on both views.
The trap this sets is gotcha 34's: `format_md` writes `Formula::raw`, so a
`FORMAT` suffix round-trips whether or not the walker parsed it. The round-trip
test therefore asserts the parsed field — injecting a walker that drops the
clause fails that one test and nothing else in `improvise-io`, 242 tests
including the whole characterization suite and the grammar properties.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
|
||
|
Edward Langley
|
20f04c831c |
docs(gp): FitOutcome::warnings now leads with the budget's notices
The field doc named two sources and an order that the improvise-e8no splice changed. Third review finding: a doc comment stating an order is a claim, and this one was one commit stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
dc2fe21911 |
review(gp): correct an unchecked cost claim and close a vacuity hole
Self-review round on wave/o-gp-fit, three findings. 1. The e8no evidence claimed the dropped-column fit "lands on the measured pre-fix champion again". Printed it: the threshold and the high leaf are the probe's, the LOW LEAF IS NOT (1090.28 against 970.3), because pre-zv8t the column was present as one inferred numeric feature and the draw sequence differed. Exactly the unchecked-cost-claim shape the same file was written to record. The claim that survives is the internal one, which the test asserts: ignoring the column and marking it a Label give the same champion. 2. `a_target_just_under_the_overflow_still_fits` looped over `leaf_numbers` without asserting the walk found any, so an emitter change that stopped reaching the leaves would have made it pass vacuously. 3. `unaffordable_categories` drops by NAME while budgeting by INDEX, which is `doctrine(forcing_by_name_while_budgeting_by_index_defeats_the_budget)`. It is safe by the same DATA DEPENDENCY that doctrine's own repair used -- `overrides` does not exist until `as_categories` returned Ok -- and gp.md now says that rather than "a `?` puts it there". Also recorded: an over-count in the advisory preflight used to cost a needless refusal and now costs a needlessly dropped column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
f616487af4 |
fix(gp): a poisoned affine map is unrepresentable (pv3g); a budget declines a column (e8no)
improvise-pv3g (P1). `fit-measure` panicked in `number_text` with a NaN leaf on an ordinary document whose target values were large but finite: 8 records at 1e20/4e20. Every input was finite and `fit_measure` checked exactly that; the overflow was derived. `standardize` sums (t - mean)^2 in f32, 8 * (1.5e20)^2 exceeds f32::MAX, `std` came back inf, every normalized target became +/-0, and `denormalize` computed 0.0 * inf + mean = NaN two crates away from the arithmetic that caused it. `Scaling` has private fields and one constructor that refuses a non-finite part, so `standardize` returns `Option<(rows, Scaling)>` and `denormalize` cannot be handed a poisoned map at all. `cv_regress` pools nothing for a fold it cannot standardize -- the silent half, which produced R2 numbers with no panic to announce them. The arithmetic is untouched: every bit pattern in `scoring_known_answers` is unchanged. The ticket's sibling path is corrected in `context/code/gp.md` SS7: a finite target with an infinite mean gives +inf variance, never NaN, so the `NaN.max(1e-6)` laundering it describes needs an infinite input the existing guard already refuses. Measured by injection. improvise-e8no (P3). The width preflight refused the whole table when one category was too wide, so a document that fitted before zv8t stopped fitting. `encoded_width` became `encoded_widths` and `unaffordable_categories` drops category columns widest-first into `Overrides::ignore` until the rest fits, reporting each as a `Dataset::warnings` notice -- data, never printed. The encoding decision is untouched: a category is still never inferred numeric and the forcing is still unconditional. `context/code/gp.md` SS11's "declined" verdict is superseded there, with the reasoning under it kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
c2bc939608 |
improvise-x1ns (2nd): the preflight counts a date measure's parts
P1 from ChatGPT Codex on #97, live at
|
||
|
Edward Langley
|
f128087999 |
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 |
||
|
Edward Langley
|
e045f9871b |
improvise-x1ns: the preflight counts the (missing) one-hot; improvise-e8no declined
improvise-x1ns (found by ChatGPT Codex on PR #90, live on main at
|
||
|
Edward Langley
|
d42253ef38 |
review(gp,io): the zv8t forcing defeated its own budget, and two narrower fixes
Adversarial review round on the branch. One real finding, two prose/comment corrections, one fixture widened. F2 (the real one, and introduced by zv8t). `as_categories` forces by column NAME; `encoded_width` counts by column INDEX. A document may legally name a regular category and a raw measure the same thing — the repo already ships that fixture in `a_category_and_a_measure_of_the_same_name_are_refused` — and the measure column was then forced categorical by the name match while the preflight still counted it numeric by index. Measured at 40 records: preflight 41, actual 80. `no_ambiguous_column_names` ran AFTER `preprocess`, so the expansion the preflight exists to prevent was materialised and only then refused; at the default max_records that is a 20k-row one-hot matrix built before the error. That is `doctrine(guard_the_cost_not_the_result)` defeated by an inconsistency this ticket introduced — not the bounded approximation that `doctrine(a_budget_check_is_advisory_or_exact_and_must_say_which)` licenses. The repair is a data dependency rather than a moved line: `as_categories` runs the check and returns `Result<Overrides>`, so the Overrides `preprocess` needs cannot exist until the header has been accepted (`doctrine(an_invariant_is_made_unrepresentable_not_validated)`). The regression guard is therefore the SIGNATURE and the injection is a compile error — swapping the two statements back gives E0425, verified. What it does not cover is stated in the evidence rather than implied: a deliberate deletion of the check from `as_categories` is still possible; the accidental reorder is not. F5. The `encoded_width` comment claimed the preflight "must count it the same way". It does not, quite: `build_categorical` counts MISSING_CAT as a distinct value and the preflight does not, so it under-counts by one per category with an absent coordinate. Bounded by the exact check and pre-existing for genuinely categorical columns — but the comment asserted an identity that does not hold. F4. The evidence for the two spelling tests said that on the unfixed tree the text "could only have been `Expr::Ref(\"Year\")`". There is no text on the unfixed tree at all — `fitted_formula` refuses before emitting. The argument for the assertion survives; the stated reason was a guess about a branch that does not run, and an evidence file is the one place that should be exact. And the iot6 fixture: `prune_empty` prunes BOTH axes and the test asserted rows only. `Feb` is now the empty column, the precondition is a pair on each layout, and the CSV header is asserted to carry `Jan` alone. 1464 passing, up from 1463. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
e6e39a03ff |
review(gp): the zv8t prose claimed a cost it has, and the cost is measurable
Self-review of
|
||
|
Edward Langley
|
99cfb7fb41 |
fix(gp): a numeric- or date-valued category is spellable (improvise-zv8t)
`fit_measure` handed `preprocess` an `Overrides::default()`, so it never said which header columns are document categories — although the very next line told `Features::of`. `preprocess` then inferred Numeric -> Date -> Categorical from the values, so a `Year` category with items 2024/2025 became a numeric column and a category of ISO dates became a date column, and both were refused as encoding artefacts with `Year IS 2024` spellable all along. `fit_measure` now builds its `Overrides` from the same `header[..category_columns]` slice it already hands `Features::of`, forcing each to `Forced::Categorical`, so the one place the document's own answer was known is now the only place either caller reads it from. `Forced::Categorical` already existed; no formula-language change was needed. Three things follow, and the last is a cost. The date decline in gp.md §11 now means what it says. A `DatePart` could arrive over a *category* column by exactly this accident, so that decline was narrower than it read; it now covers a genuine date measure and nothing else. The `Artefact` arms stay and stay correct. `crate::dataset` and `fitted_formula` take the caller's `Overrides`, so a caller that does not force can still produce a `Numeric` role over a category column. The arm is no longer reachable from `fit_measure`, which is the only production entry point — it is not dead code, and deleting it would derive the classification from one caller's habits. Forcing widens the encoding, deliberately: a two-item `Year` is now two one-hots rather than one numeric feature, and a two-hundred-item one is refused by `MAX_FEATURES` where before it was refused one stage later as an artefact. Both refuse, so nothing that fitted before stops fitting; what changes is which guard speaks, and the width guard's message names the repair. `encoded_width` had to learn the same slice for that to hold, or it counted such a column as one feature and handed the refusal to the exact check *after* materialising the matrix — the side of the cost the preflight exists to stay on (§14). Evidence. The two spelling tests needed no fault injected: on the unfixed tree `fit_measure` refuses with the two real messages the ticket named. Each fixture gives the model one regular category, no other feature column, and a target that category alone determines — because the refusal is conditioned on `used_features`, so a fit whose champion happened to be a bare leaf would be accepted and prove nothing. Asserting `formula.raw` contains `Year IS` rather than merely `is_ok()` closes that in both directions. The preflight test did need one: dropping the `col >= category_columns` clause makes it fail by name, with the *exact* check speaking where the preflight should. The prose this corrects was itself the ticket's subject. improvise-wd6n's §11 called `Artefact::NumericCategory` a permanent refusal that "no vocabulary closes", which is false and is what filed improvise-zv8t. §11 and the gp test-evidence entry are both updated to say it is closed rather than open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
630b84066c |
fix(gp): NumericCategory is a filed gap, not a permanent refusal
Review findings on this branch, all three in the gp commit. The prose was the worst of them: §11 called Artefact::NumericCategory a permanent refusal that "no vocabulary closes". It is not. fit_measure hands preprocess an Overrides::default(), so it never says which header columns are categories — although the next line tells Features::of — and preprocess then infers Numeric -> Date -> Categorical from the values. A `Year` category with items 2024/2025 becomes a numeric column and is refused, with `Year IS 2024` spellable all along. Forced::Categorical is the existing knob. Filed as improvise-zv8t. Prose that calls something permanent is what stops the ticket being filed, which makes this the branch's own defect class one level up. MissingFlag's words said "that measure", but its arm is not conditioned on is_category and dataset.rs writes "" for an absent coordinate, so a .missing flag over a category is reachable — the same "declined in words naming something they are not" the commit was written to fix. Now "that column". ValueOfAMeasure's tail likewise covered only the present case. The test module claimed "each artefact family" while its fixture reaches three of five; named for what it reaches. Re-injected the collapsed-reason fault after the reword: still fails, so the negative clauses still bite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
173d9820a6 |
feat(gp): decline the artefact families separately, in their own words
improvise-wd6n. Spelling::Artefact was one variant standing for five distinct families, the last five reached through a `_ =>` wildcard in spell(). The one refusal they fed said the tree "splits on a col.missing flag or a date part", so three of the five were declined in words naming something they are not. Artefact now names which family, spell() is exhaustive (a sixth FeatureRole is a compile error rather than a sixth thing that message misdescribes), and the refusal gives one clause per family. The two families the ticket asks about are declined in writing in context/code/gp.md §11, separately, because each needs different vocabulary: col.missing needs a word for absence that reaches apply_agg and eval_agg, and the date parts need a date type improvise does not have anywhere. Nothing new is emitted, so improvise-4zl7's route-then-aggregate rule is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH |
||
|
Edward Langley
|
5fbcb1718a |
fit: one evaluator for the fold — which was also the stall
Review of #54 found the fold reintroducing exactly the divergence the ruling exists to remove, and the same line turned out to be the 244 ms stall. One defect, two symptoms. `eval_expr_cached`'s `Expr::Ref` looks the value up in `formula_cache` at `context.with(cat, name)`. Inside the fold `context` is a MEMBER-RECORD key carrying the none_cat coordinates, while `formula_cache` is keyed by stems with none_cats STRIPPED — so the lookup could never hit, fell through to `aggregate_raw`, found no raw cell behind a formula target, and answered Err("ref:…"). A routing formula over a derived measure gave Number(11.0) uncached and Error("ref:Margin") cached, and the cached path is the one a pivot grid runs whenever any category sits on Axis::None. `eval_expr`/`eval_bool` are hoisted to module level and BOTH evaluators now hand the fold the same record-level `eval_expr`. The two agree by construction rather than by a test. Note the repair not taken: teaching the fold to hit the cache by stripping none_cats off the member key returns the GROUP's value to every member — aggregate-then-route through a cache lookup, and silent. That same per-member `aggregate_raw` was one poorly-narrowed scan per member, an O(n^2) shape. Measured with the new `examples/fold_bench.rs`, five runs a point, at 18,050 cells: 51.1-53.3 ms before, 18.1-19.2 ms after, against 7.1-9.0 ms for a linear formula. Scaling went from superlinear (45x cells, 85x time) to linear (45x cells, 45x time). Debug 2.75 s -> 132.9-135.1 ms. The fixed point runs exactly 2 passes, measured by counting, so hoisting the member set has a 2x ceiling and is declined on improvise-9iyv — a memo keyed by less than the whole `partial` is a second answer to what a cell depends on. `Expr::Is` has an arm in each evaluator and the cached one was untested: the earlier fault was injected into the shared `eval_is`, which both arms call. Replacing the cached arm alone left the suite green. The first replacement test ALSO stayed green, which is the interesting part — once the fold routes through `eval_expr`, an `Is` inside a SUM never reaches the cached arm, so that arm is reachable only by a bare `Is` outside an aggregate. That is what the new test uses, and injecting into that arm alone now fails it. `IS` gained a word boundary: `Region ISLAND` parsed as Is(Region, "LAND"), silently wrong where it was a parse error. `is_expr` is compound-atomic with `sp+` written out, because a negative lookahead in an ordinary rule tests the character AFTER pest's auto-inserted whitespace and fails backwards. The lookahead was then redundant and removed — deleting it left the suite green, while relaxing `sp+` to `sp*` fails the test, so `sp+` is the mechanism. `WHERE` has the same hole, is far less exposed, and is filed as improvise-r08u. Also: `ambiguous.sort_unstable()` before `dedup`, which only removes consecutive duplicates, so the refusal could name a column twice. Every fault re-injected after the refactor: no-fold 10 failures, shared eval_is 5, record-level Is arm 4, cached Is arm 1, feature_roles order 2, context-vs- partial 1, the new cached-fold divergence 1, `sp+` relaxed 1. Baseline 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
022e12f9b4 |
fit: route per record and aggregate, never route on the aggregate
improvise-4zl7. A tree is fitted on individual records; an improvise formula is evaluated at a CellKey, and an aggregating cell used to aggregate each Expr::Ref first and evaluate the IF once on the totals. That is the same answer for a linear formula and a different one for anything that routes, so every fitted tree meant one thing on a record row and something else one row up. The user ruled: aggregate the rule over the members. The rule needed no new aggregate vocabulary. SUM(e) already means "fold e over the members" and simply did not: eval_agg specialised Expr::Ref and, for every other inner expression, summed the raw matching cells and ignored the expression entirely. It now folds — one entry per member record, the expression evaluated at that record's own coordinates. eval_agg is shared by eval_expr and eval_expr_cached, so the fold has exactly one home; each evaluator passes its own recursion in. Expr::Is(Filter) is the new arm in both evaluators: `Cat IS Item` is 1.0 when the cell's own Cat coordinate is Item, keeping the evaluator f64-only and reusing Formula::filter's own context.get/unwrap_or(false). A grammar production in primary, before ref_expr, with both operands through improvise_formula::quote's one home; IS joins the keyword guard. The emitter reads feature index -> role off preprocess's plan rather than reconstructing it: gp_tree::data::feature_roles is the one statement of the expansion order and encode_row now drives off it too. improvise_gp::Features carries one Spelling per feature — Measure, Coordinate or Artefact — decided by role and column, never by the name's shape. A one-hot over a real category is emitted as `Region IS North` and accepted; col.missing and the date parts stay Artefact and stay refused (improvise-wd6n). fit-measure on a document a category explains now lands instead of refusing, and the refusal no longer depends on which features the search happened to pick. Evidence: eight hand-computed tests in improvise-core over a four-record fixture whose Cost straddles the threshold inside each region, so route-then-aggregate and aggregate-then-route differ at every level. Two faults injected and watched to fail — reverting the fold fails 7 of 8, and a constant-false eval_is fails exactly the 3 coordinate tests. Reordering feature_roles fails both it and encode_row_replays_the_training_expansion. The fold is O(members) per evaluation per fixed-point pass and unbenchmarked: improvise-9iyv. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
5629f4c295 |
docs(gp): drop the two ```text fences; only a runnable one earns the exemption
rustdoc does not execute a ```text fence, so `improvise-gp/src/lib.rs`'s sample
formula and `fit_csv.rs`'s invocation block were prose wearing a fence and were
claiming the checker's doctest exemption. Both move verbatim into
context/code/gp.md (§10 and §17) and the cap now applies to both headers.
What remains fenced in this unit is the two blocks promoted in
|
||
|
Edward Langley
|
c1e93dcda9 |
docs(gp): key every test-evidence heading to a real #[test]
Review finding (P2): `### improvise_eval` named a helper, not a test. The `--check-evidence` gate passed it only because it accepts every `fn`, so the entry would have become an orphan the moment the checker was tightened. The helper's scope is the scope of the tests that use it, so the narrative is now a `####` under `improvise_evaluation_matches_gp_tree_eval`, and the helper's own comment points there by name. 49 entries, every one a `#[test]`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
75e98fbfd6 |
docs(improvise-gp): cap the comment runs
Every run in crates/improvise-gp is now at most two lines; the arguments moved to context/code/gp.md and the bug narratives to context/test-evidence/gp.md. The ```text block in lib.rs is a sample of the emitted formula, not a doctest, and is kept with its prose capped around it. One stale claim dropped rather than moved: the `quote_name` re-export comment said a third copy of the quoting rule "lives in improvise-io". improvise-i2m5 deleted it — persistence/quoting.rs is gone and quote_name has one home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Ed Langley
|
a15cc737c2 |
Merge pull request #25 from fiddlerwoaroof/wave/m-gp-tree
WU-M: wire gp-tree into improvise (improvise-szrt, improvise-vx3; improvise-21d's last open child). improvise-4zl7 stays open and grew: a formula evaluated at an aggregated cell routes on the aggregate, so aggregate-then-route is not route-then-aggregate and EVERY fitted tree — numeric splits included — means one thing on a record cell and another one row up. The bead asked this for categorical splits; it is a question about the feature. |
||
|
Edward Langley
|
3bcc12609a |
fix(fit): the fit's own name must not already mean something
Copilot's fifth pass, one finding and it is right. `into` was checked against `measure_item_names()`, and `Expr::Ref` does not ask that question: `Model::find_item_category` finds a **regular category's** item before it falls back into `_Measure` (gotcha 7). So naming the fit after an item of some category installed cleanly, and every later formula referring to the fitted measure then read the category coordinate instead — silently, because both resolve. The check now asks the document the same question `Expr::Ref` will ask, which is both stronger than the measure-only form and simpler than it. Two regressions: one that a category item's name is refused, and one that earns the refusal by asserting the property it protects — a second formula `Doubled = |Score (fit)| * 2` reads twice the fit, per record, against a real document. 1232 passing (+2); clippy, fmt and the wasm gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
d8a090288b |
fix(fit): round four — a panic, a split status bar, and the question restated
Four of five fixed. The fifth is the most valuable thing in the review.
**A fitted rule means something different in an aggregated cell, and that is
improvise-4zl7's blocking question in its general form.** A tree is fitted on
individual *records*; an improvise formula is evaluated at a `CellKey`, and in a
cell that aggregates over a hidden category `evaluate_aggregated` aggregates
each `Expr::Ref` **first** and then evaluates the `IF`. Linear formulas survive
that — `SUM(Revenue) - SUM(Cost)` is `SUM(Revenue - Cost)` — and a tree does
not: aggregate-then-route is not route-then-aggregate. 4zl7 asks what a
*categorical* split means where its coordinate is aggregated away; this is the
same question for **every** fitted tree, numeric splits included, and it makes
the bead larger and more clearly blocking rather than smaller.
Characterized rather than answered, because answering it is the user's call.
Worth recording what the fixture actually does: the aggregated cell comes back
`Error("ref:Cost")`, not a wrongly-routed number — the loud failure this repo
prefers, and *not* something to rely on, since nothing states it. The assertion
is therefore the negative (it is not the sum of the per-record predictions), so
it covers both outcomes and fails the day aggregation becomes record-wise.
**An infinite target panicked out of a function that returns `Result`.** A
`.improv` cell holding `inf` parses as an `f64`; `standardize` turns an infinite
target into NaN leaves and `number_text` asserts. Verified by disabling the new
guard and watching `number_text wants a finite value, got inf`. The two
directions differ and the doc says so: an infinite *feature* poisons its range
and quietly yields a constant leaf instead. A panic and a meaningless rule, both
refused now. `NaN` needs no arm — `is_missing` already spells `"NaN"` as
missing, so it is imputed away.
**Only the success line was flattened**; refusals interpolate document names
too, and a name may legally contain a newline — so a failed fit could split the
status bar, which is when a user is most reading it. `one_line` is public and
the command's error path uses it.
**"a constant — no feature matters" overclaimed.** An empty importance ranking
establishes that no shuffle raised the error, which a constant tree satisfies
and so does a tree whose splits do not matter on these rows. It now reports what
the data established: "nothing measurable — no feature changed the error".
**Declined:** the record budget runs after `feature_table` has already grouped
and sorted, and `RecordRow::group` linear-scans (`improvise-core/src/view/
layout.rs:43`) so grouping is O(cells x rows). Correct — but that cost is
pre-existing and shared with records mode, it is in `improvise-core` which is
outside this unit's boundary, and threading a limit into `feature_table` changes
a signature `dataset()` and its tests share. Filed as an indexed-grouping ticket.
1230 passing (+2); clippy, fmt and the wasm gate clean; CLI byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
f535fba2f7 |
fix(fit): round three — two bugs in my own new guards, and a correction
Copilot's third pass. Four fixed, one declined, and two of the four were defects in the checks the *previous* commit added. **The role check read positions where positions do not hold.** `preprocess` `continue`s past an entirely-missing column **without pushing a `ColumnSpec`**, so `Dataset::columns` is not index-aligned with the header once anything is dropped: one empty category shifts every later spec left, and the positional `i >= category_columns` test then read the surviving numeric measure as a category and refused a perfectly good fit. The role now comes from the *header* — which `feature_table` built and which never shifts — and the plan is consulted by name. Proved by re-injecting the positional form and watching `an_empty_category_before_a_measure_does_not_break_the_role_check` fail alone. **And the collision test never built the collision.** Adding a category named `Cost` with items puts no `Cost` coordinate on the existing records, so `feature_table` emitted an empty column, `preprocess` dropped it, and there was no duplicate name to find — the test passed on the positional bug instead. The records carry the coordinate now, the fixture asserts the header really has two `Cost` columns before fitting, and the assertion names the ambiguity diagnostic so it fails if that check is removed. Two findings, one root: a guard written against a premise nobody checked. **`encoded_width` treated every column as categorical**, so a numeric measure with 65 distinct values was refused as 65 features, while a date or a column with missing cells could expand past the estimate. It now approximates `preprocess`'s inference, and — the part that matters — the **exact** width check is restored after preprocessing. The preflight is advisory, and says so: its only job is to refuse a rows × rows one-hot before the memory is spent, and a drift in either direction is bounded by the exact check behind it. **Nothing bounded the work by record count.** `MAX_FEATURES` bounds the width; every stage scales with rows on the thread that draws the frame. `MAX_RECORDS` (20,000 — about a second at the measured 105 ms/2,000, roughly linear) is the default of a new `FitRequest::max_records`, so a host that has somewhere else to run a fit can raise it rather than being told no. **Declined, and it corrects something I got wrong.** `Spec::ranges` is computed over all rows and the same spec goes into every training fold, so the thresholds the search samples and the branches `simplify` prunes are bounded by a held-out extremum — which reaches a fit over *complete numeric measures* too. My earlier reply on the imputation thread said "the case that currently stores leaks nothing"; that was wrong, and the doc comment on `Quality` now says so. Still declined here for the same reason: closing it needs a `cv_regress` that takes a raw table and a plan-builder, which is a `gp-tree` API change and the same one improvise-4zl7 will want. 1228 passing (+3); clippy, fmt and the wasm gate clean; CLI output still byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
6a45edcfc9 |
fix(fit): six more Copilot findings, and a false premise of my own
The second review round. All six correct; five fixed, one documented and filed.
**My fixture's determinism premise was false, and the comment asserting it was
the worst part.** Both `document()` fixtures left one regular category with a
single item and claimed the constant one-hot had a degenerate `(1, 1)` range
that `simplify` would prune, so the champion "could not" reach an unresolvable
feature. `data.rs::build_categorical` pushes `(0.0, 1.0)` for **every** one-hot
whatever the column holds, so `simplify` cannot know it is constant — the
success tests were passing on seed luck, and three comments, the PR body and a
proposed repo-map gotcha all repeated the claim. Every category in both fixtures
is now a `Label`, so the only feature is the numeric measure `Cost`, which is
also the only shape a fit can currently store.
**Resolving a name is not the same question as meaning the right column.** A
numeric category called `Cost` encodes to a feature called `Cost`; with a
`_Measure` item of the same name the emitted `Expr::Ref("Cost")` resolves
cleanly — to the measure — and the stored rule reads a different column from the
one it learned. `fitted_formula` deliberately asks only whether a name can be
*spelled*, which is right for that check and wrong as the only one. So the
question is now also asked of `preprocess`'s own plan: a used feature is
accepted only where `Dataset::columns` says it is a raw `Numeric` **measure**
column, which rules out one-hots, `.missing` flags, date parts and category
coordinates by role rather than by spelling. A document naming the same thing as
both a category and a measure is refused outright. Deliberately *not* a
reconstruction of the expansion order index-by-index — that is improvise-4zl7's
job and its ticket warns against a second implementation.
**The width guard was on the wrong side of the cost.** It ran after `preprocess`
had already materialised the rows × rows one-hot matrix it was meant to prevent.
`encoded_width` now counts what the expansion *would* produce, from the raw
table, without producing it.
**`importance_ranking` misreported small importances.** `.max(1e-9)` on the
divisor is right for the only input `fit_csv` ever had and wrong as a contract:
a lone importance of `1e-10` divided by the floor and reported 10%. Importances
this small are ordinary — error differences over a standardized target — so the
zero case returns early and the shares sum to 100% at every scale.
**A name can contain a newline.** `.improv` pipe quoting makes it legal, so a
document could hold a measure whose name split the TUI status bar in two.
`status_line` flattens the composed line once, rather than sanitising each
interpolated part and forgetting one.
**Declined and documented on `Quality`:** preprocessing runs over all rows
before `cv_regress` picks folds, so imputation means and categorical
vocabularies leak from the validation fold and `Measured` is slightly
optimistic. Bounded — a fit over complete numeric measures, the only shape that
currently stores, leaks nothing — and closing it means preprocessing per
training fold, which `cv_regress` cannot express because it takes rows that are
already encoded. That is a `gp-tree` API change, not a line here.
1225 passing (+5); clippy, fmt and the wasm gate clean; the CLI's output still
byte-identical, which is the check that gates the `importance_ranking` change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
3de90074a9 |
fix(fit): four Copilot findings, including a reachable panic
All six findings on PR #25 are correct. Four are fixed here; two are declined with reasons and documented on `fit_measure` rather than left silent. **A legal document could panic the command.** `preprocess` drops an entirely missing column, so a document with no regular category and one empty measure encodes to *zero* features — and `random_tree` then draws `rng.below(0)`, which `SplitMix64` answers with `0`, and `sample_thresh` indexes `spec.ranges[0]`. Refused now, and the refusal carries the warnings naming what was dropped. Proved by disabling the guard and watching `index out of bounds: the len is 0 but the index is 0` at `gp-tree/src/lib.rs:218`. **Nothing bounded the feature *width*.** `FitConfig` bounds the search; a regular category with one distinct value per record one-hots to one column per record, and permutation importance is `repeats × rows × features` evaluations — quadratic in records, on the thread that draws the next frame. The 6/12/105 ms benchmark this PR quoted does *not* cover that shape, because its fixture marks the row-identity category as a `Label`. `MAX_FEATURES = 64` refuses, and names `Label` as the fix. The test had to grow past 64 records to be honest: at twelve the document is refused anyway, by `fitted_formula` noticing `Tag=r0` — a different rule, which would have made the test pass with no guard at all. **`FitOutcome` did not meet its own reproducibility contract.** It carried `seed` + `config`, which is the pair gotcha 37b names — and that pair is sufficient for `gp_tree::data::Model`, whose other knobs are not in the loop. Here they are: cross-validation draws from the *same* generator `fit` then draws from, so `cv_folds` moves the champion and not merely the evidence. It carries the whole `FitRequest` now, with a test that changing only `cv_folds` changes the rule. **The status line advertised a seed the parser rejected.** `status_line` prints `{:#x}` and tells the user that reproduces the fit; `parse` took decimal only, so the one instruction it gives failed on the value it had just shown. The test round-trips through `status_line` rather than a hand-written `"0x…"`, because a literal would still pass if the printing changed — which is the half that breaks. **Declined, both documented on `fit_measure` and to be filed.** A mean-imputed feature is read *raw* by the stored formula, so a record missing that value evaluates to something other than what the fit predicted for it; closing that means choosing between refusing any feature with a missing cell — which would refuse most real documents, imputation being why `preprocess` imputes — and an `IFMISSING`-shaped primitive the formula language does not have. That is a product decision of the same kind as improvise-4zl7. And a boundary value can route differently in the formula than in the fit, because training narrows `f64` to `f32` while the formula compares the original: pre-existing in the emitter (improvise-izk), and the fix would rewrite the bit-level characterization tests that are the only evidence the routing argument holds. 1220 passing (+4); clippy, fmt and the wasm gate clean; the CLI's output still byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
365f27bbd3 |
fix(fit): work the code-review backlog — two vacuous tests and a hidden axis
The review's two HIGH findings were both this repo's signature defect, and one
of them is a straight own goal.
**`the_seed_argument_changes_the_search` proved nothing.** It compared
`effects_debug` over the whole batch, and `status_line` ends with
`seed {:#x}` — so the two strings differed *because the seed was printed*, and
`fit_measure` could have ignored `request.seed` entirely with the test green.
It now asserts on the `AddFormula` debug alone: two fields, the formula text
and the target category, neither of which the seed reaches. (A substring guard
was tried and is wrong — `1` and `99` occur inside ordinary thresholds.)
**The new `improvise-gp` fixture re-committed the bug the same commit fixed.**
`document()` built its view with `View::new` + `on_category_added`, under which
`_Measure` lands on `Axis::None` — so every `fit_measure` test ran under the
one layout that hides the axis bug, while `Workbook::new` hands out
`_Measure: Page`. The fixture now sets `Page`, and that is checkable: injecting
the `filter_coords` fault back in used to fail two `dataset` tests and now
fails **six**, the whole `fit::tests` suite included.
Also from the review:
* `Quality::Measured` at exactly two rows was garbage wearing the label — two
folds of one row, each tree fit on a single constant. `folds` is now capped
at `rows / 2`, so every fold and every training complement holds at least
two. The confusion `Quality` exists to prevent was moved from one row to
two, not removed.
* `the_same_seed_under_a_different_config_is_a_different_search` varied
`generations` over an exactly-learnable fixture, which can converge to the
same optimum and fail one day for a reason that is not a regression. It
varies `pop`, which changes the RNG draw count per generation.
* `a_fit_that_needs_a_category_is_refused_naming_the_feature` cleared `Cost`
but not `Score`, leaving `Region=North` carrying *both* target values — so
the one-hot the test is about explained nothing. It now clears both.
* `the_fitted_formula_predicts_in_the_documents_own_units` had a
`let … else { continue }`, so a loop that skipped every record asserted
nothing. It panics: "no cell evaluated" *is* the improvise-8xhy failure.
* `FitOutcome::warnings` documented the splice order backwards.
* `fitted_measure_name` joins the crate-root re-exports, and the one consumer
uses that door for all three.
And the layering cost the review asked to be measured rather than assumed.
`improvise-command` is in the worker bundle and the path from `App::new` to
this command's prototype is live, so `gp-tree` links in whether a browser
invokes it or not. Built with `wasm-pack --release`:
`improvise_worker_server_bg.wasm` 1,341,130 → 1,420,470 bytes (+79 KB, +5.9%),
498 → 531 KB gzipped (+6.5%). `improvise-wasm-client` does not depend on
`improvise-command` and is unchanged. Accepted and written down on the module,
with the feature-gate named as the lever if it ever needs shrinking.
1216 passing, unchanged; clippy, fmt and the wasm gate clean; the CLI's output
still byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
e46429ecfc |
feat(fit): fit-measure — a fitted tree as an ordinary improvise formula
improvise-szrt / improvise-vx3. `improvise-gp` was a bridge with nothing
crossing it; this is the crossing.
`improvise_gp::fit::fit_measure` is the sequence between the two halves that
already existed: `dataset` (a View's records as a feature matrix) and
`fitted_formula` (a champion as formula text the document can resolve). It
lives in `improvise-gp` rather than in the command because none of it is
terminal-specific — a browser fitting the same measure must run exactly these
steps in exactly this order or it gets a different tree from the same seed.
`command/cmd/fit.rs` is the user-facing surface: `fit-measure <target>
[<measure-name>] [<seed>]`, returning `SetStatus` + `MarkDirty` + `AddFormula`.
After it there is nothing GP-specific in the document — the rule is a
`## Formulas` line, recomputed by the fixed-point evaluator, editable in the
formula panel, drillable, saved with no grammar change.
**Where the fit runs, and why.** In `execute`, on the command side. A fit is
decision logic — a pure function of (Model, View, target, seed, FitConfig)
that mutates nothing — so it belongs where decision logic belongs; putting an
expensive stochastic search inside `Effect::apply` would mix it into the one
place the app mutates. That buys purity, not asynchrony, so vx3's AC 4 is met
on its "bounded" arm: `FitRequest::default` is a smaller search than the CLI's,
**measured** at 6ms / 12ms / 105ms over 12 / 200 / 2,000 records in release.
**Three things the sequence had to get right**, each of which fails quietly:
* The fit is standardized and the formula must not be. Written down untouched,
a champion predicts standard deviations — a well-formed formula evaluating to
±5 where the user's data is in the thousands, and nothing downstream can see
it. `denormalize` folds the affine map back into the leaves first.
* The rule is stored as a *new* measure (`<target> (fit)`), never as the target
itself: gotcha 7 makes a formula target and an explicit item one item, so the
cells the fit was learned from would stop being visible.
* `Quality` is an enum. `cv_regress` pools nothing below two folds and scoring
an empty pool gives R² exactly 0 — "nobody checked" reading as "this rule is
worth nothing", which is the loudest thing a fit can say.
**A bug this found, fixed with its own failing test first.** `_Measure` is a
filtering category, and `Workbook::new`'s default view puts it on `Axis::Page`
— so `dataset::filter_coords` pinned it to one measure and the feature matrix
collapsed. On a document whose target is `_Measure`'s first item every feature
column came back blank; otherwise the table was empty and the error blamed the
data ("no visible record has a Revenue value"). `_Measure` is the coordinate
the columns are spread across, so paging it is not "which records am I looking
at", it is "throw away every column". Every existing test in that module built
its view with `View::new` + `on_category_added`, under which an underscore
category lands on `Axis::None` — the fixture was building the one world the
code worked in. The two new regression tests compare row *contents*, because
asserting the header and the row count passes with the bug present.
Evidence, and the faults injected to earn it. Dropping the `denormalize` call
fails `the_fitted_formula_predicts_in_the_documents_own_units` and nothing
else. Both `_Measure`-on-Page tests were watched failing before the one-line
fix. Every new test asserts against a real document — never a helper that
fabricates a `_Measure` item per feature name, which is what let improvise-8xhy
hide. 1216 passing, up 27 from 1189; suite still well under 2s.
Also fixes the refusal message, which a `rustfmt` pass over its multi-line
literal had left with runs of ten spaces mid-sentence — invisible in the source
and to every test that only asked whether a substring was present, and visible
to precisely the user it exists for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edward Langley
|
e8296162ac |
fix: three totality gaps, each one refactor away from being reachable
improvise-hkt6. Three findings from the 2026-09-19 wave review. None is a bug today; each is a guard that has drifted away from the thing it guards, which is how this class of bug comes back. (a) `DataStore::matching_positions` indexed `sets[0]` and panicked on an empty partial. Both public callers guard with `if partial.is_empty()`, so it was unreachable — with the guard one level away from the indexing. The fix is not `vec![]`: an empty partial constrains nothing, so *every* cell matches, and answering "none" down here would have made deleting a caller's fast path silently flip "all" to "none". The callers keep theirs as a fast path, and the test asserts the two doors agree rather than merely that nothing panicked. (b) `GridCache::invalidate_relabelled` blanked the cells under a renamed column and left its **total** standing. Gotcha 33(b)'s own argument says preserve-by-position is right only where the label did not change, and `Invalidated` already blanks the totals because a stale sum under fresh cells is the one number a user takes at face value. Unreachable from `grid_updates`, which diffs the totals and sends them in the same batch — which is exactly the point: 33(b) exists to stop `apply` trusting a producer to remember. Rows are the negative arm and stay untouched: a total spans every row, so no one row frames it. (c) `improvise_gp::mark_features` indexed `used[*feat]` bare where `expr_text` uses `.get().expect(...)` for the same precondition. Worse than inconsistent: `fitted_formula` runs `mark_features` first, so the legible message was the one nobody ever saw. Evidence, each proved by injecting the fault and watching it fail: `an_empty_partial_matches_every_cell_from_either_door` (index out of bounds, len 0 index 0), `a_relabelled_column_drops_its_total` with `a_relabelled_row_leaves_the_totals_alone` as the negative arm that stops "blank on any relabel" passing, and `a_tree_indexing_past_the_spec_says_so`, which asserts the message rather than the panic since an out-of-bounds index panics too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edward Langley
|
74b8b1375b |
fix(gp): refuse a fitted formula the document cannot resolve (improvise-8xhy)
`preprocess` invents feature names -- `Region=North` for a one-hot,
`Cost.missing` for a missing indicator, `when.year` for a date part. Those are
encoding artefacts, not items of anyone's document, so the emitted `Expr::Ref`
failed `Model::find_item_category` and every cell of the fitted measure came
back `Error("ref:Region=North")`. `fitted_formula` said `Ok` throughout. So
"fit a tree and get a working formula" held only for a fit over numeric
measures, and nothing said so.
`fitted_formula` now takes the `&Model` the formula is to be stored in and
refuses a tree whose splits it cannot resolve. That is the same contract the
function already had -- its doc called the parse "the emitter's own contract
check, so a name or threshold this crate cannot spell surfaces here rather
than as a broken formula in the user's file" -- widened by one word, from
spell to resolve. `formula_text` is untouched: it is the tree's own text and
there is nothing else for it to write.
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 any
future sub-feature naming scheme with no second copy of `preprocess`'s
expansion rules to drift from.
THIS IS AN INTERIM, and labelled as one in the function's doc. It is the
smallest of the ticket's three remedies and is honest about what works rather
than making more work. The other two were costed. Materialising one-hots as
real `_Measure` items pollutes the document with artefacts the user never
asked for. Teaching the emitter improvise's `WHERE` vocabulary needs a
*scalar coordinate predicate*, which the formula language does not have:
`Filter` is a formula-level or aggregate-level gate (`Formula::filter`,
`Expr::Agg`'s third field), not an expression, so a categorical split needs a
new `Expr` variant, a grammar production, arms in both evaluators, and an
answer to what the predicate means in a cell that aggregates over the
category -- where `context.get("Region")` is `None` and the formula would
silently fall through to `aggregate_raw`. And it would close only the
categorical third of the defect, leaving `.missing` and the date parts exactly
where they are. Full costing in the report on improvise-8xhy.
On the test that was lying. `improvise_evaluation_matches_gp_tree_eval` could
not see any of this because its `improvise_eval` helper fabricates a
`_Measure` item for every feature name, including `outlook=rainy` -- it
asserts the emitter and `gp_tree::eval` agree against a model that could not
exist. The test is kept, because quoting and threshold routing are real things
it does check, and its doc now states its scope. The property it could not
state is asserted separately against a real document: if `fitted_formula` says
`Ok`, no cell of the target is a reference error. The characterization test
`a_split_on_a_one_hot_feature_does_not_evaluate` is flipped to
`a_split_on_a_one_hot_feature_is_refused`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|
||
|
Edward Langley
|
d03f20eaab |
fix(core): matching_cells/values yield insertion order, not hash order (improvise-shyt)
`DataStore::matching_cells` and `matching_values` walked the secondary index's `HashSet`, so their order was the hash order: different between processes, and different between two stores holding the same cells. That defeated the reason `DataStore::cells` is an `IndexMap` in the first place -- its own doc comment says "so records mode can display rows in the order they were entered" (gotcha 18) -- and made any `f64` fold over the result reproducible only within one run. The index exists for O(1) *lookup*, not for iteration, so it stays as what narrows the candidates and the primary map becomes what orders them: the new `matching_positions` resolves each surviving candidate to its `IndexMap` position with one hash lookup and sorts the positions. The shape stays O(smallest set x |partial|) with O(m log m) added over the *matches*, never a scan of the store. Measured on 18,000 cells (release, 2000 iterations/case), before -> after: matching_values 1 match 6.9us -> 7.3us matching_values 300 matches 9.0us -> 10.8us matching_values 3600 147us -> 165us matching_cells 300 matches 37.7us -> 35.0us matching_cells 3600 535us -> 500us Worst case +20% on the value-only path; `matching_cells` is slightly *faster*, sorted positions resolving CellKeys with better locality. Reached beyond records mode: the drill snapshot (`cmd/grid.rs`) froze its record list in the same unstable order, and `drill_state.pending_edits` are keyed by row index (gotcha 1), so the same drill opened twice could key staged edits to different records. No change needed there -- it inherits. The tests are built the way the instability shows. Asserting a literal order would only pin what this machine's hasher did today (doctrine check_the_checker), so one test compares against `iter_cells` -- the primary map's own order -- and one builds the same logical cells by two opposite insertion histories and requires each to list its own. `improvise-gp::dataset` sorts its rows by coordinate tuple as a workaround for this; the sort is kept but is no longer what buys stability, and its two comments saying otherwise are corrected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH |
||
|
Edward Langley
|
6ca8928100 |
docs(improvise-gp): a fit is persisted as its formula, grammar unchanged (improvise-7na)
Decision: `.improv` stores the emitted formula and no serialized
`gp_tree::data::Model`. `persistence/improv.pest` is explicitly confirmed
unchanged. Rationale on the crate docs.
Against a blob: it is a second copy of what the file already carries -- the
tree IS the formula -- with nothing keeping the two in agreement when a user
edits the formula or a cell it was fitted on, and a round-trip test over the
blob cannot see the drift (gotcha 41's second half; its first half, "a second
implementation of the grammar", does not transfer to an opaque production).
And `Tree`/`FitConfig` are gp-tree types, so improvise-io would gain a
gp-tree dependency -- the inversion this crate exists to prevent.
What the blob was for does not arise here: the encoding plan exists so
`fit_csv --predict` can score an untyped CSV row, while improvise's new row is
a record in the same document that `recompute_formulas` scores through the
stored formula. Seed + config are not written either -- gp-tree already
records them, and nothing reads them until the fit command (improvise-vx3)
exists to decide how provenance is worded.
Three tests carry the reasoning, and one of them refutes the ticket's premise.
"The emitted formula is enough to recompute existing cells" holds only for a
fit over numeric measures: a one-hot feature is named `Region=North`, an
encoding artefact that is not an item of the document, so `Expr::Ref` fails to
resolve and every cell of the fitted measure is `Error("ref:Region=North")`.
`col.missing` and the date sub-features are in the same position. Recorded as
a characterization test; the remedy belongs to improvise-vx3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|
||
|
Edward Langley
|
726168136e |
feat(improvise-gp): a gp-tree feature matrix from a View's records (improvise-4s8)
Records mode is already a feature matrix: a row is one coordinate tuple and
each measure is its own column (gotcha 1). `dataset::feature_table` enumerates
the same rows against the user's own view and lays them out as the string
table `gp_tree::data::preprocess` reads; `dataset::dataset` runs it through
preprocess so one-hot expansion, imputation and the feature ranges stay in
gp-tree.
Four selection rules settled here, each recorded on the module:
* rows are scoped by `View::filtering_cats()` (Page union Filter), not by
`categories_on(Axis::Page)` -- gotcha 27's trap, and the one that fails
silently.
* hidden items scope the rows and the feature columns ("fit what I can see");
an explicitly named target is exempt.
* every regular category is a feature whatever axis it sits on, including
Axis::None, because a coordinate is part of what makes a record distinct --
dropping it leaves two rows with equal feature vectors and different
targets. Label categories are excluded, as in records mode, by not being
`is_regular`.
* no formula-derived value enters the matrix at all: a formula target is not a
feature, and is refused *as* the target. One rule covers both halves of the
ticket's "the target, or a formula depending on it", with no dependency
analysis to get wrong.
Records are sorted by coordinate tuple: `DataStore::matching_cells` walks a
HashSet, so an unsorted table would make a seeded fit reproducible only within
one process. Numbers reach the table via `f64::to_string`, never `format_f64`
(gotcha 8). Nothing prints; skipped records come back in `warnings` (gotcha
37).
improvise-gp gains improvise-core as a real dependency (it was already a
dev-dependency).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|
||
|
Edward Langley
|
bcc0fac45a |
refactor(gp-tree): a fit reports where it started, and improvise-gp drops its loop
`improvise-gp`'s `fit_from_gen0` was a third copy of the GA loop, kept because
it also returned the generation-0 best — the one thing
`breeding_improves_on_generation_zero` needs to prove the search *moved* rather
than merely differed. Nothing in `gp_tree::fit` exposed that, so the
duplication was load-bearing.
It is not any more. `fit_detailed` returns a `Fit { tree, cost, start,
start_cost }`, where `start` is the best member of generation 0 — the
population as first seeded and scored, before a single `breed` — of the restart
that produced `tree`. `fit` is now a projection of it. "Did the search improve
on where it started" is a question any caller of a GA wants answerable: the
same seed reproducing the same answer and a different seed giving a different
one are both satisfied by a `breed` that returns its input untouched.
The loop keeps its shape: the generation scored at the top of iteration `i` is
still generation `i`, the sort is still stable (so member 0 wins its own ties
and generation 0's champion is the seed ancestor whenever nothing beat it), and
`breed` is called the same number of times with the same arguments. The cost is
one extra scoring pass per restart, which draws nothing from the generator.
The reproducibility evidence is what makes that claim, and it was re-established
rather than assumed: all six vectors in tests/scoring_known_answers.rs pass
unchanged, and the nine-scenario fit_csv run is byte-identical including the
saved model.json — so every seed already recorded still names the same tree.
Faults injected to show the new tests are not decorative: making `start` the
seed ancestor instead of generation 0's champion fails
`fit_detailed_starts_from_the_best_of_generation_zero`, and skipping the breed
fails `breeding_improves_on_generation_zero` at cost == start_cost.
improvise-ut7p
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|
||
|
Edward Langley
|
1c4683f8cb |
refactor(formula): give pipe quoting one home, in improvise-formula
The rule had three copies and two of them had already drifted:
improvise-io/src/persistence/quoting.rs has no keyword guard, the
improvise-gp copy does. improvise-formula owns the grammar that defines
what a bare name is and both other crates already depend on it, so the
writing half now lives beside the reading half as `quote::{quote_name,
is_bare_name}`. improvise-gp re-exports it and keeps its public surface.
Canonical behaviour: the keyword guard stays, so `SUM` is written
`|SUM|`. Probed first — the pest grammar does accept bare keywords in
every position (target, ref, aggregate argument, IF condition), so this
is not a correctness fix. It is accepted only because `agg_call` needs a
`(` and PEG backtracking falls through to `ref_expr`; that is rule
ordering, not a stated rule, and a pipe-quoted name is an identifier
everywhere with nothing to fall through. Cost is cosmetic.
improvise-io's copy is untouched — out of scope here, and switching it
would change `.improv` output for a keyword-named category. Reported as
a follow-up.
Faults injected to prove the new tests fire: dropping the keyword clause
fails a_keyword_is_quoted_even_though_it_looks_bare (and nothing else —
the parser round trip stays green, which is the probe's finding again);
dropping the `\\` escape fails the_escapes_match_the_grammar.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|
||
|
Edward Langley
|
a6e82a4870 |
fix(improvise-gp): refuse a non-finite threshold instead of emitting "inf"
number_text(f32::INFINITY) returned "inf", because "inf".parse::<f64>() succeeds and the round-trip loop accepted it — so a formula_text caller got text the grammar rejects with nothing on the way saying so. NaN fell through to unreachable!, blaming the decimal expansion rather than the caller's value. Both were documented as preconditions; now one assert states the precondition in code and names the value. Also corrected the doc claim, on the function and in the crate header: it emits the shortest decimal that ROUND-TRIPS to the same f64, not "the exact decimal value of the f32". The weaker claim is the true one and is all the routing argument needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH |
||
|
Edward Langley
|
6eff624292 |
test(gp-tree): pin SplitMix64 to the reference vectors
Nothing pinned the vendored RNG's constants: changing the gamma from 0x9E3779B97F4A7C15 to ...7C17 left all 27 workspace tests green, because every existing test only asserts that two runs at one seed agree. The crate's promise is stronger — a fit prints its seed to the user and saves it in data::Model, so a later run at an old seed must reproduce the old fit, and a silent cleanup of the RNG would invalidate every seed already recorded. Added known-answer tests over the published SplitMix64 vectors for seed 0 plus a non-zero seed (which also covers how the seed enters the state). They read the generator through a new SplitMix64::next_u64, the step `range` was already doing inline. Also strengthened the determinism pair, which proved less than it looked like: a GA whose breed returned its input unchanged would still fit different formulas at different seeds, because the initial random_tree already differs. breeding_improves_on_generation_zero compares the fit's cost against the best member of generation 0 — verified by stubbing breed to a no-op, which leaves both determinism tests green and fails only the new one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH |
||
|
Edward Langley
|
2b1ca2e4c4 |
fix(formula): unquote a pipe-quoted formula target
`build_formula` took the target's raw span, so `|Total Revenue| = ...`
parsed to `Formula { target: "|Total Revenue|" }` — delimiters included —
while an `Expr::Ref` for the same name came back unquoted. The target
names a `_Measure` item, so `Model::add_formula` created an item named
literally `|Total Revenue|`: the formula wrote into a ghost column, the
user's own `Total Revenue` item stayed empty, and nothing referring to
the target by name resolved.
Both user-facing entry points hit this: the TUI formula minibuffer
(ui/effect.rs AddFormula) and `.improv` load (persistence/parse.rs).
No caller relied on the raw span — the writer emits `Formula::raw`, so
file round-tripping is unaffected, and the `_Measure` exclusion set in
persistence/format.rs now matches the real item instead of the ghost.
Fixed in improvise-formula rather than at the three call sites: the
target and a reference are the same syntactic identifier, so unquoting
them differently is the defect itself, and three copies of unquote_pipe
would be three chances to drift.
The grammar comment that documented the raw span is updated, and the two
tests that asserted the pipes (parser.rs, improvise-gp) now assert the
semantic round trip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|
||
|
Edward Langley
|
3d1bf16984 |
feat(gp-tree): vendor a seeded SplitMix64 and surface the seed on every fit
improvise-lzn. `Rng` is a trait so the crate pulls in no generator, and every caller then wrote out the same SplitMix64 — gp-tree's tests, fit_csv, and next improvise. It is now `gp_tree::SplitMix64`, ~20 lines and still no dependency. Taking on `rand` would buy statistical quality this does not need: the point is that a fit, which writes a rule into a user's document, re-runs identically on the same data, and that a bug report is reconstructible from its seed. So the seed is not hidden. `SplitMix64::seed()` keeps the value the run started from (a generator that has advanced past its seed cannot answer that), fit_csv takes `--seed <n>` (decimal or 0x-hex), prints it on every fit, and `--save` records it in `data::Model`. That field is `#[serde(default)]`, so a model saved before this still loads. Default-seed output is unchanged apart from the new seed line — diffed against the pre-change baseline. Tests: gp-tree's own tests now use SplitMix64 (12 more via data.rs, unchanged otherwise), and improvise-gp asserts the acceptance property directly — a small GA fit run twice under one seed emits a character-identical formula, and a different seed emits a different one, so "reproducible" cannot be satisfied by a search that never moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH |
||
|
Edward Langley
|
2a0b937447 |
feat(improvise-gp): emit a fitted gp-tree Tree as improvise formula text
improvise-izk. New crate `improvise-gp` — the seam between gp-tree and the
formula language. It cannot live in gp-tree (that inverts the dependency) and
it cannot live in improvise-formula (which must not know GP exists).
`formula_text(tree, spec, target)` turns Split{feat,thresh,lo,hi} into
`IF(<name> < <thresh>, <lo>, <hi>)` and a Leaf into its number;
`fitted_formula` hands back the parsed Formula, so the parser is the emitter's
own contract check.
Two things the emitter owes its caller:
* Names round-trip. Feature names come from user data — spaces, operators,
pipes, keyword collisions, the one-hot encoder's own `col=value` labels —
so every name goes through CL-style pipe quoting, and a keyword is quoted
even though it would parse bare.
* Thresholds route identically. A threshold is emitted as the f32's *exact*
decimal, not its shortest round-trip form. `0.7f32` is 0.699999988…, and
"0.7" parses as an f64 strictly greater than it, so a row whose feature
value equals the threshold would fall on the other side of the split in
formula evaluation than it does in `gp_tree::eval`. There is a test for
that boundary, and it fails against the naive rendering.
Tests: parse round-trip for leaf/nested/awkward-name/awkward-target cases, the
threshold-boundary regression, and the acceptance property — over random trees
and random feature vectors, `Model::evaluate` of the emitted formula equals
`gp_tree::eval`, exactly (f32→f64 is lossless, so no tolerance is needed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
|