engine: close out the VECTOR builtin issues (#576/#578/#579)#736
Conversation
The #585 fix made VECTOR SORT ORDER rank per iterated row (innermost dim is the sorted axis, 0-based), so the engine now produces p = [0,1,1,0,0,1] for the 2-D p[DimA,DimB] = VECTOR SORT ORDER(o[DimA,DimB], ASCENDING), matching test/sdeverywhere/models/vector/vector.dat exactly. Real-Vensim multi- dimensional VSO semantics are independently confirmed by C-LEARN (simulates_clearn vs Ref.vdf), so the dormant fixture is no longer ambiguous. Removes p from the simulates_vector_xmile_genuine exclusion list, leaving only y (GitHub #578, a separate ELM MAP compile gap). Closes #576.
…-slice path
The full-array-source out_of_bounds_element_returns_nan_{vm,monolithic} tests
already catch a corrupted full_source_len (both the codegen computation and the
renumber_opcode/resolve path) -- contrary to GH #579's framing, which only
checked the three genuine-Vensim .dat simulate gates (none of which have an
out-of-range offset). The genuine residual gap was the STRICT-SLICE source
branch (base != 0, source_is_full_array == false): no end-to-end test fed an
out-of-range offset through it, so full_source_len's role there -- both the
full-array-vs-strict-slice decision and the [0, full_source_len) -> :NA: guard
-- was unverified numerically.
Adds strict_slice_source_oob_returns_nan_{vm,monolithic}: a cross-dimension
d[DimA,B1] source whose A3 row offset (5) pushes the lookup to flat 9, past d's
6-element storage, yielding NaN. Inflating full_source_len makes that read land
in-bounds (a finite value) and deflating it flips the branch to full-array
(base forced to 0); either corruption drops the expected NaN and fails the
assertion. Verified red under both corruption sites, green when correct.
Updates the symbolic.rs comment that claimed full_source_len was behaviorally
invisible end-to-end. Closes #579.
y[DimA] = VECTOR ELM MAP(x[three], (DimA-1)) -- a fully-collapsed scalar source element reference plus a per-element arithmetic offset -- previously failed to compile (NotSimulatable, "failed to compile fragments"). Two coupled gaps caused it: 1. Inside an array-producing builtin a fully-collapsed source subscript like x[three] was promoted back to the whole source array (Wildcard), discarding the element's base offset. That is only correct when the element is index 0 (the historical b[B1] case); for x[three] (base 2) the base was lost. The collapsed scalar view is now kept as a StaticSubscript whose view.offset is the element's flat index, so codegen::full_source_len recovers the full source length and the VM maps from the right base. 2. The per-element offset (DimA-1) lowers to a compile-time constant, which the array-producing ELM MAP opcode cannot consume (it needs a *view* offset). A new compile-time fold rewrites VECTOR ELM MAP(scalar_source, const_offset) into a direct read of the source variable's full storage at off + base + round(offset), or :NA: (NaN) when that flat index is out of range. The fold recurses through Op2/If so a nested form (10 + ELM MAP(...)) folds too, but never descends into reducer arguments where the ELM MAP must stay an array. Because the fold emits plain slot reads, no ELM MAP opcode is generated for this shape and the wasm backend needs no change (confirmed by the wasm parity hook and simulates_clearn_wasm). The genuine-Vensim vector.xmile gate is now fully un-narrowed -- y rejoins c/f/g/p and every other variable as a hard equality against vector.dat. Closes #578.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
=======================================
Coverage 90.60% 90.61%
=======================================
Files 224 224
Lines 136247 136278 +31
=======================================
+ Hits 123447 123485 +38
+ Misses 12800 12793 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code review complete. Traced through the changes: The context.rs change correctly preserves the scalar element-references view.offset (the elements flat index) instead of promoting collapsed Single-ops back to a wildcard view. The old promotion only happened to be correct when the element was index 0 (the historical b[B1] case, base 0); the new x[three] case (base 2) needs the preserved offset. The new fold (try_fold_scalar_source_elm_map / fold_scalar_source_elm_maps) handles the case where the array-producing ELM MAP opcode cannot consume the per-element constant offset (it requires a view offset). The restriction to Op2/If recursion is the right call - folding inside an Expr::App argument (e.g. SUM(elm_map_array)) would be wrong because the reducer consumes the ELM MAP as an array. Regression check on the legacy vector_elm_map(b[B1], a[DimA]) case: the per-element offset a[DimA] lowers to Var(a_off + i), which does not fold to a constant, so try_fold_scalar_source_elm_map returns None and the runtime opcode is used. With the new scalar source view (empty dims, view.offset = 0), RuntimeView::flat_offset(&[]) returns 0, so base_i = 0 and the existing c = [11, 12, 12] result is preserved. Simulate.rs: removing y and p from the exclusion list is consistent with the now-fixed compile path for y and the GH-585 per-iterated-row VSO fix for p. Notes (not findings):
Verdict: Overall correctness correct. No actionable findings. |
1 participant
Closes the open VECTOR-builtin engine issues. (#585 was already fixed on main by the element-cycle-resolution work and is closed separately.)
#576 -- 2-D VECTOR SORT ORDER
pbecomes a hard gateThe #585 fix made VECTOR SORT ORDER rank per iterated row (innermost dim = sorted axis, 0-based), so the engine now produces
p = [0,1,1,0,0,1]forp[DimA,DimB] = VECTOR SORT ORDER(o[DimA,DimB], ASCENDING), matchingtest/sdeverywhere/models/vector/vector.datexactly. Real-Vensim multi-dimensional VSO semantics are independently confirmed by C-LEARN (simulates_clearnvsRef.vdf), so the dormant fixture is no longer ambiguous.pis removed from thesimulates_vector_xmile_genuineexclusion list.#579 -- numeric end-to-end gate for
full_source_lenon the strict-slice pathInvestigation showed the existing full-array-source
out_of_bounds_element_returns_nan_*tests already catch a corruptedfull_source_len(both the codegen computation and therenumber_opcode/resolve path) -- #579's "behaviorally invisible" framing only checked the three.datsimulate gates. The genuine residual gap was the strict-slice source branch (base != 0): addedstrict_slice_source_oob_returns_nan_{vm,monolithic}(a cross-dimensiond[DimA,B1]source whose A3 row offset walks past the source storage -> NaN). Verified red under corruption at both sites, green when correct. The stalesymbolic.rscomment is corrected.#578 -- scalar-source / expression-offset VECTOR ELM MAP now compiles
y[DimA] = VECTOR ELM MAP(x[three], (DimA-1))previously failed withNotSimulatable: "failed to compile fragments". Two coupled gaps:x[three]) was promoted back to the whole source array, discarding the element base (only correct when the element is index 0). It now stays a scalarStaticSubscriptcarryingview.offset= the element's flat index.VECTOR ELM MAP(scalar_source, const_offset)to a direct full-storage read (or:NA:/NaN out of range), recursing throughOp2/Ifso nested forms like10 + ELM MAP(...)fold too -- but never into reducer arguments where the ELM MAP must stay an array.Because the fold emits plain slot reads, no ELM MAP opcode is generated for this shape and the wasm backend needs no change.
vector.xmileis now a fully un-narrowed genuine-Vensim gate (no exclusions).Validation
simlin-enginelib (4352) + integration (487) suites green.Ref.vdf(simulates_clearn,simulates_clearn_wasm) green.full_source_lenOOB.