Merge remote-tracking branch 'gh/main' into wave/command-tui-gaps
This commit is contained in:
+64
-14
@@ -373,20 +373,70 @@ Two of the other three are declined permanently: a one-hot over a measure and
|
||||
the `(missing)` pseudo-item name no coordinate *by construction*, so no
|
||||
vocabulary closes them. They are refusals rather than gaps.
|
||||
|
||||
**`NumericCategory` is not, and this paragraph said it was** — which is the
|
||||
more useful half of improvise-wd6n, because prose that calls something a
|
||||
permanent refusal is what stops anyone filing the ticket. `fit_measure` hands
|
||||
`preprocess` an `Overrides::default()`, so it never says which header columns
|
||||
are document categories, although the very next line tells `Features::of`.
|
||||
`preprocess` then infers `Numeric` → `Date` → `Categorical` from the cell
|
||||
values, so a `Year` category with items `2024`/`2025` becomes a *numeric column*
|
||||
and a category of ISO dates becomes a *date column* — and both are refused,
|
||||
`Year IS 2024` being exactly spellable all along. That needs no
|
||||
formula-language change whatever: `Forced::Categorical` already exists and is
|
||||
the knob. Filed as **improvise-zv8t**. The same accident is why a `DatePart`
|
||||
can arrive over a category column, so the date family's decline above is
|
||||
narrower than it looks: it covers a genuine date *measure*, not a category
|
||||
misread as one.
|
||||
**`NumericCategory` was not, and improvise-zv8t has closed it.** That it was
|
||||
*called* one is the more useful half of improvise-wd6n: prose declaring
|
||||
something a permanent refusal is what stops anyone filing the ticket, and
|
||||
nothing but a reader could have caught it — the suite was green either way.
|
||||
|
||||
The accident was one argument. `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 cell 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 then 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.
|
||||
|
||||
Three things follow, and the last is a cost rather than a win.
|
||||
|
||||
**The date decline above now means what it says.** A `DatePart` could arrive
|
||||
over a *category* column by exactly this accident, so §11's date family was
|
||||
narrower than it read. It now covers a genuine date **measure** and nothing
|
||||
else, which is the scope the decline was always argued for.
|
||||
|
||||
**The `Artefact` arms stay, and stay correct.** `crate::dataset` is `pub` and
|
||||
takes the caller’s `Overrides`, and `Features::of` reads roles off whatever
|
||||
`Dataset` it is handed — so a caller that builds one without forcing still
|
||||
produces a `Numeric` role over a category column, and `fitted_formula` still has
|
||||
to refuse it. The arm is unreachable from `fit_measure`, which is the only entry
|
||||
point a *document* goes through; it is not unreachable code, and deleting it
|
||||
would be deriving the classification from one caller’s habits rather than from
|
||||
the roles.
|
||||
|
||||
**Forcing widens the encoding, and that costs a document.** A `Year` category
|
||||
with two items was one numeric feature and is now two one-hots; with a hundred
|
||||
items it is a hundred, and `MAX_FEATURES` refuses the fit outright.
|
||||
|
||||
The first draft of this paragraph claimed "no document that fitted before stops
|
||||
fitting", on the reasoning that a tree splitting on such a column was refused as
|
||||
an artefact anyway. **That is false, and the review that caught it was of this
|
||||
prose rather than of the diff.** The refusal is conditioned on `used_features`,
|
||||
so a fit that simply *never split* on the numeric category succeeded — and it
|
||||
is precisely the wide categories that a fit has least reason to split on.
|
||||
Measured, on a hundred records with a hundred-item numeric `Year` beside a
|
||||
`Cost` that predicts `Score` on its own: before, the fit lands on
|
||||
`IF(Cost < 48.0…)` and `Year` is never touched; after, the preflight refuses
|
||||
at 101 features. `a_wide_numeric_category_now_refuses_a_fit_that_used_to_ignore_it`
|
||||
pins both halves, including that the repair the message names really works.
|
||||
|
||||
It is still the right trade, for a reason that is *not* "both refuse". The old
|
||||
success was accidental: `Year` was encoded as a single continuous number over
|
||||
1900–1999, an encoding that means nothing and that the fit could only use by
|
||||
producing a formula it would then be refused for. The document now gets a
|
||||
refusal that names the repair — mark the category a Label — and marking it
|
||||
restores the pre-zv8t feature set exactly. What is lost is the case where the
|
||||
user wanted neither: a wide numeric category left alone while the fit uses the
|
||||
measures. That is filed as **improvise-e8no**; the honest fix is a budget that
|
||||
can decline *a column* rather than the whole table, which is a change to what
|
||||
`encoded_width` returns and not to this decision.
|
||||
|
||||
The preflight `encoded_width` had to learn the same slice, or it counted such a
|
||||
column as one feature and handed the refusal to the exact check *after*
|
||||
materialising the matrix — which is the side of the cost the preflight exists
|
||||
to stay on (§14).
|
||||
|
||||
**And the refusal's own words must not name a column kind the arm does not
|
||||
know.** `FeatureRole::Missing` is *not* conditioned on `is_category` — unlike
|
||||
|
||||
@@ -731,3 +731,40 @@ Case counts went 500 → 256 in the same file, which is the default
|
||||
a further 0.39s → 0.21s but it is not the fix, and reporting it as one would
|
||||
be the mistake this entry exists to record. Net: 1.42s → 0.21s, and
|
||||
`improvise-io` is no longer the run's longest binary.
|
||||
|
||||
### export_csv_omits_the_rows_the_view_prunes
|
||||
|
||||
improvise-iot6. `render_csv` built its layout with `GridLayout::new`, which does
|
||||
**not** apply `prune_empty` — only `GridLayout::with_frozen_records` does. So
|
||||
`:export` in the TUI and the CLI export path both wrote rows the grid does not
|
||||
draw. User-visible, in production, and shipping for as long as the two
|
||||
constructors have differed.
|
||||
|
||||
This is the **fourth** instance of the same trap; the third was
|
||||
`project::grid_in`, fixed in PR #78, and this fix is that one line for line.
|
||||
No fault needed injecting: on the unfixed tree the test fails with
|
||||
|
||||
> CSV wrote a row the grid does not draw: left `["Food", "Gas", "Rent"]`,
|
||||
> right `["Food", "Gas"]`
|
||||
|
||||
**Why the fixture is discriminating, which is the whole difficulty here.** The
|
||||
sibling property written for improvise-t6db passed on the buggy tree in its
|
||||
first version, because it made its two views differ by turning pruning *off* —
|
||||
plenty of differing config, and vacuous, since with nothing to prune both code
|
||||
paths agree. So this fixture does two things that one cannot fake:
|
||||
|
||||
- it contains a row that is **actually empty** (`Rent`, an item of `Type` that
|
||||
no cell names) and leaves `prune_empty` **ON**;
|
||||
- it asserts the precondition in both directions before exporting anything —
|
||||
`with_frozen_records` draws 2 rows and `new` holds 3. If a future change makes
|
||||
pruning a no-op, or makes the two constructors agree, the precondition fails
|
||||
and says so rather than the assertion passing vacuously.
|
||||
|
||||
None of `export.rs`'s three pre-existing tests mentions pruning at all, which is
|
||||
exactly why this shipped.
|
||||
|
||||
**Both axes, because `prune_empty` prunes both.** A first version of this
|
||||
fixture had a single `Month`, so it demonstrated the row half and left the
|
||||
column half asserted by nothing -- the same defect one axis over. `Feb` is now
|
||||
the empty column, the precondition is a pair on each layout ((2, 1) drawn
|
||||
against (3, 2) unpruned), and the CSV header is asserted to carry `Jan` alone.
|
||||
|
||||
+141
-5
@@ -606,10 +606,146 @@ The fixture reaches three families from one `preprocess` call (a text measure
|
||||
column, a measure with a hole in it, a column of ISO dates), which is why the
|
||||
module is named for what it reaches rather than for "each family". The
|
||||
remaining two are not reachable from it and are not asserted here.
|
||||
`AbsentCoordinate` is a permanent refusal; `NumericCategory` is **not**, and
|
||||
`AbsentCoordinate` is a permanent refusal; `NumericCategory` was **not**, and
|
||||
the first draft of this entry and of `gp.md` §11 both called it one — review
|
||||
found that `fit_measure` simply never tells `preprocess` which columns are
|
||||
categories, so it is a filed gap (improvise-zv8t) rather than a refusal. Worth
|
||||
recording as the failure mode: the branch that *invented* a classification also
|
||||
wrote the prose declaring one of its arms unfixable, and nothing but a reader
|
||||
could have caught that.
|
||||
categories. improvise-zv8t has since closed it (below). Worth recording as the
|
||||
failure mode: the branch that *invented* a classification also wrote the prose
|
||||
declaring one of its arms unfixable, and nothing but a reader could have caught
|
||||
that — the suite was green on either reading.
|
||||
|
||||
### fit::tests::a_category_whose_items_are_numbers_is_still_spelled_as_a_coordinate
|
||||
### fit::tests::a_category_whose_items_are_dates_is_still_spelled_as_a_coordinate
|
||||
|
||||
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` infers `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. User-visible: a fit over such a document could not be stored at all.
|
||||
|
||||
No fault had to be injected, because the bug was live. On the unfixed tree both
|
||||
tests fail inside `fit_measure` with the real refusals, which are the two
|
||||
arms the ticket named and are worth quoting:
|
||||
|
||||
- `["Year"] names a category whose items all parse as numbers, so a reference
|
||||
to it would resolve to a `_Measure` item of the same name`
|
||||
- `["Year.epoch_day", "Year.year"] is a part of a date, and date arithmetic is
|
||||
something improvise does not have anywhere`
|
||||
|
||||
The second is why `gp.md` §11's date decline was narrower than it read.
|
||||
|
||||
**Why each fixture is discriminating.** `year_document` gives the model **one**
|
||||
regular category and **no other feature column**, and makes the target a
|
||||
function of that category alone — so a tree that does not split on `Year` is a
|
||||
poor fit and the GA does not settle for one. That matters because the refusal is
|
||||
conditioned on `used_features`: a fit whose champion happened to be a bare leaf
|
||||
would be *accepted* on the unfixed tree and prove nothing. Asserting
|
||||
`formula.raw.contains("Year IS")` rather than merely `is_ok()` closes that hole
|
||||
in both directions. Note what the unfixed tree actually does, because a first
|
||||
draft of this line got it wrong: it produces **no text at all** —
|
||||
`fitted_formula` refuses before emitting, which is what the messages quoted
|
||||
above are. The only unfixed path that emits anything is the bare-leaf champion
|
||||
named in the previous paragraph, and its text is `SUM(<constant>)`, carrying no
|
||||
`Year` in any spelling. So the assertion closes the hole; "it would have said
|
||||
`Expr::Ref(Year)`" was a guess about a branch that does not run.
|
||||
|
||||
The numeric test then adds the property the refusal exists to protect: the
|
||||
formula goes in through `add_formula` and
|
||||
every record evaluates to a `Number`, not `Error("ref:…")` (improvise-8xhy).
|
||||
|
||||
### fit::tests::the_preflight_counts_a_numeric_category_at_its_cardinality
|
||||
|
||||
The other half of improvise-zv8t, and the one that is a *cost* rather than a
|
||||
fix. Forcing a category categorical widens it from one numeric feature to one
|
||||
per distinct value, so `encoded_width` — the cheap preflight that exists to
|
||||
refuse a too-wide table **before** `preprocess` materialises it (§14) — had to
|
||||
learn the same `header[..category_columns]` slice, or it would under-count
|
||||
exactly the column this ticket is about.
|
||||
|
||||
Fault injected: the `col >= category_columns` clause dropped from `encoded_width`,
|
||||
leaving the pre-ticket inference. The test fails by name, and the failure message
|
||||
is the evidence — it shows the **exact** check speaking where the preflight
|
||||
should:
|
||||
|
||||
> the PREFLIGHT must refuse it, before anything is materialised: fitting "Score"
|
||||
> encoded 70 features, over the 64 this host will search.
|
||||
|
||||
A test asserting only `expect_err` would have passed under that injection, since
|
||||
both guards refuse; the discriminator is *which message*, because the two guards
|
||||
differ in what they have already paid for. The fixture also asserts its own
|
||||
precondition — that `Year` really is the header's first column — so a
|
||||
`feature_table` change that reorders the header fails here rather than silently
|
||||
slicing the wrong names.
|
||||
|
||||
### fit::tests::a_wide_numeric_category_now_refuses_a_fit_that_used_to_ignore_it
|
||||
|
||||
The **cost** of improvise-zv8t, pinned rather than argued — and it exists
|
||||
because the first draft of `gp.md` §11 claimed there was none. That draft said
|
||||
"no document that fitted before stops fitting", reasoning that a tree splitting
|
||||
on a numeric category was refused as an artefact anyway. The refusal is
|
||||
conditioned on `used_features`, so a fit that never split on the column
|
||||
succeeded, and the wide columns are exactly the ones a fit has least reason to
|
||||
split on.
|
||||
|
||||
Measured both ways with a throwaway `#[ignore]` probe before this test was
|
||||
written, on a fixture of 100 records with a 100-item numeric `Year` beside a
|
||||
`Cost` that predicts `Score` on its own:
|
||||
|
||||
- pre-fix: `FIT LANDS -> |Score (fit)| = SUM(IF(Cost < 48.0150032043457, 970.3…, 3684.7…))`
|
||||
— `Year` never appears.
|
||||
- post-fix: `REFUSED -> fitting "Score" would encode 101 features from 100
|
||||
records`.
|
||||
|
||||
So no fault needed injecting; the *change itself* is what the test observes, and
|
||||
it fails on either side if the behaviour moves. The second half is what keeps it
|
||||
from being a bare characterization: it marks `Year` a `Label` and asserts the
|
||||
fit then lands on `Cost`, which is the repair the refusal's own message names.
|
||||
A test asserting only the refusal would leave that advice unchecked — and advice
|
||||
in an error message is a promise, so it is the half worth asserting.
|
||||
|
||||
The generalisable part is the review finding, not the test: **prose claiming a
|
||||
change has no cost is a claim, and it is checkable.** The suite was green with
|
||||
the false sentence in place, exactly as it was green with improvise-wd6n's
|
||||
"permanent refusal" — the same section, the same failure mode, one paragraph
|
||||
apart. See improvise-e8no for the case the trade genuinely loses.
|
||||
|
||||
### fit::tests::a_duplicate_column_name_is_refused_before_the_matrix_is_built
|
||||
|
||||
Found by adversarial review of improvise-zv8t, and **introduced by it**:
|
||||
`as_categories` forces by column **name**, while `encoded_width` counts by
|
||||
column **index**. A document may legally name a regular category and a raw
|
||||
measure the same thing -- `a_category_and_a_measure_of_the_same_name_are_refused`
|
||||
already ships that fixture -- and then the *measure* column is forced
|
||||
categorical by the name match while the preflight still counts it numeric by
|
||||
index.
|
||||
|
||||
Measured on the fixture here (40 records, a 40-item `Dup` category beside a
|
||||
`Dup` measure): the preflight answers **41** where forcing materialises **80**.
|
||||
The refusal that catches the document, `no_ambiguous_column_names`, ran *after*
|
||||
`preprocess`, so the expansion the preflight exists to prevent was allocated and
|
||||
only then refused. At the default `max_records = 20_000` that is a 20k-row
|
||||
one-hot matrix built before the error -- `doctrine(guard_the_cost_not_the_result)`
|
||||
defeated by an inconsistency the ticket itself introduced. The bounded drift
|
||||
that `doctrine(a_budget_check_is_advisory_or_exact_and_must_say_which)` licenses
|
||||
is *approximation*, not this.
|
||||
|
||||
**The repair is a data dependency, not a moved line.** `as_categories` now runs
|
||||
the check itself and returns `Result<Overrides>`, so the `Overrides` that
|
||||
`preprocess` requires cannot exist until the header has been accepted. That is
|
||||
`doctrine(an_invariant_is_made_unrepresentable_not_validated)` -- the answer
|
||||
`prune_axis_items`, `Row<T>` and `Checked` each got.
|
||||
|
||||
**So the regression guard is the signature, and the injection is a compile
|
||||
error.** Swapping the two statements back gives
|
||||
`error[E0425]: cannot find value 'overrides' in this scope`, verified. What that
|
||||
does *not* cover, stated rather than implied: the check could still be deleted
|
||||
from `as_categories` deliberately. The accidental reordering -- the way this
|
||||
actually happened -- is what is now unrepresentable.
|
||||
|
||||
The test therefore asserts the **hazard** rather than the ordering: that the
|
||||
fixture really names one column twice, that the preflight's 41 really disagrees
|
||||
with what forcing would build, and that `as_categories` itself refuses. A test
|
||||
asserting only the error message would pass on either ordering, since the
|
||||
message is identical -- the whole defect was in work done, not in the result.
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
//! One fit, end to end: a [`View`]'s records in, an improvise [`Formula`] and the
|
||||
//! evidence for believing it out. It never prints — `context/code/gp.md` §14.
|
||||
|
||||
use gp_tree::data::{Overrides, Target, preprocess};
|
||||
use gp_tree::data::{Forced, Overrides, Target, preprocess};
|
||||
use gp_tree::fit::{
|
||||
FitConfig, RegressionQuality, cv_regress, denormalize, fit, importance_ranking, mse,
|
||||
permutation_importance, regression_quality, standardize,
|
||||
@@ -15,7 +15,7 @@ use crate::dataset::{FeatureTable, feature_table};
|
||||
|
||||
/// A cheap upper bound on how wide `preprocess` would expand this table, computed
|
||||
/// without expanding it. **Advisory, not authoritative** — `context/code/gp.md` §14.
|
||||
fn encoded_width(table: &FeatureTable) -> usize {
|
||||
fn encoded_width(table: &FeatureTable, category_columns: usize) -> usize {
|
||||
(0..table.header.len().saturating_sub(1))
|
||||
.map(|col| {
|
||||
let cells: Vec<&str> = table
|
||||
@@ -30,7 +30,12 @@ fn encoded_width(table: &FeatureTable) -> usize {
|
||||
.filter(|c| !gp_tree::data::is_missing(c))
|
||||
.collect();
|
||||
let has_missing = present.len() < cells.len();
|
||||
match !present.is_empty() && present.iter().all(|c| c.parse::<f64>().is_ok()) {
|
||||
// A document category is forced categorical below, so the preflight
|
||||
// counts it by cardinality too. Where the two still differ: gp.md §14.
|
||||
let numeric = col >= category_columns
|
||||
&& !present.is_empty()
|
||||
&& present.iter().all(|c| c.parse::<f64>().is_ok());
|
||||
match numeric {
|
||||
// A numeric column is one feature, plus the `.missing` flag.
|
||||
true => 1 + usize::from(has_missing),
|
||||
// Everything else: one column per distinct value.
|
||||
@@ -44,6 +49,19 @@ fn encoded_width(table: &FeatureTable) -> usize {
|
||||
.sum()
|
||||
}
|
||||
|
||||
/// Force every document category to be encoded as one, rather than inferred from its
|
||||
/// values. Refuses an ambiguous header first — `context/code/gp.md` §11 and §14.
|
||||
fn as_categories(header: &[String], category_columns: usize) -> anyhow::Result<Overrides> {
|
||||
no_ambiguous_column_names(header)?;
|
||||
Ok(Overrides {
|
||||
force: header[..category_columns]
|
||||
.iter()
|
||||
.map(|name| (name.clone(), Forced::Categorical))
|
||||
.collect(),
|
||||
..Overrides::default()
|
||||
})
|
||||
}
|
||||
|
||||
/// The same text with every CR and LF turned into a space, so a name a document may
|
||||
/// legally hold cannot split a one-line status bar. Public: the refusals need it too.
|
||||
pub fn one_line(text: String) -> String {
|
||||
@@ -216,7 +234,8 @@ pub fn fit_measure(
|
||||
table.rows.len(),
|
||||
request.max_records,
|
||||
);
|
||||
let width = encoded_width(&table);
|
||||
let category_columns = model.regular_category_names().len();
|
||||
let width = encoded_width(&table, category_columns);
|
||||
anyhow::ensure!(
|
||||
width <= MAX_FEATURES,
|
||||
"fitting {target:?} would encode {width} features from {} records, which is too wide \
|
||||
@@ -225,11 +244,10 @@ pub fn fit_measure(
|
||||
table.rows.len(),
|
||||
);
|
||||
|
||||
let category_columns = model.regular_category_names().len();
|
||||
let header = table.header.clone();
|
||||
let mut data = preprocess(&table.header, &table.rows, &Overrides::default());
|
||||
let overrides = as_categories(&header, category_columns)?;
|
||||
let mut data = preprocess(&table.header, &table.rows, &overrides);
|
||||
data.warnings.splice(0..0, table.warnings);
|
||||
no_ambiguous_column_names(&header)?;
|
||||
let features = crate::Features::of(&data, &header[..category_columns]);
|
||||
let rows = regression_rows(data.target, target)?;
|
||||
|
||||
@@ -501,6 +519,273 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// Bug improvise-zv8t: `fit_measure` handed `preprocess` an `Overrides::default()`,
|
||||
/// so a numeric-valued category was inferred numeric. Evidence: `context/test-evidence/gp.md`.
|
||||
#[test]
|
||||
fn a_category_whose_items_are_numbers_is_still_spelled_as_a_coordinate() {
|
||||
let (mut model, view) = numeric_category_document();
|
||||
let outcome =
|
||||
fit_measure(&model, &view, "Score", FIT_INTO, &request()).expect("the fit lands");
|
||||
assert!(
|
||||
outcome.formula.raw.contains("Year IS"),
|
||||
"the only predictor is a category; the fit spelled it {:?}",
|
||||
outcome.formula.raw
|
||||
);
|
||||
// …and the spelling really resolves: every cell a number, not `Error("ref:…")`.
|
||||
model.add_formula(outcome.formula.clone());
|
||||
model.recompute_formulas(&[]);
|
||||
for i in 0..12u32 {
|
||||
match model.evaluate(&year_cell(&format!("r{i}"), &outcome.formula.target)) {
|
||||
Some(CellValue::Number(_)) => {}
|
||||
other => panic!("record r{i} came back as {other:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// The same accident let a category of ISO dates arrive as a `DatePart`, so
|
||||
/// `gp.md` §11’s date decline covers a date *measure*, not a misread category.
|
||||
#[test]
|
||||
fn a_category_whose_items_are_dates_is_still_spelled_as_a_coordinate() {
|
||||
let (model, view) = date_category_document();
|
||||
let outcome =
|
||||
fit_measure(&model, &view, "Score", FIT_INTO, &request()).expect("the fit lands");
|
||||
assert!(
|
||||
outcome.formula.raw.contains("Year IS"),
|
||||
"a category of ISO dates must stay a coordinate: {:?}",
|
||||
outcome.formula.raw
|
||||
);
|
||||
}
|
||||
|
||||
/// The preflight must count a category the way `preprocess` will now be told to
|
||||
/// encode it, or it under-counts exactly the column improvise-zv8t is about.
|
||||
#[test]
|
||||
fn the_preflight_counts_a_numeric_category_at_its_cardinality() {
|
||||
let (model, view) = year_document(&WIDE_YEARS);
|
||||
let table = feature_table(&model, &view, "Score").expect("the table builds");
|
||||
assert_eq!(
|
||||
table.header.first().map(String::as_str),
|
||||
Some("Year"),
|
||||
"the fixture must really put the category first, or the slice is wrong"
|
||||
);
|
||||
let err = fit_measure(&model, &view, "Score", FIT_INTO, &request())
|
||||
.expect_err("a category with a value per record is too wide");
|
||||
let msg = err.to_string();
|
||||
assert!(
|
||||
msg.contains("would encode"),
|
||||
"the PREFLIGHT must refuse it, before anything is materialised: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
/// One distinct numeric `Year` per record, well past `MAX_FEATURES`.
|
||||
const WIDE_YEARS: [&str; 70] = [
|
||||
"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16",
|
||||
"17", "18", "19", "20", "21", "22", "23", "24", "25", "26", "27", "28", "29", "30", "31",
|
||||
"32", "33", "34", "35", "36", "37", "38", "39", "40", "41", "42", "43", "44", "45", "46",
|
||||
"47", "48", "49", "50", "51", "52", "53", "54", "55", "56", "57", "58", "59", "60", "61",
|
||||
"62", "63", "64", "65", "66", "67", "68", "69", "70",
|
||||
];
|
||||
|
||||
/// One **Regular** category `Year`, no other feature column, and a target the
|
||||
/// category alone determines. `items` are the coordinate values it carries.
|
||||
fn numeric_category_document() -> (Model, View) {
|
||||
year_document(&["2024", "2025"])
|
||||
}
|
||||
|
||||
fn date_category_document() -> (Model, View) {
|
||||
year_document(&["2024-01-01", "2025-01-01"])
|
||||
}
|
||||
|
||||
fn year_document(items: &[&str]) -> (Model, View) {
|
||||
let mut model = Model::new("sales");
|
||||
model.add_category("Year").expect("under the limit");
|
||||
model.add_category(TAG).expect("under the limit");
|
||||
for item in items {
|
||||
model
|
||||
.category_mut("Year")
|
||||
.expect("just added")
|
||||
.add_item(*item);
|
||||
}
|
||||
model.category_mut(TAG).expect("just added").kind = CategoryKind::Label;
|
||||
let records = items.len().max(12) as u32;
|
||||
for i in 0..records {
|
||||
model
|
||||
.category_mut(TAG)
|
||||
.expect("just added")
|
||||
.add_item(format!("r{i}"));
|
||||
}
|
||||
model
|
||||
.category_mut(MEASURE)
|
||||
.expect("_Measure always exists")
|
||||
.add_item("Score");
|
||||
for i in 0..records {
|
||||
let year = items[(i as usize) % items.len()];
|
||||
model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Year".to_string(), year.to_string()),
|
||||
(TAG.to_string(), format!("r{i}")),
|
||||
(MEASURE.to_string(), "Score".to_string()),
|
||||
]),
|
||||
CellValue::Number(if i % 2 == 0 { 1000.0 } else { 4000.0 }),
|
||||
);
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
}
|
||||
view.set_axis("Year", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
(model, view)
|
||||
}
|
||||
|
||||
fn year_cell(tag: &str, measure: &str) -> CellKey {
|
||||
CellKey::new(vec![
|
||||
("Year".to_string(), "2024".to_string()),
|
||||
(TAG.to_string(), tag.to_string()),
|
||||
(MEASURE.to_string(), measure.to_string()),
|
||||
])
|
||||
}
|
||||
|
||||
/// The cost improvise-zv8t pays, pinned rather than argued: a wide numeric
|
||||
/// category that a fit used to IGNORE is now refused. `context/code/gp.md` §11.
|
||||
#[test]
|
||||
fn a_wide_numeric_category_now_refuses_a_fit_that_used_to_ignore_it() {
|
||||
let (model, view) = wide_category_beside_a_good_measure();
|
||||
let err = fit_measure(&model, &view, "Score", FIT_INTO, &request())
|
||||
.expect_err("101 one-hots is over the width budget");
|
||||
let msg = err.to_string();
|
||||
assert!(msg.contains("would encode 101 features"), "{msg}");
|
||||
// The message must name the repair, because this document fitted before:
|
||||
// marking `Year` a Label restores the pre-zv8t encoding exactly.
|
||||
assert!(msg.contains("mark such a category as a Label"), "{msg}");
|
||||
let (mut model, view) = wide_category_beside_a_good_measure();
|
||||
model.category_mut("Year").expect("Year exists").kind = CategoryKind::Label;
|
||||
let outcome = fit_measure(&model, &view, "Score", FIT_INTO, &request())
|
||||
.expect("the repair the message names must actually work");
|
||||
assert!(
|
||||
outcome.formula.raw.contains("Cost"),
|
||||
"the fit must land on the measure it always used: {:?}",
|
||||
outcome.formula.raw
|
||||
);
|
||||
}
|
||||
|
||||
/// 100 distinct numeric `Year` items beside a `Cost` that predicts `Score`
|
||||
/// on its own — so a fit has no reason to touch `Year` at all.
|
||||
fn wide_category_beside_a_good_measure() -> (Model, View) {
|
||||
let mut model = Model::new("sales");
|
||||
model.add_category("Year").expect("under the limit");
|
||||
model.add_category(TAG).expect("under the limit");
|
||||
model.category_mut(TAG).expect("just added").kind = CategoryKind::Label;
|
||||
for i in 0..100u32 {
|
||||
model
|
||||
.category_mut("Year")
|
||||
.expect("just added")
|
||||
.add_item(format!("{}", 1900 + i));
|
||||
model
|
||||
.category_mut(TAG)
|
||||
.expect("just added")
|
||||
.add_item(format!("r{i}"));
|
||||
}
|
||||
let m = model.category_mut(MEASURE).expect("_Measure always exists");
|
||||
for item in ["Cost", "Score"] {
|
||||
m.add_item(item);
|
||||
}
|
||||
for i in 0..100u32 {
|
||||
let cost = f64::from(i);
|
||||
for (measure, value) in [
|
||||
("Cost", cost),
|
||||
("Score", if cost > 50.0 { 4000.0 } else { 1000.0 }),
|
||||
] {
|
||||
model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Year".to_string(), format!("{}", 1900 + i)),
|
||||
(TAG.to_string(), format!("r{i}")),
|
||||
(MEASURE.to_string(), measure.to_string()),
|
||||
]),
|
||||
CellValue::Number(value),
|
||||
);
|
||||
}
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
}
|
||||
view.set_axis("Year", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
(model, view)
|
||||
}
|
||||
|
||||
/// The review finding on improvise-zv8t: forcing is by NAME and the preflight
|
||||
/// counts by INDEX. Evidence: `context/test-evidence/gp.md`.
|
||||
#[test]
|
||||
fn a_duplicate_column_name_is_refused_before_the_matrix_is_built() {
|
||||
let (model, view) = a_category_sharing_a_measures_name();
|
||||
let table = feature_table(&model, &view, "Score").expect("the table builds");
|
||||
assert_eq!(
|
||||
table.header.iter().filter(|h| h.as_str() == "Dup").count(),
|
||||
2,
|
||||
"the fixture must really produce two columns named Dup"
|
||||
);
|
||||
// The hazard, stated: the preflight's count and what forcing would
|
||||
// actually materialise disagree, because one reads names and one indices.
|
||||
assert_eq!(
|
||||
super::encoded_width(&table, 1),
|
||||
41,
|
||||
"the preflight's answer"
|
||||
);
|
||||
assert!(
|
||||
super::as_categories(&table.header, 1).is_err(),
|
||||
"the overrides `preprocess` needs cannot be built for this header"
|
||||
);
|
||||
let err = fit_measure(&model, &view, "Score", FIT_INTO, &request())
|
||||
.expect_err("a name that means two columns is not a fit");
|
||||
assert!(
|
||||
err.to_string().contains("could not say which one it meant"),
|
||||
"{err}"
|
||||
);
|
||||
}
|
||||
|
||||
/// A regular category and a raw measure both named `Dup`, one distinct category
|
||||
/// value per record — so forcing by name would expand the measure per row.
|
||||
fn a_category_sharing_a_measures_name() -> (Model, View) {
|
||||
let mut model = Model::new("sales");
|
||||
model.add_category("Dup").expect("under the limit");
|
||||
model.add_category(TAG).expect("under the limit");
|
||||
model.category_mut(TAG).expect("just added").kind = CategoryKind::Label;
|
||||
for i in 0..40u32 {
|
||||
model
|
||||
.category_mut("Dup")
|
||||
.expect("just added")
|
||||
.add_item(format!("d{i}"));
|
||||
model
|
||||
.category_mut(TAG)
|
||||
.expect("just added")
|
||||
.add_item(format!("r{i}"));
|
||||
}
|
||||
let m = model.category_mut(MEASURE).expect("_Measure always exists");
|
||||
for item in ["Dup", "Score"] {
|
||||
m.add_item(item);
|
||||
}
|
||||
for i in 0..40u32 {
|
||||
for (measure, value) in [("Dup", f64::from(i)), ("Score", f64::from(i) * 2.0)] {
|
||||
model.set_cell(
|
||||
CellKey::new(vec![
|
||||
("Dup".to_string(), format!("d{i}")),
|
||||
(TAG.to_string(), format!("r{i}")),
|
||||
(MEASURE.to_string(), measure.to_string()),
|
||||
]),
|
||||
CellValue::Number(value),
|
||||
);
|
||||
}
|
||||
}
|
||||
let mut view = View::new("Default");
|
||||
for cat in model.category_names() {
|
||||
view.on_category_added(cat);
|
||||
}
|
||||
view.set_axis("Dup", Axis::Row);
|
||||
view.set_axis(MEASURE, Axis::Page);
|
||||
(model, view)
|
||||
}
|
||||
|
||||
/// gotcha 37b: the seed alone does not reproduce a fit, so the outcome
|
||||
/// carries the config beside it — and the pair really is sufficient.
|
||||
#[test]
|
||||
|
||||
@@ -30,7 +30,9 @@ fn render_csv(workbook: &Workbook, view_name: &str) -> Result<Vec<u8>> {
|
||||
.get(view_name)
|
||||
.ok_or_else(|| anyhow::anyhow!("View '{view_name}' not found"))?;
|
||||
|
||||
let layout = GridLayout::new(&workbook.model, view);
|
||||
// `with_frozen_records`, not `new`: `new` skips `prune_empty`, so an export
|
||||
// writes rows the grid does not draw (improvise-iot6).
|
||||
let layout = GridLayout::with_frozen_records(&workbook.model, view, None);
|
||||
let model = &workbook.model;
|
||||
let (fmt_comma, fmt_decimals) = crate::format::parse_number_format(&view.number_format);
|
||||
|
||||
@@ -144,6 +146,69 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Bug improvise-iot6: `render_csv` built its layout with `GridLayout::new`,
|
||||
/// which never applies `prune_empty`. Evidence: `context/test-evidence/formula-and-io.md`.
|
||||
#[test]
|
||||
fn export_csv_omits_the_rows_the_view_prunes() {
|
||||
use crate::view::{Axis, GridLayout};
|
||||
let mut wb = Workbook::new("T");
|
||||
wb.add_category("Type").unwrap();
|
||||
wb.add_category("Month").unwrap();
|
||||
for item in ["Food", "Gas", "Rent"] {
|
||||
wb.add_item("Type", item).unwrap();
|
||||
}
|
||||
// `Feb` is the empty COLUMN: pruning is symmetric and so is the bug.
|
||||
for item in ["Jan", "Feb"] {
|
||||
wb.add_item("Month", item).unwrap();
|
||||
}
|
||||
// `Rent` is the empty row: every other Type carries a Jan value.
|
||||
for t in ["Food", "Gas"] {
|
||||
wb.model.set_cell(
|
||||
coord(&[("Type", t), ("Month", "Jan")]),
|
||||
CellValue::Number(10.0),
|
||||
);
|
||||
}
|
||||
{
|
||||
let v = wb.active_view_mut();
|
||||
v.set_axis("Type", Axis::Row);
|
||||
v.set_axis("Month", Axis::Column);
|
||||
v.prune_empty = true;
|
||||
}
|
||||
|
||||
// The precondition, or this test proves nothing: pruning must really be
|
||||
// ON and must really drop an item the model still holds, on BOTH axes.
|
||||
assert!(wb.active_view().prune_empty, "pruning must stay ON");
|
||||
let drawn = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None);
|
||||
let raw = GridLayout::new(&wb.model, wb.active_view());
|
||||
assert_eq!(
|
||||
(drawn.row_count(), drawn.col_count()),
|
||||
(2, 1),
|
||||
"the view must draw two of three rows and one of two columns"
|
||||
);
|
||||
assert_eq!(
|
||||
(raw.row_count(), raw.col_count()),
|
||||
(3, 2),
|
||||
"the unpruned layout must really hold the empty row and column"
|
||||
);
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let path = dir.path().join("export.csv");
|
||||
super::export_csv(&wb, "Default", &path).unwrap();
|
||||
let mut rdr = csv::ReaderBuilder::new()
|
||||
.has_headers(false)
|
||||
.from_path(&path)
|
||||
.unwrap();
|
||||
let records: Vec<csv::StringRecord> = rdr.records().collect::<Result<_, _>>().unwrap();
|
||||
let labels: Vec<&str> = records.iter().skip(1).map(|r| &r[0]).collect();
|
||||
assert_eq!(
|
||||
labels,
|
||||
["Food", "Gas"],
|
||||
"CSV wrote a row the grid does not draw"
|
||||
);
|
||||
let header: Vec<&str> = records[0].iter().skip(1).collect();
|
||||
assert_eq!(header, ["Jan"], "CSV wrote a column the grid does not draw");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn export_csv_unknown_view_returns_error() {
|
||||
let m = two_cat_model();
|
||||
|
||||
Reference in New Issue
Block a user