From dacf219ea119dd53592993d42ae0a48f1dbdb837 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Fri, 1 Aug 2025 17:48:04 +0100 Subject: [PATCH] Change from axis direction set by ints to set by bools. Add a lot more tests. --- .../things/stage/__init__.py | 20 +- .../things/stage/dummy.py | 8 +- .../things/stage/sangaboard.py | 9 +- tests/test_stage.py | 189 ++++++++++++++++-- 4 files changed, 194 insertions(+), 32 deletions(-) diff --git a/src/openflexure_microscope_server/things/stage/__init__.py b/src/openflexure_microscope_server/things/stage/__init__.py index e03532a7..d8556951 100644 --- a/src/openflexure_microscope_server/things/stage/__init__.py +++ b/src/openflexure_microscope_server/things/stage/__init__.py @@ -85,9 +85,9 @@ class BaseStage(lt.Thing): ) """Whether the stage is in motion.""" - axis_direction = lt.ThingSetting( - initial_value={"x": 1, "y": 1, "z": 1}, - model=Mapping[str, int], + axis_inverted = lt.ThingSetting( + initial_value={"x": False, "y": False, "z": False}, + model=Mapping[str, bool], readonly=True, ) """Used to convert coordinates between the program frame and the hardware frame.""" @@ -97,14 +97,14 @@ class BaseStage(lt.Thing): ) -> list[int] | Mapping[str | int]: if isinstance(position, (list, tuple)): return [ - pos * ax_dir - for pos, ax_dir in zip(position, self.axis_direction.values()) + -pos if inverted else pos + for pos, inverted in zip(position, self.axis_inverted.values()) ] if isinstance(position, Mapping): try: return { - axis: position[axis] * self.axis_direction[axis] - for axis in position + ax: -position[ax] if self.axis_inverted[ax] else position[ax] + for ax in position } except KeyError as e: raise KeyError( @@ -126,12 +126,12 @@ class BaseStage(lt.Thing): :param axis: The axis name (x, y or z) to invert. """ # Not mutating in place so that setting is saved on change. - direction = self.axis_direction + direction = self.axis_inverted try: - direction[axis] *= -1 + direction[axis] = not direction[axis] except KeyError as e: raise KeyError(f"The axis {axis} is not defined.") from e - self.axis_direction = direction + self.axis_inverted = direction @lt.thing_action def move_relative( diff --git a/src/openflexure_microscope_server/things/stage/dummy.py b/src/openflexure_microscope_server/things/stage/dummy.py index 92d75e84..8c01c186 100644 --- a/src/openflexure_microscope_server/things/stage/dummy.py +++ b/src/openflexure_microscope_server/things/stage/dummy.py @@ -36,9 +36,9 @@ class DummyStage(BaseStage): def __exit__(self, _exc_type, _exc_value, _traceback): """Nothing to do when the Thing context manager is closed.""" - axis_direction = lt.ThingSetting( - initial_value={"x": -1, "y": 1, "z": 1}, - model=Mapping[str, int], + axis_inverted = lt.ThingSetting( + initial_value={"x": True, "y": False, "z": False}, + model=Mapping[str, bool], readonly=True, ) """Used to convert coordinates between the program frame and the hardware frame.""" @@ -51,7 +51,6 @@ class DummyStage(BaseStage): ): """Make a relative move. Keyword arguments should be axis names.""" displacement = [kwargs.get(k, 0) for k in self.axis_names] - print(f"d {displacement}") self.moving = True try: fraction_complete = 0.0 @@ -80,7 +79,6 @@ class DummyStage(BaseStage): ax: self._hardware_position[ax] + int(fraction_complete * disp) for ax, disp in zip(self.axis_names, displacement) } - print(self._hardware_position) self.instantaneous_position = self._hardware_position def _hardware_move_absolute( diff --git a/src/openflexure_microscope_server/things/stage/sangaboard.py b/src/openflexure_microscope_server/things/stage/sangaboard.py index 3aecc6c9..4f3d661d 100644 --- a/src/openflexure_microscope_server/things/stage/sangaboard.py +++ b/src/openflexure_microscope_server/things/stage/sangaboard.py @@ -4,6 +4,7 @@ from __future__ import annotations import logging import threading import time +from copy import copy from typing import Iterator, Literal from contextlib import contextmanager from collections.abc import Mapping @@ -36,7 +37,7 @@ class SangaboardThing(BaseStage): Sangaboard class """ - self.sangaboard_kwargs = kwargs + self.sangaboard_kwargs = copy(kwargs) self.sangaboard_kwargs["port"] = port super().__init__(**kwargs) @@ -66,9 +67,9 @@ class SangaboardThing(BaseStage): with self._sangaboard_lock: yield self._sangaboard - axis_direction = lt.ThingSetting( - initial_value={"x": -1, "y": 1, "z": -1}, - model=Mapping[str, int], + axis_inverted = lt.ThingSetting( + initial_value={"x": True, "y": False, "z": True}, + model=Mapping[str, bool], readonly=True, ) """Used to convert coordinates between the program frame and the hardware frame.""" diff --git a/tests/test_stage.py b/tests/test_stage.py index 6e457324..d47f4192 100644 --- a/tests/test_stage.py +++ b/tests/test_stage.py @@ -2,10 +2,12 @@ from collections.abc import Mapping import tempfile +import itertools from fastapi.testclient import TestClient import pytest from httpx import HTTPStatusError +from hypothesis import given, settings, HealthCheck, strategies as st import labthings_fastapi as lt from labthings_fastapi.exceptions import NotConnectedToServerError @@ -16,6 +18,15 @@ from openflexure_microscope_server.things.stage import ( ) from openflexure_microscope_server.things.stage.dummy import DummyStage +# Keep the size and number of moves fairly small or the tests can take forever +point3d = st.tuples( + st.integers(min_value=-100, max_value=100), + st.integers(min_value=-100, max_value=100), + st.integers(min_value=-100, max_value=100), +) + +path3d = st.lists(point3d, min_size=5, max_size=10) + @pytest.fixture def dummy_stage(): @@ -79,7 +90,7 @@ def test_override_base_movement(): def _set_axis_direction(dummy_stage, direction): """Set the axis direction directly even if not connected to server.""" try: - dummy_stage.axis_direction = direction + dummy_stage.axis_inverted = direction except NotConnectedToServerError: pass @@ -87,7 +98,7 @@ def _set_axis_direction(dummy_stage, direction): def test_apply_axis_direction_all_pos(dummy_stage): """Test the apply axis direction function behaves as expected when axis +v3.""" # Directly create a stage not through a ThingServer to access private methods - _set_axis_direction(dummy_stage, {"x": 1, "y": 1, "z": 1}) + _set_axis_direction(dummy_stage, {"x": False, "y": False, "z": False}) # A list of positions to try positions = [ @@ -107,7 +118,7 @@ def test_apply_axis_direction_all_pos(dummy_stage): def test_apply_axis_direction_mixed(dummy_stage): """Test the apply axis direction function behaves as expected when axis dirs are mixed.""" # Make x and z negative - _set_axis_direction(dummy_stage, {"x": -1, "y": 1, "z": -1}) + _set_axis_direction(dummy_stage, {"x": True, "y": False, "z": True}) # A list of (input position, output position) to try position_pairs = [ @@ -142,29 +153,181 @@ def test_default_values(stage_client, dummy_stage): # position starts at 0, 0, 0 assert stage_client.position == {"x": 0, "y": 0, "z": 0} # axis direction starts is -1, 1, 1 for the dummy stage - assert stage_client.axis_direction == {"x": -1, "y": 1, "z": 1} + assert stage_client.axis_inverted == {"x": True, "y": False, "z": False} # And check the thing state assert dummy_stage.thing_state == {"position": {"x": 0, "y": 0, "z": 0}} -def test_direction_inversion(stage_client): +def test_direction_inversion(stage_client, dummy_stage): """Check axes invert as expected when called.""" # Can't set an arbitrary value via a client as read only: with pytest.raises(HTTPStatusError) as excinfo: - stage_client.axis_direction = {"x": 2, "y": 1, "z": 1} + stage_client.axis_inverted = {"x": 2, "y": 1, "z": 1} # Read only should set a 405 error code assert excinfo.value.response.status_code == 405 # And not modify th initial value - assert stage_client.axis_direction == {"x": -1, "y": 1, "z": 1} + assert stage_client.axis_inverted == {"x": True, "y": False, "z": False} stage_client.invert_axis_direction(axis="x") - assert stage_client.axis_direction == {"x": 1, "y": 1, "z": 1} + assert stage_client.axis_inverted == {"x": False, "y": False, "z": False} stage_client.invert_axis_direction(axis="x") - assert stage_client.axis_direction == {"x": -1, "y": 1, "z": 1} + assert stage_client.axis_inverted == {"x": True, "y": False, "z": False} stage_client.invert_axis_direction(axis="y") - assert stage_client.axis_direction == {"x": -1, "y": -1, "z": 1} + assert stage_client.axis_inverted == {"x": True, "y": True, "z": False} stage_client.invert_axis_direction(axis="y") - assert stage_client.axis_direction == {"x": -1, "y": 1, "z": 1} + assert stage_client.axis_inverted == {"x": True, "y": False, "z": False} stage_client.invert_axis_direction(axis="z") - assert stage_client.axis_direction == {"x": -1, "y": 1, "z": -1} + assert stage_client.axis_inverted == {"x": True, "y": False, "z": True} stage_client.invert_axis_direction(axis="z") - assert stage_client.axis_direction == {"x": -1, "y": 1, "z": 1} + assert stage_client.axis_inverted == {"x": True, "y": False, "z": False} + + # Should error if axis doesn't exist, this is a KeyError in the server + with pytest.raises(KeyError): + dummy_stage.invert_axis_direction(axis="theta") + # But a 422 over HTTP + with pytest.raises(HTTPStatusError) as excinfo: + stage_client.invert_axis_direction(axis="theta") + assert excinfo.value.response.status_code == 422 + + +def _test_move_relative(stage_client, dummy_stage, axis_inverted, path): + """Test moving relative, ensuring position and hardware position behave as expected. + + :param axis_inverted: Is used to set the inversion. + :param path: The 3d path to move over, generated by hypothesis. + """ + _set_axis_direction(dummy_stage, axis_inverted) + # Explicitly do axes calculation here to check logic in main code. + x_dir = -1 if axis_inverted["x"] else 1 + y_dir = -1 if axis_inverted["y"] else 1 + z_dir = -1 if axis_inverted["z"] else 1 + + position = list(stage_client.position.values()) + for movement in path: + stage_client.move_relative(x=movement[0], y=movement[1], z=movement[2]) + position = [pos + move for pos, move in zip(position, movement)] + stage_pos = stage_client.get_xyz_position() + hw_pos = dummy_stage._hardware_position + assert position[0] == stage_pos[0] == hw_pos["x"] * x_dir + assert position[1] == stage_pos[1] == hw_pos["y"] * y_dir + assert position[2] == stage_pos[2] == hw_pos["z"] * z_dir + + +@given(path=path3d) +@settings( + max_examples=3, + suppress_health_check=[HealthCheck.function_scoped_fixture], + deadline=10000, +) +def test_move_relative(stage_client, dummy_stage, path): + """Loop over different inversion options and check that the stage moves as expected. + + 3 paths are tried for each case of axis inversion. This checks both that the + reported position changes as is input in path, and that the hardware position is + inverted when appropriate. + """ + # Note that the fixture is not reset. This is fine, because the stage_should work + # no matter the starting position. + + axis_names = stage_client.axis_names + # Create every combination of True/False for x,y,z. + inversion_combinations = [ + dict(zip(axis_names, inverted)) + for inverted in itertools.product([True, False], repeat=len(axis_names)) + ] + print(path) + for axis_inverted in inversion_combinations: + _test_move_relative( + stage_client=stage_client, + dummy_stage=dummy_stage, + axis_inverted=axis_inverted, + path=path, + ) + + +def _test_move_absolute(stage_client, dummy_stage, axis_inverted, path): + """Test moving relative, ensuring position and hardware position behave as expected. + + :param axis_inverted: Is used to set the inversion. + :param path: The 3d path to move over, generated by hypothesis. + """ + _set_axis_direction(dummy_stage, axis_inverted) + # Explicitly do axes calculation here to check logic in main code. + x_dir = -1 if axis_inverted["x"] else 1 + y_dir = -1 if axis_inverted["y"] else 1 + z_dir = -1 if axis_inverted["z"] else 1 + + position = list(stage_client.position.values()) + for move_to in path: + stage_client.move_absolute(x=move_to[0], y=move_to[1], z=move_to[2]) + position = move_to + stage_pos = stage_client.get_xyz_position() + hw_pos = dummy_stage._hardware_position + assert position[0] == stage_pos[0] == hw_pos["x"] * x_dir + assert position[1] == stage_pos[1] == hw_pos["y"] * y_dir + assert position[2] == stage_pos[2] == hw_pos["z"] * z_dir + + +@given(path=path3d) +@settings( + max_examples=3, + suppress_health_check=[HealthCheck.function_scoped_fixture], + deadline=10000, +) +def test_move_absolute(stage_client, dummy_stage, path): + """Loop over different inversion options and check that the stage moves as expected. + + 3 paths are tried for each case of axis inversion. This checks both that the + reported position changes as is input in path, and that the hardware position is + inverted when appropriate. + """ + # Note that the fixture is not reset. This is fine, because the stage_should work + # no matter the starting position. + + axis_names = stage_client.axis_names + # Create every combination of True/False for x,y,z. + inversion_combinations = [ + dict(zip(axis_names, inverted)) + for inverted in itertools.product([True, False], repeat=len(axis_names)) + ] + print(path) + for axis_inverted in inversion_combinations: + _test_move_absolute( + stage_client=stage_client, + dummy_stage=dummy_stage, + axis_inverted=axis_inverted, + path=path, + ) + + +def test_thing_description_equivalence(dummy_stage, mocker): + """Stop extra actions getting added to child classes without explicit approval. + + To add an extra action to a stage this test needs to be updated, highlighting it at + review. This tests should explain why the action isn't on the general base class. + """ + mock_sangaboard = mocker.Mock() + mocker.patch.dict("sys.modules", {"sangaboard": mock_sangaboard}) + from openflexure_microscope_server.things.stage.sangaboard import SangaboardThing + + # Flash LED isn't a standard stage action, most stages do not control illumination. + extra_sanga_actions = ["flash_led"] + + base_td = BaseStage().thing_description() + base_actions = set(base_td.actions.keys()) + base_properties = set(base_td.properties.keys()) + + dummy_td = dummy_stage.thing_description() + dummy_actions = set(dummy_td.actions.keys()) + dummy_properties = set(dummy_td.properties.keys()) + + sanga_td = SangaboardThing().thing_description() + # Remove known extra actions + sanga_actions = list(sanga_td.actions.keys()) + for action in extra_sanga_actions: + index = sanga_actions.index(action) + sanga_actions.pop(index) + sanga_actions = set(sanga_actions) + sanga_properties = set(sanga_td.properties.keys()) + + assert sanga_actions == dummy_actions == base_actions + assert sanga_properties == dummy_properties == base_properties