Refactor: Move create_extractors_from_params to plot_params and inline redundant wrapper

- Move create_extractors_from_params() from extractors.py to plot_params.py where
  parameter types are defined. This eliminates awkward local import of WindowMode and
  improves cohesion.
- Move TestCreateExtractorsFromParams tests to new plot_params_test.py file
- Inline PlottingController._create_extractors() as it's a thin wrapper with single
  call site; the 2-line function call is self-explanatory
- Remove unused UpdateExtractor import from plotting_controller.py
- Move UpdateExtractor from TYPE_CHECKING to regular import in
  temporal_buffer_manager.py as it's used at runtime

Original request: create_extractors_from_params might belong to a different file - see awkward import handling. Can you find a better place and move it as well as its tests?
Follow-up: The PlottingController._create_extractors methods feels redundant, just inline?

🤖 Generated with Claude Code

Co-Authored-By: Claude <[email protected]>

Author: SimonHeybrock

src/ess/livedata/dashboard/extractors.py

@@ -4,18 +4,12 @@
 
 from abc import ABC, abstractmethod
 from collections.abc import Callable
-from typing import TYPE_CHECKING, Any
+from typing import Any
 
 import scipp as sc
 
 from .plot_params import WindowAggregation
 
-if TYPE_CHECKING:
-    from ess.livedata.config.workflow_spec import ResultKey
-
-    from .plot_params import WindowParams
-    from .plotting import PlotterSpec
-
 
 class UpdateExtractor(ABC):
     """Extracts a specific view of buffered data."""
@@ -169,52 +163,3 @@ def extract(self, data: sc.DataArray) -> Any:
                 raise ValueError(f"Unknown aggregation method: {self._aggregation}")
 
         return self._aggregator(windowed_data, self._concat_dim)
-
-
-def create_extractors_from_params(
-    keys: list[ResultKey],
-    window: WindowParams | None,
-    spec: PlotterSpec | None = None,
-) -> dict[ResultKey, UpdateExtractor]:
-    """
-    Create extractors based on plotter spec and window configuration.
-
-    Parameters
-    ----------
-    keys:
-        Result keys to create extractors for.
-    window:
-        Window parameters for extraction mode and aggregation.
-        If None, falls back to LatestValueExtractor.
-    spec:
-        Optional plotter specification. If provided and contains a required
-        extractor, that extractor type is used.
-
-    Returns
-    -------
-    :
-        Dictionary mapping result keys to extractor instances.
-    """
-    # Avoid circular import by importing here
-    from .plot_params import WindowMode
-
-    if spec is not None and spec.data_requirements.required_extractor is not None:
-        # Plotter requires specific extractor (e.g., TimeSeriesPlotter)
-        extractor_type = spec.data_requirements.required_extractor
-        return {key: extractor_type() for key in keys}
-
-    # No fixed requirement - check if window params provided
-    if window is not None:
-        if window.mode == WindowMode.latest:
-            return {key: LatestValueExtractor() for key in keys}
-        else:  # mode == WindowMode.window
-            return {
-                key: WindowAggregatingExtractor(
-                    window_duration_seconds=window.window_duration_seconds,
-                    aggregation=window.aggregation,
-                )
-                for key in keys
-            }
-
-    # Fallback to latest value extractor
-    return {key: LatestValueExtractor() for key in keys}

src/ess/livedata/dashboard/plot_params.py

@@ -2,13 +2,22 @@
 # Copyright (c) 2025 Scipp contributors (https://github.com/scipp)
 """Param models for configuring plotters via widgets."""
 
+from __future__ import annotations
+
 import enum
 from enum import StrEnum
+from typing import TYPE_CHECKING
 
 import pydantic
 
 from ..config.roi_names import get_roi_mapper
 
+if TYPE_CHECKING:
+    from ess.livedata.config.workflow_spec import ResultKey
+
+    from .extractors import UpdateExtractor
+    from .plotting import PlotterSpec
+
 
 def _get_default_max_roi_count() -> int:
     """Get the default maximum ROI count from the mapper configuration."""
@@ -226,3 +235,55 @@ class PlotParamsROIDetector(PlotParams2d):
         default_factory=ROIOptions,
         description="Options for ROI selection and display.",
     )
+
+
+def create_extractors_from_params(
+    keys: list[ResultKey],
+    window: WindowParams | None,
+    spec: PlotterSpec | None = None,
+) -> dict[ResultKey, UpdateExtractor]:
+    """
+    Create extractors based on plotter spec and window configuration.
+
+    Parameters
+    ----------
+    keys:
+        Result keys to create extractors for.
+    window:
+        Window parameters for extraction mode and aggregation.
+        If None, falls back to LatestValueExtractor.
+    spec:
+        Optional plotter specification. If provided and contains a required
+        extractor, that extractor type is used.
+
+    Returns
+    -------
+    :
+        Dictionary mapping result keys to extractor instances.
+    """
+    # Import here to avoid circular imports at module level
+    from .extractors import (
+        LatestValueExtractor,
+        WindowAggregatingExtractor,
+    )
+
+    if spec is not None and spec.data_requirements.required_extractor is not None:
+        # Plotter requires specific extractor (e.g., TimeSeriesPlotter)
+        extractor_type = spec.data_requirements.required_extractor
+        return {key: extractor_type() for key in keys}
+
+    # No fixed requirement - check if window params provided
+    if window is not None:
+        if window.mode == WindowMode.latest:
+            return {key: LatestValueExtractor() for key in keys}
+        else:  # mode == WindowMode.window
+            return {
+                key: WindowAggregatingExtractor(
+                    window_duration_seconds=window.window_duration_seconds,
+                    aggregation=window.aggregation,
+                )
+                for key in keys
+            }
+
+    # Fallback to latest value extractor
+    return {key: LatestValueExtractor() for key in keys}

src/ess/livedata/dashboard/plotting_controller.py

@@ -18,11 +18,8 @@
 
 from .config_store import ConfigStore
 from .configuration_adapter import ConfigurationState
-from .extractors import (
-    UpdateExtractor,
-    create_extractors_from_params,
-)
 from .job_service import JobService
+from .plot_params import create_extractors_from_params
 from .plotting import PlotterSpec, plotter_registry
 from .roi_detector_plot_factory import ROIDetectorPlotFactory
 from .roi_publisher import ROIPublisher
@@ -232,32 +229,6 @@ def _save_plotting_config(
         )
         self._config_store[plotter_id] = config_state.model_dump()
 
-    def _create_extractors(
-        self,
-        keys: list[ResultKey],
-        spec: PlotterSpec,
-        params: pydantic.BaseModel,
-    ) -> dict[ResultKey, UpdateExtractor]:
-        """
-        Create extractors based on plotter requirements and parameters.
-
-        Parameters
-        ----------
-        keys:
-            Result keys to create extractors for.
-        spec:
-            Plotter specification containing data requirements.
-        params:
-            Plotter parameters potentially containing window configuration.
-
-        Returns
-        -------
-        :
-            Dictionary mapping result keys to extractor instances.
-        """
-        window = getattr(params, 'window', None)
-        return create_extractors_from_params(keys, window, spec)
-
     def create_plot(
         self,
         job_number: JobNumber,
@@ -325,7 +296,8 @@ def create_plot(
 
         # Create extractors based on plotter requirements and params
         spec = plotter_registry.get_spec(plot_name)
-        extractors = self._create_extractors(keys, spec, params)
+        window = getattr(params, 'window', None)
+        extractors = create_extractors_from_params(keys, window, spec)
 
         pipe = self._stream_manager.make_merging_stream(extractors)
         plotter = plotter_registry.create_plotter(plot_name, params=params)

src/ess/livedata/dashboard/roi_detector_plot_factory.py

@@ -19,8 +19,12 @@
     DataSubscriber,
     MergingStreamAssembler,
 )
-from .extractors import LatestValueExtractor, create_extractors_from_params
-from .plot_params import LayoutParams, PlotParamsROIDetector
+from .extractors import LatestValueExtractor
+from .plot_params import (
+    LayoutParams,
+    PlotParamsROIDetector,
+    create_extractors_from_params,
+)
 from .plots import ImagePlotter, LinePlotter, PlotAspect, PlotAspectType
 from .roi_publisher import ROIPublisher
 from .stream_manager import StreamManager

src/ess/livedata/dashboard/temporal_buffer_manager.py

@@ -7,16 +7,13 @@
 import logging
 from collections.abc import Hashable, Iterator, Mapping
 from dataclasses import dataclass, field
-from typing import TYPE_CHECKING, Generic, TypeVar
+from typing import Generic, TypeVar
 
 import scipp as sc
 
-from .extractors import LatestValueExtractor
+from .extractors import LatestValueExtractor, UpdateExtractor
 from .temporal_buffers import BufferProtocol, SingleValueBuffer, TemporalBuffer
 
-if TYPE_CHECKING:
-    from .extractors import UpdateExtractor
-
 logger = logging.getLogger(__name__)
 
 K = TypeVar('K', bound=Hashable)

tests/dashboard/extractors_test.py

@@ -2,8 +2,6 @@
 # Copyright (c) 2025 Scipp contributors (https://github.com/scipp)
 from __future__ import annotations
 
-from unittest.mock import Mock
-
 import pytest
 import scipp as sc
 
@@ -12,13 +10,8 @@
     LatestValueExtractor,
     UpdateExtractor,
     WindowAggregatingExtractor,
-    create_extractors_from_params,
-)
-from ess.livedata.dashboard.plot_params import (
-    WindowAggregation,
-    WindowMode,
-    WindowParams,
 )
+from ess.livedata.dashboard.plot_params import WindowAggregation
 
 
 class TestLatestValueExtractor:
@@ -505,129 +498,6 @@ def test_consistent_frame_count_with_perfect_timing(self):
         assert sc.allclose(result.data, expected_sum)
 
 
-class TestCreateExtractorsFromParams:
-    """Tests for create_extractors_from_params factory function."""
-
-    def test_fallback_to_latest_value_when_no_params(self):
-        """Test fallback to LatestValueExtractor when no window params provided."""
-        keys = ['key1', 'key2']
-
-        extractors = create_extractors_from_params(keys=keys, window=None, spec=None)
-
-        assert len(extractors) == 2
-        assert all(isinstance(ext, LatestValueExtractor) for ext in extractors.values())
-        assert set(extractors.keys()) == {'key1', 'key2'}
-
-    def test_create_latest_value_extractors_with_window_mode_latest(self):
-        """Test creation of LatestValueExtractor when window mode is 'latest'."""
-        keys = ['key1']
-        window = WindowParams(mode=WindowMode.latest)
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=None)
-
-        assert len(extractors) == 1
-        assert isinstance(extractors['key1'], LatestValueExtractor)
-
-    def test_create_window_aggregating_extractors_with_window_mode_window(self):
-        """Test creation of WindowAggregatingExtractor when window mode is 'window'."""
-        keys = ['key1', 'key2']
-        window = WindowParams(
-            mode=WindowMode.window,
-            window_duration_seconds=5.0,
-            aggregation=WindowAggregation.nansum,
-        )
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=None)
-
-        assert len(extractors) == 2
-        assert all(
-            isinstance(ext, WindowAggregatingExtractor) for ext in extractors.values()
-        )
-
-        # Verify behavior through public interface
-        extractor = extractors['key1']
-        assert extractor.get_required_timespan() == 5.0
-
-    def test_spec_required_extractor_overrides_window_params(self):
-        """Test that plotter spec's required extractor overrides window params."""
-        keys = ['key1', 'key2']
-        window = WindowParams(mode=WindowMode.latest)
-
-        # Create mock spec with required extractor
-        spec = Mock()
-        spec.data_requirements.required_extractor = FullHistoryExtractor
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=spec)
-
-        # Should use FullHistoryExtractor despite window params
-        assert len(extractors) == 2
-        assert all(isinstance(ext, FullHistoryExtractor) for ext in extractors.values())
-
-    def test_spec_with_no_required_extractor_uses_window_params(self):
-        """Test that window params are used when spec has no required extractor."""
-        keys = ['key1']
-        window = WindowParams(mode=WindowMode.window, window_duration_seconds=3.0)
-
-        # Create mock spec without required extractor
-        spec = Mock()
-        spec.data_requirements.required_extractor = None
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=spec)
-
-        assert isinstance(extractors['key1'], WindowAggregatingExtractor)
-        assert extractors['key1'].get_required_timespan() == 3.0
-
-    def test_creates_extractors_for_all_keys(self):
-        """Test that extractors are created for all provided keys."""
-        keys = ['result1', 'result2', 'result3']
-        window = WindowParams(mode=WindowMode.latest)
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=None)
-
-        assert len(extractors) == 3
-        assert set(extractors.keys()) == {'result1', 'result2', 'result3'}
-        assert all(isinstance(ext, LatestValueExtractor) for ext in extractors.values())
-
-    def test_empty_keys_returns_empty_dict(self):
-        """Test that empty keys list returns empty extractors dict."""
-        keys = []
-        window = WindowParams(mode=WindowMode.latest)
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=None)
-
-        assert extractors == {}
-
-    def test_window_aggregation_parameters_passed_correctly(self):
-        """Test that window aggregation parameters result in correct behavior."""
-        keys = ['key1']
-        window = WindowParams(
-            mode=WindowMode.window,
-            window_duration_seconds=10.5,
-            aggregation=WindowAggregation.mean,
-        )
-
-        extractors = create_extractors_from_params(keys=keys, window=window, spec=None)
-
-        extractor = extractors['key1']
-        assert isinstance(extractor, WindowAggregatingExtractor)
-        # Verify timespan through public interface
-        assert extractor.get_required_timespan() == 10.5
-
-        # Verify aggregation behavior by extracting data
-        data = sc.DataArray(
-            sc.array(dims=['time', 'x'], values=[[2, 4], [4, 6]], unit='m'),
-            coords={
-                'time': sc.array(dims=['time'], values=[0.0, 1.0], unit='s'),
-                'x': sc.arange('x', 2, unit='m'),
-            },
-        )
-        result = extractor.extract(data)
-        # Mean of [2, 4] and [4, 6] = [3, 5], verifying mean aggregation was used
-        assert sc.allclose(
-            result.data, sc.array(dims=['x'], values=[3.0, 5.0], unit='m')
-        )
-
-
 class TestUpdateExtractorInterface:
     """Tests for UpdateExtractor abstract interface."""