[Bifrost] Update workflows based on feedback #508
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
The bragg-peak-monitor workflow was initially implemented as a separate workflow, but this requirement was misunderstood. Instead, we should extend the generic monitor_interval_timeseries workflow with wavelength support. Changes: - Extend MonitorTimeseriesParams in workflows.py to support both time-of-arrival and wavelength interval modes via IntervalMode enum - Add _get_interval_by_wavelength() function that depends on WavelengthMonitor instead of MonitorData for wavelength-based interval selection - Update monitor_timeseries_workflow() to conditionally use either _get_interval() or _get_interval_by_wavelength() based on the selected mode - Remove the standalone bragg_peak_monitor workflow from bifrost.py - Clean up unused imports and code in bifrost.py This makes the workflow more maintainable and consistent with the codebase architecture by reusing the generic monitor workflow instead of creating instrument-specific duplicates. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- Original prompt: Consider changes to @src/ess/livedata/config/instruments/bifrost.py is the last commit. The requirement for the bragg-peak-monitor rate in #503 was misunderstood: We should not add a new workflow, but extend the generic monitor workflow from @src/ess/livedata/config/workflows.py with the new params. If wavelength is selected as IntervalMode we need to prepare a slightly different workflow, with a variant of`_get_interval` that needs to depend on `WavelengthMonitor` instead of `MonitorData`.
- Add get_start() and get_stop() methods to RangeModel base class - Both TOARange and WavelengthRange now inherit these common methods - Simplifies TOARange.range_ns property to use inherited methods - Create common _extract_interval() helper function in workflows.py - Accepts RangeModel directly instead of separate start/stop parameters - Handles all common logic: coordinate unit conversion, time_coord selection - Works for both binned and histogrammed data - Eliminates duplicate time_coord extraction logic - Simplify _get_interval() and _get_interval_by_wavelength() - Reduced from ~12 lines each to 3-4 lines each - Both functions now delegate to common helper - Only specify the dimension to slice along Benefits: - Maximum DRY principle: all duplicate code eliminated - Single source of truth for interval extraction logic - Better abstraction: helper works with any RangeModel subclass - Cleaner API: pass objects instead of extracting values first - Complete unit safety: automatic conversion handled centrally Original prompt: In @src/ess/livedata/config/workflows.py - Try to use a common helper function for the two get-interval functions. - range_m does not exist, we should just get start and stop and convert to the unit of the coord we are slicing by. Follow-up: Can we unify more, if we note that `range_ns` does the same as the code we have now, and the ranges have a common base class? Follow-up: Should we directly pass the interval to `_extract_interval`, instead of passing start and stop? Follow-up: Is the code to extract `time_coord` duplicate as well? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Rename _reduction_workflow to reduction_workflow to make it public - Insert _detector_ratemeter function into main workflow at module level - Add TestDetectorRatemeter class with comprehensive unit tests - Create _make_test_event_data helper to construct NXevent_data test fixtures - Update existing test to use public workflow name Tests verify: - Event summing in selected arc - Pixel range selection - Arc isolation with multiple arcs - Time coordinate presence The workflow is now testable without the stream-processor wrapper, enabling direct testing of the reduction pipeline. Original prompt: We plan to extend the detector ratemeter (See @src/ess/livedata/config/instruments/bifrost.py). As a preparation: - Insert ratemeter into main `_reduction_workflow` (not just in function being registered). - Make the workflow public, so we can test it. - Add TestDetectorRatemeter for the workflow. Test directly with the (renamed) `_reduction_workflow`, not the stream-processor wrapper. - Check out @src/ess/livedata/handlers/to_nxevent_data.py on how to construct a event-data DataArray that should be compatible with the NeXusData input that the workflow expects. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
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.
Part of #503. This adds: