Open
Conversation
Implement a sciline-based monitor workflow that runs in the data_reduction namespace alongside the existing monitor_data service workflow. This uses ess.reduce.streaming.StreamProcessor for accumulation. Key components: - WindowAccumulator: clears after each finalize via on_finalize() hook - histogram_monitor_data: handles both event-mode (binned events) and histogram-mode (already-histogrammed) monitor data - StreamProcessorWorkflow wrapper for sciline Pipeline integration - Auto-registration of monitor_view spec via Instrument.__post_init__ The workflow produces the same outputs as MonitorStreamProcessor: cumulative histogram, current window histogram, and ratemeter counts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add time, start_time, end_time coords to monitor workflow outputs to match behavior of MonitorStreamProcessor. The StreamProcessorWorkflow now tracks the time range during accumulate() and adds coords to specified window outputs in finalize(). Window outputs (current, counts_total, counts_in_toa_range) get the coords; cumulative output does not since it spans all time. Prompt: The previous commit adds an alternative monitor workflow implementation. Please look into adding missing time/start_time/end_time coords to match behavior of the old MonitorStreamProcessor. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Match the title and description of the new data_reduction/monitor_view workflow spec with the existing monitor_data/monitor_histogram spec, since monitor_view is intended to eventually replace it. Also remove "V2" temporal reference from test docstring. --- Prompt: Please review changes since `add-start-end-time-coords`. Is everything in good shape and ready for a PR? Follow-up: Aside from what you found, I think we should also make the new workflow title and description match the old one, as we plan to eventually replace it? Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Strip the old MonitorStreamProcessor and MonitorHandlerFactory, move the new StreamProcessor-based monitor workflow from data_reduction namespace to monitor_data namespace, and adjust preprocessor setup accordingly. Changes: - Delete monitor_data_handler.py (contained only removed classes) - Rename create_monitor_view_workflow to create_monitor_workflow - Move monitor workflow spec registration from data_reduction to monitor_data namespace - Use ReductionHandlerFactory in monitor_data service instead of MonitorHandlerFactory - Remove CollectTOA accumulator (only used by removed MonitorHandlerFactory) - Update tests: move TestMonitorDataParams, update spec registration tests The new workflow uses ToNXevent_data preprocessor for events and Cumulative for histogram data, supporting both event-mode and histogram-mode monitor inputs. Integration tests confirm all monitor data service configurations work correctly. User request: (1) strip the old one and (2) make the new workflow part of the `monitor_data` namespace (and adjust preprocessor setup)
Re-export MonitorEvents from accumulators.py to maintain API compatibility with message_adapter.py which imports it from there. Fix test import in monitor_workflow_test.py that incorrectly used the sciline NewType TOAEdges instead of the Pydantic model from parameter_models. Prompt: Please review changes since `add-start-end-time-coords` - we have replaced a bespoke monitor workflow with the more generic approach based on StreamProcessorWorkflow. Is everything consistent and in good shape? Are tests adequate? Does the functionality look identical? Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
Which is the part in |
Member
Author
|
I will update this once the larger (but equivalent) detector workflow PR is merged, as there is slight overlap. |
Merge branch 'main' into monitors-as-sciline-workflow. Align PR #655 with patterns from PR #658: - Move NoCopyAccumulator and NoCopyWindowAccumulator to accumulators.py as shared infrastructure for both monitor and detector workflows - Update monitor workflow to use NoCopy accumulators (matching detector view) - Remove GroupIntoPixels and CollectTOA (each removed by one of the PRs) - Update detector_view/workflow.py and factory.py to import from accumulators - Update tests to use consolidated accumulator locations Both PRs independently added the window_outputs feature to StreamProcessorWorkflow, which merged cleanly. --- Prompt: We need to make #655 ready. Aside from merging with main, we need to ensure this is in-line with the equivalent changes in #658 (for detectors instead of for monitors). If in doubt, we want to adopt the patterns as in #658. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extend the monitor workflow to support time-of-flight (TOF) coordinate mode, sharing infrastructure with the detector workflow. Changes: - Rename TOA-specific types in monitor_workflow_types.py to be mode-agnostic (TOAEdges → HistogramEdges, etc.) with backwards compatibility aliases - Extend MonitorDataParams with coordinate_mode, tof_edges, tof_range, wavelength_edges, wavelength_range fields and helper methods - Add histogram_raw_monitor and histogram_tof_monitor providers for mode-specific histogramming - Update build_monitor_workflow to accept coordinate_mode parameter - Update create_monitor_workflow to accept coordinate_mode and range_filter - Add tests for coordinate mode functionality The monitor workflow now supports: - TOA mode (default): uses time-of-arrival edges and range - TOF mode: uses time-of-flight edges and range - Wavelength mode: declared but raises NotImplementedError Original prompt: Implement the following plan: Add TOF Mode Support to Monitor Workflow
Refactor monitor workflow to use the same pattern as detector view workflow for TOF mode support: - Use GenericTofWorkflow as base workflow (provides TOF conversion providers) - histogram_raw_monitor now takes RawMonitor[SampleRun, NXmonitor] for TOA mode - histogram_tof_monitor now takes TofMonitor[SampleRun, NXmonitor] for TOF mode - Input type is always NeXusData[NXmonitor, SampleRun] (same as detector view) - Add geometry_filename parameter (required for TOF mode, for Ltotal computation) - Add tof_lookup_table_filename parameter (required for TOF mode) - For TOA mode without geometry file, provide minimal EmptyMonitor directly - Remove backward compatibility aliases from monitor_workflow.py and monitor_workflow_types.py Prompt: Please understand how the detector view workflow implements TOF mode based on GenericTofWorkflow. Then find out how to apply the same approach in monitor_workflow.py which currently uses an incorrect shim. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace auto-registration in Instrument with explicit registration per instrument, following the established detector view pattern. This allows instruments to configure TOF lookup tables for monitor workflows. Changes: - Remove monitor auto-registration from Instrument.__post_init__ - Remove monitor_params field from Instrument dataclass - Add TOAOnlyCoordinateModeSettings and TOAOnlyMonitorDataParams for instruments without TOF lookup tables - Add explicit monitor workflow registration for all instruments with monitors (dummy, dream, bifrost, loki, odin, nmx) - DREAM uses DreamMonitorDataParams with InstrumentConfiguration for TOF mode support - Other instruments use TOAOnlyMonitorDataParams (TOA mode only) - Update tests to use explicit registration pattern Original prompt: Consider monitor_workflow.py - we need to allow configuring/passing tof_lookup_table_filename. Let us try for DREAM, following the detector view pattern.
- Change fake_monitors da00 mode to use 'frame_time' dimension instead of 'toa'. The essreduce _time_of_flight_data_histogram function expects one of 'time_of_flight', 'tof', or 'frame_time' as the coordinate name. 'frame_time' matches what the production data uses. - Add regression test for TOF coordinate mode with histogram input data. The test currently fails due to non-monotonic TOF coordinates from the lookup table interpolation, which causes rebin_strictly_increasing to fail. This documents the known issue for future fixing. Prompt: Getting an error when running DREAM monitor workflow in TOF mode, see logs.json - note that the actual problem is likely the warnings in accumulate, before the error. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test was failing because monitor_bunker has an Ltotal of only 6.62 m (source at z=-76.55m, monitor at z=-69.93m), which is outside the DREAM TOF lookup table range (59.85-80.15 m). The lookup table is designed for detector distances, not the short bunker monitor path. Changes: - Use monitor_cave (Ltotal 72.33 m) in the TOF histogram input test since it's within the lookup table range - Relax count assertions since TOF rebinning doesn't preserve exact counts (some bins fall outside the target TOF range) - Add factory-level validation in DREAM to reject TOF mode for monitor_bunker with a clear error message - Add tests for the new validation Prompt: Run pytest -m slow tests/handlers/monitor_workflow_test.py and try to find out if something is wrong with either the input data or the position information, leading to the error. Follow-up: Add validator to prevent selecting TOF mode for bunker monitor. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SimonHeybrock
changed the title
Extract shared logic into helper functions: - _resolve_tof_lookup_table_filename: used by detector view and monitor factories - _configure_powder_workflow: common powder workflow setup - _powder_dynamic_keys, _powder_accumulators, _focussed_target_keys: shared config Prompt: @factories.py has some duplicate code between monitor and detector, e.g., for tof_lookup_table_filename, can we avoid this? Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test class was duplicated in three files. Consolidated the most comprehensive version (7 tests) into accumulators_test.py and removed duplicates from monitor_workflow_test.py and providers_test.py. Prompt: This PR appears to introduce some duplicate tests, e.g., TestNoCopyWindowAccumulator (and the other NoCopy accum). Can you find out what is duplicated? -> consolidate them into accumulators_test.py -> commit this Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace internal state check with observable behavior test in test_can_attach_factory_to_handle. Instead of accessing factory._factories, the test now uses factory.create() to verify the factory works correctly. Also removes a duplicate assertion. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
2 participants
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
This implement support for TOF mode (and prepares for "wavelength" mode) for monitors, similar to the equivalent changes for detector views in #658. This is crucial for users, as time-of-arrival is just an artifical offset since the last pulse and not meaningful . Time-of-arrival mode will likely still be used for commissioning and other sanity checks.
TOF mode is only available for instrument that have a set of known chopper configurations and associated TOF-lookup tables as part of the instrument package.
Under the hood
Replaces the legacy
MonitorStreamProcessorwith a new Sciline-based workflow usingStreamProcessorWorkflow, aligning monitor processing with the detector view architecture from #658.Key changes
histogram_raw_monitor,histogram_tof_monitor,cumulative_view,window_view,counts_total,counts_in_range) instead of the bespokeMonitorStreamProcessorclassNoCopyAccumulatorandNoCopyWindowAccumulatorfrom the consolidatedaccumulators.pymodulecoordinate_modeparameter with'toa'(time-of-arrival) and'tof'(time-of-flight) modes, mirroring the detector workflow capabilityToNXevent_datapreprocessor for events instead of the customCollectTOAaccumulatorRemoved
MonitorStreamProcessorclassMonitorHandlerFactoryclassCollectTOAaccumulatormonitor_data_handler.pymoduleThe new workflow produces identical outputs (
cumulative,current,counts_total,counts_in_toa_range) with the same time coordinate handling.Test plan
🤖 Generated with Claude Code