Add ROI request plotters as composable layers by SimonHeybrock · Pull Request #644 · scipp/esslivedata

SimonHeybrock · 2026-01-07T12:38:08Z

Summary

This PR implements ROI request plotters (BoxEdit, PolyDraw) as composable layers, completing the layer-based composition system for ROI editing.

Adds BaseROIRequestPlotter base class with shared logic for ROI request layers
Implements RectanglesROIRequestPlotter and PolygonsROIRequestPlotter as concrete layers
Removes the old ROIDetectorPlotFactory and its special-case handling
Centralizes max_roi_count configuration in detector_view_specs.py
Consolidates ROI plotter registration to prevent DataRequirements divergence

Fixes #585 (most of it—the general layer system was implemented in the base branch).

Known Caveats

These limitations will be addressed in follow-up work:

Three layers complicate setup: ROI plots now require three separate layers (image, readback, request), which complicates configuration and results in three space-consuming toolbars
Readback no longer updates request: Editing the request layer does not sync with readback changes
Five layers needed for rectangles + polygons: If both rectangle and polygon ROIs are needed, this requires five separate layers

🤖 Generated with Claude Code

Implement RectanglesRequestPlotter and PolygonsRequestPlotter that allow users to draw ROIs interactively via BoxEdit/PolyDraw. These plotters follow the composable layer pattern, enabling ROI drawing to be added as an overlay on detector images. Key design decisions: - Subscribe to detector image output (not ROI readback) to obtain job_id, since readback only exists after ROIs are published - Use same data/spec requirements as roi_detector for compatibility - Store streams as instance variables to prevent garbage collection This is preparatory work for eventually removing the monolithic ROIDetectorPlotFactory. --- Prompt: Please think through @docs/developer/plans/roi-request-plotters.md and read related code. Do you have a sufficient understanding to be able to start implementation? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Store roi_publisher as an instance variable instead of accessing it through ROIDetectorPlotFactory's private attribute. This decouples the ROI request plotters from the factory, preparing for its removal. --- Prompt: Can we store the ROIPublisher in PlottingController.__init__ and use that in create_plot_from_pipeline? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Delete the monolithic ROIDetectorPlotFactory in favor of the new composable layer system with independent ROI request and readback plotters. Changes: - Move RectangleConverter and PolygonConverter to roi_request_plots.py - Remove roi_detector special case handling from PlottingController - Remove roi_detector registration from plotter registry - Delete roi_detector_plot_factory.py entirely - Delete related tests (roi_detector_plot_factory_test.py, roi_plot_state_test.py, roi_backend_update_test.py) - Clean up hv.Layout references in plot_orchestrator.py The new composable plotters (rectangles_request, polygons_request, rectangles_readback, polygons_readback) replace the integrated roi_detector approach. --- Prompt: Yes, please commit, then we should be able to perform the big removal of ROIDetectorPlotFactory? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

…dinates OptionalRectanglesCoordinates now inherits from RectanglesCoordinates, overriding validate_coordinates to allow empty values. This eliminates duplicate parsing and validation logic while making the relationship between the two classes explicit. Original prompt: Could OptionalRectanglesCoordinates inherit RectanglesCoordinates and simply implement validate_coordinates as a thin wrapper?

Request plotters now depend on ROI readback output instead of detector image. This is semantically correct and avoids triggering plot() on every image update. The backend sends empty readback on first finalize to break the chicken-egg problem where no readback exists until the first ROI request. Known limitation: if dashboard connects after job start, it will miss the initial readback. A future fix could inject empty readback in DataService. --- Original prompt: Consider how roi_request_plots.py has a dummy-dependency on the ROI image output, for the purpose of extracting (parts of) the ResultKey, to allow for defining the output stream name. I wondered if one could option the same in a cleaner way to adding a fake output to the ROI detector output spec (defined in backend), `Rectangle ROI (request)`. Then the user could simply select that in the UI, which would be more obvious than the detour via selecting the detector image.

When plotters were first called with empty data (before ROI readback arrived from the workflow), they returned unstyled elements. When data arrived later, HoloViews updated the data but preserved the missing styling options. Changing style settings worked because it recreated the plotter with data present. Now both RectanglesReadbackPlotter and PolygonsReadbackPlotter apply styling options even when returning empty elements. Prompt: I was seeing a weird issue with PolygonsReadbackPlotter - the styling was not applied initially, nor was the dynamic line color. Only once I changed the style settings (which actually recreates the plot) it worked. Can you think about why that might have happended? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Extract BaseROIRequestPlotter using the Template Method pattern to eliminate duplicated setup logic in RectanglesRequestPlotter and PolygonsRequestPlotter. The base class implements the complex plot() method and shared helpers (_on_edit, _apply_styling, _publish_rois), while subclasses provide hooks for type-specific behavior. This reduces code duplication by ~120 lines and ensures the two plotters cannot diverge in their core setup sequence. Prompt: Please figure out how to extract a shared base class for the request plotters in @src/ess/livedata/dashboard/roi_request_plots.py - in particular the setup logic in `plot()` is very complex so we need to avoid bugs and divergence from having two implementations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The plot() method now stores the created DynamicMap and returns it on subsequent calls, preventing full recreation of streams, pipes, and subscriptions. This prepares for future enhancements where readback updates might trigger plot() again. Prompt: Consider BaseROIRequestPlotter.plot - I think we somehow need to prevent full recreation on every call. My hunch is that we should skip the reinit, potentially leaving a plot() as a no-op for all but the first call.

ROI spectra were all zeros because request plotters sent canvas coordinates (in physical units like meters) with unit=None, causing the backend to interpret them as pixel indices. The fix has two parts: 1. Backend (detector_view.py): Pass coordinate units from the detector view to ROI.to_concatenated_data_array() so empty readbacks include proper units. 2. Frontend (roi_request_plots.py): Extract units from readback data and use them when parsing BoxEdit/PolyDraw events. Also added coord_units parameter to ROI.to_concatenated_data_array() for cleaner API when creating empty readbacks with specific units. Prompt: RectanglesRequestPlotter suddenly results in ROI spectra plots that are all zeros. It worked previously, so I suspect a recent refactor. Blind hunch to check first: Are units and axis ranges correct when converting the canvas coord to the schema used for sending coords to backend? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

After the ROI request plotter refactoring, some code in plot_params.py became orphaned: - _get_default_max_roi_count() helper function - ROIOptions class - PlotParamsROIDetector class These were used by the deleted ROIDetectorPlotFactory. This commit: - Adds _get_max_rois_for_geometry() helper to roi_request_plots.py that gets per-geometry limits from the central ROI configuration - Updates RectanglesRequestOptions and PolygonsRequestOptions to use this helper for default and max values - Removes the orphaned code from plot_params.py - Fixes detector_data_test.py to expect 8 messages on first finalize (6 data + 2 initial ROI readbacks) instead of 6 Prompt: Please review everything carefully. Is there more to do or cleanup? Do you have concerns? Follow-up: Why is _get_default_max_roi_count unused? Should the new plotters use it? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Replace direct private attribute assignment with a public setter method and protocol-based type checking for cleaner dependency injection. - Add ROIPublisherAware protocol to define the contract for plotters that can publish ROI updates - Add set_roi_publisher() method to BaseROIRequestPlotter - Update PlottingController to use protocol isinstance check instead of concrete type check against RectanglesRequestPlotter/PolygonsRequestPlotter Prompt: Please think if there is a cleaner way to inject _roi_publisher into the request plotters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

…rgence Move all ROI plotter registration from separate modules into plotting.py, placing readback and request plotters side-by-side with shared requirement constants. This ensures the DataRequirements for rectangle and polygon plotters cannot diverge between request and readback variants. - Define _RECTANGLE_ROI_REQUIREMENTS and _POLYGON_ROI_REQUIREMENTS once - Register all four ROI plotters together in plotting.py - Add from_params classmethod to request plotters for symmetry - Remove now-empty registration functions from roi_*.py modules Prompt: _register_roi_request_plotters and _register_roi_readback_plotters duplicate a lot. Can we share better, ensuring in particular the DataRequirements do not diverge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

nvaytet

Five layers needed for rectangles + polygons: If both rectangle and polygon ROIs are needed, this requires five separate layers

😞 Can we have a single overlay layer that plots rectangles, polygons and static lines?

nvaytet · 2026-01-07T21:01:44Z

        plotter = plotter_registry.create_plotter(plot_name, params=params)

+        # ROI request plotters need a publisher and return a DynamicMap with
+        # BoxEdit/PolyDraw streams already attached. Don't wrap again.


Can you explain a bit more what is meant by "Don't wrap again"?
What is being wrapped below (outside of the if), and what is the wrapper doing?

The DynamicMap. Other plotters do not create a DynamicMap themselves, so the controller wraps what them.

nvaytet · 2026-01-07T21:04:23Z

+            return hv.Polygons([]).opts(
+                fill_alpha=style.fill_alpha,
+                line_width=style.line_width,
+            )


Do we also want to add show_legend=False for the polygons, in case the default behaviour of holoviews changes in the future?

nvaytet · 2026-01-07T21:48:50Z

+        y_unit:
+            Unit for y coordinates (from the detector data coordinates).
+        index_offset:
+            Starting index for polygon ROIs (e.g., 4 for indices 4-7).


Can you explain in what use cases do we need to have an index_offset?

It ensures that all ROIs have different colors, i.e., rectangles and polygons do not both start with the same color sequence (blue->red->yellow->...).

nvaytet · 2026-01-07T21:51:00Z

+
+        rois = {}
+        for i, (xs, ys) in enumerate(zip(xs_list, ys_list, strict=True)):
+            # Skip polygons with fewer than 3 vertices


I'm guessing from this that polygons are always closed, so 3 vertices make a triangle which also has an edge between the last and the first vertex. Right? (basically my question is you only need 3 vertices, not 4?).

Correct, they are always closed.

nvaytet · 2026-01-07T21:52:37Z

+        for idx in sorted(rois.keys()):
+            roi = rois[idx]
+            poly_dict: dict[str, Any] = {
+                'x': [float(v) for v in roi.x],


Do you have to convert to lists or can you send numpy arrays to holoviews?

nvaytet · 2026-01-07T21:59:24Z

+        pipe = hv.streams.Pipe(data=[])
+        dmap = hv.DynamicMap(self._create_element, streams=[pipe])
+
+        # Create edit stream (store to prevent GC)


What exactly gets GCed if this is omitted?

nvaytet · 2026-01-07T22:02:05Z

+        style = self._get_style()
+        return dmap.opts(
+            color=style.color,
+            fill_alpha=0,


Shouldn't the alpha also come from the style?

nvaytet · 2026-01-07T22:09:35Z

+        )
+
+    def _should_skip_edit(self, new_rois: dict[int, PolygonROI]) -> bool:
+        """Skip while user is actively drawing (trailing duplicate vertex)."""


Not sure I followed the logic. We want to skip editing while the user is initially drawing the polygon? I would have thought we are not editing the polygon when drawing it, but instead expected editing to only apply to a polygon that already exists...

This prevents publishing incomplete polygons (while still drawing). This caused all kinds of trouble (already in the original implementation), so this approach was just "moved over" from the old implementation. I'll check if some comment got lost....

SimonHeybrock · 2026-01-08T07:36:03Z

Five layers needed for rectangles + polygons: If both rectangle and polygon ROIs are needed, this requires five separate layers

😞 Can we have a single overlay layer that plots rectangles, polygons and static lines?

No, because that would complicate the mechanism. We will instead likely support "collapsing" the headers, or alternatively support something like layer-groups.

…mprovements - Clarify DynamicMap wrapping comment in plotting_controller.py - Add show_legend=False to Polygons readback plotter for consistency - Add explanatory comments for: index_offset purpose, polygon closure, float conversion for Bokeh, GC reference rationale, fill_alpha=0 intent - Preserve detailed PolyDraw skip logic comment from old implementation - Add get_default_index_offset() and get_default_num_rois() helpers to roi_names.py to eliminate hardcoded magic number fallbacks Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

SimonHeybrock and others added 15 commits January 6, 2026 11:45

Improve naming

29fb853

Cleanup

0530784

Consistent output naming

f68efe2

nvaytet self-assigned this Jan 7, 2026

Base automatically changed from roi-readback-plotters to main January 7, 2026 13:39

nvaytet reviewed Jan 7, 2026

View reviewed changes

SimonHeybrock and others added 2 commits January 8, 2026 07:49

Merge branch 'main' into roi-request-plotters

d773710

nvaytet approved these changes Jan 8, 2026

View reviewed changes

SimonHeybrock merged commit 80ef561 into main Jan 8, 2026
4 checks passed

SimonHeybrock deleted the roi-request-plotters branch January 8, 2026 08:16

Conversation

SimonHeybrock commented Jan 7, 2026

Summary

Known Caveats

Uh oh!

nvaytet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonHeybrock Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonHeybrock commented Jan 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SimonHeybrock Jan 8, 2026 •

edited

Loading