engine: add ROUND builtin with round-half-to-even semantics - #1015
Conversation
ROUND(x) rounds to the nearest integer, sending exact .5 ties to the even neighbor -- Python round() / IEEE roundTiesToEven -- rather than the more common round-half-away-from-zero. The XMILE v1.0 spec defines no ROUND function (zero occurrences in docs/reference/xmile-v1.0.html), so this is a Simlin extension and ties-to-even is a deliberate choice: it is statistically unbiased over uniformly distributed ties and matches what Python users expect. The VM implements it as f64::round_ties_even and the wasm backend as the single native f64.nearest instruction, which the wasm spec defines as IEEE roundToIntegralTiesToEven -- so the two backends agree bit for bit by construction, signed-zero ties included. It is wired through every layer a unary scalar builtin touches (parser arity table, the four AST stages, lowering, codegen, invariance classification, LTM polarity as a non-decreasing step function like INT, units checking and inference as units-preserving, the patch reprinter, and the pretty printer), always in the same group as INT. Constant folding deliberately leaves ROUND applications unfolded, matching the existing stance for ABS/INT/MIN/MAX (fold.rs: not worth a special case until measurement says otherwise). Also corrects the MCP instruction docs' description of INT, which claimed truncation; the engine implements INT as floor, and the two differ for negative arguments.
Six findings from review, each verified before fixing: - The MCP Vensim-syntax skill's INTEGER row claimed floor semantics; the in-repo Vensim DSS ground truth (test/test-models/tests/rounding/ output.tab: INTEGER(-9.9) = -9) and vensim.com's fn_integer page show INTEGER truncates toward zero. The row now states the real divergence from Simlin's floor-based INT instead of papering over it (the underlying import-semantics gap is tracked separately). - The MDL writer silently exported round(x) as ROUND(x), which Vensim does not define. It now records an ExportWarning (the transpose operator's exact contract): there is no Vensim-expressible round-half-to-even, so the call is emitted as-is and disclosed. - The BuiltinFn::Round comment framed ROUND as an unconstrained Simlin choice; Stella does define ROUND (tie rule unspecified), so the comment now cites that and marks the tie-behavior divergence risk as unverified rather than nonexistent. - The three ROUND case tables (e2e, VM apply, wasm parity) were hand-maintained copies and the wasm one had already drifted. They now drive one shared table (test_common::ROUND_TIES_TO_EVEN_CASES) with a single comparison contract, so the backends cannot drift apart. The e2e test skips only the negative-zero INPUT row, which the equation language cannot spell (unary minus lowers to 0 - x). - The engine CLAUDE.md Tests section now lists round_builtin_tests.rs. - The dead project_io.proto BuiltinId enum (referenced by no message and no code, with a comment claiming bytecode serialization that never happens) was deleted and bindings regenerated; an unreferenced enum contributes nothing to any serialized instance, so wire compatibility is unaffected.
The mdl CLAUDE.md enumerates every construct the ExportWarning channel flags; the ROUND builtin's warning joins that list.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1015 +/- ##
==========================================
+ Coverage 92.18% 92.21% +0.03%
==========================================
Files 247 247
Lines 158229 158247 +18
==========================================
+ Hits 145857 145932 +75
+ Misses 12372 12315 -57 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: c65ee3876c
ℹ️ 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".
| // Whether Stella agrees at exact .5 ties is UNVERIFIED; if it rounds ties | ||
| // away from zero (the Excel/C convention), an imported model that hits an | ||
| // exact tie diverges by 1 there. Vensim defines no ROUND at all, and the |
There was a problem hiding this comment.
For Stella-authored .stmx models, this changes ROUND from a visible UnknownBuiltin failure into ties-to-even execution while explicitly admitting that Stella's tie behavior is unverified. If Stella rounds exact halves differently, imported experiments silently diverge by one at every such value; verify the behavior against an official reference or ground-truth output and implement those semantics, or continue rejecting Stella ROUND until it is verified.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
| /// page), so the catch-all rename emits a `ROUND(...)` call Vensim rejects. | ||
| /// There is no Vensim-expressible equivalent of round-half-to-even, so the | ||
| /// writer emits the call as-is and says so rather than degrade silently -- |
There was a problem hiding this comment.
For any model using ROUND and exported through the plain project_to_mdl API, this deliberately emits a call that Vensim rejects while that API discards the warning, leaving an MDL file that cannot run. The premise that half-to-even is not Vensim-expressible is false for finite values: it can be composed from IF THEN ELSE, INTEGER, and MODULO by branching on sign, the half-integer case, and integer parity. Lower the call to those supported primitives (or synthesize an equivalent helper macro) instead of emitting invalid MDL.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
| //! ROUND is a Simlin extension: the XMILE v1.0 spec defines no ROUND function | ||
| //! (verified against `docs/reference/xmile-v1.0.html`, which contains zero | ||
| //! occurrences of the word). Simlin defines it as Python's `round()` / |
There was a problem hiding this comment.
The cited in-repo specification does not contain “zero occurrences” of round: docs/reference/xmile-v1.0.html uses the word in its precision description, including rounding to hundredths and halves. The spec may still omit a ROUND function, but this asserted verification does not establish that premise; check the function catalog and cite the exact supporting section instead of recording a demonstrably false source result.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
Code ReviewThorough review of #1015 — end-to-end ROUND builtin. The change is well-scoped, all AST/lowering/codegen/units/LTM/patch/pretty/MDL layers are updated in lockstep, and the shared I checked several potential concern areas and they held up:
Finding: [P3]
|
Three substantive points from the PR #1015 reviews: - The round_builtin_tests header claimed the XMILE spec contains zero occurrences of the word 'round'. False: the check had been run with a grep that silently skips the spec's non-UTF8 HTML as binary. A proper search finds 39 occurrences -- all prose (display precision, 'rounded rectangle', 'background') -- and no ROUND function form in the builtin catalog, so the substantive claim stands and the comment now cites the verification that actually supports it. - The MDL writer's rationale overstated 'no Vensim-expressible round-half-to-even'. It is arithmetically composable from IF THEN ELSE / INTEGER / MODULO for finite values; the writer still refuses to lower it, now for the stated real reasons: a lowering duplicates the argument expression (wrong outright for stochastic arguments, which would be re-drawn per copy) and rests on Vensim edge semantics that are unverified or known to diverge (INTEGER/MODULO, GH #610), so a subtly-wrong silent lowering is strictly worse than a loud warning. - The monotone-polarity enumeration test covered six of the seven arms of polarity.rs's non-decreasing builtin group; Round is now a row, so moving it out of that arm reds the test. On the Stella tie-semantics question: Python's ties-to-even is the product requirement regardless of Stella's (undocumented) tie rule; matching Stella is explicitly a non-goal, and the BuiltinFn::Round comment now records that as a decision rather than an open caveat.
|
Addressed the review feedback in fcf8a80:
@codex review |
|
Reviewed the changes end-to-end. The ROUND builtin is threaded correctly through every AST layer, compiler pass, analysis, and backend, with bit-exact backend parity guaranteed by construction ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcf8a8086e
ℹ️ 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".
| Ramp(Box<Expr>, Box<Expr>, Option<Box<Expr>>), | ||
| // ROUND(x): nearest integer, exact .5 ties to the EVEN neighbor | ||
| // (Python round() / IEEE roundTiesToEven). The XMILE v1.0 spec defines no | ||
| // ROUND builtin (its function catalog stops at INT, which footnote 7 |
There was a problem hiding this comment.
The corrected citation in round_builtin_tests.rs now says only that the catalog contains no ROUND entry, but this separate comment still claims the catalog “stops at INT.” Section 3.5.1 continues after INT with LN, LOG10, MAX, and other functions, so replace this with the narrower verified claim that the catalog lists INT but not ROUND; otherwise the code retains another false external-spec premise.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
| /// `ROUND(...)` as-is (there is no Vensim-expressible round-half-to-even to | ||
| /// degrade to), and the writer's job is to say so. |
There was a problem hiding this comment.
Fresh evidence in the current tree is that writer.rs now explicitly acknowledges round-half-to-even is arithmetically composable for finite values and rejects lowering because it duplicates stochastic arguments and depends on unsafe edge semantics. This test comment still states the rebutted premise that no Vensim-expressible form exists, so it should describe that deliberate safety tradeoff instead of preserving a false external-tool claim.
AGENTS.md reference: AGENTS.md:L103-L109
Useful? React with 👍 / 👎.
1 participant
Adds a
ROUND(x)builtin that rounds to the nearest integer with exact .5 ties going to the even neighbor -- Pythonround()/ IEEE roundTiesToEven -- wired through every layer of the engine plus the wasm backend.Why ties-to-even
The XMILE v1.0 spec defines no ROUND function (zero occurrences in the in-repo spec), so the semantics are Simlin's to choose. Ties-to-even matches Python and is statistically unbiased over uniformly distributed ties. Two external-tool caveats are documented at the
BuiltinFn::Roundvariant rather than left implicit:.stmxcalling ROUND now imports and simulates instead of failing withUnknownBuiltin; whether Stella agrees at exact ties is unverified.ExportWarningwhen an equation calls it (the transpose operator's exact contract): there is no Vensim-expressible round-half-to-even, so the call is emitted as-is and disclosed instead of degrading silently.Implementation
f64::round_ties_eveninapply().f64.nearestinstruction, which the wasm spec defines as IEEE roundToIntegralTiesToEven -- the two backends agree bit for bit by construction, signed-zero ties included.Tests
One shared case table (
test_common::ROUND_TIES_TO_EVEN_CASES, bit-pattern comparison contract) drives all three backend tests -- the end-to-end parse->compile->VM pipeline, the VMapply()unit test, and the wasm parity test (which executes the emitted blob under the wasm interpreter) -- so the backends cannot drift apart on rows one test carries and another omits. The exact .5 ties are the load-bearing rows:f64::round(ties away from zero) agrees on every non-tie input, so a suite without them would pass with the wrong function. Further coverage: elementwise arrays, ROUND under a reducer (temp materialization path), NaN/infinity propagation, case-insensitivity, arity errors, units preservation, MDL export warnings, and the patch print-reparse round trip.Review findings folded in
/code-reviewran on the branch; all six findings were verified and fixed:project_io.protoBuiltinIdenum (referenced by no message and no code, with a comment claiming bytecode serialization that never happens) and regenerated bindings; an unreferenced enum contributes nothing to any serialized instance, so wire compatibility is unaffected🤖 Generated with Claude Code
https://claude.ai/code/session_01XjDuCGQqFp59hGWk2vf77G