-
Notifications
You must be signed in to change notification settings - Fork 5
Scanspec 2 updates #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hyperrealist
wants to merge
14
commits into
bluesky:v2-dev
Choose a base branch
from
hyperrealist:196-changes-for-2-0-tdd
base: v2-dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Scanspec 2 updates #212
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
29721a7
Serialize non-JSON-serializable axes via repr (#208)
hyperrealist 6422c16
Update dependency https://github.com/DiamondLightSource/python-copier…
renovate[bot] 70fdebf
Add Linspace.bounded, Range, Ellipse, Polygon, Line to scanspec2
hyperrealist 8d3dffb
Implement ADR 260513: TriggerPattern centred-livetime semantics and W…
hyperrealist bf2b2cf
Add ADR 0006: TriggerPattern centred-livetime semantics and Window.po…
hyperrealist b84d5be
Fix Range.bounded corner case when lower == upper
hyperrealist 249f996
Validate Ellipse x_diameter and y_diameter are non-zero
hyperrealist 2d03534
Fix Range.compile to use actual last setpoint instead of self.stop
hyperrealist 5c6c2a5
Fix ADR 0006 ptychography spacer example
hyperrealist 9aff5db
Implement trigger_index pause/resume in Scan.with_start
hyperrealist 3076c70
docs: add ADR 0007 checkpoint-based pause/resume and trigger tree
hyperrealist 74511e6
add cr suggestions; fix hallucinations
hyperrealist f4321a3
adr 0007: verify assumptions against PandA SEQ docs; inline BITA fiel…
hyperrealist f04ffd8
adr 0007: move chained-tables limit to assumption A3; reference from …
hyperrealist File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
80 changes: 80 additions & 0 deletions
80
...ions/decisions/0006-trigger-pattern-centred-livetime-and-positions-signature.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # 6. TriggerPattern centred-livetime semantics and Window.positions() signature | ||
|
|
||
| Date: 2026-05-13 | ||
|
|
||
| ## Status | ||
|
|
||
| Tentative | ||
|
|
||
| ## Context | ||
|
|
||
| Two meetings (01 May and 13 May 2026) clarified ambiguities in the trigger | ||
| model that had been left implicit in the original design. | ||
|
|
||
| **Livetime ordering**: ADR 0005 defined `TriggerPattern(repeats, livetime, | ||
| deadtime)` but did not specify whether execution was `livetime → deadtime` | ||
| or some centred arrangement. Hardware implementations (specifically the PandA | ||
| sequencer table and position-compare triggering) require the detector's active | ||
| window to be centred on the nominal scan position. With `livetime → deadtime` | ||
| semantics the active window is leading-edge-aligned, which produces a | ||
| systematic position error proportional to `½·deadtime`. | ||
|
|
||
| **Zero livetime**: The tomography variable-gap ptychography use case requires | ||
| a pure dead-gap spacer `TriggerPattern` with `livetime=0.0` to encode | ||
| non-uniform inter-trigger spacing. The original design did not explicitly | ||
| address this case. | ||
|
|
||
| **`Window.positions()` argument type**: The original signature accepted only a | ||
| `float dt` (servo-cycle interval). Consumers that want positions at each | ||
| trigger instant — equivalent to what scanspec 1.x returned — had no | ||
| first-class way to express this. A `TriggerPattern` carries all the | ||
| information needed to derive those instants, so it is a natural argument type. | ||
|
|
||
| ## Decision | ||
|
|
||
| 1. **`TriggerPattern` execution is centred**: the execution order for each | ||
| repeat is `½·deadtime → livetime → ½·deadtime`. The struct fields are | ||
| unchanged; only the interpretation changes. The total period per repeat | ||
| remains `livetime + deadtime`. | ||
|
|
||
| 2. **`livetime=0.0` is explicitly valid**: a `TriggerPattern(repeats, 0.0, | ||
| deadtime)` is a pure dead-gap spacer. Because centred-livetime semantics | ||
| already produce symmetric `½·deadtime` gaps around each active window, | ||
| there is no need for explicit leading/trailing spacers in uniform sequences. | ||
| The spacer pattern is required only when two groups of frames must be | ||
| separated by a gap that differs from the intra-group deadtime, for example | ||
| in variable-gap ptychography: | ||
|
|
||
| ```python | ||
| [ | ||
| TriggerPattern(N1, livetime1, deadtime), # first burst | ||
| TriggerPattern(1, 0.0, gap), # inter-burst spacer | ||
| TriggerPattern(N2, livetime2, deadtime), # second burst | ||
| ] | ||
| ``` | ||
|
|
||
| 3. **`Window.positions()` accepts `float | TriggerPattern`**: | ||
|
|
||
| ```python | ||
| def positions( | ||
| self, | ||
| dt_or_pattern: float | TriggerPattern, | ||
| max_duration: float | None = None, | ||
| ) -> Iterator[dict[AxisT, np.ndarray]]: | ||
| ``` | ||
|
|
||
| Passing a `float` returns positions at a fixed servo-cycle rate. Passing a | ||
| `TriggerPattern` returns positions at each trigger instant (centred on each | ||
| active window). Both paths yield the same `dict[axis → np.ndarray]` | ||
| interface. | ||
|
|
||
| ## Consequences | ||
|
|
||
| - PandA sequence table builders must emit `½·deadtime` pre-delay, `livetime` | ||
| active window, and `½·deadtime` post-delay — not `livetime` then `deadtime`. | ||
| - Consumers must not reject `TriggerPattern` instances with `livetime=0.0`. | ||
| - `Window.positions()` implementations must branch on the argument type and | ||
| compute trigger instants from the centred-livetime formula when a | ||
| `TriggerPattern` is passed. | ||
| - `core.py` docstrings on `TriggerPattern` must document both the centred | ||
| semantics and the `livetime=0.0` case. | ||
230 changes: 230 additions & 0 deletions
230
docs/explanations/decisions/0007-checkpoint-based-pause-resume-and-trigger-tree.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,230 @@ | ||
| # 7. Checkpoint-based pause/resume and hierarchical trigger tree | ||
|
|
||
| Date: 2026-06-05 | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed | ||
|
|
||
| ## Context | ||
|
|
||
| ### The original pause/resume design | ||
|
|
||
| ADR 0006 and the initial implementation introduced `Scan.with_start(window, | ||
| time=0.0)` to resume a scan after a mid-scan pause (e.g. synchrotron top-up). | ||
| A subsequent GitHub issue ([214](https://github.com/bluesky/scanspec/issues/214)) replaced `time: float` with `trigger_index: int` on | ||
| the grounds that hardware controllers (specifically PandA) track progress as a | ||
| count of completed trigger repeats, not elapsed seconds. The change also | ||
| introduced a single-`TriggerGroup` constraint for intra-window resume: | ||
|
|
||
| ```python | ||
| def with_start(self, window: int, trigger_index: int = 0) -> Scan: ... | ||
| ``` | ||
|
|
||
| If `trigger_index > 0` and the target window contained more than one | ||
| `TriggerGroup`, `Scan.__iter__` raised `ValueError`. | ||
|
|
||
| ### The problem with the single-TriggerGroup constraint | ||
|
|
||
| A single `Acquire` node with multiple `DetectorGroup`s whose trigger rates are | ||
| integer multiples of each other produces multiple `TriggerGroup`s in each | ||
| window. This is a deliberate design feature (`WindowedStream` groups detectors | ||
| at integer-multiple rates). The constraint therefore makes intra-window resume | ||
| impossible for the common case of a SAXS/WAXS detector running at 1× alongside | ||
| a PandA encoder group running at 10×. | ||
|
|
||
| Options considered: | ||
|
|
||
| 1. **Multiple `trigger_index` values** (one per `TriggerGroup`): requires | ||
| callers to track N counters, and still does not tell PandA which repeats are | ||
| safe stopping points. | ||
| 2. **Revert to `start_time`**: reintroduces rounding ambiguity for | ||
| variable-gap patterns. | ||
| 3. **`checkpoint_every` on `TriggerPattern`**: tells the hardware how many | ||
| repeats form one safe-pause granule, but cannot rescue a single long repeat | ||
| with no internal structure. | ||
| 4. **Hierarchical trigger tree**: make `TriggerPattern` recursive by adding | ||
| `children: list[TriggerPattern]`. Root-level pattern boundaries are | ||
| checkpoints; child patterns execute inside each parent repeat at a higher | ||
| rate. A single `trigger_index` across the flat root-level count unambiguously | ||
| resumes all nested groups. | ||
|
|
||
| ### How the hardware implements checkpoints (PandA sequencer table) | ||
|
|
||
| A PandA sequencer table row is: trigger condition, repeat count, livetime, | ||
| deadtime. The pause mechanism works by: | ||
|
|
||
| 1. Bluesky signals "continue to next checkpoint" when a top-up is imminent. | ||
| 2. PandA finishes the current root-level repeat, then pauses on a gate row | ||
| (controlled via a bit that BlueSky holds high during normal running). | ||
| 3. BlueScript reads the sequencer table index to determine the `trigger_index` | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
| (how many root-level repeats are complete in this window). | ||
| 4. Resume constructs `scan.with_start(window, trigger_index)`. | ||
|
|
||
| This mechanism requires the hardware concept of "checkpoint" to be structurally | ||
| encoded in the `TriggerPattern` data delivered to PandA — it cannot be inferred | ||
| at consumption time from a flat list of patterns. | ||
|
|
||
| ### The SWIFT blank-section case | ||
|
|
||
| A slow scan with a long blank section (e.g. `TriggerPattern(1, 0.0, 50.0)`) is | ||
| a single 50-second repeat with no internal structure. If a top-up arrives | ||
| mid-blank, there is no checkpoint at which PandA can safely pause. The | ||
| solution is to split it into many short repeats: `TriggerPattern(500, 0.0, 0.1)`, | ||
| giving 500 checkpoint opportunities spaced 100 ms apart. This must be decided | ||
| at spec-construction time; a `checkpoint_every` field on a single-repeat | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
| pattern cannot help. | ||
|
|
||
| ## Decision | ||
|
|
||
| ### 1. `TriggerPattern` becomes a tree node | ||
|
|
||
| Add a `children: list[TriggerPattern]` field (default `[]`) to `TriggerPattern`: | ||
|
|
||
| ```python | ||
| @dataclass | ||
| class TriggerPattern: | ||
| repeats: int | ||
| livetime: float | ||
| deadtime: float | ||
| children: list[TriggerPattern] = field(default_factory=list) | ||
| ``` | ||
|
|
||
| The root-level list in `TriggerGroup.trigger_patterns` defines the checkpoint | ||
| cadence. Any boundary between consecutive root-level repeats is a valid | ||
| pause point. Children execute inside each parent repeat's livetime window at | ||
| a higher rate, and their completion is structurally guaranteed whenever the | ||
| parent repeat completes. | ||
|
|
||
| ### 2. Integer-multiple rate groups expressed as parent/child, not sibling groups | ||
|
|
||
| The 10× PandA / 1× SAXS case becomes one root-level `TriggerPattern` with | ||
| the high-rate group as a child, instead of two independent sibling | ||
| `TriggerGroup`s: | ||
|
|
||
| ```python | ||
| # Before (two sibling TriggerGroups, resume blocked by constraint): | ||
| TriggerGroup(["saxs", "waxs"], [TriggerPattern(500, 0.003, 0.001)]) | ||
| TriggerGroup(["x_enc", "y_enc"], [TriggerPattern(5000, 0.0003, 8e-9)]) | ||
|
|
||
| # After (parent/child, single checkpoint cadence): | ||
| TriggerGroup(["saxs", "waxs"], [ | ||
| TriggerPattern(500, 0.003, 0.001, children=[ | ||
| TriggerPattern(10, 0.0003, 8e-9) # PandA encoders, 10× per SAXS frame | ||
| ]) | ||
| ]) | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
| ``` | ||
|
|
||
| Each of the 500 root repeats is a checkpoint. At `trigger_index=N` all | ||
| `10*N` child repeats are also complete — the nesting makes this a structural | ||
| invariant, not a runtime check. | ||
|
|
||
| ### 3. Relax the single-TriggerGroup constraint for `with_start` | ||
|
|
||
| Because checkpoint alignment is now structural (parent/child rather than | ||
| independent siblings), multiple `TriggerGroup`s in the same window can each | ||
| have their root patterns truncated at the same `trigger_index`. The | ||
| `ValueError` raised when `len(tgs) > 1 and trigger_index > 0` is replaced by | ||
| a validation that all groups have the same total root-level repeat count. | ||
| `_truncate_trigger_patterns` carries `children` through unchanged when | ||
| splitting a root pattern mid-way. | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
|
|
||
| ### 4. Blank sections need granular repeats | ||
|
|
||
| Any `TriggerPattern` used in a position where top-up pauses must be supported | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
| must have sufficient `repeats` to provide the desired pause granularity. A | ||
| single-repeat spacer lasting tens of seconds is not pausable. Spec | ||
| constructors should split long blank sections into short repeats. This is a | ||
| documentation and construction-time concern; no runtime enforcement is added. | ||
|
|
||
| ### 5. `Scan.active_stream_sets` for up-front validation | ||
|
|
||
| Add `active_stream_sets: list[frozenset[str]]` to `Scan`. This is the set of | ||
| all combinations of windowed-stream names that are simultaneously active in the | ||
| same window, enabling PandA (or any consumer) to validate up-front whether the | ||
| required sequencer-table capacity is available. | ||
|
|
||
| **Algorithm**: walk every window produced by `Scan.__iter__` and, for each | ||
| window, collect the names of all `windowed_streams` whose detectors appear in | ||
| at least one `TriggerGroup` in that window. Record each resulting | ||
| `frozenset[str]` and deduplicate. The final list contains one entry per | ||
| distinct simultaneous-active combination observed across all windows. | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
|
|
||
| Note that `active_stream_sets` must **not** be computed simply as | ||
| `[frozenset(s.name for s in scan.windowed_streams)]` — that would always | ||
| return a single set containing every stream, which is wrong for `Concat` | ||
| scans. | ||
|
|
||
| **The `Concat` case**: when two `Acquire` nodes with different `stream_name` | ||
| values are concatenated (e.g. a diffraction pass followed by a fluorescence | ||
| pass), each window belongs to exactly one stream. The correct result is two | ||
| singleton sets — `[frozenset({"diffraction"}), frozenset({"fluorescence"})]` | ||
| — not one two-element set. From PandA's perspective each singleton maps to | ||
| one sequencer table, and because the two streams are never simultaneously | ||
| active they can share a single table rather than requiring two. | ||
|
|
||
| The two axes of PandA resource consumption are therefore **orthogonal**: | ||
|
|
||
| - **Nesting depth** (levels of `TriggerPattern.children`) determines how many | ||
| *chained* sequencer tables a single simultaneous active set requires (PandA | ||
| maximum is two chained tables). | ||
| - **Number of distinct simultaneous active sets** determines how many | ||
| independent sequencer tables (or table time-slots) are needed across the | ||
| full scan. | ||
|
|
||
| Concretely: | ||
|
|
||
| - Single `Acquire`, one `TriggerGroup`, flat patterns → one table, one level. | ||
| - Single `Acquire`, one `TriggerGroup`, one level of `children` → two chained | ||
| tables (PandA limit reached). | ||
| - `Concat` of two `Acquire` nodes with different streams → two singleton sets, | ||
| one table used alternately (not simultaneously), within PandA limits. | ||
| - `Concat` where each segment already uses two chained tables → still two | ||
| singleton sets, two chained tables per segment, still within PandA limits. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Code changes required | ||
|
|
||
| 1. **`TriggerPattern`** (`core.py`): add `children: list[TriggerPattern]` | ||
| field with `default_factory=list`. The `@dataclass` decorator handles | ||
| equality and repr automatically. | ||
|
|
||
| 2. **`_truncate_trigger_patterns`** (`core.py`): when splitting a root pattern | ||
| mid-way, carry `children` into the remainder: | ||
| ```python | ||
| TriggerPattern(tp.repeats - remaining, tp.livetime, tp.deadtime, tp.children) | ||
| ``` | ||
|
|
||
| 3. **`Scan.__iter__`** (`core.py`): replace the `len(tgs) > 1` `ValueError` | ||
| with a check that all `TriggerGroup`s have the same total root-level repeat | ||
| count; truncate each group independently at `trigger_index`. | ||
|
|
||
| 4. **`Scan.active_stream_sets`** (`core.py`): computed by iterating all | ||
| windows at construction time. For each window, intersect the window's | ||
| `TriggerGroup` detector lists against each `WindowedStream`'s detectors to | ||
| determine which stream names are active; collect and deduplicate the | ||
| resulting `frozenset[str]` values. This correctly handles the `Concat` | ||
| case where different windows activate different streams (producing multiple | ||
| singleton sets) rather than one set containing all stream names. | ||
|
|
||
| 5. **`Acquire._bake_trigger_groups`** (`specs.py`): construct parent/child | ||
| `TriggerPattern` trees instead of independent sibling `TriggerGroup`s when | ||
| multiple `DetectorGroup`s are present. The highest-rate group becomes the | ||
| innermost child; the lowest-rate group is the root. | ||
|
|
||
| 6. **Tests** (`tests/scanspec2/test_core.py`): update the existing | ||
| `test_with_start_trigger_index_multi_group_raises` test to reflect the | ||
| relaxed constraint; add tests for parent/child truncation preserving | ||
| children, the SWIFT blank-section pattern, and `active_stream_sets` | ||
| covering at minimum: (a) single `Acquire` → one frozenset, (b) `Concat` | ||
| of two differently-named `Acquire` nodes → two singleton frozensets, (c) | ||
| single `Acquire` with two co-active streams → one two-element frozenset. | ||
|
|
||
| ### Backward compatibility | ||
|
hyperrealist marked this conversation as resolved.
Outdated
|
||
|
|
||
| `TriggerPattern(repeats, livetime, deadtime)` remains valid (children defaults | ||
| to `[]`). Existing single-group `TriggerGroup`s are unaffected. Code that | ||
| constructs sibling `TriggerGroup`s for integer-multiple rates will continue to | ||
| function but will not support intra-window resume; migration to the parent/child | ||
| form is required for that capability. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ dev = [ | |
| "tox-uv", | ||
| "types-mock", | ||
| "Pillow", | ||
| "ophyd-async", | ||
| ] | ||
|
|
||
| [project.scripts] | ||
|
|
||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.