doc: XMILE conveyor support spec + test fixtures#845
Conversation
Conveyors are a first-class Stella stock type that XMILE marks optional; many in-the-wild .stmx models use them. Today simlin drops the <conveyor> block silently on import, then fails compilation with a misleading empty_equation error on the (spec-mandated) equation-less conveyor outflow, and loses the block on export -- so import-then-export corrupts a Stella model. This adds a design doc that a fresh implementer can build against. The doc separates what the XMILE 1.0 spec and isee's "Computational Details" help pages make unambiguous (syntax, the slat model, steady-state init formulas, linear/exponential leakage prose, non-FIFO placement) from what still needs a maintainer decision or a Stella reference run to pin exactly (non-integer transit_time/DT rounding, per-DT interleave, non-Euler handling, core VM-native-vs-desugar strategy, queue scope). Those are collected in a prioritized "decisions needed" section. It also maps the feature onto the actual engine: datamodel, the compatibility-critical protobuf field numbering, readers/writers, the wasmgen no-silent-fallback rule, the GH #486 Euler-only precedent, and LTM degradation. Vendors a small conveyor fixture corpus under test/conveyors/ with full provenance and license attribution (two CC BY 4.0 models from the peterhovmand COVID-19-SD-generic-structures repo plus a hand-authored minimal model). These are reference fixtures, deliberately not wired into the integration harness: none ship expected-output CSVs and the real models use unrelated unsupported features, so they become executable oracles only once conveyors are implemented and Stella reference output exists. The README records why, plus the GitHub-code-search method note (.stmx content is not indexed).
|
Docs-only PR (design spec + vendored test fixtures). I spot-checked the code references it makes — [P3] Broken intra-doc anchor links to §4.3/§4.4/§4.5/§6.4
Overall correctnessCorrect — no runtime code changes; the design spec's factual claims about the codebase check out and the CC BY 4.0 attribution is present in |
![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: 03fdc3200b
ℹ️ 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".
| - `minimal_conveyor.xmile` — hand-authored Phase-1 target (transit time + | ||
| capacity, single outflow, no leak). | ||
| - `sir_social_distancing_mixnot.stmx` — peterhovmand corpus, CC BY 4.0, | ||
| transit-time-only, deterministic; simplest real oracle. |
There was a problem hiding this comment.
When this fixture is promoted as the “simplest real oracle,” it is not enough to implement plain transit-time conveyors and LOOKUPMEAN: the Not_Mixing submodel feeds its conveyor stock with infecting marked isee:spreadflow="dist" and isee:distrib_eq (test/conveyors/sir_social_distancing_mixnot.stmx lines 331-343). That spreadflow placement is one of the deferred conveyor-input semantics documented earlier, so treating this file as transit-time-only will produce Stella mismatches unless the model is trimmed or the README/design doc calls out this blocker.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #845 +/- ##
==========================================
- Coverage 90.91% 90.88% -0.04%
==========================================
Files 225 225
Lines 144306 145049 +743
==========================================
+ Hits 131199 131823 +624
- Misses 13107 13226 +119 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rework the conveyor design doc from a research memo with open decisions into a complete, implementable specification, and address the PR review. The doc now commits to concrete algorithms and formulas rather than deferring: the exact per-DT update order, the linear and exponential leakage math (with leak zones and integer leakage), non-integer transit-time rounding (N = round(T/DT), effective transit N*DT, compile Warning), the steady-state initialization (a general unit-cohort forward-simulation plus closed forms), capacity/inflow-limit pushback, variable transit time with <sample>, all five isee spread-input placement methods, arrayed conveyors, the conveyor side of queue coupling, a VM-native runtime design, and an Euler-only hard requirement (GH #486 precedent). The prior "decisions needed from the maintainer" section is gone; what remains is logistics (cross-engine confirmation, diagram authoring), not spec holes. To validate the spec rather than assert it, the per-DT algorithm, leakage, capacity/limit, and initialization were transcribed into an executable reference prototype (test/conveyors/reference_prototype.py) and exercised over seven scenarios: steady state, fill-from-empty transit delay, linear leak (outflow/inflow = 1-f), exponential leak (matches 250*(1-f*dt)^N closed form), capacity clip, inflow-limit clip, and non-integer transit rounding. All checks pass; the trajectories are recorded as the acceptance oracles in the doc's Worked Examples section, so the core continuous conveyor can be test-driven without Stella access. Review fixes: correct the broken intra-doc anchor links flagged by the Claude review, and address Codex's point that sir_social_distancing_mixnot.stmx is not transit-time-only -- its Not_Mixing submodel uses isee:spreadflow="dist" distribution-based inflow placement (now a fully specified feature, §8), with LOOKUPMEAN called out as the separate remaining builtin gap.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cd7b81006
ℹ️ 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".
| are updated, run this sequence for each conveyor, in topological order of the | ||
| conveyor chain (upstream conveyors before downstream, so a downstream conveyor's | ||
| pull is visible to its upstream source this same step): |
There was a problem hiding this comment.
For a conveyor A feeding a capacity-constrained conveyor B, updating upstream first means A's step 3 removes its exit slat before B's step 4 knows how much it can admit. If B admits less than A's available outflow, the spec can no longer keep the blocked material upstream because it has already been removed from A's side state; this needs a downstream-pull/two-phase reservation order rather than upstream-before-downstream for conveyor chains.
Useful? React with 👍 / 👎.
| self.slats.append(Slat(0.0, 0.0)) | ||
| tgt = self.slats[d - 1] | ||
| tgt.content += admitted | ||
| tgt.entry_amount = admitted |
There was a problem hiding this comment.
When latched_transit shrinks, the new entry depth can point at a slat that already contains an older cohort; this code adds the new content but replaces the slat's entry_amount, so later linear leakage for the combined slat is based only on the newly admitted volume. That corrupts leakage totals for variable-transit conveyors; either accumulate the entry amount for merged cohorts or represent multiple cohorts per slat.
Useful? React with 👍 / 👎.
| in_carry: float = 0.0 | ||
|
|
||
| def N(self): | ||
| n = round(self.latched_transit / self.dt) |
There was a problem hiding this comment.
The design doc specifies half-away-from-zero rounding for T / DT, but Python's round() uses bankers rounding, so a half-slat case such as T / DT == 16.5 produces 16 here while the spec/Rust f64::round() would produce 17. Since this prototype is described as the executable acceptance oracle, half-step transit fixtures would lock in the wrong effective transit time.
Useful? React with 👍 / 👎.
ReviewScope: docs-only PR (design spec + vendored test fixtures + a Python reference prototype). No engine behavior changes. Reviewed the spec's per-DT algorithm (§4.3), leakage math (§5.1–§5.4), initialization (§7.1), capacity/inflow-limit accounting (§6.3), and cross-checked against
Only minor notes, none blocking:
Overall correctness verdictCorrect. No blocking issues — this is a design document plus vendored, well-attributed fixtures. Both minor notes above are edge-case improvements to nail down before the code lands, not defects in what's being merged here. |
Address the second round of Codex review, each finding fixed at its root: Conveyor chains: the previous upstream-before-downstream ordering was unsound for a conveyor feeding a capacity-constrained conveyor (the upstream pops its exit slat before the downstream decides admission). Rather than a two-phase reservation protocol, adopt the isee-documented structural rule that a conveyor-driven flow is never blocked by its destination (capacity transiently exceeded, in_limit inapplicable; blocking between conveyors requires a queue, matching isee guidance). With back-pressure gone the pass becomes two phases -- leak+exit from start-of-step state, then admit+shift+insert -- and iteration order within each phase is irrelevant, so chains and even conveyor cycles are deterministic with no topological sort. The lone exception (arrested destination holds material at the upstream exit) depends only on arrest flags known before the pass, preserving order-freedom. Cohort merging: inserting into an occupied slat (shortened transit) replaced entry_amount, corrupting linear-leak totals. Replaced entry_amount with a per-cohort fixed leak schedule (per-DT allowance + remaining budget) computed at insertion, which merges by summation -- exact, since linear leak is linear in the entering volume. This also pins the previously drifting M denominator (now fixed at entry, matching isee's "based on the transit time that was used when the material was put on") and gives spread-input cohorts their prorated budgets for free. Rounding: Python's round() is banker's rounding, so the prototype disagreed with the spec's half-away-from-zero at exact half-slats (16.5 -> 16 vs 17). The prototype now uses floor(x + 0.5) and S7 pins the half case. Also newly specified while here: a unified discrete-conveyor section (quantized admission with carry, per-time-unit inflow window, start-of-unit init) and a per-step conservation identity (admitted - out - leak = delta-contents). The reference prototype now runs every scenario through the two-phase pass with conservation asserted on every step, and adds S8 (chain into a capacity-constrained conveyor: transient exceedance, exact chain conservation) and S9 (transit shrink with merging: conservation and the lifetime leak budget bound).
Address all 19 findings from an adversarial model review of the conveyor spec. The one numerics bug (finding 1): a cohort's linear-leak allowance was denominated by the physical belt length, so after a transit shrink new entry cohorts leaked only a fraction of the documented f (stale tail inflated the denominator) -- contradicting the spec's own prose and the isee rule it quotes. The schedule is now path-based: alloc = f*A / M(entry path d), budget = alloc * M(own path d_c), so a default-placement cohort leaks exactly f*A over its own journey regardless of belt length, and spread-input shares degrade to the prorated isee rule through the same formula. S9 now pins the regression (post-shrink steady outflow returns to (1-f)*inflow). Precision fixes that would each have cost an implementer a debugging session: sample/latch is now step 0 of the per-DT algorithm (arrested conveyors do not re-latch); step 6 always executes so the belt never shrinks below the entry depth; primary-outflow selection honors XMILE's leak-marking override (first NON-leak outflow, with no-outflow/all-leak compile errors); driven-flow visibility is pinned to normal VM flow/stock timing (readers dependency-order after the conveyor pass; the stock value reads start-of-step, breaking cycles); a runtime-hygiene section defines clamps for non-finite/negative expression values; quant_carry is distinct from the boundary-resetting in_carry; all five spread placements get per-slat formulas (positions, normalization, fallbacks); scalar container access and array builtins over the belt are specified with the OOB->NaN rule while the isee cycle-time builtin family is explicitly de-scoped; explicit-list init is disambiguated by list length; and a new 9.8 enumerates diagnostics, unit rules, VM reset/module-instance lifecycle, PREVIOUS/INIT semantics, and double-clamp composition. Honesty fixes: the worked-examples section now records the prototype's exact coverage (executed vs prose-only rules) and its reporting convention (start-of-step stock values matching simlin CSV semantics, previously mixed); the capacity formula is documented as extending -- not matching -- isee's by also crediting leak. The prototype gains S10, executing the held-exit merge (the one structurally novel rule review found unexecuted): an arrested destination accumulates material at the upstream exit and releases it as one lump, conserving exactly. 17/17 checks pass.
Three corner fixes from the reviewer's verification pass, none material: cap a dest-placement share's leak budget at min(M(d_c), M(d)) so a share landing on a stale-tail slat beyond the entry depth cannot schedule more than the documented f*A (and scope 5.1's d_c <= d terminology accordingly); define the time-unit block count as floor((N-1)*DT) + 1 so a non-divisor DT (e.g. 0.3) never produces an empty block with no deepest slat to fill; and reword 9.3's update-hook ordering to defer to 4.3's visibility rules instead of the looser "after ordinary flow equations" phrasing.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45da89b655
ℹ️ 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".
| 1. **Quantized admission.** After step 4 computes `admitted`, a discrete | ||
| conveyor adds it to `quant_carry` ([§4.2](#42-conveyor-runtime-state)) and | ||
| actually inserts `floor(quant_carry)` whole units, retaining the fractional | ||
| remainder (same carry pattern as `<leak_integers/>`, |
There was a problem hiding this comment.
For a discrete conveyor with a tight capacity, flooring the accumulated quant_carry after step 4 can insert more than the current cap_room allowed because prior fractional carry was not included in the capacity check. For example, with quant_carry = 0.9 and cap_room = 0.2, step 4 admits only 0.2, but floor(1.1) inserts a full unit, so an equation-driven inflow violates the §6.3 capacity bound. The quantized whole-unit insert needs to be capped/rechecked against the current capacity room before reporting/removing that material.
Useful? React with 👍 / 👎.
| stock's units `S` and the model's time unit `t` — `<len>` and `<sample>` | ||
| context: `t`; `<capacity>`: `S`; `<in_limit>`: `S/t`; a linear leak fraction: | ||
| dimensionless; an exponential leak rate: `1/t`; `<arrest>`: dimensionless. |
There was a problem hiding this comment.
This unit rule gives <sample> time units, but the syntax table and XMILE prose describe <sample> as a conditional expression evaluated for nonzero/truth. A real condition such as TIME > 10 type-checks as dimensionless in Simlin, so following this rule would reject valid sample predicates while <arrest> uses the correct dimensionless treatment. Only <len> should require time units here; <sample> should be checked like a condition.
Useful? React with 👍 / 👎.
|
Reviewed the doc and reference prototype. Traced the algorithms (two-phase update including held exit / merge, cohort schedule with entry-vs-own-depth budget, steady-state init, non-integer transit rounding) and verified the S1–S10 numeric expectations against closed forms. No blocking issues found; scope disclaimers around untested code paths (discrete quantization, integer leakage, spread placements, explicit-list init) are explicit. Ready to ship as the design artifact for the implementation phase. Overall correctness: correct. |
Address the third round of Codex review, both findings confirmed real. Quantized admission could breach capacity: quant_carry accumulated across steps was never capacity-cleared as a whole unit, so floor(carry + clearance) could insert a unit into room step 4 had cleared only fractionally (carry 0.9, cap_room 0.2 -> a full unit lands in 0.2 of room). Rule 1 now inserts units = min(floor(quant_carry), floor(cap_room)) -- the unit waits until a whole unit of room exists -- with the in_limit window still accounted at clearance time, un-inserted carry never taken from upstream, and conveyor-driven inflow bypassing quantization (never-blocked rule; integral anyway when the upstream is discrete, as the queue-fed case mandates). The prototype now implements quantized admission and S11 executes exactly this corner (fractional requests against capacity 3: contents stay integral, never exceed capacity, and flow does not deadlock), moving discrete quantization from prose-only to machine-verified; 20/20 checks pass. The 9.8 unit rule gave <sample> time units, but sample -- like arrest -- is a condition evaluated for nonzero: a predicate such as TIME > 10 type-checks dimensionless, so the time-units rule would reject valid sample expressions. Only <len> requires time units; sample and arrest are dimensionless.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 545556e6b0
ℹ️ 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".
| be the normal outflow). A conveyor outflow MUST NOT carry a normal equation- | ||
| valued `<eqn>` — but note the leak-fraction encoding below. Real Stella models |
There was a problem hiding this comment.
The strict rejection here conflicts with the Stella fixtures added in this commit: normal conveyor outflows carry placeholder <eqn>0</eqn> values in test/conveyors/sir_social_distancing_mixnot.stmx and test/conveyors/covid19_severity.stmx. If the implementation follows this literally, those newly vendored reference models and similar real Stella exports remain unsimulatable even after conveyor support lands; the reader should preserve but ignore zero/placeholder equations on conveyor-driven primary outflows, or the fixtures/spec need to be changed together.
Useful? React with 👍 / 👎.
| |---|---| | ||
| | `beginning` (default) | `A_i = A` for `i = d−1` (the entry slat), 0 elsewhere: one cohort at depth `d`. | | ||
| | `even` | `A_i = A / d` for every `i ∈ 0..d−1` — including the exit slat, whose share exits after one DT (isee: "equal amount every position"). | | ||
| | `dest` | `A_i = A × content_i / Σ_j content_j`, summed over **all** slats of the current belt (length `L`, not just the first `d`) — proportional to existing content. If total content is 0, fall back to `beginning`. | |
There was a problem hiding this comment.
When dest runs after a transit shrink, §5.1 explicitly anticipates a share landing on stale-tail slats beyond d, but this row normalizes over all current belt content while §8 defines target shares only for i ∈ 0..d−1. In that stale-tail case, content beyond d contributes to the denominator without receiving an A_i, so an implementation would either lose admitted material (Σ A_i < A) or have to invent behavior; define dest over all 0..L−1 slats or normalize only over the first d.
Useful? React with 👍 / 👎.
|
Reviewed the design doc, reference prototype, and vendored fixtures. This is a docs + test-fixtures PR — no engine code changes. Verified as accurate:
Minor: doc §3.1 calls No blocking findings. Overall correctness: correct. |
Address the fourth round of Codex review, both findings confirmed against the vendored fixtures. Primary conveyor outflows in real Stella exports carry a placeholder <eqn>0</eqn> (plus <non_negative/>) despite XMILE's MUST NOT -- both vendored fixtures do. A literal reading of the spec's prohibition would leave those models unsimulatable even after conveyor support lands. The reader now preserves-but-ignores an <eqn> on a primary conveyor outflow (round-trip fidelity, no role in simulation, never an error), while the writer emits the spec-strict form, consistent with the existing non_negative writer rule. Only a leak-marked flow's <eqn> is meaningful (the leak fraction), and the 9.3 compiled-representation bullet now reflects that both spellings compile to the driven-by-conveyor marker. dest placement normalized over all belt content but defined shares only for the first d slats, so content on a stale tail contributed to the denominator without receiving a share -- admitted material would be silently lost (sum A_i < A). dest now targets the whole physical belt 0..L-1 (an empty slat's share is 0 by construction, sum A_i = A exactly); a share landing beyond d is exactly the d_c > d case the 5.1 min-cap already schedules. The section 8 preamble now states the no-material-lost invariant explicitly.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e2333eac9
ℹ️ 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".
| 1. **Quantized admission.** Step 4's *equation-driven* clearance accumulates | ||
| rather than inserting directly: `quant_carry += eq_admitted` | ||
| ([§4.2](#42-conveyor-runtime-state); `quant_carry` never resets — it is | ||
| distinct from the boundary-resetting `in_carry`), then | ||
| `units = min(floor(quant_carry), floor(cap_room))` whole units are inserted |
There was a problem hiding this comment.
For a discrete conveyor with multiple equation-driven inflows, or with an inflow request that drops to 0 after fractional clearances accumulated, this conveyor-level quant_carry no longer identifies which upstream flow owns the eventual whole unit. Step 4 promises listed-order apportionment and line 573 says uninserted carry was not taken upstream, but a later floor(quant_carry) insertion would have to report/debit some inflow anyway, misattributing or creating material. The spec needs per-inflow/source carry or an insertion-time apportionment rule before this can be implemented safely.
Useful? React with 👍 / 👎.
| 0. **Latch.** Evaluate `<sample>`; when it is nonzero (and the conveyor is not | ||
| arrested — an arrested conveyor's time is suspended, so it does not | ||
| re-latch), update `latched_transit` from `<len>` (with the runtime hygiene of | ||
| [§4.4](#44-runtime-expression-hygiene)). This happens before any Phase-A | ||
| mutation, so this step's insert (step 6) uses the newly latched depth. |
There was a problem hiding this comment.
In a step where <arrest> and <sample> are both nonzero and <len> changed, this ordering asks implementers to update latched_transit before <arrest> is evaluated, even though an arrested conveyor's time should be suspended and not re-latch. Following the numbered algorithm would freeze material but still change the post-release entry depth, so the phase should precompute arrest before the latch or reorder these steps.
Useful? React with 👍 / 👎.
Review verdict: correctReviewed the design doc and reference prototype for internal consistency (no engine behavior changes in this PR). What I verified:
Overall correctness: correct. No qualifying findings. |
Address the fifth round of Codex review; both findings judged material. Per-inflow quantization: a single conveyor-level quant_carry could not say which upstream flow owns a whole unit that inserts on a later step -- with two equation-driven inflows, or one whose request drops to 0 after fractional clearances accrued, an implementer would have to invent an attribution rule and a wrong one creates or destroys material at an upstream stock. The carry is now per equation-driven inflow: step 4's listed-order clearance accrues to each inflow's own carry, and insertion walks the inflows in listed order under a shared floor(cap_room) budget, each inflow reporting exactly its own inserted units. The prototype implements the rule (harness extended to multiple inflows per conveyor) and S12 pins the bookkeeping identity cum_cleared_j == cum_reported_j + carry_j at every step, the post-shutoff residual bound, and integral per-inflow totals; 23/23 checks pass. Arrest-vs-latch ordering: step 0 (latch) consumed the arrest flag that step 1 formally computed -- the numbered algorithm contradicted its own prose, and in a step where arrest, sample, and a <len> change coincide a literal reading would freeze the material yet still change the post-release entry depth. The steps are swapped: arrest is decided first and an arrested conveyor skips everything including the latch.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 368e8750ad
ℹ️ 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".
| matching isee's "the amount that is leaked is based on the transit time that was | ||
| used when the material was put on the conveyor" (`f` itself is likewise sampled | ||
| once, at the cohort's entry DT — [§4.4](#44-runtime-expression-hygiene)). |
There was a problem hiding this comment.
For a linear leak whose <eqn> changes over time, this freezes f for each cohort at entry, but isee's Traditional Conveyor Computation says current leak-fraction values are used when computing leakages. That means models where attrition/mortality/testing rates vary during the run would keep leaking old cohorts at stale rates and fail Stella parity; the entry-time value should be the transit-time/slat schedule, not the leak equation value itself.
Useful? React with 👍 / 👎.
| Overlapping zones from multiple exponential flows compound (each applied in | ||
| step-2 order to the running content). Exponential leakage carries no per-cohort |
There was a problem hiding this comment.
When two exponential leak zones overlap, applying each leak to the running remaining content makes later leak flows too small; for example two 0.1/time leaks at DT=1 remove 0.19 of a slat instead of the 0.20 Stella computes by adding overlapping rates. isee's Traditional Conveyor Computation specifies that overlapping exponential leakage fractions are added, so implementing this order-dependent rule would produce wrong reference results for multi-leak exponential conveyors.
Useful? React with 👍 / 👎.
Review summaryThorough spec + fixtures with no correctness issues found. Design doc. Traced the mathematical claims: the linear/exponential closed-form steady states in §7.1 ( Reference prototype. The two-phase Phase A / Phase B split, held-exit merge, discrete per-inflow Fixtures. Overall correctness: correct. No blocking issues. |
Address the sixth round of Codex review; both findings verified against the
cited isee Traditional Conveyor Computation page (fetched and read in full)
and judged material.
Current-values rule: the spec froze a linear leak fraction at cohort entry,
but isee states "leak fractions can change over time and the current values
will be used when computing leakages" -- what is fixed at entry is the
transit-derived schedule (isee's own worked example: transit 5 with a 50%
leak sheds 10% per time period). The per-cohort state is now a fraction-free
basis (A / M(entry path), volume per in-zone DT) and a travel window
(basis x in-zone DTs), with the CURRENT f multiplying the basis each DT.
Merging still sums exactly (both fields linear in entering volume) and every
constant-f trajectory is bit-unchanged, which S1-S12 confirm. S13 pins the
new behavior: a fraction stepping 0.2 -> 0.4 doubles the leak rate for ALL
cohorts at the switch step and re-equilibrates the outflow at (1-0.4)*inflow.
Additive exponential: overlapping exponential leaks were specified as
sequential compounding on the running remainder, but isee adds overlapping
rates ("if the leak zones overlap the leakage fractions will be added"; all
exponential leakages "are computed always") -- and the spec's own 7.1
initialization already computed the additive form, an internal inconsistency
a single-leak scenario could not catch. Every exponential flow now computes
from the slat's start-of-step content (order-independent), scaling
proportionally if the sum would overdrain. S14 pins it: two 0.1/time leaks
behave exactly like one 0.2/time leak, each reporting half.
Reading the isee page also surfaced a divergence Codex did not flag: for
STAGGERED linear leak zones, isee's default applies f to material remaining
at zone start while OASIS XMILE defines f against the inflowing amount.
simlin implements the XMILE reading (identical for the common
identical-zones case); section 5.1 now documents the delta explicitly as a
cross-engine check item. 27/27 prototype checks pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e86bc289e2
ℹ️ 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".
| - *Discrete conveyor:* the full per-time-unit budget may enter within a single | ||
| DT. `in_carry` tracks volume admitted since the last integer time boundary | ||
| and resets to 0 when the simulation clock crosses an integer time unit; | ||
| `limit_vol = in_limit − in_carry`. |
There was a problem hiding this comment.
When in_limit is time-varying, a discrete conveyor can spend part of the current integer-time budget and then see in_limit drop below in_carry before the boundary reset. This formula then makes limit_vol negative, and the next line's min(req_vol, cap_room, limit_vol) admits a negative volume/reported inflow despite conveyor inflows being uniflows; clamp the remainder to max(0, in_limit - in_carry).
Useful? React with 👍 / 👎.
|
Reviewed the spec and reference prototype. This is a docs+fixtures-only PR with no engine behavior changes, so no correctness-blocking findings. The §4–§7 algorithms are internally consistent with the reference prototype's implementation and every closed-form check I spot-verified (S3 contents = 906.25, S4 outflow = 250·(1−fDT)^N, S6 = in_limit·T, S10 release lump = 250/DT = 1000) matches. Conservation invariant is asserted on every step of every scenario. Overall correctness verdict: correct — no blocking issues. |
Adopt the maintainer's precedence rule as a load-bearing part of the spec: where OASIS XMILE prose and documented or observed Stella behavior conflict, simlin follows Stella. Conveyors exist in exactly one implementation that real models were built against; the spec is a description of it, and the description is far more likely to be imperfect than any independent implementation of the description is to match Stella. Applying the rule flips three earlier spec-over-Stella choices. Staggered linear leak zones now use isee's default basis -- each fraction applies to the material REMAINING at its zone start, encoded as a projected retained factor r_k in the cohort schedule (merge-additivity preserved; r_k = 1 for identical/full zones, so every constant-f trajectory is unchanged). isee's "Ignore losses from earlier leak zones" toggle (r_k = 1, which is also the XMILE inflowing-amount reading) rides a new ignore_earlier_zone_losses field. S15 executes isee's own worked example: two 0.5 leaks on [0,.5] and [.5,1] remove 75 percent, flows reporting 125 and 62.5 -- moving staggered/partial zones from prose-only to machine-verified; 29/29 checks pass. The writer rules flip from spec-strict to preserve-as-read (placeholder <eqn> and <non_negative/> on conveyor-driven flows round-trip; simlin never invents them), and the capacity formula is re-framed as implementing isee's "total outflow volume" (leakage flows are outflows) rather than extending it. The cross-engine section now states that a Stella disagreement is a spec bug to fix, not a delta to document, and lists the interpretation points to probe first.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b92ec86a3e
ℹ️ 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".
| 2. Let `S = Σ_i c[i]`. Set the cohort scale `E = V / S` (or `E = 0` if `S = 0`). | ||
| 3. Initialize each slat `i` as a cohort of entering volume `E` that has already | ||
| traveled to position `i`: `content = E × c[i]`, | ||
| `leak_basis[k] = E / M_k`, and `leak_window[k] = leak_basis[k] ×` |
There was a problem hiding this comment.
For staggered leak zones, this initialization formula drops the r_k retained-at-zone-start factor that §5.1 uses for newly inserted cohorts and that reference_prototype.py applies in unit_basis. If an implementer follows this text, a model like S15 initializes later-zone schedules from E / M_k instead of E * r_k / M_k; the second 0.5 leak then leaks against the original entry amount rather than the surviving half, so the initial belt is not at the documented 75%-leak steady state. Please carry r_k through this basis calculation (and the corresponding window) or explicitly delegate to the §5.1 cohort schedule.
Useful? React with 👍 / 👎.
[P3]
|
Address the seventh round of Codex review: two spots where the doc text lagged the reference prototype (which already implemented both correctly), i.e. exactly the divergence an implementer following the prose would ship. The discrete inflow-limit remainder is now max(0, in_limit - in_carry): with a time-varying in_limit dropping below the budget already spent this window, the unclamped form went negative and min(req, cap, limit) admitted a negative volume, violating the uniflow rule. The 7.1 initialization step now states that each slat carries exactly the 5.1 schedule of an entry cohort -- including the r_k zone-start-remaining factor introduced for staggered zones -- rather than the bare E / M_k basis, which for an S15-shaped model would have initialized later-zone flows against the original entry amount instead of the surviving material, starting the belt off its documented steady state.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Reviewed this docs+fixtures PR — no bugs to flag. Traced through the reference prototype scenarios S3 (linear-leak steady state), S9 (transit shrink with cohort merge, incl. the finding-1 regression that new post-shrink cohorts still leak the full fraction), and S15 (staggered zones with r_k projection resolving to 0.5, giving the claimed 125/62.5/62.5 rates). Confirmed the doc's factual claims about current HEAD ( |
1 participant
What
Adds
docs/design/conveyors.md— a spec-and-requirements document for XMILE conveyor support that a fresh implementer or agent can build against — plus a small vendored conveyor test-fixture corpus undertest/conveyors/.Why
Conveyors are a first-class Stella stock type; XMILE marks them optional but many in-the-wild
.stmxmodels use them. Today simlin fails them silently and confusingly:<conveyor>block (the XMILE reader struct has no field for it).empty_equationerror on the conveyor's outflow (which per spec MUST NOT have an equation — the conveyor drives it).<uses_conveyor/>header survives while the stocks silently become plain stocks).Verified against HEAD with the vendored
minimal_conveyor.xmile.The doc
Sourced from the OASIS XMILE 1.0 spec (behavioral prose) and isee's "Computational Details" help pages (the only place the per-DT math is documented). It deliberately separates:
DECISION:) or needs a Stella reference run (EXPERIMENT:) — collected and prioritized in a "decisions needed from the maintainer" section. The load-bearing ones: core implementation strategy (VM-native conveyor object vs. compile-time desugar to a slat aging chain), oracle generation (do we have Stella to produce reference CSVs?), non-integertransit_time/DThandling, and non-Euler integration policy.It also maps the feature onto the actual engine: datamodel, the compatibility-critical protobuf field numbering, readers/writers, the wasmgen no-silent-fallback rule, the GH #486 Euler-only precedent, LTM degradation, and a Phase 0→4 rollout.
Test fixtures
test/conveyors/vendors a curated subset with full provenance and license attribution:minimal_conveyor.xmile— hand-authored Phase-1 target (transit time + capacity, single outflow, no leak).sir_social_distancing_mixnot.stmx,covid19_severity.stmx— from the peterhovmand COVID-19-SD-generic-structures repo (CC BY 4.0; authored by isee/Ventana principals, so authoritative), pinned to a commit. Transit-time-only and leakage+arrayed respectively; both deterministic.These are reference fixtures, not wired into
tests/integration/main.rs. None ship expected-output CSVs, and the real models use unrelated unsupported features (LOOKUPMEAN, some unit errors,isee:spreadflow). They become executable oracles only once conveyors are implemented and Stella reference output exists — the README records this, the per-file feature/blocker matrix, and the GitHub-code-search method note (.stmxcontent isn't indexed).No open-source model was found exercising
<capacity>,<discrete>,<in_limit>,<arrest>, or upstream queues; those phases will need hand-authored fixtures.Scope
Documentation and test fixtures only — no engine behavior changes. This is the design artifact that precedes implementation.
🤖 Generated with Claude Code