Add wire and strip views for additional DREAM detector banks #650
Conversation
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
Extend wire_view to support mantle and both endcap detectors (which share the same dimension structure). Extend strip_view to support all 5 detector banks by making get_strip_view generic - it now sums over all dimensions except 'strip' rather than hardcoding specific dimension names. Prompt: Please consider the structure of DETECTOR_BANK_SIZES for the views in dream/views.py - I think we could enable support for the wire view for the 2 endcap banks, and the strip view for all 5 banks? Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The ROI readback publishing code in DetectorView.finalize() called _get_detector_coord_units() unconditionally, which assumes 2D output. This caused crashes for 1D views like strip_view and wire_view that have roi_support=False. Pass the roi_support flag through the factory chain so DetectorView can skip ROI publishing when ROIs are not supported. Prompt: A recent change that made DetectorView publish empty ROI info at start seems to have introduced a bug: The code in question, including `_get_detector_coord_units` seems to be called even if the view does not support ROIs at all, and this raises, e.g., for views that return 1d output, such as the 'strip_view', even though roi_support is set to `False`. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Views with roi_support=False were still showing ROI outputs in the frontend because the workflow spec's outputs model always included ROI fields regardless of the roi_support setting. Split DetectorViewOutputs into DetectorViewOutputsBase (without ROI fields) and DetectorViewOutputs (with ROI fields). Updated make_detector_view_outputs() to accept roi_support parameter and return the appropriate outputs class. Add tests verifying spec outputs exclude ROI fields when roi_support=False. Prompt: DREAM wire_view and strip_view define roi_support=False, but for some reason the frontend still shows the ROI output in the list of available outputs. Is the option not forwarded correctly? Are we missing a test? Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
jl-wynen
approved these changes
Jan 9, 2026
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.
Summary
roi_support=Falseparameter to disable ROI functionality for aggregated views that don't need itNew outputs look like this when plotted:
Test plan
roi_support=Falsebehavior in DetectorView🤖 Generated with Claude Code