From feedback

src/openflexure_microscope_server/things/autofocus.py

@@ -21,7 +21,7 @@ from pydantic import BaseModel, computed_field, field_validator, model_validator
 import labthings_fastapi as lt
 from labthings_fastapi.types.numpy import NDArray
 
-from .camera import BaseCamera, CaptureParams, validate_capture_params
+from .camera import BaseCamera, CaptureParams
 from .stage import BaseStage
 
 LOGGER = logging.getLogger(__name__)
@@ -75,6 +75,22 @@ class StackParams(BaseModel):
     origin: StackOrigin = StackOrigin.START
     """Where the stack is positioned relative to the current z position."""
 
+    @field_validator("images_to_save")
+    @classmethod
+    def validate_images_to_save(cls, value: int) -> int:
+        """Validate that the number of images to save is greater than zero."""
+        if value <= 0:
+            raise ValueError(f"Invalid number of images to save: {value}. Must be > 0.")
+        return value
+
+    @field_validator("settling_time")
+    @classmethod
+    def validate_settling_time(cls, value: float) -> float:
+        """Validate that settling time is zero or positive."""
+        if value < 0:
+            raise ValueError(f"Invalid settling time: {value}. Must be positive or 0.")
+        return value
+
 
 class SmartStackParams(StackParams):
     """A class for holding smart stack parameters, and returning computed ones."""
@@ -765,10 +781,6 @@ class AutofocusThing(lt.Thing):
             - Final z position
             - List of z positions captured
         """
-        # Validate parameters and raise Exception if unsuitable
-        validate_stack_params(stack_parameters=stack_parameters)
-        validate_capture_params(capture_parameters=capture_parameters)
-
         captures: list[CaptureInfo] = []
         z_positions: list[int] = []
 
@@ -869,24 +881,3 @@ def _count_turning_points(sharpnesses: np.ndarray) -> int:
     d_sharpnesses = d_sharpnesses[prominent]
     # count the sign changes
     return int(np.sum(d_sharpnesses[1:] * d_sharpnesses[:-1] < 0))
-
-
-def validate_stack_params(stack_parameters: StackParams) -> None:
-    """Validate stack parameters for a z-stack acquisition.
-
-    Ensures that the parameters allow a physical stack, without negative or zero
-    values where they would cause crashes.
-
-    :param stack_parameters: StackParams object containing stacking settings.
-
-    :raises ValueError: If any stack_parameters are found to be unusable.
-    """
-    if stack_parameters.images_to_save <= 0:
-        raise ValueError(
-            f"Invalid number of images to save: {stack_parameters.images_to_save}. Must be > 0."
-        )
-
-    if stack_parameters.settling_time < 0:
-        raise ValueError(
-            f"Invalid settling time: {stack_parameters.settling_time}. Must be positive or 0."
-        )

src/openflexure_microscope_server/things/camera/__init__.py

@@ -20,7 +20,7 @@ from typing import Any, Literal, Mapping, Optional, Self, Tuple
 import numpy as np
 import piexif
 from PIL import Image
-from pydantic import BaseModel
+from pydantic import BaseModel, field_validator
 
 import labthings_fastapi as lt
 from labthings_fastapi.types.numpy import NDArray
@@ -54,37 +54,28 @@ class CaptureParams(BaseModel):
     images_dir: str
     save_resolution: tuple[int, int]
 
+    @field_validator("save_resolution")
+    @classmethod
+    def validate_save_resolution(cls, value: Tuple[int, int]) -> Tuple[int, int]:
+        """Validate that save_resolution is a tuple with exactly two positive integers."""
+        if not isinstance(value, tuple):
+            raise TypeError("Save resolution should be a tuple")
+        if len(value) != 2 or any(not isinstance(x, int) or x <= 0 for x in value):
+            raise ValueError(
+                f"Invalid save_resolution: {value}. "
+                "Must be a tuple of two positive integers."
+            )
+        return value
 
-def validate_capture_params(capture_parameters: CaptureParams) -> None:
+    @field_validator("images_dir")
-    """Validate capture parameters for a capture.
+    @classmethod
-
+    def validate_images_dir(cls, value: str) -> str:
-    Ensures that the save resolution is a tuple of two positive integers
+        """Validate that images_dir is a non-empty string."""
-    and that the images directory is a non-empty string.
+        if not isinstance(value, str) or not value:
-
+            raise ValueError(
-    :param capture_parameters: CaptureParams object containing acquisition settings.
+                f"Invalid images_dir: {value}. Must be a non-empty string."
-
+            )
-    :raises ValueError: If `save_resolution` is not a tuple of two positive integers,
+        return value
-        or if `images_dir` is not a non-empty string.
-    """
-    if (
-        not isinstance(capture_parameters.save_resolution, tuple)
-        or len(capture_parameters.save_resolution) != 2
-        or not all(
-            isinstance(x, int) and x > 0 for x in capture_parameters.save_resolution
-        )
-    ):
-        raise ValueError(
-            f"Invalid save_resolution: {capture_parameters.save_resolution}. Must "
-            "be a tuple of two positive integers."
-        )
-
-    if (
-        not isinstance(capture_parameters.images_dir, str)
-        or not capture_parameters.images_dir
-    ):
-        raise ValueError(
-            f"Invalid images_dir: {capture_parameters.images_dir}. Must be a non-empty string."
-        )
 
 
 class NoImageInMemoryError(RuntimeError):

src/openflexure_microscope_server/things/scan_workflows.py

@@ -240,10 +240,14 @@ class RectGridWorkflow(ScanWorkflow[SettingModelType], Generic[SettingModelType]
             correlation_resize=STITCHING_RESOLUTION[0] / self.save_resolution[0],
         )
 
+    def _build_scan_settings(self, base_kwargs: dict) -> SettingModelType:
+        """Construct the _settings_model."""
+        return self._settings_model(**base_kwargs)
+
     def all_settings(
         self, images_dir: str
     ) -> tuple[SettingModelType, Optional[StitchingSettings]]:
-        """Return the scan settings and the stitching settings.
+        """Return scan settings and the stitching settings.
 
         :param images_dir: The directory that images are to be written to.
         :return: A tuple containing the settings model for this workflow and the
@@ -252,19 +256,17 @@ class RectGridWorkflow(ScanWorkflow[SettingModelType], Generic[SettingModelType]
         stitching_settings = self._get_stitching_settings_model()
         dx, dy = self._calc_displacement_from_overlap(self.overlap)
 
-        capture_params = CaptureParams(
+        base_kwargs = {
-            images_dir=images_dir, save_resolution=self.save_resolution
+            "overlap": self.overlap,
-        )
+            "dx": dx,
+            "dy": dy,
+            "capture_params": CaptureParams(
+                images_dir=images_dir, save_resolution=self.save_resolution
+            ),
+            "autofocus_params": AutofocusParams(dz=self.autofocus_dz),
+        }
 
-        autofocus_params = AutofocusParams(dz=self.autofocus_dz)
+        scan_settings = self._build_scan_settings(base_kwargs)
-
-        scan_settings = self._settings_model(
-            overlap=self.overlap,
-            dx=dx,
-            dy=dy,
-            capture_params=capture_params,
-            autofocus_params=autofocus_params,
-        )
 
         return scan_settings, stitching_settings
 
@@ -389,38 +391,14 @@ class HistoScanWorkflow(RectGridWorkflow[HistoScanSettingsModel]):
             return True
         return self._background_detector.ready
 
-    def all_settings(
+    def _build_scan_settings(self, base_kwargs: dict) -> HistoScanSettingsModel:
-        self, images_dir: str
+        return HistoScanSettingsModel(
-    ) -> tuple[HistoScanSettingsModel, StitchingSettings]:
+            **base_kwargs,
-        """Return the workflow settings and stitching settings.
-
-        :param images_dir: The directory that images are to be written to.
-        :return: A tuple containing the settings model for this workflow and the
-            settings model for stitching.
-        """
-        stitching_settings = self._get_stitching_settings_model()
-        dx, dy = self._calc_displacement_from_overlap(self.overlap)
-
-        capture_params = CaptureParams(
-            images_dir=images_dir,
-            save_resolution=self.save_resolution,
-        )
-        autofocus_params = AutofocusParams(dz=self.autofocus_dz)
-        smart_stack_params = self.create_smart_stack_params()
-
-        scan_settings = HistoScanSettingsModel(
-            overlap=self.overlap,
             max_dist=self.max_range,
-            dx=dx,
-            dy=dy,
             skip_background=self.skip_background,
-            capture_params=capture_params,
+            smart_stack_params=self.create_smart_stack_params(),
-            autofocus_params=autofocus_params,
-            smart_stack_params=smart_stack_params,
         )
 
-        return scan_settings, stitching_settings
-
     def create_smart_stack_params(
         self,
     ) -> SmartStackParams:
@@ -615,38 +593,14 @@ class RegularGridWorkflow(RectGridWorkflow[RegularGridSettingsModel]):
     _planner_cls = RegularGridPlanner
     _grid_style: Literal["snake", "raster"]
 
-    def all_settings(
+    def _build_scan_settings(self, base_kwargs: dict) -> RegularGridSettingsModel:
-        self, images_dir: str
+        return RegularGridSettingsModel(
-    ) -> tuple[RegularGridSettingsModel, Optional[StitchingSettings]]:
+            **base_kwargs,
-        """Return the workflow and stitching settings.
-
-        :param images_dir: The directory that images are to be written to.
-        :return: A tuple containing the settings model for this workflow and the
-            settings model for stitching.
-        """
-        stitching_settings = self._get_stitching_settings_model()
-        dx, dy = self._calc_displacement_from_overlap(self.overlap)
-
-        capture_params = CaptureParams(
-            images_dir=images_dir,
-            save_resolution=self.save_resolution,
-        )
-
-        autofocus_params = AutofocusParams(dz=self.autofocus_dz)
-
-        scan_settings = self._settings_model(
-            overlap=self.overlap,
-            dx=dx,
-            dy=dy,
             x_count=self.x_count,
             y_count=self.y_count,
             style=self._grid_style,
-            capture_params=capture_params,
-            autofocus_params=autofocus_params,
         )
 
-        return scan_settings, stitching_settings
-
     def pre_scan_routine(self, settings: RegularGridSettingsModel) -> None:
         """Perform these steps before starting the scan.
 

tests/unit_tests/test_stack.py

@@ -27,6 +27,7 @@ from openflexure_microscope_server.things.autofocus import (
     _get_capture_index_by_id,
     _get_peak_turning_point,
 )
+from openflexure_microscope_server.things.camera import CaptureParams
 from openflexure_microscope_server.things.scan_workflows import HistoScanWorkflow
 
 RANDOM_GENERATOR = np.random.default_rng()
@@ -108,7 +109,8 @@ def test_stack_params_negative_images_to_save(save_ims, extra_ims):
     # Depending on the values multiple messages are possible
     match = (
         "(Can't test for focus with fewer than 3 images|"
-        "Images to save must be positive and odd)"
+        "Images to save must be positive and odd)|"
+        "Invalid number of images to save"
     )
     with pytest.raises(ValueError, match=match):
         SmartStackParams(
@@ -152,7 +154,8 @@ def test_even_images_to_save(save_ims, extra_ims):
     """
     match = (
         "(Can't test for focus with fewer than 3 images|"
-        "Images to save must be positive and odd)"
+        "Images to save must be positive and odd)|"
+        "Invalid number of images to save"
     )
     with pytest.raises(ValueError, match=match):
         SmartStackParams(
@@ -942,80 +945,61 @@ def test_run_basic_stack_end_origin(
     assert final_z == expected_final_z, "Final Z position for END origin incorrect"
 
 
-def test_invalid_stack_images_raises(autofocus_thing, mocker):
+def test_invalid_stack_images_raises():
     """Test basic stack raises expected error for negative or zero image count."""
     for capture_count in [-3, 0]:
-        stack_params = StackParams(
-            stack_dz=10,
-            images_to_save=capture_count,
-            settling_time=0,
-            backlash_correction=0,
-            origin=StackOrigin.START,
-        )
-        capture_params = mocker.Mock()
-
         with pytest.raises(ValueError, match="Invalid number of images to save"):
-            autofocus_thing.run_basic_stack(stack_params, capture_params)
+            StackParams(
+                stack_dz=10,
+                images_to_save=capture_count,
+                settling_time=0,
+                backlash_correction=0,
+                origin=StackOrigin.START,
+            )
 
 
-def test_invalid_stack_settling_raises(autofocus_thing, mocker):
+def test_invalid_stack_settling_raises():
     """Test basic stack raises expected error for negative settling time."""
-    stack_params = StackParams(
-        stack_dz=10,
-        images_to_save=1,
-        settling_time=-10,
-        backlash_correction=0,
-        origin=StackOrigin.START,
-    )
-    capture_params = mocker.Mock()
-
     with pytest.raises(ValueError, match="Invalid settling time"):
-        autofocus_thing.run_basic_stack(stack_params, capture_params)
+        StackParams(
+            stack_dz=10,
+            images_to_save=1,
+            settling_time=-1,
+            backlash_correction=0,
+            origin=StackOrigin.START,
+        )
 
 
-def test_invalid_capture_dir_raises(autofocus_thing, mocker):
+@pytest.mark.parametrize(
+    ("bad_path", "match_err"),
+    [
+        ("", "Must be a non-empty string"),
+        (None, "Input should be a valid string"),
+        (67, "Input should be a valid string"),
+    ],
+)
+def test_invalid_capture_dir_raises(bad_path, match_err):
     """Test basic stack raises expected error for bad image dir paths."""
-    for bad_path in ["", None, 67, ["path"]]:
+    with pytest.raises(ValueError, match=match_err):
-        stack_params = StackParams(
+        CaptureParams(images_dir=bad_path, save_resolution=(20, 20))
-            stack_dz=10,
-            images_to_save=1,
-            settling_time=0,
-            backlash_correction=0,
-            origin=StackOrigin.START,
-        )
-        capture_params = mocker.Mock()
-        capture_params.images_dir = bad_path
-        capture_params.save_resolution = (100, 100)
-        mocker.patch.object(autofocus_thing._cam, "save_from_memory")
-        mocker.patch.object(autofocus_thing._cam, "clear_buffers")
-
-        with pytest.raises(ValueError, match="Invalid images_dir"):
-            autofocus_thing.run_basic_stack(stack_params, capture_params)
 
 
-def test_invalid_capture_res_raises(autofocus_thing, mocker):
+@pytest.mark.parametrize(
+    ("bad_res", "match_err"),
+    [
+        ((-100, 50), "Invalid save_resolution:"),
+        ((20, 0), "Invalid save_resolution:"),
+        ("", "Input should be a valid tuple"),
+        (None, "Input should be a valid tuple"),
+        (67, "Input should be a valid tuple"),
+        (
+            ["path"],
+            "Input should be a valid integer, unable to parse string as an integer",
+        ),
+        ((20, 20, 20), "Tuple should have at most 2 items"),
+    ],
+)
+def test_invalid_capture_res_raises(bad_res, match_err):
     """Test basic stack raises expected error for invalid save resolutions."""
-    for bad_res in [
+    with pytest.raises(ValueError, match=match_err):
-        "",
+        CaptureParams(images_dir="dummy", save_resolution=bad_res)
-        None,
-        67,
-        ["path"],
-        (-100, 50),
-        (20, 0),
-        (20, 20, 20),
-        [30, 30],
-    ]:
-        stack_params = StackParams(
-            stack_dz=10,
-            images_to_save=1,
-            settling_time=0,
-            backlash_correction=0,
-            origin=StackOrigin.START,
-        )
-        capture_params = mocker.Mock()
-        capture_params.images_dir = "dummy"
-        capture_params.save_resolution = bad_res
-        autofocus_thing.capture_stack_image = mocker.Mock()
-
-        with pytest.raises(ValueError, match="Invalid save_resolution:"):
-            autofocus_thing.run_basic_stack(stack_params, capture_params)