feat: two-level trigger tree, centred-livetime semantics, and checkpoint pause/resume - #217
Open
hyperrealist wants to merge 26 commits into
Open
feat: two-level trigger tree, centred-livetime semantics, and checkpoint pause/resume#217hyperrealist wants to merge 26 commits into
hyperrealist wants to merge 26 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds two new ADR documents describing the intended trigger model evolution (two-level trigger structure + checkpoint pause/resume) and centred-livetime semantics for trigger timing / Window.positions().
Changes:
- Added ADR 0007 proposing
TriggerRepeat/TriggerSequence, parallel child triggers, and checkpoint-based pause/resume semantics. - Added ADR 0006 documenting centred-livetime semantics and the
Window.positions()signature rationale.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| docs/explanations/decisions/0007-two-level-trigger-structure-and-checkpoint-pause-resume.md | Introduces the proposed two-level trigger structure and pause/resume checkpoint model |
| docs/explanations/decisions/0006-trigger-pattern-centred-livetime-and-positions-signature.md | Documents centred-livetime semantics and Window.positions() argument behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Step 1 of ADR 0006/0007 implementation: - Add TriggerRepeat (replaces TriggerPattern with num/livetime/deadtime) - Add TriggerSequence (replaces TriggerGroup with detectors/trigger_repeat/children) - Add test_trigger_repeat, test_trigger_sequence, test_trigger_sequence_children - Remove stale ADR 0006 Decision wording that conflicted with 0007
…equences Steps 2+3 of ADR 0006/0007 implementation: - Window.trigger_groups -> trigger_sequences (type: list[TriggerSequence]) - WindowGenerator.trigger_groups -> trigger_sequences - Add temporary _bake_trigger_sequences in specs.py (one sequence per DetectorGroup; proper parent/children logic deferred to Step 8) - _compute_duration uses sum (sequential) instead of max (parallel) - Multi-rate test commented out until Step 8 adds parallel children - Update all call sites in tests - Copy trigger_sequences on receipt in Window and WindowGenerator - Fix double space in Window docstring
Step 4 of ADR 0006/0007 implementation: - Delete TriggerPattern and TriggerGroup dataclasses from core.py - Update DetectorGroup docstring to reference Acquire.compile() - Remove old imports and test_trigger_pattern/test_trigger_group from test_core.py
Step 5 of ADR 0006/0007 implementation: - Add TriggerRepeat branch: yields one position per repeat centred on each active livetime window (i + 0.5) * (livetime + deadtime) - Update docstring to describe both float and TriggerRepeat modes - Add test_window_positions_trigger_repeat in test_core.py - Add test_window_positions_trigger_repeat_chunking for max_duration - Document caller responsibility for TriggerRepeat duration matching
coretl
reviewed
Jul 14, 2026
Comment on lines
+171
to
+173
| - **Nesting depth** (parent + children in a `TriggerSequence`) determines how | ||
| many SEQ block levels a single active set requires. This ADR fixes depth at | ||
| two (parent + one child layer), which fits in a single SEQ block. |
Contributor
There was a problem hiding this comment.
One parent plus one child fits in a single SEQ block. Each additional child requires an additional SEQ block
| many SEQ block levels a single active set requires. This ADR fixes depth at | ||
| two (parent + one child layer), which fits in a single SEQ block. | ||
| - **Number of distinct simultaneous active sets** determines how many independent | ||
| SEQ tables are needed across the full scan. A `Concat` of two differently-named |
Contributor
There was a problem hiding this comment.
independent streams can re-use different outputs of the same SEQ block. There are 6 outputs, so that means up to 6 streams are supported per SEQ block
Comment on lines
+187
to
+190
| 2. **`Window.positions()`** (`core.py`): update signature from | ||
| `float | TriggerPattern` to `float | TriggerRepeat`. When a `TriggerRepeat` | ||
| is passed, compute trigger instants using the centred-livetime formula for | ||
| that repeat's `livetime` and `deadtime`. |
Contributor
There was a problem hiding this comment.
I'm not convinced this is correct. Let's say I want "all the positions that the Tetramm acquires at". This is a child of a TriggerSequence, and there could be a list of TriggerSequences. I also want to be able to specify the position compare points for gated detectors. This means that for a sequence of N frames we need to make N+1 rows that make N*2 edges. The first row compares at the position at 1/2 deadtime and makes a high edge, then N-1 rows that compare at the position at n*(livetime + deadtime) - 1/2 deadtime that make a low edge for deadtime then a high edge, then the last row at N*(livetime + deadtime) - 1/2 deadtime that makes a low edge. Here's a diagram:
windows: /‾‾‾‾‾‾‾‾‾‾‾‾‾‾\/‾‾‾‾‾‾‾‾‾‾‾‾‾‾\/‾‾‾‾‾‾‾‾‾‾‾‾‾‾\
livetime: __‾‾‾‾‾‾‾‾‾‾‾‾____‾‾‾‾‾‾‾‾‾‾‾‾____‾‾‾‾‾‾‾‾‾‾‾‾__
pcomp: | | | |
action: high low+high low+high low
This kind of complexity feels like it should be given to PandA. That's why I suggested we have just a straight def positions(times) method that returns the positions at given time intervals, then we push the time chunk generation into the PMAC or PandA logic.
What do you think?
Either way, this diagram needs to make it into the spec as I spent waaay too long drawing it...
|
|
||
| ### A1 — `_bake_trigger_sequence` always produces a single-entry list; the spacer pattern has no authoring surface yet | ||
|
|
||
| `_bake_trigger_sequence` always produces a single-entry `list[TriggerSequence]` |
| ### A2 — Two nesting levels fit in a single PandA SEQ block | ||
|
|
||
| The parent `TriggerSequence.trigger_repeat` and its `children` both encode into a | ||
| single SEQ block — no chained tables are required. The one-child-layer limit in |
Contributor
There was a problem hiding this comment.
the one child layering is not so much for SEQ, more for the fact that there must be a single top level stream that we can checkpoint on. Any layers of nesting below that are also expressible collapsed into a second layer
| The pause/resume guarantee for live rows (max latency = one root-level repeat | ||
| period) requires a `TRIGGER=BITB=1` gate row before each root-level parent repeat. | ||
| The consumer must not collapse N repeats into a single `REPEATS=N` SEQ row — that | ||
| reduces N checkpoints to one. A valid minimal encoding is a two-row sub-table |
Contributor
There was a problem hiding this comment.
wrong. The trigger will be checked on every repeat of every row.
Step 6 of ADR 0006/0007 implementation: - Add _truncate_trigger_sequence: walks list[TriggerSequence], skips completed sequences, truncates the in-progress one's trigger_repeat.num - Scan.__init__: remove unused start_time, add _trigger_index - Scan.with_start: rename time parameter to trigger_index - Scan.__iter__: apply truncation on the first yielded window - Add test_truncate_trigger_sequence and test_with_start_trigger_index_truncates - Move first_yielded flag outside truncation branch
Steps 7+8 of ADR 0006/0007 implementation: - Add active_stream_sets parameter to Scan.__init__ - Add _compute_active_stream_sets with dedup helper for spec tree - Detector-less Acquire nodes don't contribute stream names - When outer Acquire has detectors, active_set is just its name - Replace sibling sequences with single parent TriggerSequence - Slowest-rate group becomes parent; same-rate/timing groups merge - Child num is per-parent-repeat (exposures_per_collection) - Validate: child duration ≤ parent livetime using math.isclose - Clarify children semantics in docstrings - Add tests for active_stream_sets and error-path validation
Step 9 of ADR 0006/0007 implementation.
…nternal inconsistencies The trigger-model rework (TriggerRepeat/TriggerSequence, active_stream_sets, checkpoint truncation resume) has already landed in code, but PRD.md still described it in several places as pending or used stale future tense left over from ADR 0005/0006. Also resolves an already-answered open question (Concat dedup of same-named streams), corrects the child-vs-parent num formula in the trigger model description, and fixes the implementation status section (__init__.py exports, integer-ratio validation gap).
The docstring described "right has more generators than left" as a supported case with left-padding of left, but the code (and 1.x parity) actually requires len(left) >= len(right) and left-pads right's generator list, raising ValueError otherwise.
This was referenced Jul 23, 2026
…tios Implements the story-4 compile-time check that _bake_trigger_sequence was missing: a child group's period must divide evenly into the parent's livetime, not just fit within it. Fixes test fixtures that used a 0.00029 livetime (a prior workaround for the "exceeds parent livetime" check) with the properly-derived value so both checks hold together. Flags the still-open question of whether child livetime should exclude its own deadtime in PRD.md §11.
ADR 0005's original repeats formula only counted exposures_per_collection, silently dropping collections_per_event even though both require their own physical trigger pulse. Amends ADR 0005 with the correct exposures_per_event formula and updates PRD.md to match, resolving the "swap vs multiply" open question in favour of the confirmed multiply answer. Code fix in _bake_trigger_sequence to follow as a separate change.
Trigger repeat counts (both parent total_num and child child_num) only multiplied by exposures_per_collection, silently dropping collections_per_event even though each recorded collection needs its own trigger pulse. Every DetectorGroup in the existing test suite used collections_per_event=1, masking the omission. Adds DetectorGroup.exposures_per_event and uses it throughout, plus regression tests with collections_per_event > 1 on both the parent and a child group (verified to fail without the fix).
…uesky#219) coretl (PR bluesky#217 review): blanks should not count. Records the decision and rationale in ADR 0007 §4 and PRD §6 -- gaps are minimum requirements, not exact durations, so a paused blank always replaying in full on resume can only overshoot the intended gap, never undershoot it. Counting blanks would risk resume skipping the unexecuted remainder, undershooting the minimum. Notes the currently-unreachable multi-blank-per-window edge case for later.
…luesky#219) _truncate_trigger_sequence now skips blank sequences (livetime == 0.0) entirely -- never decremented against, always carried through unchanged -- so a pause landing anywhere inside a blank replays it in full on resume rather than risking the remainder being silently dropped. Regression test covers both a pause inside/after the blank and a pause mid-burst before it.
…ncoding Window.positions(dt: float | TriggerRepeat, max_duration) is decided to change to a plain positions(times: np.ndarray), dropping the TriggerRepeat branch, max_duration, and all internal chunking -- the caller supplies explicit time instants and owns iteration entirely. Not yet implemented; this records the decision and rationale only. Adds ADR 0007 Assumption A4: PandA position-compare encoding for N frames needs N+1 rows / 2N edges, a distinct mechanism from A3's BITB pause gate (A3 gates pausing, A4 gates exposures). This row/edge generation is PandA-hardware-specific and belongs in the consumer/driver layer, which is why it moves out of Window.positions() rather than being encoded there. Updates PRD §3.3, §7, and §8's known-gap item 1 to reference the pending change; the max_duration infinite-loop bug (§8 gap 1) is obsoleted entirely rather than needing a guard, since chunking goes away along with it.
AxisMotion.start_velocity/end_velocity and Window.positions()'s underlying position function both operated in the position function's index domain (0..length) while claiming/accepting real seconds, with no conversion factor applied anywhere. Every existing test masked this: either the window had no detectors (duration falls back to length, making seconds-per-index coincidentally 1) or assertions only checked internal consistency (start_velocity == end_velocity, direction of travel) rather than actual expected values, so the bug survived undetected. Confirmed impact: for a 100-point window with a detector-derived 0.004s/point duration, reported start_velocity was off by exactly 250x (1/0.004) from the true physical velocity, and Window.positions(dt) at a real quarter-duration interval returned samples clustered in a 0.4%-wide sliver near the window start instead of spanning the full physical range. Fix: WindowGenerator._fly_window now computes seconds_per_index once (self.duration if set, else 1.0 -- the same fallback already used for Window.duration) and applies it consistently: velocities divide by it, and the positions_fn closure divides its (real-seconds) input by it before evaluating the underlying index-domain position function. Added two regression tests in test_compile.py with independently-derived expected values (not re-derived from the implementation) -- verified both fail against the pre-fix code with the exact buggy symptom described above.
Persists the lesson from e220756: tests that only check shape, direction, or internal consistency between derived quantities can pass even when the underlying computation is wrong by a constant factor, since such bugs preserve those relationships. Expected values must be derived independently from the spec/math, not from running the implementation and asserting on its own output.
…ity sign Window.positions(dt: float | TriggerRepeat, max_duration) becomes a plain positions(times: np.ndarray) -> dict[axis, np.ndarray], returned directly rather than yielded in chunks. TriggerRepeat and max_duration are gone entirely; the caller supplies explicit time instants and owns all iteration/chunking. Since _fly_window's positions_fn closure already correctly maps real seconds to the position function's index domain (e220756), the new method body is just the existing RuntimeError guard plus a direct pass-through. While strengthening test_pmac_trajectory_positions with real expected-value assertions (per the new AGENTS.md rule), found a second latent bug in the same _fly_window: velocity was computed as d(position)/d(index) / seconds _per_index, missing the sign factor that accounts for index decreasing with time on a reversed (snake) window. The underlying LinearSource is direction-agnostic by design (always increasing with index); positions_fn already multiplied by sign correctly, but the velocity computation never did, so every reversed window's start_velocity/end_velocity carried the wrong sign -- confirmed against API_SPEC.md's motor-record ramp formulas, which depend on velocity's sign to extend the ramp in the correct direction. No existing test checked velocity sign for reversed windows. Fixed by multiplying both velocity computations by sign, with a dedicated regression test verified to fail without the fix. Test changes: - test_core.py: removed the two TriggerRepeat/max_duration-specific tests (nothing left to test once those branches are gone); added tests for the new public contract (returns dict directly, RuntimeError guard). - test_compile.py: updated the existing positions() regression test to the new call shape; added a dedicated reversed-velocity-sign regression test. - test_use_cases.py (with explicit permission): updated 5 call sites to the new signature. test_pmac_trajectory_positions rewritten so the consumer generates and consumes one chunk of times at a time, never materializing the full servo-rate array (matching the actual architectural point of this change), with real expected-value assertions replacing the previous shape-only checks.
…gaps found ADR 0007 Assumption A4 + Consequences item 2, and PRD §3.3/§7 all described positions(times: np.ndarray) as "confirmed direction, not yet implemented" -- it landed in 431043f, so this drops the pending framing and states it plainly. While touching §8, found two more stale entries in the same list: the "Implemented and passing" paragraph still named positions(float | TriggerRepeat) (the old signature) instead of positions(times: np.ndarray), and known-gap item 1 (positions max_duration infinite loop) is obsoleted entirely now that max_duration/chunking don't exist, not just guarded. Also found known-gap item 3 (collections_per_event omitted from num) was already fixed in 5bc03f4 but never moved out of the gaps list -- moved it into "Implemented and passing" and renumbered the remaining gaps.
A3's claim that collapsing N repeats into a single REPEATS=N SEQ row loses checkpoints was disputed in PR bluesky#217 review and never resolved; its worked example also assumed TRIGGER=Immediate exposure triggering, incompatible with A4's position-compare requirement. Since PandA rows support only one trigger condition, replace the stale claim/example with the current direction (position-compare first, then BITB, row-level structure still open) and fix A4's cross-reference to match. No scanspec code is affected either way.
Corrects four points from PR bluesky#217 review: SEQ-block capacity is per-child, not a blanket "fits in one block" (§5, A2, PRD §4.3/§9, with the flagship example's actual 2-block footprint stated); SEQ blocks have 6 outputs so independent streams can share one (§5, PRD §4.3); A2's rationale is checkpointing, not SEQ capacity (also clarified with a concrete example of deeper nesting collapsing into one child layer); A1 rewritten for clarity with no factual change. Folds in adjacent staleness found while editing the same sections: PRD's positions(TriggerRepeat) references (now times: np.ndarray) in two spots, a false "except for the spacer pattern" claim about compiled specs, the ADR's own Context section still describing the pre-A4 positions() type, and a leftover "the gate row" phrasing assuming A3's now-superseded single-row framing.
Nests the 2.0 package inside the existing scanspec distribution
(src/scanspec2/ -> src/scanspec/v2/, tests mirrored) instead of keeping it
as a separate sibling top-level package. 1.x is completely unmodified;
nothing takes over the top-level scanspec name yet.
This is Phase 1 of a two-phase migration (see PRD §12): letting ophyd-async
start integrating against scanspec.v2 immediately, without waiting for or
risking anything on 1.x. Phase 2 (later, separate) promotes v2 to the
top-level name and demotes 1.x to scanspec.v1 once 2.0 is actually ready to
release.
v2 is deliberately not imported from scanspec/__init__.py, staying opt-in
(import scanspec.v2) rather than pulled in by default for 1.x-only
consumers. As a side effect this also keeps it out of the Sphinx docs
build's autosummary recursion for now (verified) -- rewriting docs/ for
the 2.0 API is separate, later work (PRD §12 Phase 2).
Updates AGENTS.md, PRD.md (path references + two-phase §12), and
API_SPEC.md to match. Also fixes an unrelated stale AGENTS.md note
("Known churn") that still described a pre-existing TriggerNode/
trigger_nodes design that was never actually adopted -- the real names
that landed are TriggerRepeat/TriggerSequence and Window.trigger_sequences.
Verified: editable install + import scanspec.v2 works; 219 tests pass on
the new paths; full suite (333 passed, 1 skipped, 1 xfailed) confirms 1.x
is unaffected; pyright and ruff both clean on src/scanspec/v2 and
tests/scanspec/v2.
ADR 0006 -> Accepted (its substance has been settled and implemented for a while; only Decision 3, the positions() argument type, is stale in practice, noted explicitly rather than silently left inconsistent). ADR 0007 -> Proposed (implementation substantially complete, pending Assumption A3) -- nearly everything in it has landed in code this session; A3 (PandA BITB/position-compare row composition) is the one genuinely open question, waiting on the maintainer. ADR 0003 -> Accepted (partially superseded by ADR 0005) -- several of its decisions (TriggerPattern baking, Window.trigger_groups) were formalized and superseded by ADR 0005, which is itself Accepted. ADRs 0001, 0002, 0004, 0005 are unchanged: none of their core decisions are actually superseded by an already-Accepted ADR. Notably ADR 0005's TriggerGroup/TriggerPattern types have in fact been fully replaced by ADR 0007's TriggerRepeat/TriggerSequence, but declaring that supersession now would be premature while ADR 0007 itself is still Proposed -- revisit once 0007 is accepted. AGENTS.md and PRD.md Sec9 updated to match the new statuses, including an explicit caveat that ADR 0006 Decision 3 is stale in practice despite being formally Accepted, so nothing reads as more settled than it is.
… scope ADR 0007 (two-level trigger structure + checkpoint pause/resume) moves from Proposed to Accepted now that Assumption A3 is resolved to the extent scanspec needs: PandA position-compare and BITB pause-gating require separate, interleaved rows that cannot be rolled into a single REPEATS=N, but the exact row/table structure is a consumer-side (ophyd-async PandA driver) concern, not something this ADR has to pin down. Supersession chain updated to match: ADR 0005 marked Superseded by ADR 0007 (fully, not partially); ADR 0006 Decision 3 (positions() argument type) marked superseded in its own Status field; ADR 0003's Status line now cites ADR 0007 directly for both its stale points (Decisions bluesky#1/bluesky#2 and bluesky#5) rather than the intermediate ADR 0005 hop, since 0005 is no longer itself Accepted. ADR 0003's body is left untouched as a historical record. New Assumption A5 documents a real gap surfaced this session: TriggerRepeat.livetime/deadtime lost the float | None "unresolved, filled in later by ophyd-async" support their ADR 0005 predecessor had, somewhere in the ADR 0006/0007 restructuring. Needs to be restored before the Acquire authoring-surface redesign lands. PRD.md and AGENTS.md synced to match: §9 shrunk from "in-flight design changes" to a short "ADR review status" note; §11 open question 2 (spacer authoring surface) marked resolved (not needed, confirmed); §11 item 3 (child vs. parent livetime) left open pending the Acquire redesign.
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.
Implements ADR 0007 (two-level trigger structure + checkpoint pause/resume), which supersedes ADR 0005 and the
positions(TriggerPattern)part of ADR 0006. Centred-livetime semantics (ADR 0006) are preserved.