Skip to content

engine: lookup-only tables produce no series (#606)#609

Merged
bpowers merged 1 commit into
mainfrom
lookup-only-static-table
May 21, 2026
Merged

engine: lookup-only tables produce no series (#606)#609
bpowers merged 1 commit into
mainfrom
lookup-only-static-table

Conversation

@bpowers
Copy link
Copy Markdown
Owner

@bpowers bpowers commented May 21, 2026

Summary

Fixes #606. A standalone Vensim lookup -- a graphical function with no driving input (e.g. Historical GDP LOOKUP[COP]((1850,...),...)) -- is a table indexed by an explicit input, not a time series. The #590 import-pipeline fix lowered such a table to gf(Time), synthesising a phantom Time-indexed series: a latent unit error (benign in C-LEARN only by coincidence -- year-indexed tables, consumers calling LOOKUP(self, Time / One year), One year == 1), and a value genuine Vensim never saves.

This makes a lookup-only table a first-class non-value-bearing static table (Variable::Var::is_table_only / db::source_var_is_table_only): excluded from the runlist and from the saved output, so it produces no series of its own. Its data is reached only through LOOKUP(table, x) call sites, which resolve the table by ident -> base_gf (independent of the runlist), so two calls at different arguments are independent intermediate expressions.

Key changes

  • A lookup-table reference is a layout reference, not a data-flow dependency: a new builtins::BuiltinContents::LookupTable walk variant keeps it off the dependency / causal / LTM graphs, while a referenced_tables set on VariableDeps / ImplicitVarDeps re-supplies the fragment compiler's metadata + tables map (so consumers still resolve the table).
  • A bare reference to a table (used as a value, no argument) is now a compile error: ErrorCode::LookupReferencedWithoutArgument. An empty equation paired with a graphical function is no longer flagged as EmptyEquation.
  • The MDL importer emits the canonical empty-equation form for a bare lookup instead of the "0+0" sentinel. "0+0" is still accepted on read for already-serialized models; fully removing that read-shim (gated on a data migration) is tracked in engine: remove legacy "0+0" LOOKUP_SENTINEL read-shim from lookup-only detection (gated on datamodel migration) #608.
  • Removed the dead gf(Time) lowering (lookup_only_index_expr, LookupOnlyLayout); the shared lookup-only predicate moved to src/variable.rs.

Tests

  • lookup_only_tests.rs rewritten to the new contract: no saved series across scalar / apply-to-all / arrayed shapes, consumers read the table, multi-call independence (gap = g(Time) - g(Time-1)), bare-reference compile error, and the legacy "0+0" form still accepted.
  • C-LEARN end-to-end (simulates_clearn + clearn_residual_exactness, --ignored) pass; the EXPECTED_VDF_RESIDUAL carve-out shrinks from 13 to 4 (the 9 lookup-only ghost columns leave the comparison cleanly).
  • Full pre-commit gate green (Rust fmt/clippy/test, TypeScript lint/build/tsc/test, pysimlin).

A standalone Vensim lookup -- a graphical function with no driving input -- is a table indexed by an explicit input, not a time series. Lowering it to gf(Time) (the #590 import fix) synthesised a phantom Time-indexed series: a unit error in general, and a value genuine Vensim never saves. Such a table is now a first-class non-value-bearing static table (Variable::Var::is_table_only / db::source_var_is_table_only): excluded from the runlist and the saved output, its data reached only through LOOKUP(table, x) call sites that resolve the table by ident.

A lookup-table reference is a layout reference, not a data-flow dependency, so a new BuiltinContents::LookupTable walk variant keeps it off the dependency/causal/LTM graphs while referenced_tables on VariableDeps/ImplicitVarDeps re-supplies the fragment compiler's metadata and tables map. A bare reference to a table (no argument) is now a compile error (LookupReferencedWithoutArgument); an empty equation paired with a graphical function is no longer flagged as EmptyEquation. The MDL importer emits the canonical empty-equation form instead of the "0+0" sentinel, still accepted on read for already-serialized models (full removal tracked in #608). The dead gf(Time) lowering is removed; the C-LEARN EXPECTED_VDF_RESIDUAL carve-out shrinks from 13 to 4.
@bpowers bpowers merged commit d15ea7e into main May 21, 2026
12 of 13 checks passed
@bpowers bpowers deleted the lookup-only-static-table branch May 21, 2026 16:12
@claude
Copy link
Copy Markdown

claude Bot commented May 21, 2026

Review: engine: lookup-only tables produce no series (#606)

I reviewed the full diff. The dependency/causal/LTM split (the new BuiltinContents::LookupTable variant), the referenced_tables re-supply path into the fragment compiler, the bare-reference compile error, and the MDL importer/writer changes all hang together, and the two table-only predicates (variable::var_is_lookup_only / db::source_var_is_table_only) stay consistent (parse_table is total on Some(gf), so !tables.is_empty()gf.is_some()). I found one correctness issue.


[P1] calc_flattened_offsets_incremental undercounts a module's slots when a submodel contains a lookup-only table

src/simlin-engine/src/db.rs:5768

A module variable advances the running offset i by sub_size, computed as the sum of the sizes of the submodel's named offset entries. Now that source_var_is_table_only variables are excluded from this map (their slot is reserved — i advances by var_sz at line 5796 — but their name is omitted), that sum no longer equals the submodel's true slot count whenever the submodel contains a standalone lookup-only table: it undercounts by the table's reserved slots. compute_layout still sizes the module by sub_layout.n_slots (db.rs:2861, which counts those slots), so the results map and the runtime layout diverge — every parent-model variable ordered after such a module gets a too-small results offset and its reported series is read from a slot inside the module's interior (a silently wrong series, no error). This only triggers when a submodel instantiated as a module contains a bare lookup table, so it isn't exercised by the single-model lookup_only_tests or (apparently) C-LEARN. Advancing i by the submodel's compute_layout(...).n_slots instead of the sum of named entries would keep the two layouts in lockstep. The identical pattern at db.rs:5831 is reachable only via stdlib implicit modules (SMOOTH/DELAY/TREND), which carry no lookup tables, so it is benign today.


Overall correctness

Not correct as-is — blocking on the offset-lockstep issue above. It produces silently incorrect simulation output for any parent-model variable sequenced after a module whose submodel contains a standalone lookup-only table. The rest of the change is sound: the runlist/output exclusion stays in lockstep at the root level, all BuiltinContents consumers handle the new variant exhaustively, and the consumer's fragment correctly re-resolves referenced tables via referenced_tables.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 91.89189% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.86%. Comparing base (7b2cf1a) to head (bd2bdd4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/simlin-engine/src/db.rs 91.17% 3 Missing ⚠️
src/simlin-engine/src/variable.rs 90.32% 3 Missing ⚠️
src/libsimlin/src/lib.rs 0.00% 1 Missing ⚠️
src/simlin-engine/src/ast/expr2.rs 0.00% 1 Missing ⚠️
src/simlin-engine/src/common.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #609      +/-   ##
==========================================
- Coverage   82.87%   82.86%   -0.02%     
==========================================
  Files         261      261              
  Lines       69813    69836      +23     
==========================================
+ Hits        57856    57867      +11     
- Misses      11957    11969      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine: standalone lookup-only variable is lowered to gf(Time) (unit error; phantom Time-indexed series for a bare graphical function)

1 participant