fix(expander): default output window starts at t_start, not min(0, t_start) (gh#143)#446
Open
vsbuffalo wants to merge 1 commit into
Open
fix(expander): default output window starts at t_start, not min(0, t_start) (gh#143)#446vsbuffalo wants to merge 1 commit into
vsbuffalo wants to merge 1 commit into
Conversation
…start) (gh#143)
`expand_output` defaulted the trajectory output schedule's start to
`min(0.0, t_start)`. For any model with `t_start > 0` (an anchored
`from` later than `origin`, or an unanchored `from > 0`) and no explicit
`output` block, this forced start = 0, so the trajectory was emitted —
and recorded — over [0, t_end] instead of the requested [from, to]. The
result was rows at times the dynamics never visited and, next to the
t_start prologue snapshot, non-monotonic time.
The gh#143 `min(0, t_start)` was aimed at the negative-t_start case
(a `from` before `origin`): starting at 0 leaves no snapshots in
[t_start, 0), so `--obs-only` / snap_at can't find a pre-origin
snapshot and hard-exits. But `min(0, t_start)` equals t_start for
t_start < 0 and clamps to 0 for t_start > 0 — it fixed the negative
direction and broke the positive one. `start = t_start` is correct in
both directions (and trivially at t_start = 0), so it subsumes the
negative-t_start fix rather than replacing it.
Reproduction (origin 2020-01-01, from 2020-07-01, to 2020-07-12 →
t_start=182, t_end=193; no output block):
before: `camdl simulate m.camdl --dates --stdout` emits 194 data
rows with non-monotonic t (182, then 1,2,…,193).
after: 12 data rows over [182, 193], strictly increasing t.
Golden-neutral: no committed golden has t_start > 0 (all are unanchored
or use `at = [...]`), so no IR golden or trajectory baseline changes —
verified by scanning all 108 committed *.ir.json.
TDD (red-then-green), output_schedule test 4:
# RED (current code):
[FAIL] anchored t_start>0 → output.start = t_start
ASSERT output.start = t_start (window is [from, to], not [0, to])
Expected: `182'
Received: `0'
# GREEN (after fix):
Test Successful in 0.003s. 12 tests run.
The A.2 obs/output "divergence" guard is repurposed as an *agreement*
guard: both surfaces now lower `start = t_start`, so they line up and a
future shared schedule_core lowering helper would be safe. Spec §16
synced to the fixed reality (`[t_start, t_end]`, start defaults to
`t_start`).
Full `make test` green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The default trajectory output window started at
min(0, t_start)insteadof
t_start. For any model witht_start > 0— an anchoredfromlaterthan
origin, or an unanchoredfrom > 0— and **no explicitoutputblock, this forced the window start to 0, so the trajectory was emitted
(and recorded) over
[0, t_end]instead of the requested[from, to].Why it was wrong
min(0, t_start)was the gh#143 fix for the negative-t_startcase (afrombeforeorigin): starting at 0 leaves no snapshots in[t_start, 0),so
--obs-only/snap_atcan't find a pre-origin snapshot and hard-exits.But
min(0, t_start)equalst_startfort_start < 0and clamps to 0 fort_start > 0— it fixed the negative direction and silently broke thepositive one.
start = t_startis correct in both directions (andtrivially at
t_start = 0), subsuming the gh#143 negative-t_startfixrather than replacing it.
Reproduction
Minimal model (
origin = 2020-01-01,from = 2020-07-01,to = 2020-07-12→t_start = 182,t_end = 193), nooutputblock:camdl simulate m.camdl --dates --stdoutemits 194 data rowswith non-monotonic time (
182, then1, 2, …, 193).[182, 193], strictly increasing time.Tests
TDD, red → green. New
output_scheduleunit test for the anchoredt_start > 0case:The former A.2 obs/output divergence guard is repurposed as an
agreement guard: both surfaces now lower
start = t_start, so they lineup (and a future shared
schedule_corelowering helper would be safe).Golden impact
None. No committed golden has
t_start > 0(all are unanchored or useat = [...]), verified by scanning all 108 committed*.ir.json. No IRgolden or trajectory baseline changes.
Spec §16 synced to the fixed reality (
[t_start, t_end]; start defaults tot_start).Full
make testgreen.Follow-up (not in this PR)
The landed proposal
docs/dev/proposals/2026-06-04-schedule-unification.md(§A.2) still records the old divergence and instructs a future Option-B
implementer not to factor a shared lowering helper "because it would shift
observation times." That premise is now inverted — obs and output agree — so
that guidance is stale. Left untouched here to keep this PR to one logical
change; the live regression guard (the renamed agreement test) already pins
the new invariant.