ViSR plugin: scan-aware /binned and a prototype /render endpoint

[hyperrealist]

Two changes to the ViSR plugin: /binned now follows the scan's own spec, and a new prototype endpoint, /render, draws the figure on the server.

/binned follows the scan

The image showed up transposed, coarse (numpy's default 10×10) and jittery (its range grew on nearly every poll). Now:

• rows are y; with no bins requested, the image uses the scan's setpoint grid from start.spec, fixed from the first poll, with null for empty cells
• fly scans: PandA-captured positions are offset from the motors (≈1 in x, 10.388 in y on b01-1); the endpoint measures and removes the offset and reports it as position_offset
• a file still behind the catalog gets a short retry, then 503 with Retry-After (was 422); identical polls share one result for 0.5 s, with ETag/304
• fixed: slice_dim failed every request; setpoints=true swapped x/y and stalled about 0.5 s per poll during a scan

A 1024-point fly scan replayed the way the ViSR UI draws it. Top row: before. Bottom row: after.

The deployed UI needs no change. Checked on every recent step and fly scan in the b01-1 Tiled: each puts one point per cell on its native grid.

/render (prototype)

GET /api/v1/render/{run} returns the figure for a viewport (x0, x1, y0, y1, size in pixels), so the UI can pan and zoom by changing it. The response is JSON per channel, or a PNG with format=png. kind picks how points are drawn:

• cells: the scan's grid cells
• rectangles: a step-sized rectangle on each recorded position
• gaussian: a beam-sized blob per point
• ovals: a beam-sized oval per point with a faint outline; overlaps are split between neighbours rather than blended, and areas no beam covered stay empty

The beam size comes from the request, then start["beam_size"], then the scan step. Nothing uses /render yet.

Not tested yet

A live scan with this build deployed, in particular the 503 path and the cache through the ingress.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[gilesknap]

Thanks Hiran, this is a clear improvement and the PR description is excellent. I checked it out at 6a306a9. The full suite passes (57 tests), as do ruff, ruff format and pyright. The transpose, the null bins, the 503/Retry-After path (which matches tiled's own DataNotReadyError handling), axis matching by name, the slice_dim fix and the non-waiting setpoints all look correct.

Most of my comments are about the grid and offset heuristics. When they misfire, they quietly produce a plausible image that is wrong. None of these cases affect the b01-1 runs you checked, because there the y offset (10.4) is much larger than the grid. Two of them are worth fixing before merge:

1. The "fits" check only uses the bounding box (positions_fit_grid, visr.py:282-309). If an offset is small enough that 90% of points stay inside the grid edges, alignment is never attempted. For example, a 20×20 grid with x off by 0.5 cell comes out with 110/400 cells null and 96 cells double-filled, so the image is striped. With an offset of 0.9 cell the whole image shifts one column and position_offset stays null. This is the "capture partly fixed" case, e.g. y corrected but x still about 1 off. Suggestion: decide fit from each point's distance to its own setpoint, always compute the median offset, and apply it when it is more than about 0.1 cell and the residual check passes. A test with a 0.5-cell offset would cover it.
2. The offset is matched by index (align_to_setpoints, visr.py:312-335). If a fly scan drops its first frame or gains a run-up frame, every pair is one step out. On a non-snaked raster the differences all agree, so the residual is about 0 and the offset is accepted one cell wrong: (1.526, 10.388) instead of (1.0, 10.388), and the image is shifted by one column. Snaked scans fail the residual check, so they are safe. Suggestion: also try lags −1, 0 and +1 and keep the best, or take the offset modulo the cell pitch. A test that drops index 0 on a raster would cover it. Do you know whether the PandA can drop or add a frame at the start?

Smaller points:
3. Cancellation in _shared (visr.py:75-106). The first caller computes the result in its own task. If that task is cancelled, the CancelledError is stored in the future, so every waiter fails with it too. Running compute() as its own task and having all callers await asyncio.shield(task) would avoid this.
4. Alignment at n = 1 or 2 always "succeeds", because the residual is 0 by construction. In the x_scale=3 test case, the endpoint uses the grid with a bogus offset at the first poll, then falls back to 10×10 once n reaches 3, which brings back the layout jumping. A minimum point count before accepting an offset would stop this.
5. NaN. json.dumps (visr.py:579) writes NaN, which is not valid JSON, for bins that have points but a NaN mean, so JSON.parse throws in the UI. Previously JSONResponse raised instead. Mapping non-finite values to None and using allow_nan=False would handle both.
6. _REPR_NAME_RE (visr.py:175) has no word boundary, so Motor(device_name="foo", name="...") picks up foo. Use \bname= or (?:\(|,\s*)name=.
7. ETag. tiled's compression middleware gzips the body but keeps the ETag, so it should be a weak one (W/"...").
8. Performance. MAX_GRID_CELLS=100000 permits a 316×316 grid. That takes about 0.11 s to bin and serialise on the event loop and produces a 5.8 MB payload. Consider a lower default, or anyio.to_thread.run_sync. _grid_from_spec's lru_cache(32) holds the full setpoint arrays, which could reach about 0.5 GB in the worst case with large 3-D specs.
9. Cache key. It has no principal. That is fine today, because the endpoint reads through root_tree.lookup_adapter and bypasses tiled's access policy anyway. If per-run access control is added later, the key needs the principal or the access tags. A comment at visr.py:562 would save someone a surprise.

Nits: n_points is counted before slice_dim masking. A negative DLS_TILED_VISR_NOT_READY_RETRIES reaches the "unreachable" assert and returns a 500, so clamp it. RETRY_AFTER_SECONDS could reuse tiled's TILED_ARRAY_RETRY_AFTER. _axis_letters assumes start["motors"] is in spec.axes() order; can you confirm that holds for the older plans? slice_dim on a 3-D scan whose z axis is innermost falls back to 10×10. That is safe, but aligning before masking would keep the grid.

Happy to approve once 1 and 2 are addressed or you've judged them not to apply.

[hyperrealist]

Thanks Giles, all of these were right. Fixed in nine commits (c11a6d8..c5d54df), one per point, each with a test.

1. The "fits" check only uses the bounding box (positions_fit_grid, visr.py:282-309). If an offset is small enough that 90% of points stay inside the grid edges, alignment is never attempted. [...] A test with a 0.5-cell offset would cover it.

Done in c11a6d8: place_on_grid replaces positions_fit_grid and align_to_setpoints. It pairs each point with its own setpoint, always takes the median offset (ignoring anything under 0.1 cell), and uses the grid when most points then sit within half a cell of their setpoint. Your 0.5- and 0.9-cell cases on a 20×20 grid now give the offset and one point per cell.

2. The offset is matched by index (align_to_setpoints, visr.py:312-335). If a fly scan drops its first frame or gains a run-up frame, every pair is one step out. [...] Suggestion: also try lags −1, 0 and +1 and keep the best, or take the offset modulo the cell pitch. A test that drops index 0 on a raster would cover it.

Also in c11a6d8: it tries lags −1, 0 and +1 and keeps the one that puts the most points on their setpoint. The median error can't pick the lag (the wrong one scores ~0 too); only the points where a row wraps tell them apart, so a non-zero offset is accepted only once the scan is past its first row. Tests drop the first frame and add a run-up frame on a raster; with index-only pairing the dropped frame gives x 2.0 instead of 1.0.

Do you know whether the PandA can drop or add a frame at the start?

I don't. I'm asking the fly-scan owner.

3. Cancellation in _shared (visr.py:75-106). [...] Running compute() as its own task and having all callers await asyncio.shield(task) would avoid this.

Done that way in 3578903, with a test that cancels the first caller while three others wait.

4. Alignment at n = 1 or 2 always "succeeds", because the residual is 0 by construction. [...] A minimum point count before accepting an offset would stop this.

Covered by the first-row rule in 2. Until then, a scan that needs an offset keeps the default binning, so there's one switch to the grid early on rather than a grid with a guessed offset.

5. NaN. json.dumps (visr.py:579) writes NaN, which is not valid JSON, for bins that have points but a NaN mean [...]. Mapping non-finite values to None and using allow_nan=False would handle both.

Done in f44f304, for /binned and /render. NaN and infinite means are null, and the test parses the response as strict JSON.

6. _REPR_NAME_RE (visr.py:175) has no word boundary, so Motor(device_name="foo", name="...") picks up foo.

Now (?:\(|,\s*)name= (1d30aba), tested with device_name= and prefix= before name=.

7. ETag. tiled's compression middleware gzips the body but keeps the ETag, so it should be a weak one (W/"...").

Weak now (b133e2d). If-None-Match matches on the quoted tag, so it works sent back either way.

8. Performance. MAX_GRID_CELLS=100000 permits a 316×316 grid. [...] Consider a lower default, or anyio.to_thread.run_sync. _grid_from_spec's lru_cache(32) holds the full setpoint arrays [...]

Both, in 6a7abba. Binning and JSON encoding run in a worker thread (as does /render's drawing), and the default cap is now 40000 cells: 200×200 is 0.03 s and 2.3 MB here, against 0.08 s and 5.8 MB for 316×316. The spec-grid cache holds 8 runs.

9. Cache key. It has no principal. [...] A comment at visr.py:562 would save someone a surprise.

Added at both keys (36d6795).

Nits: n_points is counted before slice_dim masking.

Now counted after. Positions are also placed on the grid before the mask, so the pairing by index survives slicing.

A negative DLS_TILED_VISR_NOT_READY_RETRIES reaches the "unreachable" assert and returns a 500, so clamp it. RETRY_AFTER_SECONDS could reuse tiled's TILED_ARRAY_RETRY_AFTER.

Both in 2df073a. Retry counts are clamped at 0, and Retry-After follows TILED_ARRAY_RETRY_AFTER.

_axis_letters assumes start["motors"] is in spec.axes() order; can you confirm that holds for the older plans?

It doesn't. On the b01-1 runs whose spec axes are object reprs, 39 of 103 list x first for a spec whose outer axis is y. For those runs place_on_grid now also tries x and y swapped and keeps whichever the recorded points fit (c5d54df). All 35 such runs with moving motors now bin on their grid the right way round, and the 63 listed in order are unchanged; the rest are runs where the motors didn't move. Specs that name their own axes (everything recent) are taken as they are. setpoints=true still follows the motors list for those old runs, since it doesn't read the positions.

slice_dim on a 3-D scan whose z axis is innermost falls back to 10×10. That is safe, but aligning before masking would keep the grid.

Done: aligned before masking, with a test of a z-innermost scan with an offset.

The branch also now has the /render prototype from the description, which shares this loading and placement.

[gilesknap]

Thanks Hiran, that's a thorough set of fixes. I checked it out at c5d54df: 102 tests pass, ruff/format/pyright clean. The per-point placement and the lag search look right to me. Using the row wraps to choose the lag is the key insight, and waiting for the first wrap also deals with point 4. Shield-based _shared, strict JSON, weak ETag and the off-loop binning all look good, and thanks for checking the motors-list ordering against the real b01-1 runs.

One question on /binned: MIN_POINTS_ON_SETPOINT is 0.5, down from 0.9. That lets a run with nearly half its points more than half a cell off use the grid, which brings the striping back. Was 0.5 needed for the b01-1 data, or could it be higher?

On the new /render prototype:

1. Line scans 500 for rectangles and gaussian. For 1-D scans y is NaN (visr.py:448 / fill_data), and int(numpy.floor(nan)) raises at rendering.py:121 and :158. oval_layout already skips non-finite positions; the other two need the same.
2. Request cost is unbounded. beam_*/cell_* aren't validated, so a huge beam makes every point paint the whole viewport: 2000 points at 1024 px took 5.3 s here, and the limits allow 40k points at 2048 px. Varying a parameter gets past the dedup. A zero beam divides by zero. I'd require them to be positive and finite, and cap them at the viewport size, or cap points × window area.
3. The grid is used even when placement failed. load_points returns grid whether or not on_grid is set, so /render takes its extent, step and cells edges from a grid the unshifted points don't sit on (e.g. blank during a fly scan's first row). /binned falls back here; /render should check points.on_grid too.

Nits: x0 > x1 silently draws nothing for rectangles/gaussian (normalise or reject it), and an empty path makes uid = [...][0] an IndexError.

I'm happy to approve the /binned side now. Since /render is a prototype, would you rather fix 1–3 here or split it into its own PR?