Treat correlation histograms as special plot type #642
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
This implements the UI components for configuring correlation histogram plots through the PlotConfigModal wizard (issue #611). Correlation histograms are now presented as plotter types that can be selected in Step 2 of the wizard. Key changes: - Add find_timeseries_outputs() helper to discover timeseries from workflow registry before data exists in DataService - Add default_factory templates to timeseries output specs so they can be identified by find_timeseries_outputs() - Register correlation_histogram_1d and correlation_histogram_2d plotters with same DataRequirements as timeseries plotter - Add simplified param models (x_bins, y_bins) for wizard - plotter will auto-determine bin ranges from data at runtime - Extend PlotterSelectionStep with correlation axis dropdown(s) that appear when correlation histogram plotter is selected - Store correlation axes in PlotConfig.data_sources list PlotOrchestrator support for actually rendering multi-source correlation histogram plots is not yet implemented. Prompt: "We need to continue with the implementation here. For the params in 'Step 3', I wonder if we could for now use a pragmatic solution and simple provide and `int` field to set the number of bins? The plotter would determine range on its own." 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The on_first_data callback was triggered as soon as ANY data source had data, but correlation histograms require ALL data sources (primary + axes) to be available before the plotter can initialize. Add requires_all_keys property to StreamAssembler base class (defaults to False for progressive plotting) and set it to True in OrderedCorrelationAssembler. The DataSubscriber now checks this property before invoking the callback. Prompt: Please read @docs/developer/plans/correlation-histogram-plot-config.md and help me fix the error I get when creating such a plot: @log.txt 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement infrastructure for layers that need data from multiple workflows, such as correlation histograms that correlate timeseries against axis sources. Changes: - Add _subscribe_multi_source_layer() for coordinating multiple workflow subscriptions - Add _on_multi_source_workflow_available() to track workflow readiness - Add _setup_multi_source_pipeline() to set up data pipelines when all workflows ready - Add setup_data_pipeline_from_keys() to PlottingController for pre-built ResultKeys - Update cleanup logic to handle multi-source subscriptions - Update plan document with implementation status This is preparatory work for the correlation histogram refactor that will separate data sources from axis sources for proper multi-histogram support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The simplified correlation histogram param classes were nesting display options (layout, plot_scale, ticks, plot_aspect) inside a single `display` field. This caused ModelWidget to fail because ParamWidget can only handle flat primitive fields, not nested Pydantic models. Changes: - SimplifiedCorrelationHistogram1dParams now inherits from PlotDisplayParams1d - SimplifiedCorrelationHistogram2dParams now inherits from PlotDisplayParams2d - Correlation plotters access display params directly instead of via .display - CorrelationHistogramAssembler.assemble() defensively checks for missing keys - Updated tests to use new data_source API instead of data_sources list 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This class was marked as deprecated and replaced by CorrelationHistogramAssembler during the correlation histogram refactoring. No code in the codebase uses it anymore. The new CorrelationHistogramAssembler provides explicit separation of data sources and axis sources, which is clearer than the implicit ordering used by the old class. Original prompt: Can we remove OrderedCorrelationAssembler?
Move correlation histogram plotter classes from correlation_histogram.py to new correlation_plotter.py module. This separates the new plotter implementations (meant to eventually replace old functionality) from the existing configuration adapter and controller logic. Changes: - Create correlation_plotter.py with: - Simplified parameter models (SimplifiedCorrelationHistogram1d/2dParams) - Bin parameter models (Bin1d/2dParams) - CorrelationHistogramData and CorrelationHistogramAssembler - Plotter classes (CorrelationHistogram1d/2dPlotter) - Helper function _compute_edges_from_data - Restore correlation_histogram.py to match main branch - Update imports in plot_orchestrator.py, plotting_controller.py, and plotting.py to use new module correlation_histogram.py now has no diff to main as requested. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --- Original prompt: Please have a look at @src/ess/livedata/dashboard/correlation_histogram.py and the changes since `static-geometry-overlay`. The changes are menat to eventually replace the old functionality so I would like to move everything we have changed to a new file. Verify that src/ess/livedata/dashboard/correlation_histogram.py does not appear in the diff to `main` after you are done.
The correlation histogram plotters were computing bin edges only once during initialization, causing data outside the initial range to be excluded as the range grew over time. Now use scipp's bin count support (hist/bin with integer argument) which auto-determines edges from the current data range on each call. This ensures the histogram always covers the full data range. --- Prompt: Have a look at correlation_plotter.py - this is a bit broken since it determines edges only once on init, not taking into account that the range may grow. Please think about a solution. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
LinePlotter now accepts an `as_histogram` parameter that, when True, preserves bin edges so histograms render as step-style plots rather than smooth curves. This is used by CorrelationHistogram1dPlotter to render correlation histograms properly. Prompt: Plotters in correlation_plotter.py use LinePlotter which defaults to converting histograms to lines. Add an option to disable this behavior so correlation histograms are actually plotted as histograms. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The new correlation plotter approach (correlation_plotter.py) replaces the old workflow-based correlation histogram implementation. The new approach: - Uses the standard plotter registry pattern instead of special-case workflow handling in WorkflowController - Auto-determines bin edges from data (simpler UI with just bin counts) - Uses existing LinePlotter/ImagePlotter for rendering Removed: - correlation_histogram.py and its tests - CorrelationHistogramController from dashboard_services.py - Correlation histogram special handling from workflow_controller.py - Related design/planning .md documentation files Moved CorrelationHistogramParams and NormalizationParams to correlation_plotter.py as they are still used by the new plotters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --- Original prompt: This branch adds a new way of doing correlation histograms via correlation_plotter.py. We can thus remove correlation_histogram.py; remove tests etc.
Simplify PlottingController API by merging setup_correlation_histogram_pipeline into setup_data_pipeline_from_keys with an optional axis_keys parameter. When axis_keys is provided, uses CorrelationHistogramAssembler to separate data sources from axis sources. Otherwise uses the standard MergingStreamAssembler. This reduces API surface while keeping the semantic distinction between data and axis sources explicit through the parameter. Prompt: Please think through the CorrelationHistogramAssembler mechanism added in this branch, including the bespoke PlottingController.setup_correlation_histogram_pipeline; I wonder if this could be avoided slightly modifying the old logic to catch exceptions in `on_first_data`. Then the correlation setup could raise if the axes are not available yet. Follow-up: Yes, let us try the merge and see how that looks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused **kwargs from CorrelationHistogram1dPlotter and CorrelationHistogram2dPlotter __init__ methods - Remove redundant _correlation_histogram_1d_factory and _correlation_histogram_2d_factory wrapper functions, use from_params class methods directly in the plotter registry - Rename SimplifiedCorrelationHistogram1dParams to CorrelationHistogram1dParams and SimplifiedCorrelationHistogram2dParams to CorrelationHistogram2dParams - Rename base class CorrelationHistogramParams to _CorrelationHistogramBase to avoid name collision Prompt: Why does CorrelationHistogram1dPlotter.__init__ take kwargs? Why is there the _correlation_histogram_1d_factory wrapper? Follow-up: Yes please, and also remove the `Simplified` prefix from the params?
This reverts commit d1c4f3a.
When data timestamps are before the first axis timestamp, sc.lookup with mode='previous' returns NaN for all lookups, causing hist() to fail with "Empty data range". This can happen during startup when data arrives before axis data is available. The fix uses mode='nearest' when data_max_time < axis_min_time, mapping data to the closest axis value instead of producing NaNs. Prompt: Somehow we end up with empty (length 0) in `dependent` in __call__ (which then raises in `hist`). I think this should not be possible because the assembler prevents triggering in such cases? Follow-up: Let us try a pragmatic solution: If data max timestamp is less than axis timestamp(s), use `mode='nearest'`? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Display meaningful axis labels (e.g., "motor_position") on correlation histogram plots instead of generic "x" and "y". - Add frozen x_axis_source field to Bin1dParams (already present) - Add frozen x_axis_source and y_axis_source fields to Bin2dParams - Update plotters to use axis source names as coordinate names - Add _inject_axis_source_names helper to populate fields from wizard The axis source names are set when configuring the plot in the wizard, displayed as disabled fields in the UI, and used as axis labels in the rendered plots. Prompt: The new correlation plotter plots just show "x" (and "y") on the axes, instead of using the names of what we correlated against. Please think about how we can achieve this? One idea I had was to set it in CorrelationHistogram2dParams (or 1d) as a disabled field (then the UI would display it but the user cannot edit it). Follow-up: Should it be part of, e.g., Bin1dParams? That would put it nicely side-by-side in the UI with the bin count input. I think all that needs to change in __call__ is to use proper coord names when setting the result of the lookup operation, instead of 'x' and 'y', right? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The axis source names were only injected in _on_config_collected, but the params widget is created earlier in _create_config_panel. This caused the disabled axis source fields to appear empty during initial configuration. Now, for new correlation histograms, the config_state is pre-populated with axis source names from the wizard's correlation_axes selection. Prompt: Works, except that the injected sources do not show in the modal (there is no value set). Only when reopening the modal (to edit settings) the source names appear. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When reconfiguring a correlation histogram plot, SpecBasedConfigurationStep.on_enter was creating a PlotterSelection from the initial config but not copying axis_sources into correlation_axes. This caused the orchestrator to use the standard pipeline (MergingStreamAssembler) instead of the correlation pipeline, resulting in the plotter receiving a dict instead of CorrelationHistogramData. Prompt: Getting error in @log.txt when reconfiguring a correlation plot (2d), or reloading the page (sometimes). Please think and find out what is going on.
Introduce LayerSubscription to handle subscribing to multiple workflows for a single plot layer. This enables uniform handling of both simple plots and future correlation histograms. Key changes: - Add ready_condition parameter to DataSubscriber to gate on_first_data - Extend StreamManager.make_merging_stream() to accept ready_condition - Create LayerSubscription class that tracks multiple workflow lifecycles - Add unified setup_pipeline() method to PlottingController - Refactor PlotOrchestrator to use LayerSubscription for all non-static layers, removing the "Multiple data sources not yet supported" stub The ready_condition ensures correlation plots wait for all required data sources before plot creation, while allowing progressive display within each data source (at least one key from each DataSourceConfig). Original prompt: "Please read @docs/developer/plans/multi-job-subscription-abstraction.md and implement on a new branch." 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move the special ROI detector logic from setup_data_pipeline to setup_pipeline so that roi_detector plots work through the unified PlotOrchestrator path. ROI detector needs separate streams per detector (not merged), with coordination to invoke the callback with a dict[ResultKey, Pipe] when all detectors have data. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the legacy setup_data_pipeline method from PlottingController since all functionality is now in setup_pipeline. Update tests to use the new unified interface. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Both _build_result_keys and _build_ready_condition were iterating self._data_sources and constructing ResultKey objects with identical logic. Extract the shared iteration and key construction into _keys_by_data_source, which returns keys grouped by data source. Prompt: LayerSubscription has some near-duplication iterating self._data_sources and building ResultKey. Can this be done cleaner? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…otter-clean-orchestrator
…urces Simplify correlation histogram pipeline by removing the special CorrelationHistogramData dataclass. Plotters now receive a plain dict[ResultKey, sc.DataArray] and identify axis data by matching source_name against the configured axis source names in params. Changes: - correlation_plotter.py: Remove CorrelationHistogramData, add _separate_axis_data() helper, update plotters to accept plain dict - plot_config_modal.py: Flatten axis_sources into data_sources when creating PlotConfig, add _extract_axis_sources_from_config() for edit mode reconstruction - correlation_plotter_test.py: Update tests to use plain dict with ResultKey instead of CorrelationHistogramData - detector_view_specs.py: Fix pre-existing duplicate default_factory syntax errors Prompt: We are in the middle of refactoring the correlation plotting mechanism. I think we got all components working. We now need to use the new multi-subscription mechanism for the actual setup.
The _ready_fired flag was a one-shot mechanism that prevented on_ready from firing again when a job ended and was replaced by a new one. This broke plot updates when workflows were restarted. The fix simplifies the mechanism: - Remove _ready_fired flag entirely (JobOrchestrator guarantees unique job_numbers on each commit, so duplicate notifications can't happen) - Fire on_ready whenever all data sources have job_numbers - On stop, clear only the stopped source's job_number so the length check fails until restart - Bind ds_index to on_stopped callback to know which source stopped Also fixes FakeJobOrchestrator in tests to properly track workflow subscriptions and only notify relevant subscribers. Prompt: Please consider LayerSubscription, the start/stop/ready mechanism has at least one bug. In particular when a job ends and is subsequently replaced by a new one we need to be able to correctly call _self._on_ready again. The single-shot mechanism based on on self._ready_fired is incorrect/insufficient. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change PlotConfig.data_sources to use dict[str, DataSourceConfig] instead of
list[DataSourceConfig]. This enables cleaner role-based data organization
throughout the subscription and assembly chain.
Key changes:
- PlotConfig.data_sources now uses role keys ("primary", "x_axis", "y_axis")
- LayerSubscription tracks by role and produces keys_by_role dict
- DataSubscriber unified to handle both single-role and multi-role assembly:
- Single role: returns flat dict[ResultKey, data] for standard plots
- Multiple roles: returns dict[str, dict[ResultKey, data]] for correlation plots
- Ready condition built internally in DataSubscriber (data from each role)
- Removed separate StreamAssembler, MergingStreamAssembler, RoleAwareAssembler
- StreamManager.make_stream() simplified to use keys_by_role directly
- Added data_roles.py with role constants to avoid circular imports
This simplifies the LayerSubscription/PlottingController interaction and
eliminates awkward axis source extraction logic in plot_config_modal.
Prompt: "In both correlation_plotter.py and plot_config_modal.py we have some
'ugly' logic extracting axis names/sources from PlotConfig. Please think about
whether changing PlotConfig to data_sources: dict[str, DataSourceConfig] would
avoid this, while keeping the design sound."
Follow-up: "can't this be done directly in the DataSubscriber since the only
ready condition we want is 'there is data for each role'? And do we really need
to distinguish MergingStreamAssembler and RoleAwareAssembler or can it be one
and the same thing?"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract a generalized CorrelationHistogramPlotter base class that handles arbitrary numbers of axes. The 1D and 2D plotters now become thin wrappers that configure the base class with appropriate axis specs and renderers. This eliminates duplicated code for: - Axis data extraction and validation - Lookup table creation for coordinate assignment - Histogram computation with optional normalization Also extract helper functions: - _histogram_with_normalization: handles binning with optional rate normalization - AxisSpec dataclass: specifies role, coordinate name, and bin count per axis Tests are reorganized to: - Test _make_lookup and _histogram_with_normalization directly - Test the base CorrelationHistogramPlotter with parameterized axis counts - Reduce 1D/2D wrapper tests to simple wiring verification Prompt: Consider @correlation_plotter.py - would it be reasonable to share most of the code between the 1d and 2d plotters? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
CORRELATION_HISTOGRAM_PLOTTERS was defined in both correlation_plotter.py and plot_config_modal.py. Moved the definition to correlation_plotter.py and imported it in plot_config_modal.py to eliminate duplication. User request: CORRELATION_HISTOGRAM_PLOTTERS seems to be defined twice?
The _histogram_with_normalization helper function was only called once. Following the principle of not creating utilities for one-time operations, inline its logic directly into the __call__ method where it's used. Also remove the corresponding private method tests since we don't test implementation details. The normalization behavior is implicitly tested through the public plotter interface tests. User request: _histogram_with_normalization seems pointless, can we inline?
The convert_image_2d function interprets dims[0] as the vertical axis and dims[1] as the horizontal axis. The plotter was creating axes in [X, Y] order, resulting in X on vertical and Y on horizontal - swapped. Reorder axes to [Y, X] so that X maps to horizontal and Y to vertical. Prompt: Please consider CorrelationHistogram2dPlotter - is the axes setup correct, such that x and y are indeed the horizontal and vertical axis when plotting? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…elation plotters Refactor correlation histogram plotters to use LinePlotter.from_params() and ImagePlotter.from_params() instead of manually extracting display parameters. This eliminates fragility where adding new display parameters would require updating both the parameter class and the plotter constructors. - Add Curve1dRenderMode enum with 'curve' and 'histogram' modes - Add curve_mode field to PlotDisplayParams1d (defaults to 'curve') - Override curve_mode in CorrelationHistogram1dParams to default to 'histogram' - Update LinePlotter.from_params() to read curve_mode and set as_histogram - Simplify CorrelationHistogram1dPlotter and CorrelationHistogram2dPlotter to use factory methods - Fix pre-existing test assertion for 2D axis order (Y first, then X) - Fix 5 tests incorrectly using PlotParams2d with LinePlotter.from_params() Prompt: Could we instead add a hist/curve switch in PlotDisplayParams1d (and use it to replace the `as_histogram` param) and default to True in the correlation plotter params? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Resolved conflicts: - workflow_spec.py: Kept find_timeseries_outputs (new feature), removed WorkflowStatusType/WorkflowStatus (replaced by JobState in main) - plotting.py: Kept correlation histogram plotters (new feature), removed roi_detector plotter (removed in main, replaced by new ROI plotters) - plotting_controller.py: Kept new setup_pipeline API with keys_by_role, removed roi_detector special case handling (plotter removed in main) - roi_detector_plot_factory.py: Accepted deletion (replaced by new ROI plotter architecture in main) - plotting_controller_test.py: Removed TestROIDetectorTwoPhaseCreation tests (roi_detector plotter removed in main)
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
Fixes #611
Refactors correlation histograms from a "workflow-based" approach to treating them as a special kind of plot. This aligns with user expectations: users think of "plotting detector counts against temperature", not "running a correlation workflow".
Key changes:
correlation_plotter.pymodule with dedicated plotter implementations (replacescorrelation_histogram.py)LayerSubscriptionabstraction that hides single vs multi-job subscription complexity fromPlotOrchestratorPlotConfig.data_sourcesfromlisttodict[role, DataSourceConfig]with role keys (primary,x_axis,y_axis)PlotConfigModalas_histogramoption toLinePlotterfor step-style histogramsTest plan
LayerSubscriptionand correlation plotter🤖 Generated with Claude Code