Fix five hybrid-sim per-interval sample-grid bugs

[PhotonicVelocity]

Branch: fix/hybrid-sim-loop

This PR fixes five bugs in the per-interval simulation loop that compromise out.t, out.x, watch logs, and any graphics output (live window or MP4 writer) for diagrams with continuous state. Each fix is a self-contained commit with its own context below.

Four of the bugs were uncovered while debugging a downstream MP4 workflow where a 5 s simulation rendered as a 6.9 s video at 30 fps. The fifth was uncovered during this PR's review of the residual-interval path — same observable symptom as fix 3, reached via a different upstream trigger.

All five fixes ship with regression tests that pin the externally observable behavior on out.t (see Verification).

Summary of fixes

#	Commit	Symptom	Root cause
1	9548365	out.t contains duplicate adjacent timestamps and len(out.t) exceeds T*fps + 1 for any hybrid run where 1/fps is an integer multiple of dt. The inflation begins ~3 s into the run at fps=30; effective sink-tick rate doubles past that point (e.g. 207 samples over 150 intervals for T=5).	_anim_frame rescheduled itself via t + _dt; floating-point error in t accumulated until result.t[0] drifted out of isclose(atol=1e-15) with t0 in the per-interval dedup, breaking the dedup and double-logging samples.
2	1caebd1	out.t[0] is 1/fps, not 0 — the t=0 initial-condition sample is missing from out.t, out.x, watch logs, the live animation window, and any movie writer output. Affects every diagram with continuous states (bd.nstates > 0).	The interval-overlap dedup at the top of _interval_hybrid was correct for intervals 2+ but unconditionally skipped result.t[0] even on the very first interval, when there was no previous interval to have logged the sample. The discrete-only path at the top of BDSim.run already had a pre-pass for this; the hybrid path didn't.
3	24f8e1c	On runs with non-dt-aligned scheduled boundaries (e.g. dt=0.1, fps=3), each such boundary triggered a residual solve_ivp call that emitted every natural integrator step into result.t — 100s of sink-ticks between two clean dt-grid samples. Visible as a dense per-max_step cluster of artist updates before each boundary.	Two coupled bugs: (a) _interval_hybrid returned treached = result.t[-1] (the last OUTPUT sample), not the integrator's actual reach — so natural completion looked like "early termination" whenever t_eval didn't include t1; (b) the residual call built an empty t_eval grid, fell through to a t_eval-less solve_ivp call, and emitted every internal step.
4	3b0ada1	MP4 had one frame per sink-tick instead of one per fps-paced display refresh — so a 5 s sim recorded ~207 frames of "what the artists looked like during the sink loop" at the writer's nominal 30 fps.	GraphicsBlock.step called writer.grab_frame() unconditionally on every sink tick. The live window worked because flush_events() (which actually presents the canvas to screen) only fires at the fps cadence from _anim_frame → display_manager.refresh(); the movie writer had no equivalent presenting hook.
5	9b03bcb	out.t contains a dense max_step-spaced cluster inside any interval too small to contain a k*dt multiple — event-terminated residuals, mismatched (dt, fps, clock_period) combinations, tf near a scheduled boundary, or near-simultaneous events. Same t_eval=None → solve_ivp natural-step dump path fix #3 closed for one trigger; reachable through four others (detailed below).	_build_t_eval_grid returns None for empty intervals; the call site treats None as "don't set t_eval", and solve_ivp dumps every internal integrator step into result.t.

Per-commit detail

9548365 — fix(run_sim): snap _anim_frame to k*dt grid to prevent drift

Issue. Two grids of "tick times" coexist during a hybrid sim and are supposed to coincide whenever 1/fps is an integer multiple of dt (so every anim_frame tick lands on a k*dt grid point — dt = 1/fps is just the N=1 case):

• The t_eval grid, built per interval by _build_t_eval_grid as dt * np.arange(k0, k1 + 1) — locked to world time (k * dt), rebuilt fresh each interval, one IEEE multiplication per point.
• The _anim_frame grid, built incrementally — the eventq callable reschedules itself via t + _dt each time it fires, so tick k's scheduled time is the result of k-1 sequential additions starting from the previous tick's fire time.

For the first few ticks the two agree to the bit:

  k | t_eval (k*dt)          | anim_frame (accumulated) | drift
----+------------------------+--------------------------+--------
  1 | 0.03333333333333333    | 0.03333333333333333      | 0
  2 | 0.06666666666666667    | 0.06666666666666667      | 0
  3 | 0.1                    | 0.1                      | 0
  4 | 0.13333333333333333    | 0.13333333333333333      | 0
  5 | 0.16666666666666666    | 0.16666666666666666      | 0
  6 | 0.2                    | 0.19999999999999998      | -2.78e-17

Sequential addition accumulates rounding error. The interval-overlap dedup at the top of _interval_hybrid compares result.t[0] (sourced from the k*dt grid) against t0 (the animframe tick time) with np.isclose(rtol=0.0, atol=1e-15). Once the anim_frame value drifts _below k*dt by more than 1e-15 — which it does around t ≈ 3.1 for fps=30 — np.clip no longer pulls result.t[0] down onto t0, the dedup check fails, and the interval-start sample gets double-logged.

  k | t_eval (k*dt)          | anim_frame (accumulated) | drift
----+------------------------+--------------------------+----------
 92 | 3.0666666666666664     | 3.0666666666666655       | -8.88e-16
 93 | 3.1                    | 3.0999999999999988       | -1.33e-15  ← dedup breaks
 94 | 3.1333333333333333     | 3.133333333333332        | -1.33e-15
 95 | 3.1666666666666665     | 3.166666666666665        | -1.33e-15

Every subsequent interval drifts slightly further from the k*dt grid, causing integration points to be doubled.

Demo. Trivial integrator at fps=30, dt=1/30, run out to T=3.5 — past the ~t=3.1 drift threshold. Without the fix, len(out.t) overshoots the expected 105 and out.t shows a doubled boundary entry for each interval after the threshold at 3.1s.

import bdsim

FPS = 30
DT = 1 / FPS

sim = bdsim.BDSim(animation=True, animation_rate=FPS)
bd = sim.blockdiagram()

bd.connect(bd.CONSTANT(1.0), bd.INTEGRATOR(0.0))
bd.compile()

out = sim.run(bd, T=3.5, dt=DT)

# Should get 3.5 * 30 = 105 frames
expected = 105
actual = len(out.t)
print(f"samples: expected {expected}, got {actual}")
for i, t in enumerate(out.t):
    print(f"  [{i:3d}] {t!r}")
assert actual == expected, f"BUG: {actual - expected} extra samples (drift)"

Fix. Snap the next event time to the canonical k * _dt grid by recomputing the tick index from the current t (round(t / _dt) + 1). No accumulation, no drift, and the dedup keeps working as designed. Demo script shows exactly 105 samples, cleanly spaced on k*dt.

Test. tests/test_run_sim.py::HybridSimSampleGridTest::test_no_drift_doubled_samples.

1caebd1 — fix(run_sim): capture t=0 IC sample in hybrid-diagram runs

Issue. For diagrams with continuous states, the very first sample at t=0 is never logged. The interval-overlap dedup at the top of _interval_hybrid unconditionally skips result.t[0] when it matches t0, and t0 = 0 on the first call. The discrete-only path at the top of BDSim.run has a pre-pass that handles this; the hybrid path doesn't. Net effect:

• simstate.tlist[0] is t = 1/fps, not 0
• the live window's first visible frame is the t = 1/fps state
• the MP4 writer's first grabbed frame is also t = 1/fps
• watch logs are missing the IC sample for every continuous-state diagram

Demo. Same trivial integrator rig — first sample should be at t=0 but isn't.

import bdsim

sim = bdsim.BDSim(animation=True, animation_rate=30)
bd = sim.blockdiagram()
bd.connect(bd.CONSTANT(1.0), bd.INTEGRATOR(0.0))
bd.compile()

out = sim.run(bd, T=1.0, dt=1 / 30)
assert out.t[0] == 0.0, f"BUG: first sample is t={out.t[0]}, expected 0.0"

Fix. Mirror the pre-existing t=0 evaluation that the discrete-only path already does so hybrid diagrams get the same initial-condition coverage: evaluate at t = 0, y = x0 (no integration done — we're reading the diagram's outputs from the IC), then call _record_sample_and_service_hooks(0, x0) so the IC lands in tlist, xlist, watch logs, and sinks.

After that, also call display_manager.refresh() once so the IC state gets out of the canvas buffer: flush_events() presents it to the live window, and grab_frame() writes it into any movie writer. (Without this, the canvas buffer is current at t=0 from the pre-pass's canvas.draw(), but the refresh hook only normally fires from _anim_frame, scheduled at interactive_dt not 0.)

With this fix, the interval-dedup invariant — "every interval's result.t[0] was already logged by the previous step" — becomes genuinely true for all intervals, including the first.

Test. tests/test_run_sim.py::HybridSimSampleGridTest::test_out_t_starts_at_zero.

24f8e1c — fix(run_sim): treat solve_ivp natural completion as reaching t1

Demo. dt=0.1, fps=3 makes the anim_frame boundaries (1/3, 2/3) miss the dt grid. max_step=1e-3 forces the residual solve_ivp call to emit ~33 internal steps per boundary. Expected: 13 samples (11 on the dt grid + 2 boundaries at 1/3, 2/3). Today: 112, with a dense per-max_step cluster right before each anim_frame tick.

import bdsim

sim = bdsim.BDSim(animation=True, animation_rate=3)
bd = sim.blockdiagram()
bd.connect(bd.CONSTANT(1.0), bd.INTEGRATOR(0.0))
bd.compile()

out = sim.run(bd, T=1.0, dt=0.1, max_step=1e-3)
print(f"samples: got {len(out.t)}, expected 13")
print(f"last 10: {[round(t, 5) for t in out.t[-10:]]}")
assert len(out.t) == 13, "BUG: residual interval emitted spurious samples"

Issue. Two coupled problems fire on every interval whose t_eval grid doesn't end at t1 — i.e. every anim_frame boundary that isn't on the k*dt grid.

Bug 1: undershoot of treached. _interval_hybrid returns treached = result.t[-1] — the last OUTPUT sample time, not the integrator's actual reach. On natural completion solve_ivp integrates all the way to t1, but result.t[-1] is the last t_eval point before t1. The main loop sees treached < t1, takes the "integration ended early" branch, bumps t0 by event_tol, and re-enters integration on a tiny residual interval [t0_bumped, t1]. The residual call builds an empty t_eval grid (no dt multiples remain), falls through to a t_eval-less solve_ivp call, and emits every natural integrator step into result.t. With max_step=1e-3 that is ~33 samples per boundary; with adaptive RK45 and a non-trivial graph it can be 100-200+ samples per boundary.

Bug 2: anim_frame boundaries missing from tlist. Once Bug 1 is fixed (so treached == t1), the spurious residual call is gone, but the sample loop only logs t_eval points — non-dt-aligned anim_frame boundaries no longer land in tlist at all. Downstream, artists don't update at the boundary moment (the MP4 grab at the refresh hook captures stale state from the most recent dt-grid sample), and watch logs miss the boundary times.

Fix.

• Bug 1: use t1 for treached when result.status == 0 (natural completion); fall back to result.t[-1] only for event-terminated runs (status == 1). This kills the spurious residual call.
• Bug 2: extend t_eval to include t1 when it isn't already on the dt grid. The integrator's output naturally lands on every scheduled boundary, sinks fire there, artists update, and the refresh-hook grab captures the right state.

Tests.

• Bug 1: tests/test_run_sim.py::HybridSimSampleGridTest::test_no_residual_sample_explosion.
• Bug 2: tests/test_run_sim.py::HybridSimSampleGridTest::test_anim_frame_boundary_in_tlist.

3b0ada1 — fix(graphics): grab MP4 frames at fps cadence, not per sink tick

Demo. Tiny ANIMATION block writing an MP4 at fps=3 with dt=0.1 The MP4 should hold 4 frames (one per fps refresh + the IC at t=0). Today it holds 13 — one per sink tick. Requires ffmpeg (which bundles ffprobe).

import subprocess, tempfile
from pathlib import Path
import bdsim

def init(block, fig, ax):
    ax.set_xlim(0, 1); ax.set_ylim(-1, 2)
    (block.dot,) = ax.plot([], [], "ro")

def update(block, t, inports):
    block.dot.set_data([t], [inports[0]])

with tempfile.TemporaryDirectory() as tmpdir:
    movie_path = Path(tmpdir) / "out.mp4"
    sim = bdsim.BDSim(animation=True, animation_rate=3, backend="Agg", hold=False)
    bd = sim.blockdiagram()
    integ = bd.INTEGRATOR(0.0)
    bd.connect(bd.CONSTANT(1.0), integ)
    anim = bd.ANIMATION(init, update, movie=str(movie_path))
    bd.connect(integ, anim)
    bd.compile()

    sim.run(bd, T=1.0, dt=0.1)
    bd.done()  # finalize the FFmpeg writer

    res = subprocess.run(
        ["ffprobe", "-v", "error", "-select_streams", "v:0", "-count_frames",
         "-show_entries", "stream=nb_read_frames",
         "-of", "default=noprint_wrappers=1:nokey=1", str(movie_path)],
        capture_output=True, text=True, check=True,
    )
    n_frames = int(res.stdout.strip())
    print(f"MP4 frames: got {n_frames}, expected 4")
    assert n_frames == 4, f"BUG: MP4 has {n_frames} frames, expected 4"

Issue. GraphicsBlock.step calls writer.grab_frame() on every sink tick, so the MP4 ends up with one frame per integration time point instead of one per fps-paced display refresh. The live window works correctly because matplotlib has a buffer/present split: canvas.draw() rasterises into the figure's buffer (cheap, fires per sink-tick), flush_events() presents the buffer to screen (the visible part — fires only from MatplotlibDisplayManager.refresh(), which runs at fps cadence from _anim_frame). The MP4 writer had no equivalent gating — every grab_frame() writes a frame.

Fix. The live-window refresh path is already fps-paced: DisplayManager.refresh() runs from _anim_frame at interactive_dt cadence, iterates figures, and calls flush_events() to present each canvas buffer. Moving the movie grab onto the same hook gives it the same cadence for free. Concretely:

• Added — in _start_movie, attach the writer to fig._bdsim_movie_writer so the display manager can find it without walking bd.blocklist.
• Added — a new _grab_movie_frame(fig) helper in display.py, called from refresh() and refresh_figure() of both MatplotlibDisplayManager and NotebookDisplayManager (right alongside flush_events()).
• Removed — the per-tick writer.grab_frame() call in GraphicsBlock.step. The per-tick canvas.draw() (rasterise into the buffer) stays.
• Changed — _anim_frame's reschedule guard widens from next_t < tf - tol to next_t <= tf + tol so a frame landing exactly on tf still fires. For T=1, fps=3, frames now land at t = 0, 1/3, 2/3, 1 (4 frames) instead of stopping at t = 0, 1/3, 2/3 (3 frames).

Tests. tests/test_display_manager.py::test_grab_movie_frame_no_writer_is_noop, ::test_grab_movie_frame_with_writer_grabs_once, ::test_grab_movie_frame_swallows_attribute_error. The MP4-frame-count behavior (the bug's externally observable signature) isn't directly tested because it requires ffmpeg as a runtime dependency; the Demo above stays as the manual verification.

9b03bcb — fix(run_sim): always include endpoints in _build_t_eval_grid

Issue. _build_t_eval_grid returns None whenever no k*dt multiple lands in [t0, t1]. The integrator call site treats None as "don't set t_eval", and solve_ivp falls back to natural per-step output — the same residual-sample explosion fix 3 closed for the natural-completion path. Fix 3 only stopped one producer of the empty-grid case; the case itself is still reachable through:

• Event-terminated re-entry. solve_ivp returns status == 1 partway through an interval; the main loop bumps t0 by event_tol and re-enters on [t0_bumped, t1]. If the event landed within dt of t1, no k*dt lies in the residual.
• Cross-domain timing. Any combination of (dt, fps, clock periods) that aren't integer multiples of one another produces tiny intervals between consecutive boundaries. e.g. dt=0.1, fps=3, clock_period=0.07: the gap between anim_frame at t=1/3 and the next clock tick at t=0.35 is 0.017 < dt.
• tf close to the last scheduled boundary. T=0.34, dt=0.1, fps=3 leaves a final interval [1/3, 0.34] of width 0.007 < dt (the Demo below).
• Multiple events at nearly the same time. Two events at t and t + ε (for ε > event_tol) leave a tiny gap between them.

In any of those cases the same out.t explosion fires.

Demo. Same trivial integrator. With dt=0.1, fps=3, anim_frame fires at t=1/3 and the next anim_frame at 2/3 would be past tf, so the final interval is [1/3, tf]. The next k*dt multiple above 1/3 is 0.4, so any tf ∈ (1/3, 0.4) produces an empty-grid final interval. Picking tf=0.34 and max_step=1e-3 gives an interval of width 0.007 that the integrator must subdivide into ~7 steps, all of which dump into out.t.

Expected: 6 samples (0, 0.1, 0.2, 0.3, 1/3, tf). Today: 12, with a dense max_step-spaced cluster across the residual.

import bdsim

sim = bdsim.BDSim(animation=True, animation_rate=3, quiet=True)
bd = sim.blockdiagram()
bd.connect(bd.CONSTANT(1.0), bd.INTEGRATOR(0.0))
bd.compile()

# Any T in (1/3, 0.4) exposes this with dt=0.1, fps=3.
out = sim.run(bd, T=0.34, dt=0.1, max_step=1e-3)
print(f"samples: got {len(out.t)}, expected 6")
print(f"out.t: {[round(t, 5) for t in out.t]}")
assert len(out.t) == 6

Fix. Change the builder's contract: always return a non-empty grid containing at least the interval endpoints, plus any k*dt multiples in between. That subsumes fix 3 Bug 2 (the explicit "extend t_eval to include t1" block at the call site) and lets us delete the None fallback entirely.

def _build_t_eval_grid(t0: float, t1: float, dt: float) -> np.ndarray:
    """Build a t_eval grid over [t0, t1]: endpoints plus k*dt multiples."""
    tol = 1e-12
    k0 = int(np.ceil((t0 - tol) / dt))
    k1 = int(np.floor((t1 + tol) / dt))
    if k1 >= k0:
        interior = dt * np.arange(k0, k1 + 1, dtype=float)
        interior = interior[(interior >= (t0 - tol)) & (interior <= (t1 + tol))]
        interior = np.clip(interior, t0, t1)
    else:
        interior = np.array([], dtype=float)
    return np.unique(np.concatenate([[t0], interior, [t1]]))

Call site collapses to:

if simstate.dt is not None:
    t_eval = self._build_t_eval_grid(float(t0), float(t1), float(simstate.dt))
    ivp_args.setdefault("t_eval", t_eval)

The fix 3 Bug 2 "extend t_eval to include t1" block at the call site is removed (now redundant).

Upstream validation that makes this safe:

• dt > 0 is enforced at run start (if dt is not None and float(dt) <= 0: raise ValueError).
• t0 < t1 is enforced by the main loop's while t0 < tf - event_tol: plus the if t1 <= t0 + event_tol: continue guard, so we never call the builder with a degenerate interval.

Behavior shifts to be aware of:

• result.t[0] is now always exactly t0. The interval-overlap dedup at the top of _interval_hybrid already handles this — np.isclose(result.t[0], t0, atol=1e-15) trips and skips the duplicate. The IC pre-pass from fix 2 covers the first-interval case.
• result.t[-1] is now always exactly t1 on natural completion. The t_final = t1 if status == 0 else result.t[-1] branch from fix 3 becomes equivalent to t_final = result.t[-1] on that path — but the status check stays for the event-termination case (where result.t[-1] is the event time, not t1).

Tests. Unit tests for _build_t_eval_grid directly (currently has zero coverage):

• t1 - t0 < dt and no k*dt in [t0, t1] → grid is [t0, t1].
• t0 == k*dt and t1 == (k+n)*dt → clean dt-aligned sequence with no duplicates.
• t0 = 0.123, t1 = 0.456, dt = 0.1 → grid is [0.123, 0.2, 0.3, 0.4, 0.456].

Plus a behavioral test mirroring the Demo: tests/test_run_sim.py::HybridSimSampleGridTest::test_no_empty_grid_sample_explosion.

Verification

• Test suite: 442 pass, 7 fail, 9 skipped. The 7 failures are pre-existing on base 12b26c2 and unrelated to this PR (all in tests/test_display_manager.py::test_notebook_*, walked-back per commit and confirmed).
• Test additions in this PR (all pass): - tests/test_run_sim.py::HybridSimSampleGridTest — 5 behavioral tests, one per fix's externally observable signature on out.t. - tests/test_run_sim.py::BuildTEvalGridTest — 3 unit tests for the new _build_t_eval_grid contract. - tests/test_display_manager.py::test_grab_movie_frame_* — 3 unit tests for the new helper.

[codacy-production]

Not up to standards ⛔

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[PhotonicVelocity]

@petercorke If you'd prefer these be raised in issues rather than directly in PR I can document them there.