Repository navigation
Add "Per Detector Pixel" plot option for per-ROI outputs - #1369
Merged
Merged
Conversation
4 tasks done
SimonHeybrock
force-pushed
the
roi-per-pixel-plot-option
branch
from
October 8, 2026 14:50
42ff0aa to
cae648b
Compare
SimonHeybrock
force-pushed
the
overlay-1d-slice-dim
branch
from
October 8, 2026 15:22
9e1b2bd to
7a50323
Compare
SimonHeybrock
force-pushed
the
roi-per-pixel-plot-option
branch
2 times, most recently
from
October 8, 2026 16:11
1b6c2c0 to
454f625
Compare
SimonHeybrock
marked this pull request as draft
October 9, 2026 06:05
SimonHeybrock
force-pushed
the
roi-per-pixel-plot-option
branch
from
October 9, 2026 08:40
7d5fa53 to
3b89cf5
Compare
Lines and Overlay 1D can divide each ROI's values by the number of detector pixels inside the ROI, read from the `detector_pixels` coord that per-ROI detector-view outputs carry. This makes ROIs of different size comparable. It is applied before rate normalization, so both together give counts per second per detector pixel. The config UI offers the option only when the output template declares the coord. If the data still arrives without it, the plot shows an error frame rather than unnormalized values under a per-pixel setting. Both normalizations now run inside the render try block, so a failure shows the error frame instead of propagating out of compute(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
scipp has no unit for a detector pixel, so dividing by the dimensionless pixel count left the y-axis reading "counts" or "counts/s". The plotters now append " per detector pixel" to the unit label of the values when the option is on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With 1D support in Bars and Table, the per-ROI totals carry the detector_pixels coord into these plotters too. Timeseries Overlay plots the history of the per-ROI totals; dividing by the pixel count does not depend on the time range, so unlike rate normalization it is valid for a full history. The unit label gets the same suffix in all three. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The field is persisted in saved plot configs, and `detector_pixels` also
named the backend coord it divides by. `pixel_normalization:
{per_detector_pixel}` mirrors `rate: {normalize_to_rate}`. The tab title
stays "Detector Pixels".
The dashboard uses the coord name from `config/roi_names.py` instead of
defining its own. Whether an output can back the option is decided by
`DetectorPixelMixin.hidden_pixel_fields`, next to the field it names,
as `WindowModeMixin.hidden_fields` does for the window controls.
The tab description now states what a detector pixel is instead of
explaining when the option applies.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`Plotter._value_unit` holds the rule that the per-detector-pixel normalization shows only in the unit label, since scipp has no unit for a detector pixel. Bars, Table and the line converters take the resulting label instead of each appending a suffix. `HvConverter1d` and `create_value_dimension` accept a `unit` label override for this. SlicerPlotter applies the normalizations through the base `_normalize` instead of its own copy of the rate loop. An ROI without detector pixels has no counts either, which is why its per-pixel values are NaN (0/0); the docstring now says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The extractor tests for the `detector_pixels` coord repeated what the end-to-end window test in plots_test.py checks; only the test that a changed ROI geometry restarts the window remains. The Overlay 1D division test is covered by the rate and empty-ROI tests. The `(time, roi)` history helper moves to module level so tests outside TestOverlay1DPlotterHistory need not reach into that class. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"counts/s per detector pixel" was too long for axis labels and table headers. The label now reads counts/pixel, or counts/pixel/s together with "Counts Per Second". scipp unit aliases cannot express this: scipp has no pixel unit, and an alias only renames an exact unit, so compound units such as counts/pixel/s would print the underlying unit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"ROI total in range" made long axis labels. "ROI sum" would read as a sum over all ROIs, so the title uses Σ instead, e.g. "Σ ROI [counts/pixel]". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The "Rate" and "Detector Pixels" tabs held two options of the same kind,
whose descriptions had to point at each other. Both now sit in one
"Normalization" tab, as `normalization: {per_second, per_detector_pixel}`.
The rate option is named per_second like in the correlation histogram's
"Normalization" tab; that model is renamed to
CorrelationNormalizationParams to keep the class names distinct.
Each plotter's model holds only the options it supports: rate only for
2D and 3D plots, per detector pixel only for Timeseries Overlay. The
per-detector-pixel option still depends on the selected output, so
ModelWidget now accepts dotted hidden fields (`group.field`) that hide a
single field of a group, and hides a group whose fields are all hidden.
Saved plot configs with the `rate` key lose that setting and load with
rate normalization off.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SimonHeybrock
force-pushed
the
roi-per-pixel-plot-option
branch
from
October 9, 2026 08:50
3b89cf5 to
568e136
Compare
SimonHeybrock
marked this pull request as ready for review
October 9, 2026 09:17
Member
Author
|
LGTM |
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
1 participant
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.
What
Lines, Overlay 1D, Bars, Table and Timeseries Overlay get a "Per Detector Pixel" option. It divides each ROI's values by the number of detector pixels inside the ROI, taken from the
detector_pixelscoord of the per-ROI detector-view outputs. It is applied before "Counts Per Second", so both together give counts per second per detector pixel.An ROI that contains no detector pixels has no counts either, so it shows no value (0/0 = NaN).
The option shares a new "Normalization" tab with "Counts Per Second", which had its own "Rate" tab. Saved plot configs store both as
normalization: {per_second: ..., per_detector_pixel: ...};per_secondmatches the correlation histogram's existing "Normalization" tab. Each plotter offers only the options it supports: 2D and 3D plots only "Counts Per Second", Timeseries Overlay only "Per Detector Pixel". Breaking: saved plot configs with the oldrate: {normalize_to_rate: true}load with rate normalization off.scipp has no unit for a detector pixel, so the per-pixel division is shown only in the unit label: "counts/pixel", or "counts/pixel/s" together with "Counts Per Second", on the y-axis or in the table header. scipp unit aliases cannot express this, because an alias only renames one exact unit, and compound units such as counts/pixel/s would print the underlying unit.
The per-ROI totals from #1360 are retitled from "ROI total in range" to "Σ ROI", which keeps axis labels short, e.g. "Σ ROI [counts/pixel/s]". "ROI sum" was avoided because it reads as a sum over all ROIs.
Timeseries Overlay has no "Counts Per Second" option, because its full history spans many intervals. Dividing by the pixel count does not depend on time, so "Per Detector Pixel" is offered there.
Why
ROIs of different size cannot be compared by their summed counts, for example ROI spectra overlaid in Overlay 1D. This also covers the TBL requirement "display counts/pixel in selected ROI". A plot option avoids a separate per-pixel output for each per-ROI quantity.
When the option is offered
The "Counts Per Second" option does nothing when the data lacks its time coords. For this option that would leave a plot labelled per pixel that shows counts per ROI. Instead:
detector_pixelscoord. Otherwise the option is hidden and stays off; Timeseries Overlay then has no "Normalization" tab. For this, the config UI can now hide a single option inside a tab, not only whole tabs.The window aggregation sums counts over time but keeps the
detector_pixelscoord as it is, because the dashboard buffer treats non-time coords as constant. When an ROI edit changes the pixel count, the coord changes and the buffer restarts, so a window never divides counts gathered under one pixel count by another. An edit that keeps the pixel count does not restart the window, the same as without this option.Related
Stacked on #1358, which adds 1D support to Bars and Table and adds Timeseries Overlay, and on #1360 below it, which adds the
detector_pixelscoord to theroi_spectra_*androi_counts_in_range_*outputs (viewsroi_spectraandroi_total_in_range) and their templates. #1368 is stacked on this PR.Test plan
🤖 Generated with Claude Code