engine: close the LTM mapped-dimension and array-operand gaps (#995, #996, #997) - #1010
engine: close the LTM mapped-dimension and array-operand gaps (#995, #996, #997)#1010bpowers wants to merge 8 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ff773b4c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if is_snapshot_view(&operand) { | ||
| return operand; | ||
| } | ||
| let Some(source_view) = super::find_expr_array_view(&operand) else { |
There was a problem hiding this comment.
When a computed operand broadcasts a narrower left-hand array into a wider one—for example, VECTOR SORT ORDER(small[A] + wide[A,B], 1)—find_expr_array_view returns the first view it encounters, so this creates a temporary shaped only [A]. A production-path reproduction returned [0,0,0,1,1,1], while commuting the addition to wide[A,B] + small[A] produced the correct [0,1,2,0,1,2]. Because addition is commutative and subset broadcasting is supported, operand order now silently changes simulation results; derive the computed expression's merged output dimensions and cover both operand orders.
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, reproduced, and fixed in 1510a59. The mechanism was worse than an [A]-shaped truncation: the narrow temp's iteration found no match for the wide array's extra axis and read NaN for every element, so the wrong order returned the sort of three NaNs. find_expr_array_view now joins every array-shaped subexpression by a maximality filter over dim names/sizes (order-independence is structural, not accumulated), the If condition is visited (it was previously skipped entirely -- a wider condition silently collapsed the IF to its ELSE arm), and incomparable shapes, axis-order ties, and repeated-dimension views decline loudly rather than guessing. Both operand orders of your reproduction now agree, pinned with numeric tests plus a three-array permutation row; instrumented blast-radius probes measured zero divergence from the old answer across the corpus and the C-LEARN heavy tier.
| Some(( | ||
| corr.iter().map(|e| e.as_str().to_string()).collect(), | ||
| Some(slots), |
There was a problem hiding this comment.
When two source axes both map to the same active target dimension—for example, target[State] = matrix[Region1,Region2] with both region dimensions mapped to State—execution resolves both indices from the same current State element and reads only matrix[map1(s),map2(s)]. This arm returns each MappedRead axis as an independent list, and the generic product below combines them cartesianly, so the element graph also emits cross rows such as matrix[map1(s1),map2(s2)] that execution never reads; downstream target projection uses only the first duplicate slot coordinate, creating phantom causal edges and false candidate loops. Zip axes sharing dim, or decline this repeated projection before returning rows.
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real and fixed in 1510a59, with one correction to the report: the cross rows never became link scores (the score-name derivation is keyed by a dim-name map, so it always produced the diagonal) -- the damage was phantom element EDGES and loop candidates, 9 where execution reads 3, with names and edges describing different graphs. read_slice_row_parts now enumerates drivers: axes naming the same target dimension share one enumerated coordinate (zip, not product), preserving row order for every unrepeated slice; an all-Iterated subscript repeating a target dimension also retargets Bare -> PerElement when the target names that dimension once. The repeated-TARGET sibling (cube[D1,D1]) deliberately keeps Bare -- retargeting flips its score surface to the loud square-source skip, and expand_same_element's name-keyed target positions carry both phantom and missing edges there either way; that residual is pinned in both directions as a stated follow-up rather than absorbed here.
| SymStaticViewBase::PrevVar(var_ref) => (resolve_var(var_ref)?, ViewStorage::Prev), | ||
| SymStaticViewBase::InitialVar(var_ref) => (resolve_var(var_ref)?, ViewStorage::Initial), |
There was a problem hiding this comment.
When an array-valued PREVIOUS or INIT is compiled inside a nested module, resolve_var returns an offset relative to that module's body layout, but these new variants become absolute Prev/Initial static views. Vm::PushStaticView and wasm's PrevAbsolute/InitialAbsolute loads never add the runtime module_off, unlike scalar LoadPrev/LoadInitial, so an instance reads root or sibling snapshot slots instead of its own array; multiple instances therefore cannot produce independent results. Preserve a module-relative snapshot base through resolution, or apply module_off when the view is pushed or loaded.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real -- and bigger than reported. Reproducing it showed the Curr control aliasing too: PushStaticView never applied module_off for ANY region, on main before this branch, on both backends identically (the wasm lowering documented the VM verbatim, which is why the parity gate never caught it; reads from the root were correct because module_off is 0 there, which is why the existing cross-module array tests passed). So every array reducer, vector op, and arrayed GF evaluated INSIDE a module instance read the root's storage -- the snapshot views inherited the defect rather than introduced it. Fixed in 1510a59 for all regions: StaticArrayView::to_runtime_view takes the executing instance's base for Curr/Prev/Initial and not Temp (per-evaluation shared storage), the wasm ViewBase arms are renamed for their region with the absolute/relative split gone, and two-instance plus nested two-hop fixtures pin instance addressing on both backends (the nested one kills a last-hop-only mutation the one-hop fixture cannot see).
…questions Before any LTM describer changes for GH #997, pin what the engine actually executes for a reference across mapped dimensions. The new mapped_reference_semantics_tests module enumerates reference spelling (iterated-dim subscript, source-own-dim subscript, bare-in-equation, stock-flow wiring) x mapping kind (positional, permuted element map, many-to-one, reverse-cardinality, shared-element-names, no-mapping controls) x both declaration directions, every cell asserted against the VM with the competing rules' answers shown absent. Three resolution rules fall out, not two: the iterated and bare-in-equation spellings are positional (lower_pass0 rewrites a bare ref into the iterated spelling before anything can consult a map), while the source-own-dim and stock-flow spellings resolve name-first and only then through the declared element map. The stock-flow spelling and the name-first precedence were both previously undocumented; the mapped_element_correspondence rustdoc now records them, and its Vensim claims are quoted from the raw subscript-mapping reference page instead of a paraphrase (Example 3 turns out to refute the old doubt about the iterated spelling's legality, while still not settling positional vs map-following, which stays marked unverified). For GH #996: the allocator restaging itself landed with GH #994's merge, so this records the remaining evidence. get_implicit_subscripts' reachability is measured and documented (exactly two production callers, both wiring; ordinary expressions never arrive), and the end-to-end hazard fixture the_996_hazard_shape_compiles_and_reads_name_first pins name-first allocation numerically on the executed path, using non-canonical dimension names so a canonicalization slip cannot pass vacuously. C-LEARN re-measure: the 135 mapping-order declines are gone; the residual declines are single-axis (no ordering hazard), attributable to the element-map decline (GH #997) and rank-like partials (GH #995). TestProject gains named_dimension_with_mappings for the multi-target dimension the hazard shape needs.
…ibers An LTM describer must resolve a mapped-dimension reference the way the executed simulation does, and the execution rule depends on the reference site's SPELLING, which the pair-keyed mapped_element_correspondence could not see -- so it declined every explicit element map and class-D edges (a dep read through an element-mapped axis) were declined loudly instead of scored (GH #997). The executed name-then-element-map-then-mapped-parent resolution is now one function, DimensionsContext::resolve_mapped_read, called by all three compiler sites (get_implicit_subscript_off, build_view_from_ops, and the dynamic IndexExpr3::Dimension arm, whose divergent no-name-first pairing and dead numeric fallback are gone). On top of it the correspondence API is spelling-keyed: positional_correspondence for the iterated and bare-in-equation spellings (now returning the positional diagonal for an element-mapped pair, which is what execution does), executed_read_correspondence for the source-own-dim and stock-flow spellings (name-first, any cardinality), and bare_reference_correspondence, the union RefShape::Bare needs because a structural flow-to-stock edge and an in-equation bare reference resolve by different rules. Element edges emit the union (never fewer edges than execution reads); the arrayed link-score retarget is gated on the union being a singleton per target element (mapped_pair_projects_uniquely), so where the two rules disagree the edge keeps its loud skip rather than scoring a phantom from-node with the real edge's series. Classification gains AxisRead::MappedRead / OccurrenceAxis::MappedRead for a subscript naming a non-active dimension resolvable through exactly one target-iterated dim (ambiguity declines to the conservative cross-product, with an attribution control); the row derivations, per-element link scores, and both pin paths consume it through the shared derivations -- dep_element_pins now carries both spellings' rows per dep, since one table serves a map-following subscripted reference and a positional bare one. On C-LEARN the 13 remaining unprojectable-dep declines (five X-Aggregated deps across the 3-onto-7 element map) all become scored per-element link scores (+91 vars, one scalar slot each); simulates_clearn and the whole heavy tier stay green, and the Phase A semantics matrix is unmoved.
Codegen requires an array-valued operand of a vector builtin to be a view over storage, and any computed array must have been moved into an AssignTemp earlier in the fragment -- but nothing did that for the apply-to-all spellings, so ordinary hand-written models like out[d] = VECTOR SORT ORDER(vals[d] * 2, 1) failed to compile with LTM off (the compiler half of GH #995; the LTM half landed with GH #1003's freeze helpers). The issue's suggested Pass-1 relaxation turns out to rest on a wrong diagnosis, established by instrumenting real lowered fragments: the type checker bounds an A2A elementwise Op2 as SCALAR, so needs_decomposition declines before the has_a2a defer is ever consulted, and an Expr3-level hoist would collapse the operand per element (the AssignTemp arm lowers without wildcard promotion). The fix is instead a last lowering pass, compiler/array_operand.rs, run at the Var::new chokepoint on the fully lowered fragment where every view is already concrete: it rewrites exactly the operands codegen would reject (the negation of walk_expr_as_view's accepting arms), so every fragment that compiled before is byte-identical -- zero golden churn, max temps-per-fragment unchanged at 21 across the corpus and C-LEARN. The Rank arm in Pass 1 additionally decomposes its array argument like its five siblings (pinned at the lowered-fragment level, since the new pass subsumes its behavior). An operand CONTAINING an array-valued PREVIOUS/INIT subexpression is declined loudly rather than materialized: the frozen reference lowers element-collapsed while its sibling supplies the array shape, so the temp would broadcast one element's previous value across the row -- wrong numbers where HEAD failed to compile. Those rows are pinned per view position for the follow-on snapshot-view work to flip. Also made loud, in symbolic::resolve_static_view: a static view over a temp id past the u8 TempId namespace, which previously wrapped modulo 256 and returned a different array from element 128 on (a pre-existing GH #583 instance, reproduced at the base commit; the write-side truncation coincidence is now pinned rather than assumed). New corpus fixture test/vector_computed_operand gates the computed-operand shapes through the VM, both round-trips, and the wasm parity hook with time-varying hand-computed expectations.
…ot buffers An array-valued operand must be a view over storage, and PREVIOUS/INIT had no view to offer: LoadPrev/LoadInitial address one static slot, so out[d] = VECTOR SORT ORDER(PREVIOUS(vals[d]), 1) -- and the real user shape VECTOR SELECT(sel[Row,*], PREVIOUS(matrix[Row,*]), ...) -- failed to compile. This is GH #995's option D: the view records' is_temp bool becomes ViewStorage { Curr, Temp, Prev, Initial } (with distinct symbolic PrevVar/InitialVar bases, so Temp-plus-snapshot is unrepresentable and every dereference site had to say which region it means), the three chunk regions share curr's slot numbering, and both backends read through the base. First-step semantics are BRANCHES, not buffer contents: a Prev view reads the scalar fallback while use_prev_fallback is set and an Initial view reads curr during the Initials phase, mirroring the scalar opcodes -- the wasm reset does not zero its snapshot regions, so relying on zeroed memory would have been silently wrong on a second run, which the repeated-run parity test pins. A non-default array PREVIOUS fallback, PREVIOUS/INIT of a temp, and a snapshot in ALLOCATE AVAILABLE's priority-profile position (whose expander rebuilds the full view only from a direct variable reference, so a snapshot there would allocate over one column) all decline loudly. builtins_visitor now passes an array-shaped PREVIOUS/INIT argument through to lowering instead of pinning or capturing it at parse time, so the array-vs-element decision is made where the operand position is known (compiler::snapshot_view_arg, one classifier shared by codegen's consumers and the operand materializer); the C1+C2 nested-operand decline is replaced by correct materialization, and every row of GH per-element capture oracles on time-varying fixtures. The dep graph classifies the new bases like their scalar twins (a Prev view is never a same-step edge; an Initial view is an initials-phase edge), each arm pinned by a fixture its mutation kills. C-LEARN is byte-identical on all four LTM harnesses -- the LTM wrap already materializes its freezes as helper variables, which stay (their axis-qualified rows are still load-bearing for non-prefix subdimensions; retiring them onto these views is assessed in the freeze module's doc as follow-on). New corpus fixture test/vector_snapshot_operand gates the shapes through the VM, both round-trips, and the wasm parity hook with hand-derived expectations. The PartialEquationError cluster moves to a path-mounted ltm_augment_partial_error.rs sibling, keeping ltm_augment.rs under the per-file line cap.
…ew module_off bug beneath one Three review findings on the array-operand and mapped-describer work, each reproduced before fixing; the third led somewhere bigger than its report. A computed array operand took the FIRST subexpression's shape: VECTOR SORT ORDER(vals[d] + matrix[e,d], 1) materialized a [d]-shaped temp whose iteration read matrix as NaN, so commuting the addition changed the answer. find_expr_array_view now joins every array-shaped subexpression by a maximality filter over dim names and sizes -- order-independence is structural, not accumulated -- and the If condition is visited (previously a wider condition silently collapsed the IF to its ELSE arm). Incomparable shapes, axis-order ties, and a view naming one dimension twice decline loudly; the repeated-name decline replaces a pre-existing wrong-both-ways answer whose mechanism (project_var_index_to_temp matching temp axes to variable axes by name, first hit wins) is pinned by a control so a future projection fix must restate it. The join lives in find_expr_array_view itself because the enclosing hoisted AssignTemp's view is derived from the same function -- a join beside it produced new wrong numbers through the temp-index projection. Measured zero divergence from the old answer across the corpus and heavy tier. Two source axes projected onto one target dimension enumerated their rows cartesianly while execution resolves both from the same active element: 9 element edges and 9 loop candidates where execution reads 3 (link-score names were already diagonal, so names and edges described different graphs). read_slice_row_parts now enumerates drivers -- axes naming the same target dimension share one -- preserving row order for every unrepeated slice, and an all-Iterated subscript repeating a target dimension retargets from Bare to PerElement when the target names that dimension once. The repeated-TARGET shape keeps Bare deliberately: the retarget would flip its score surface to the loud square-source skip, a separate decision, and expand_same_element's name-keyed target positions carry both phantom and missing edges there either way -- pinned in both directions as a residual with the CLAUDE.md never-fewer-edges claim carrying the measured exception. The reported snapshot-view module_off gap turned out to predate the branch and cover every region: PushStaticView never applied module_off at all, so any static view pushed while executing INSIDE a module instance -- every array reducer, vector op, and arrayed GF in a sub-model, Curr included -- read the root's storage, identically on both backends (the wasm lowering documented the VM verbatim, which is why parity never caught it; reads from the root were correct because module_off is 0 there, which is why the cross-module array tests passed). StaticArrayView::to_runtime_view now takes the executing instance's base for Curr/Prev/Initial and not for Temp (temp storage is per-evaluation and shared), the wasm ViewBase arms are named for their region with the absolute/relative split gone, and two-instance plus nested two-hop fixtures pin instance addressing on both backends -- the nested one kills a last-hop-only mutation the one-hop fixture cannot see.
1ff773b to
1510a59
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
+ Coverage 91.86% 92.02% +0.16%
==========================================
Files 247 249 +2
Lines 159059 160329 +1270
==========================================
+ Hits 146116 147542 +1426
+ Misses 12943 12787 -156 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1510a59aa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if outer.dims == inner.dims && outer.dim_names == inner.dim_names { | ||
| return true; |
There was a problem hiding this comment.
When a computed operand has a repeated dimension, such as VECTOR SORT ORDER(matrix[d,d] * 2, 1), this identity shortcut supplies [d,d] as the materialization shape even though downstream projection matches axes only by name and maps both temp axes to the first d coordinate. The parent revision rejected this operand, but this change now compiles it and silently returns rows like [0,0,0,1,1,1,2,2,2] instead of each row's correct [0,1,2]; reject repeated-name views even when they are the operand's sole shape until axis-identity projection is implemented.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and your parent-revision claim was the correct one -- our earlier 'pre-existing' framing had been measured against a branch commit rather than the merge base. At ccf7ed3 the computed spelling (and the PREVIOUS/INIT spellings, which turn out to share the defect) all fail to compile, so this branch had introduced them compiling to garbage. Fixed in 0e61d13: the operand materializer refuses a repeated-name view even as the sole shape, and snapshot_static_view refuses a repeated-dimension snapshot source (that route never consults the join). Review caught that placing the refusal in the shared join instead panicked the VM on reducer-nested shapes that work at the merge base (through the hoisters' silent fallback), so the placement is itself pinned by a regression test. The DIRECT spelling compiles at the merge base and stays byte-identical, pinned as a disclosed residual; vensim-probes/repeated_dimension.mdl is queued for a real Vensim DSS run to learn whether the declaration is even legal there before investing in the axis-identity projection fix.
| AxisRead::Iterated { dim, .. } | AxisRead::MappedRead { dim, .. } => { | ||
| Some(dim.as_str()) | ||
| } |
There was a problem hiding this comment.
When a mapped target dimension contains a quoted element whose canonical name includes a comma, this newly accepted MappedRead path passes its slot through ReadSliceRow.slot, and line 420 reparses that comma-joined string with split(','). For a one-axis element such as "a,b", the real target[a,b] coordinate is therefore interpreted as separate a and b coordinates, so the element graph can omit the actual executed edge and create an edge to a nonexistent target; carry ReadSliceRowParts::slot_parts through this projection instead of serializing and splitting it.
AGENTS.md reference: src/simlin-engine/AGENTS.md:L95-L95
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real and constructible -- canonicalize on a quoted XMILE element "a,b" preserves the comma, and a model declaring one compiles and simulates. Fixed in 0e61d13: the PerElement element-edge arm consumes ReadSliceRowParts::slot_parts directly (structured, no serialize-and-split), with the comma-joined form cfg(test)-gated at the db::ltm boundary and a pin whose comma element comes FIRST so a mis-split shifts every following coordinate (mutation: reintroducing the round-trip reds with exactly the predicted phantom target). Disclosed alongside: the link-SCORE emitters carry their own split(',') sites and the defect is live there (measured on the same comma element carried into an agg -- the two halves of one agg name different variables); that sweep is its own change, and the invariant (never serialize a coordinate tuple you will re-split) is now stated in the engine CLAUDE.md.
| // Materializing an ELM MAP *source* deliberately changes which storage | ||
| // the mapping ranges over, and the choice is this: the temp. Genuine | ||
| // Vensim maps over the source VARIABLE's full row-major storage from | ||
| // the base arg-1's element reference establishes, and |
There was a problem hiding this comment.
For a computed source such as VECTOR ELM MAP(matrix[1,*] * 1, offsets), this branch makes a new semantic choice: materialization confines mapping to the temporary instead of retaining the source variable's full-storage addressing. The nearby test only pins that chosen behavior against Simlin itself; it provides no official Vensim citation or genuine Vensim output for the computed-source case, so the unverified premise can make newly compiling models silently disagree with Vensim. Verify this exact case against the function reference or checked-in DSS output and cite it here; if it cannot be checked, mark the behavior unverified rather than letting it determine lowering.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the standard, and the research split the question. The VARIABLE-source full-storage rule is now cited rather than asserted (0e61d13): the fn_vector_elm_map reference page says the function returns the value of 'the variable' offset from vec, with :NA: outside 'the range of the variable', and its multi-subscript example makes the flat whole-variable addressing explicit; the checked-in vector.dat ground truth confirms it (f[A2,B1] = 5 reads past its own B1 slice), recomputed independently. The COMPUTED-source case is genuinely outside what the page reaches -- every example there and in the corpus spells the argument as a variable reference pinned to an element -- so it is now marked UNVERIFIED at the lowering arm, in the test, and in CLAUDE.md, with the helper-equivalence argument stated as the reason for the choice rather than as evidence. A discriminating probe model (vensim-probes/elm_map_computed_source.mdl, with self-checking controls that reproduce vector.dat and per-rule prediction tables) is queued for a real Vensim DSS run; the lowering will match whatever that run shows, including refusing the shape if Vensim rejects inline expressions there.
… structured Second round of PR #1010 review findings, each claim measured before acting -- including against the true merge base, which corrected a mis-baselined claim from the prior round: the repeated-dimension computed shapes (VECTOR SORT ORDER(matrix[d,d] * 2, 1) and the PREVIOUS/INIT spellings) all FAIL at ccf7ed3, so this branch had introduced them compiling to garbage, not inherited them. They decline loudly now, at the two positions that can produce a diagnostic: the operand materializer refuses a repeated-name view even as the sole shape, and snapshot_static_view refuses a repeated-dimension snapshot source (the PREVIOUS/INIT route never consults the join). Placing the refusal in the shared join instead panicked the VM on reducer-nested shapes that work at the merge base (SUM(VECTOR SORT ORDER(matrix[d,d], 1)) and kin) through the hoisters' silent unwrap_or_else fallback -- caught in review, and the placement is now constrained by a regression test rather than a comment. The DIRECT repeated-dimension spellings compile at the merge base and stay byte-identical, pinned as a disclosed residual stating the correct answers alongside ours (the mechanism is project_var_index_to_temp's first-hit name matching); vensim-probes/repeated_dimension.mdl exists to learn whether Vensim even accepts the declaration before anyone invests in the fix. A canonical element name can contain a comma (quoted XMILE "a,b" canonicalizes to a,b and simulates), so the PerElement element-edge arm now consumes the row derivation's structured slot parts instead of re-splitting a comma-joined string that mis-parsed such a name into two coordinates -- one real edge dropped, one minted to a nonexistent target. The comma-joined form is cfg(test)-gated at the db::ltm boundary; the link-score emitters' own split sites are a measured, disclosed residual on the pin test. VECTOR ELM MAP's full-storage variable-source rule is now CITED (the fn_vector_elm_map reference page, quoted, plus an independent recomputation of the checked-in vector.dat ground truth where f[A2,B1] = 5 reads past its own B1 slice) instead of asserted. The COMPUTED-source case is marked unverified everywhere it is decided: every example on the page and in the corpus spells the argument as a variable reference pinned to an element, so the documented rule does not reach an inline expression; vensim-probes/elm_map_computed_source.mdl carries the discriminating fixture with per-rule prediction tables and self-checking controls, for a real Vensim DSS run to settle.
…points at The engine CLAUDE.md's unverified-semantics note references vensim-probes/elm_map_computed_source.mdl, which existed only untracked, so the documentation-links check passed locally (the file is on disk) and failed in CI (it is not in the checkout). The kit is real project material -- two discriminating probe models plus a README with per-rule prediction tables and self-checking controls, awaiting a real Vensim DSS run -- so it belongs in the tree. Nothing lands in test/ until that run produces genuine output, exactly as the kit's README states.
Code review — no blocking findingsReviewed the four commits across the array-operand materializer, the ViewStorage refactor, the LTM describer mapped-read threading, and the module_off fix. Verified the following areas that would be the most likely sources of latent bugs, and found nothing to flag:
The disclosed residuals (nested array-producing builtin inside arithmetic, repeated-TARGET name-keyed edges, u8 TempId ceiling, ALLOCATE AVAILABLE profile look-through, freeze-helper retirement) match what the PR body calls out and are pinned in-tree. Overall correctness verdict: correctNo blocking issues; the patch matches its stated contract and the adversarial-review + mutation-testing groundwork is visible in the test structure. |
The probe .mdl files carried empty sketch sections, so they opened in Vensim with a blank diagram. examples/layout_probe_models.rs runs generate_best_layout over each and splices ONLY the generated sketch block into the original file: the first attempt re-serialized the whole project through the MDL writer, which re-spells an apply-to-all equation per element -- an element-pinned left-hand side over a right-hand side naming subscript ranges, which is not the shape the probes ask Vensim about -- so the equations stay byte-identical and the rendered output serves only as a sketch donor. The harness is checked in rather than its output alone so the views can be regenerated if the probes change.
Review: no blocking findingsReviewed the four largest concerns of the PR in parallel: the new Key claims verified:
Explicitly-disclosed residuals in the PR body (nested array-producing builtin in arithmetic operands, u8 TempId ceiling at ~128 elements #583, repeated-dimension direct read, Overall correctness verdict: correct. The change ships one new module, threads a spelling-keyed API through the LTM describers, and turns one pre-existing cross-module correctness gap ( |
1 participant
Closes out the LTM mapped-dimension and array-operand gap cluster in four commits, each implemented and then adversarially reviewed (with mutation testing) to convergence before landing.
Fixes #995
Fixes #996
Fixes #997
1. Pin executed mapped-reference semantics; close out #996 (dc86dae)
Before touching any describer,
mapped_reference_semantics_testspins what the engine executes for a reference across mapped dimensions: 48 cells over reference spelling (iterated-dim subscript, source-own-dim subscript, bare-in-equation, stock-flow wiring) x mapping kind (positional, permuted element map, many-to-one, reverse-cardinality, shared-element-names, no-mapping controls) x both declaration directions, each asserted against the VM with the competing rules' answers shown absent. Three resolution rules fall out, not two: iterated and bare-in-equation spellings are positional; source-own-dim and stock-flow spellings resolve name-first, then through the declared element map. The stock-flow spelling and the name-first precedence were both previously undocumented. Themapped_element_correspondenceVensim claims are now quoted from the raw subscript-mapping reference page (Example 3 refuted two of the old rustdoc's claims; the review caught that).For #996: the allocator restaging itself landed inside #994's merge, so this records the remaining evidence --
get_implicit_subscriptsreachability measured and documented (exactly two production callers, both wiring), the end-to-end hazard fixturethe_996_hazard_shape_compiles_and_reads_name_firstpinning name-first allocation numerically with non-canonical dimension names, and the C-LEARN re-measure showing the 135 mapping-order declines resolved (residual declines are single-axis: no ordering hazard).2. Thread the reference spelling through the LTM describers -- #997 (a0b513c)
The executed name/element-map/mapped-parent resolution becomes one function,
DimensionsContext::resolve_mapped_read, called by all three compiler sites (a divergent no-name-first arm and a dead numeric fallback are gone). On top of it the correspondence API is spelling-keyed:positional_correspondence,executed_read_correspondence, andbare_reference_correspondence(the unionRefShape::Bareneeds, since a structural flow-to-stock edge and an in-equation bare reference resolve by different rules). Element edges emit the union -- never fewer edges than execution reads -- while the arrayed link-score retarget is gated on the union being a singleton per target element, so where the two rules disagree the edge keeps its loud skip rather than scoring a phantom from-node with the real edge's series (the review demonstrated that hazard and the gate's test pins it). Classification gainsAxisRead::MappedReadfor the source-own-dim spelling, consumed through the shared row derivations by the element graph, the per-element link scores, and both pin paths.On C-LEARN the 13 remaining unprojectable-dep declines (five
X Aggregated[Aggregated Regions]deps across the 3-onto-7 element map) all become scored per-element link scores: +91 vars, one scalar slot each.simulates_clearn(real Vensim ground truth) and the whole heavy tier stay green.3. Materialize computed array operands -- #995, compiler half (295a927)
out[d] = VECTOR SORT ORDER(vals[d] * 2, 1)failed to compile in ordinary models with LTM off. The issue's suggested Pass-1 relaxation rests on a wrong diagnosis (measured: the type checker bounds an A2A elementwise Op2 as scalar, soneeds_decompositiondeclines before the deferral it blamed is consulted); the fix is a last lowering pass,compiler/array_operand.rs, running on the fully lowered fragment where every view is concrete. It rewrites exactly the operands codegen would reject, so every previously-compiling fragment is byte-identical: zero golden churn, max temps-per-fragment unchanged. An operand containing an array-valued PREVIOUS/INIT subexpression declined loudly (materializing it would broadcast one element's previous value -- wrong numbers where HEAD failed loudly; the review caught this before it shipped). Also made loud: a static view over a temp id past the u8 TempId namespace, which previously wrapped modulo 256 and silently returned a different array from element 128 on -- a pre-existing #583 instance reproduced at the base commit. New corpus fixturetest/vector_computed_operandwith hand-derived expectations gates the shapes through VM, both round-trips, and wasm parity.4. Array PREVIOUS/INIT as snapshot views -- #995, option D (1ff773b)
The view records'
is_tempbool becomesViewStorage { Curr, Temp, Prev, Initial }with distinct symbolicPrevVar/InitialVarbases, and both backends read views through the base. First-step semantics are branches mirroring the scalar opcodes (a Prev view reads the fallback whileuse_prev_fallbackis set; an Initial view reads curr during Initials) -- load-bearing on wasm, whose reset does not zero snapshot regions, pinned by a repeated-run parity test.builtins_visitorpasses array-shaped PREVIOUS/INIT arguments through to lowering, where the operand position is known; the phase-3 nested-operand decline is replaced by correct materialization. Every row of #995's original table now compiles with numbers pinned against per-element capture oracles on time-varying fixtures, including the PR #1001 user shapeVECTOR SELECT(sel[Row,*], PREVIOUS(matrix[Row,*]), ...). Loud declines where the view cannot be right: non-default array PREVIOUS fallback, PREVIOUS/INIT of a temp, and ALLOCATE AVAILABLE's priority-profile position (whose expander would otherwise allocate over one column -- the review measured wrong allocations and the decline restores HEAD's behavior). The dep graph classifies the new bases like their scalar twins, each arm pinned by a fixture its mutation kills. C-LEARN is byte-identical across all four LTM harnesses; the LTM freeze helpers stay (their axis-qualified rows are still load-bearing for non-prefix subdimensions), with the retire-onto-views simplification assessed as follow-on in the freeze module's doc.Verification
Every commit passed the full pre-commit gauntlet (fmt, clippy, cargo test, TS lint/build/tsc/test, wasm build, pysimlin). Heavy C-LEARN tier green at each phase (
simulates_clearn,simulates_clearn_wasm,clearn_pinned_climate_loop_is_scored,discovery_clearn_matches_vm_wasm, ...). C-LEARN LTM state on the branch tip: 0 unprojectable-dep declines (was 13), 0 failing LTM fragments, 5 rank-like declines (deliberate, the #995 option-C semantic trap staying closed), 6848 LTM vars / 29808 slots.Known residuals, disclosed in-tree rather than deferred silently
VECTOR SORT ORDER(VECTOR ELM MAP(a,b) + c, 1)) fails identically to HEAD -- a different contract, pinned by its own test.UnprojectableDep's rustdoc.5. Review response -- the three Codex P1s, and the bug beneath one (1510a59)
All three findings were reproduced before fixing (branch also rebased onto the salsa-0.28 main).
find_expr_array_viewtook the first subexpression's view, so commutingvals[d] + matrix[e,d]changed the answer (the narrow temp read NaN for the unmatched axis). Now a structural join (maximality over dim names/sizes) with the If condition visited; incomparable shapes, axis-order ties, and repeated-dimension views decline loudly. Zero measured divergence on the corpus.cube[D1,D1]) keeps its conservative path deliberately;expand_same_element's name-keyed target positions carry both phantom and missing edges there (pre-existing, third instance of the name-is-not-an-axis-identity family after ltm: per-element bare-Var pinning ignores the dep's declared dims (subset-dims dep silently zeroes scores; mapped live-source analogue) #974/ltm: classify_axis_access resolves a subscript index dimension-name-first, opposite to the compiler's element-first precedence #986), pinned in both directions as a follow-up.Currcontrol aliasing too --PushStaticViewnever appliedmodule_offfor any region, on main before this branch, on both backends identically. Every static view pushed while executing inside a module instance read root storage; reads from the root were correct (module_off == 0), which is why existing cross-module tests passed and why VM/wasm parity could not catch it (both wrong identically). Fixed for all regions with two-instance and nested two-hop fixtures on both backends. This half is a pre-existing correctness fix that stands apart from engine: an array-valued operand must be a view over storage, so PREVIOUS() of an array fails to compile (affects ordinary A2A models, not just LTM) #995/engine: allocate_implicit_axes_partial is order-greedy -- a mapping match on an earlier dep dim steals the slot a later dim matches by name #996/ltm: mapped_element_correspondence cannot see the reference spelling, so it declines element maps it should honour #997.