Merge branch 'stack-height-sanitise' into 'v3'

Verify stack params at start of scan main loop

Closes #455

See merge request openflexure/openflexure-microscope-server!400

src/openflexure_microscope_server/things/autofocus.py

@@ -16,7 +16,7 @@ from dataclasses import dataclass
 
 from fastapi import Depends
 import numpy as np
-from pydantic import BaseModel
+from pydantic import BaseModel, field_validator, computed_field, model_validator
 
 import labthings_fastapi as lt
 from labthings_fastapi.types.numpy import NDArray
@@ -26,54 +26,23 @@ from .camera import CameraDependency as CameraClient
 from .stage import StageDependency as Stage
 
 LOGGER = logging.getLogger(__name__)
+MIN_TEST_IMAGE_COUNT = 3
+MAX_TEST_IMAGE_COUNT = 9
 
 
 class NotStreamingError(RuntimeError):
     """No images captured from stream. The camera is almost certainly not streaming."""
 
 
-class StackParams:
+class StackParams(BaseModel):
     """A class for holding for stack parameters, and returning computed ones."""
 
-    def __init__(
+    stack_dz: int
-        self,
+    images_to_save: int
-        *,
+    min_images_to_test: int
-        stack_dz: int,
+    autofocus_dz: int
-        images_to_save: int,
+    images_dir: str
-        min_images_to_test: int,
+    save_resolution: tuple[int, int]
-        autofocus_dz: int,
-        images_dir: str,
-        save_resolution: tuple[int, int],
-    ) -> None:
-        """Initialise the parameters. All arguments are keyword only.
-
-        :param stack_dz: The number of motor steps between images
-        :param images_to_save: The number of images to save to disk
-        :param min_images_to_test: The minimum number of images in the stack before,
-            the stack is evaluated for focus. As more images are captured evaluation
-            of the focus is always evaluated with the same number of images. i.e. if
-            ``min_images_to_test=9``, then 9 images are captured, if the stack is not
-            well focused, a 10th image is captured and images 2 to 10 are evaluated
-            for focus
-        :param autofocus_dz: The number of steps in a full autofocus (when required)
-        :param images_dir: The directory to save images to disk
-        :param save_resolution: The resolution to save the captures to disk with
-        """
-        if min_images_to_test < images_to_save:
-            raise ValueError("Can't save more images than the minimum number tested")
-        if min_images_to_test % 2 == 0 or min_images_to_test <= 0:
-            raise ValueError(
-                "Minimum number of images to test should be positive and odd"
-            )
-        if images_to_save % 2 == 0 or images_to_save <= 0:
-            raise ValueError("Images to save must be positive and odd")
-
-        self.stack_dz = stack_dz
-        self.images_to_save = images_to_save
-        self.min_images_to_test = min_images_to_test
-        self.autofocus_dz = autofocus_dz
-        self.images_dir = images_dir
-        self.save_resolution = save_resolution
 
     # Using docstrings under variables as this is how pdoc would expect
     # attributed to be documented
@@ -102,6 +71,41 @@ class StackParams:
     max_attempts: int = 3
     """Maximum number of times to attempt fast stack"""
 
+    @field_validator("min_images_to_test")
+    @classmethod
+    def check_images_to_test(cls, min_images_to_test: int) -> int:
+        """Verify that the images to test parameter matches various constraints."""
+        if min_images_to_test < MIN_TEST_IMAGE_COUNT:
+            raise ValueError(
+                f"Can't test for focus with fewer than {MIN_TEST_IMAGE_COUNT} images."
+            )
+        if min_images_to_test > MAX_TEST_IMAGE_COUNT:
+            raise ValueError(
+                f"Testing with more than {MAX_TEST_IMAGE_COUNT} images is likely to focus on the cover slip, or strike the sample."
+            )
+        if min_images_to_test % 2 == 0 or min_images_to_test <= 0:
+            raise ValueError(
+                "Minimum number of images to test should be positive and odd"
+            )
+        return min_images_to_test
+
+    @field_validator("images_to_save")
+    @classmethod
+    def check_images_to_save(cls, images_to_save: int) -> int:
+        """Verify that the images to save parameter is positive and odd."""
+        if images_to_save % 2 == 0 or images_to_save <= 0:
+            raise ValueError("Images to save must be positive and odd")
+        return images_to_save
+
+    @model_validator(mode="after")
+    def check_image_limits(self) -> "StackParams":
+        """Ensure the number of images to save isn't more than the minimum tested."""
+        if self.images_to_save > self.min_images_to_test:
+            raise ValueError("Can't save more images than the minimum number tested.")
+        return self
+
+    # Note MyPy doesn't support decorating properties. See MyPy Pull #16571 and issue #14461.
+    @computed_field  # type: ignore[prop-decorator]
     @property
     def stack_z_range(self) -> int:
         """The range of the z stack, in steps.
@@ -112,6 +116,7 @@ class StackParams:
         """
         return self.stack_dz * (self.min_images_to_test - 1)
 
+    @computed_field  # type: ignore[prop-decorator]
     @property
     def steps_undershoot(self) -> int:
         """The distance to deliberately undershoot the estimated optimal starting point."""
@@ -121,6 +126,7 @@ class StackParams:
         # requires extra +z movements and captures.
         return self.stack_dz * self.img_undershoot
 
+    @computed_field  # type: ignore[prop-decorator]
     @property
     def max_images_to_test(self) -> int:
         """The maximum number of images that will be captured and tested in a stack.
@@ -467,7 +473,7 @@ class AutofocusThing(lt.Thing):
     enough more images may be captured. After new images are captured the number sets
     the number of images used for checking if focus is central.
 
-    Defaults to 9 which balances reliability and speed/
+    Defaults to 9 which balances reliability and speed.
     """
 
     stack_dz = lt.ThingSetting(initial_value=50, model=int)
@@ -480,15 +486,86 @@ class AutofocusThing(lt.Thing):
     * 200 for 20x
     """
 
+    @lt.thing_action
+    def create_stack_params(
+        self,
+        images_dir: str,
+        autofocus_dz: int,
+        save_resolution: tuple[int, int],
+        logger: lt.deps.InvocationLogger,
+    ) -> StackParams:
+        """Set up the parameters used for all stacks in a scan.
+
+        :param images_dir: the folder to save all images
+        :param autofocus_dz: the range to autofocus over if a stack fails
+        :param save_resolution: The resolution to save the captures to disk with
+        :param logger: A logger passed from the calling Thing
+
+        :returns: A StackParams object with the required parameters.
+        """
+        # Coerce min_images_to_test parameter
+        min_images_to_test = self.stack_min_images_to_test
+        if min_images_to_test < MIN_TEST_IMAGE_COUNT:
+            logger.warning(
+                f"Cannot test only {min_images_to_test} image(s) as this will fail. "
+                "Setting min images to test to lowest possible value of"
+                f"{MIN_TEST_IMAGE_COUNT}."
+            )
+            min_images_to_test = MIN_TEST_IMAGE_COUNT
+        elif min_images_to_test > MAX_TEST_IMAGE_COUNT:
+            logger.warning(
+                f"Testing {min_images_to_test} images will cause defocus. "
+                "Setting min images to test to highest possible value of "
+                f"{MAX_TEST_IMAGE_COUNT}."
+            )
+            min_images_to_test = MAX_TEST_IMAGE_COUNT
+        elif min_images_to_test % 2 == 0:
+            min_images_to_test += 1
+            logger.warning(
+                "Minimum number of images to test should be odd, setting to "
+                f"{min_images_to_test}."
+            )
+        # Set the Thing property to the coerced value
+        self.stack_min_images_to_test = min_images_to_test
+
+        # Coerce the images to save parameter to be positive, odd, and less than
+        # min_images_to_save
+        images_to_save = self.stack_images_to_save
+        if images_to_save <= 0:
+            logger.warning(
+                "At least 1 images must be saved. Setting images to save to 1."
+            )
+            images_to_save = 1
+        elif images_to_save > min_images_to_test:
+            logger.warning(
+                f"Cannot save {images_to_save} images as this above the minimum "
+                f"number to test. Setting images to save to {MAX_TEST_IMAGE_COUNT}."
+            )
+            images_to_save = min_images_to_test
+        elif images_to_save % 2 == 0:
+            images_to_save += 1
+            logger.warning(
+                f"Images to save should be odd, setting to {images_to_save}."
+            )
+        # Set the Thing property to the coerced value
+        self.stack_images_to_save = images_to_save
+
+        return StackParams(
+            stack_dz=self.stack_dz,
+            images_to_save=self.stack_images_to_save,
+            min_images_to_test=self.stack_min_images_to_test,
+            autofocus_dz=autofocus_dz,
+            images_dir=images_dir,
+            save_resolution=save_resolution,
+        )
+
     @lt.thing_action
     def run_smart_stack(
         self,
         cam: CameraClient,
         stage: Stage,
         sharpness_monitor: SharpnessMonitorDep,
-        images_dir: str,
+        stack_parameters: StackParams,
-        autofocus_dz: int,
-        save_resolution: tuple[int, int],
     ) -> tuple[bool, int]:
         """Run a smart stack.
 
@@ -503,27 +580,14 @@ class AutofocusThing(lt.Thing):
         :param stage: Stage Dependency supplied by LabThings dependency injection
         :param sharpness_monitor: Sharpness Monitor Dependency (for focus detection)
             supplied by LabThings dependency injection
-        :param images_dir: the folder to save all images
+        :param stack_parameters: A StackParams object containing the required
-        :param autofocus_dz: the range to autofocus over if a stack fails
+            parameters to run a stack.
-        :param save_resolution: The resolution the images should be saved at, the
-            images will be resampled if this doesn't match the camera's capture
-            resolution
 
         :returns: A tuple containing:
 
             * A boolean, True if stack was successfully
             * The z position of the sharpest image
         """
-        # Set the variables to prevent changes from the GUI or other windows
-        stack_parameters = StackParams(
-            stack_dz=self.stack_dz,
-            images_to_save=self.stack_images_to_save,
-            min_images_to_test=self.stack_min_images_to_test,
-            autofocus_dz=autofocus_dz,
-            images_dir=images_dir,
-            save_resolution=save_resolution,
-        )
-
         tries = 0
         # Loop until a stack is successful
         while tries < stack_parameters.max_attempts:

src/openflexure_microscope_server/things/smart_scan.py

@@ -26,7 +26,7 @@ from openflexure_microscope_server import scan_planners
 from openflexure_microscope_server import stitching
 
 # Things
-from .autofocus import AutofocusThing
+from .autofocus import AutofocusThing, StackParams
 from .camera_stage_mapping import CameraStageMapper
 from .camera import CameraDependency as CameraClient
 from .stage import StageDependency as StageDep
@@ -121,7 +121,7 @@ class SmartScanThing(lt.Thing):
         self._stage: Optional[StageDep] = None
         self._cam: Optional[CameraClient] = None
         self._csm: Optional[CSMDep] = None
-
+        self._stack_params: Optional[StackParams] = None
         self._ongoing_scan: Optional[scan_directories.ScanDirectory] = None
         self._scan_data: Optional[scan_directories.ScanData] = None
         self._preview_stitcher: Optional[stitching.PreviewStitcher] = None
@@ -189,6 +189,7 @@ class SmartScanThing(lt.Thing):
             self._scan_lock.release()
             # Ensure any PreviewStitcher created cannot be reused.
             self._preview_stitcher = None
+            self._stack_params = None
 
         # Remove any scan folders containing zero images.
         self.purge_empty_scans(logger=logger)
@@ -363,6 +364,12 @@ class SmartScanThing(lt.Thing):
             self._cam.start_streaming(main_resolution=(3280, 2464))
             self._scan_data = self._collect_scan_data()
             self._ongoing_scan.save_scan_data(self._scan_data)
+            self._stack_params = self._autofocus.create_stack_params(
+                images_dir=self._ongoing_scan.images_dir,
+                autofocus_dz=self.autofocus_dz,
+                save_resolution=self._scan_data.save_resolution,
+                logger=self._scan_logger,
+            )
             self._preview_stitcher = stitching.PreviewStitcher(
                 self._ongoing_scan.images_dir,
                 overlap=self._scan_data.overlap,
@@ -451,9 +458,7 @@ class SmartScanThing(lt.Thing):
                 continue
 
             focused, focused_height = self._autofocus.run_smart_stack(
-                images_dir=self._ongoing_scan.images_dir,
+                stack_parameters=self._stack_params
-                autofocus_dz=self._scan_data.autofocus_dz,
-                save_resolution=self._scan_data.save_resolution,
             )
 
             current_pos_xyz = (new_pos_xyz[0], new_pos_xyz[1], focused_height)

tests/test_smart_scan.py

@@ -292,8 +292,11 @@ def scan_thing_mocked_for_scan_data(smart_scan_thing, mocker):
     mock_stage = mocker.Mock()
     type(mock_stage).position = mocker.PropertyMock(return_value=MOCK_START_POS)
 
+    mock_autofocus = mocker.Mock()
+
     smart_scan_thing._ongoing_scan = mock_ongoing_scan
     smart_scan_thing._stage = mock_stage
+    smart_scan_thing._autofocus = mock_autofocus
     return smart_scan_thing
 
 

tests/test_stack.py

@@ -4,20 +4,29 @@ Currently these tests don't test the Thing itself, just surrounding functionalit
 """
 
 from typing import Optional
+import tempfile
 from random import randint
+import logging
 
 import pytest
 import numpy as np
 from hypothesis import given, strategies as st
+from fastapi.testclient import TestClient
+
+import labthings_fastapi as lt
 
 from openflexure_microscope_server.things.autofocus import (
+    AutofocusThing,
     StackParams,
     CaptureInfo,
+    MIN_TEST_IMAGE_COUNT,
+    MAX_TEST_IMAGE_COUNT,
     _get_capture_by_id,
     _get_capture_index_by_id,
 )
 from openflexure_microscope_server.scan_directories import IMAGE_REGEX
 
+LOGGER = logging.getLogger("mock-invocation_logger")
 RANDOM_GENERATOR = np.random.default_rng()
 
 
@@ -42,21 +51,24 @@ def even_integers(min_value=0, max_value=1000):
 
 
 @given(
-    save_ims=odd_integers(min_value=0, max_value=10),
+    save_ims=odd_integers(min_value=1, max_value=9),
     extra_ims=even_integers(min_value=0, max_value=10),
 )
 def test_stack_params_validation(save_ims, extra_ims):
-    """Test the validation of the image numbers for a stack.
+    """Test the validation of the image numbers for a stack for valid combinations.
 
     save_ims is the number to save (must be odd and positive)
     extra_ims is how many more images there are in min_images_to_test than
     images_to_save. (even so that, min_images_to_test is odd and larger than
     images_to_save
     """
+    # Coerce min_images_to_test as the max extra ims depends on save_ims so is hard
+    # to do automatically in hypothesis. This clamps the number between 3 and 9.
+    min_images_to_test = max(min(save_ims + extra_ims, 9), 3)
     StackParams(
         stack_dz=50,
         images_to_save=save_ims,
-        min_images_to_test=save_ims + extra_ims,
+        min_images_to_test=min_images_to_test,
         autofocus_dz=2000,
         images_dir="/this/is/fake",
         save_resolution=(1640, 1232),
@@ -228,3 +240,95 @@ def test_retrieval_of_captures(start):
         _get_capture_by_id(captures, start - 1)
     with pytest.raises(ValueError):
         _get_capture_by_id(captures, start + 21)
+
+
+@pytest.fixture
+def autofocus_thing():
+    """Yield an autofocus thing connected to a server."""
+    autofocus_thing = AutofocusThing()
+    with tempfile.TemporaryDirectory() as tmpdir:
+        server = lt.ThingServer(settings_folder=tmpdir)
+        server.add_thing(autofocus_thing, "/autofocus/")
+        with TestClient(server.app):
+            yield autofocus_thing
+
+
+def test_create_stack(autofocus_thing, caplog):
+    """Run create stack with default values and check there is no coercion or logging."""
+    initial_min_images_to_test = autofocus_thing.stack_min_images_to_test
+    initial_images_to_save = autofocus_thing.stack_images_to_save
+    with caplog.at_level(logging.INFO):
+        stack_params = autofocus_thing.create_stack_params(
+            autofocus_dz=2000,
+            images_dir="/this/is/fake",
+            save_resolution=(1640, 1232),
+            logger=LOGGER,
+        )
+
+    assert len(caplog.records) == 0
+    assert autofocus_thing.stack_min_images_to_test == initial_min_images_to_test
+    assert autofocus_thing.stack_images_to_save == initial_images_to_save
+    assert stack_params.min_images_to_test == autofocus_thing.stack_min_images_to_test
+    assert stack_params.images_to_save == autofocus_thing.stack_images_to_save
+
+
+@pytest.mark.parametrize(
+    ("initial_test_ims", "coerced_test_ims", "expected_log_start"),
+    [
+        (0, MIN_TEST_IMAGE_COUNT, "Cannot test only 0"),
+        (-1, MIN_TEST_IMAGE_COUNT, "Cannot test only -1"),
+        (10, MAX_TEST_IMAGE_COUNT, "Testing 10 images will"),
+        (4, 5, "Minimum number of images to test should be odd"),
+    ],
+)
+def test_coercing_stack_test_ims(
+    initial_test_ims, coerced_test_ims, expected_log_start, autofocus_thing, caplog
+):
+    """Run create stack with images to test set to values requiring coercion, and check result."""
+    autofocus_thing.stack_min_images_to_test = initial_test_ims
+
+    with caplog.at_level(logging.WARNING):
+        stack_params = autofocus_thing.create_stack_params(
+            autofocus_dz=2000,
+            images_dir="/this/is/fake",
+            save_resolution=(1640, 1232),
+            logger=LOGGER,
+        )
+
+    assert len(caplog.records) == 1
+    assert str(caplog.records[0].msg).startswith(expected_log_start)
+    # Check the value is coerced in the stack_params
+    assert stack_params.min_images_to_test == coerced_test_ims
+    # Check that the setting in the Thing was updated to the coerced value
+    assert stack_params.min_images_to_test == autofocus_thing.stack_min_images_to_test
+
+
+@pytest.mark.parametrize(
+    ("initial_save_ims", "coerced_save_ims", "expected_log_start"),
+    [
+        (0, 1, "At least 1 images must be saved"),
+        (-1, 1, "At least 1 images must be saved"),
+        (10, 9, "Cannot save 10 images"),
+        (4, 5, "Images to save should be odd, setting to 5"),
+    ],
+)
+def test_coercing_stack_save_ims(
+    initial_save_ims, coerced_save_ims, expected_log_start, autofocus_thing, caplog
+):
+    """Run create stack with images to save set to values requiring coercion, and check result."""
+    autofocus_thing.stack_images_to_save = initial_save_ims
+
+    with caplog.at_level(logging.WARNING):
+        stack_params = autofocus_thing.create_stack_params(
+            autofocus_dz=2000,
+            images_dir="/this/is/fake",
+            save_resolution=(1640, 1232),
+            logger=LOGGER,
+        )
+
+    assert len(caplog.records) == 1
+    assert str(caplog.records[0].msg).startswith(expected_log_start)
+    # Check the value is coerced in the stack_params
+    assert stack_params.images_to_save == coerced_save_ims
+    # Check that the setting in the Thing was updated to the coerced value
+    assert stack_params.images_to_save == autofocus_thing.stack_images_to_save