LogicalView for ROI views #281
Conversation
Introduces a cleaner design for LogicalDownsampler that separates the "grouping"
logic (transform) from the "aggregation" logic (sum over reduction dimensions).
This is more flexible and conceptually clearer than the previous histogram-based
approach.
Key changes:
- LogicalDownsampler now takes a `transform` callable (e.g., fold operations)
and a `reduction_dim` parameter
- __call__() applies transform then sums over reduction_dim
- input_indices() applies transform to detector indices and creates binned
mapping for ROI filtering
- Supports both single and multiple reduction dimensions
- Add comprehensive unit tests using TDD approach
The new design follows the same pattern as regular logical views:
- transform defines how to group pixels (e.g., via fold operations)
- reduction explicitly sums the groups
- More flexible than hardcoded histogramming with uniform bins
Example usage:
downsampler = LogicalDownsampler(
transform=lambda da: da.fold('x_pixel_offset',
{'x_pixel_offset': 512, 'x_bin': 2}),
reduction_dim='x_bin',
detector_number=detector_number,
)
Tests added:
- Single-dimension downsampling (1D with 2x binning)
- Multi-dimension downsampling (2D with 2x2 binning)
- input_indices() correctness for 1D and 2D cases
- Varying input values are correctly summed
All 36 tests in raw_test.py pass.
Original prompt: "Please consider the latest commit. I don't think it makes
much sense as-is. Would it be more useful and conceptually cleaner if we
defined LogicalDownsampler based on a \`transform\` callable (just like an
regular logical view), but specified a dim to "sum" over via and extra
\`reduction_dim\` argument? Then \`__call__\` could simple
\`transform(data).sum(self._reduction_dim)\` and \`input_indices\` would use
\`transform(detector_number)\` and turn it to binned data along
\`_reduction_dim\`?"
Follow-up: "Yes, \`sum\` supports multiple dims (or we could flatten
beforehand). Please start a new branch off main and implement
LogicalDownsampler there as we discussed. Start by writing tests, use TDD."
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Updates RollingDetectorView to support LogicalDownsampler as a projection by replacing isinstance checks with duck typing. This makes the code more flexible and follows best practices. Key changes: - Add replicas property to LogicalDownsampler (always returns 1) - Update make_roi_filter() to use hasattr for input_indices method - Update transform_weights() to use hasattr for apply_full method - Both methods now work with Histogrammer, LogicalDownsampler, or custom callables Integration tests added: - RollingDetectorView with LogicalDownsampler as projection - make_roi_filter() works correctly with LogicalDownsampler - transform_weights() works correctly with LogicalDownsampler All 39 tests in raw_test.py pass, including 8 LogicalDownsampler tests. This completes the LogicalDownsampler implementation with full RollingDetectorView integration and ROI filtering support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Groups the LogicalDownsampler tests added in this branch into organized test classes with cleaner, shorter test names: - TestLogicalDownsampler: Tests for core LogicalDownsampler functionality - test_single_dim_downsampling - test_multi_dim_downsampling - test_input_indices_single_dim - test_input_indices_multi_dim - test_with_varying_input_values - TestRollingDetectorViewWithLogicalDownsampler: Integration tests - test_as_projection - test_make_roi_filter - test_transform_weights All 39 tests pass. No changes to tests added before this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
LogicalDownsampler now uses input_sizes (dict[str, int]) instead of detector_number (sc.Variable) parameter. This change: - Simplifies the implementation by only requiring dimension sizes, not coordinate values - Works with detectors that don't provide detector_number coordinate - Makes it clear that only the dimension structure is needed, not the actual detector number values Add RollingDetectorView.with_logical_downsampler() factory method to create a RollingDetectorView with a LogicalDownsampler projection, automatically inferring input_sizes from detector_number.sizes. This provides a clean API without hidden side effects. Update all tests to use the new API. --- Original prompt: Help me understand why LogicalDownsampler is using detector_number. I thought it could simply operate on an `arange`, like Histogramer.input_indices? Follow-up: - Can we remove the detector-number handling and simplify everything? We actually have detectors that do not provide a detector_number. - Can we infer it from detector_number if we setup via RollingDetectorView? - I am not happy about "patching" the _input_sizes in __init__. Can we instead add a staticmethod that creates a RollingDetectorView with a LogicalDownsampler from detector_number and the args the downsampler needs?
- Use math.prod instead of manual loop for total_size calculation - Unify single/multi reduction dim handling by always using transpose+flatten - Clearer variable naming (_temp, _reduction, _flat) instead of reusing flat_dim - Reduce intermediate variables and remove unnecessary comments Prompt: "Please think through LogicalDownsample implementation. I wonder if we can simplify the logic slightly?" -> "Yes, let us make input_indices cleaner." 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Rename class to better reflect its purpose as a general logical view - Make reduction_dim optional (defaults to empty list for pure transforms) - Add early return in input_indices() for non-reducing case (returns dense indices) - Rename factory method to with_logical_view() - Add tests for non-reducing transforms (slicing use case) This enables using LogicalView for transforms that don't reduce, such as selecting a front layer from a volumetric detector. Original prompt: Please consider changes in this branch. Think about how to support logical views that do not actually sum but instead just transform, e.g., to select a certain slice of the input. Can we extend LogicalDownsampler to support this case (reduction dims as empty list?)? Would it be a better alternative do have a separate class instead? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
SimonHeybrock
changed the title
There was a problem hiding this comment.
Did you consider adapting https://github.com/scipp/essdiffraction/blob/35310a285ba0c325def3aa3ae5795915cc10db19/src/ess/dream/diagnostics.py#L19 ? This would be able to show all voxels / pixels without user-defined projections.
There was a problem hiding this comment.
That is too large for streaming and imho not useful, since finding failing components in a view with >1 Mpixel is not trivial. Also I am not sure how it relates to ROIs?
There was a problem hiding this comment.
since finding failing components in a view with >1 Mpixel is not trivial.
True. But when you combine pixels, failures can affect many display pixels and are combined with noise from working components. So seeing a failing pixel can be tricky, too.
Also I am not sure how it relates to ROIs?
I don't think it does unless the pixels are ordered in a meaningful way.
What kinds of ROIs is this meant for? Is this about data reduction or about diagnostics? It seems like the former because the projections you made for dream are about physical location of voxels.
There was a problem hiding this comment.
Not data reduction. This PR is unrelated to the DREAM PR. This is mainly for things such as ODIN, TBL, or NMX which display a 2D image detector (with our without downsampling).
There was a problem hiding this comment.
since finding failing components in a view with >1 Mpixel is not trivial.
True. But when you combine pixels, failures can affect many display pixels and are combined with noise from working components. So seeing a failing pixel can be tricky, too.
ROI support is for displaying TOF spectra for the selected ROI. This is not for seeing failing pixels.
There was a problem hiding this comment.
Do you want to select an actual region or a single (output) pixel? The latter could be done with a flat voxel view as well.
There was a problem hiding this comment.
A region. And we are not doing flat voxel views in live reduction.
src/ess/reduce/live/raw.py
Outdated
| Implements a view by applying a user-defined transform (e.g., fold or slice | ||
| operations) optionally followed by reduction (summing) over specified dimensions. | ||
| This provides a clean separation between "how to reshape/select pixels" (transform) | ||
| and "how to aggregate them" (sum over reduction dimensions). |
There was a problem hiding this comment.
Not really correct. Both transformation and reduction are passed as arguments to the same method and are used in the same class. I wouldn't call this separation.
It looks to me like the real reason is that you need to know what reductions have been performed to construct the binned input_indices.
There was a problem hiding this comment.
Not sure I understand the comment. Without using this we have something like https://github.com/scipp/esslivedata/blob/4228f6721fdff66d922609612a6c1912811834be/src/ess/livedata/config/instruments/odin/factories.py#L14-L23 - a single transform performs reshape and reduction. What this class is a separation of the two steps. Yes the reason for wanting this is that we need to apply a different reduction to the indices, but it is still a separation of the old-style transform that does both. But happy to reformulate if you have a suggestion?
There was a problem hiding this comment.
From a caller perspective, it doesn't matter why there is a separation. All they see is that they have to specify the projection in two pieces instead of one. But ultimately, it's still lumped together as an instance of LogicalView.
I wrote this comment because the docstring threw me off when reading the code. How about this?
Implements a view by applying a user-defined transform (e.g., fold or slice
operations) optionally followed by reduction (summing) over specified dimensions.
Transformation and reduction must be specified separately for `LogicalView` to construct a mapping from output indices to input indices.
So ``transform`` must not perform and reductions.
This is intended for addressing scipp/esslivedata#531.
Changes
LogicalViewclass provides a transform-based approach for creating logical detector viewsRollingDetectorView.with_logical_view()factory method for easy integrationExample usage