From ca46439269d7fbd55578b6219629af5c571ee87d Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 14 Jan 2026 15:09:18 +0000 Subject: [PATCH] Rename "active_detector" for background detecor. Improve thing selector coercion --- .../things/camera/__init__.py | 47 +++++++++------ .../things/camera/opencv.py | 8 ++- .../things/camera/picamera.py | 27 +++++---- .../things/camera/simulation.py | 15 +++-- .../things/smart_scan.py | 5 +- .../utilities.py | 44 ++++++++++++++ .../picamera2/test_calibration.py | 4 +- tests/unit_tests/test_simulated_camera.py | 2 +- tests/unit_tests/test_thing_selector.py | 59 +++++++++++++++++++ .../paneBackgroundDetect.vue | 23 +++++--- 10 files changed, 182 insertions(+), 52 deletions(-) create mode 100644 tests/unit_tests/test_thing_selector.py diff --git a/src/openflexure_microscope_server/things/camera/__init__.py b/src/openflexure_microscope_server/things/camera/__init__.py index 238a7078..d8c37011 100644 --- a/src/openflexure_microscope_server/things/camera/__init__.py +++ b/src/openflexure_microscope_server/things/camera/__init__.py @@ -28,6 +28,7 @@ from openflexure_microscope_server.things.background_detect import ( BackgroundDetectAlgorithm, ) from openflexure_microscope_server.ui import ActionButton, PropertyControl +from openflexure_microscope_server.utilities import coerce_thing_selector class JPEGBlob(lt.blob.Blob): @@ -158,7 +159,7 @@ class BaseCamera(lt.Thing): ``__init__`` method of the subclass. """ - background_detectors: Mapping[str, BackgroundDetectAlgorithm] = lt.thing_slot() + _all_background_detectors: Mapping[str, BackgroundDetectAlgorithm] = lt.thing_slot() mjpeg_stream = lt.outputs.MJPEGStreamDescriptor() lores_mjpeg_stream = lt.outputs.MJPEGStreamDescriptor() @@ -176,12 +177,17 @@ class BaseCamera(lt.Thing): # Default is never updated but is used if the value set from settings is # incorrect. In the future a better way to set defaults for thing slot mappings # would be ideal. - self._default_detector = "bg_channel_deviations_luv" - self._detector_name = "bg_channel_deviations_luv" + self._default_background_detector = "bg_channel_deviations_luv" + self._background_detector_name: Optional[str] = None def __enter__(self) -> Self: """Open hardware connection when the Thing context manager is opened.""" - raise NotImplementedError("CameraThings must define their own __enter__ method") + self._background_detector_name = coerce_thing_selector( + thing_mapping=self._all_background_detectors, + selected=self._background_detector_name, + default=self._default_background_detector, + ) + return self def __exit__( self, @@ -190,7 +196,7 @@ class BaseCamera(lt.Thing): _traceback: Optional[TracebackType], ) -> None: """Close hardware connection when the Thing context manager is closed.""" - raise NotImplementedError("CameraThings must define their own __exit__ method") + pass @lt.property def calibration_required(self) -> bool: @@ -581,30 +587,31 @@ class BaseCamera(lt.Thing): # Note that the default detector name is set at init. This is over written if # setting is loaded from disk. @lt.setting - def detector_name(self) -> str: + def background_detector_name(self) -> Optional[str]: """The name of the active background selector.""" - return self._detector_name + return self._background_detector_name - @detector_name.setter - def _set_detector_name(self, name: str) -> None: - """Validate and set detector_name.""" - if name not in self.background_detectors: + @background_detector_name.setter + def _set_background_detector_name(self, name: str) -> None: + """Validate and set background_detector_name.""" + if name not in self._all_background_detectors: self.logger.warning(f"{name} is not a valid background detector name.") - self._detector_name = name + self._background_detector_name = name @property - def active_detector(self) -> BackgroundDetectAlgorithm: + def background_detector(self) -> Optional[BackgroundDetectAlgorithm]: """The active background detector instance.""" - if self.detector_name not in self.background_detectors: - self.logger.warning(f"No detector named {self.detector_name}") - self.detector_name = self._default_detector - return self.background_detectors[self.detector_name] + if self.background_detector_name is None: + return None + return self._all_background_detectors[self.background_detector_name] @lt.action def image_is_sample(self) -> tuple[bool, str]: """Label the current image as either background or sample.""" + if self.background_detector is None: + raise RuntimeError("No background detectors available.") current_image = self.grab_as_array(stream_name="lores") - return self.active_detector.image_is_sample(current_image) + return self.background_detector.image_is_sample(current_image) @lt.action def set_background(self) -> None: @@ -616,8 +623,10 @@ class BaseCamera(lt.Thing): future images to the distribution, to determine if each pixel is foreground or background. """ + if self.background_detector is None: + raise RuntimeError("No background detectors available.") background = self.grab_as_array(stream_name="lores") - self.active_detector.set_background(background) + self.background_detector.set_background(background) @property def thing_state(self) -> Mapping[str, Any]: diff --git a/src/openflexure_microscope_server/things/camera/opencv.py b/src/openflexure_microscope_server/things/camera/opencv.py index fb45d65f..8295b8f8 100644 --- a/src/openflexure_microscope_server/things/camera/opencv.py +++ b/src/openflexure_microscope_server/things/camera/opencv.py @@ -41,6 +41,7 @@ class OpenCVCamera(BaseCamera): def __enter__(self) -> Self: """Start the capture thread when the Thing context manager is opened.""" + super().__enter__() self.cap = cv2.VideoCapture(self.camera_index) self._capture_enabled = True self._capture_thread = Thread(target=self._capture_frames) @@ -49,9 +50,9 @@ class OpenCVCamera(BaseCamera): def __exit__( self, - _exc_type: type[BaseException], - _exc_value: Optional[BaseException], - _traceback: Optional[TracebackType], + exc_type: type[BaseException], + exc_value: Optional[BaseException], + traceback: Optional[TracebackType], ) -> None: """Release the camera when the Thing context manager is closed. @@ -62,6 +63,7 @@ class OpenCVCamera(BaseCamera): if self._capture_thread is not None: self._capture_thread.join() self.cap.release() + super().__exit__(exc_type, exc_value, traceback) @lt.property def stream_active(self) -> bool: diff --git a/src/openflexure_microscope_server/things/camera/picamera.py b/src/openflexure_microscope_server/things/camera/picamera.py index c7fd1fa5..901a1c60 100644 --- a/src/openflexure_microscope_server/things/camera/picamera.py +++ b/src/openflexure_microscope_server/things/camera/picamera.py @@ -380,6 +380,7 @@ class StreamingPiCamera2(BaseCamera): This opens the picamera connection, initialises the camera, sets the sensor_modes property, and then starts the streams. """ + super().__enter__() self._initialise_picamera(check_sensor_model=True) # Sensor modes is a cached property read it once after initialising the camera _modes = self.sensor_modes @@ -417,15 +418,16 @@ class StreamingPiCamera2(BaseCamera): def __exit__( self, - _exc_type: type[BaseException], - _exc_value: Optional[BaseException], - _traceback: Optional[TracebackType], + exc_type: type[BaseException], + exc_value: Optional[BaseException], + traceback: Optional[TracebackType], ) -> None: """Close the picamera connection when the Thing context manager is closed.""" self.stop_streaming() with self._streaming_picamera() as cam: cam.close() del self._picamera + super().__exit__(exc_type, exc_value, traceback) @lt.action def start_streaming( @@ -759,15 +761,16 @@ class StreamingPiCamera2(BaseCamera): self.set_static_green_equalisation() self.set_ce_enable_to_off() self.calibrate_lens_shading() - for _i in range(3): - try: - time.sleep(self._sensor_info.long_pause) - self.set_background() - # Return if background is set - return - except ChannelBlankError: - # If channel is blank, sleep a second and try again. - pass + if self.background_detector is not None: + for _i in range(3): + try: + time.sleep(self._sensor_info.long_pause) + self.set_background() + # Return if background is set + return + except ChannelBlankError: + # If channel is blank, sleep a second and try again. + pass raise RuntimeError("Couldn't set background") @lt.property diff --git a/src/openflexure_microscope_server/things/camera/simulation.py b/src/openflexure_microscope_server/things/camera/simulation.py index 42058aa4..1f956661 100644 --- a/src/openflexure_microscope_server/things/camera/simulation.py +++ b/src/openflexure_microscope_server/things/camera/simulation.py @@ -181,7 +181,9 @@ class SimulatedCamera(BaseCamera): @lt.property def calibration_required(self) -> bool: """Whether the camera needs calibrating.""" - return not self.active_detector.ready + if self.background_detector is None: + return True + return not self.background_detector.ready def generate_sprites(self) -> None: """Generate sprites to populate the image.""" @@ -344,20 +346,22 @@ class SimulatedCamera(BaseCamera): def __enter__(self) -> Self: """Start the capture thread when the Thing context manager is opened.""" + super().__enter__() self.generate_canvas() self.start_streaming() return self def __exit__( self, - _exc_type: type[BaseException], - _exc_value: Optional[BaseException], - _traceback: Optional[TracebackType], + exc_type: type[BaseException], + exc_value: Optional[BaseException], + traceback: Optional[TracebackType], ) -> None: """Close the capture thread when the Thing context manager is closed.""" if self._capture_thread is not None and self._capture_thread.is_alive(): self._capture_enabled = False self._capture_thread.join() + super().__exit__(exc_type, exc_value, traceback) @lt.action def start_streaming( @@ -463,7 +467,8 @@ class SimulatedCamera(BaseCamera): """ self.remove_sample() time.sleep(0.2) - self.set_background() + if self.background_detector is not None: + self.set_background() time.sleep(0.2) self.load_sample() diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py index 5add6866..ebbe37f2 100644 --- a/src/openflexure_microscope_server/things/smart_scan.py +++ b/src/openflexure_microscope_server/things/smart_scan.py @@ -234,7 +234,10 @@ class SmartScanThing(lt.Thing): self._csm.assert_calibration() if self.skip_background: - if not self._cam.active_detector.ready: + if ( + self._cam.background_detector is None + or not self._cam.background_detector.ready + ): raise RuntimeError( "Background is not set: you need to calibrate background detection." ) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index ca1f740e..8165a73b 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -13,6 +13,8 @@ from typing import ( Callable, Concatenate, Literal, + Mapping, + Optional, ParamSpec, TypeAlias, TypeVar, @@ -21,6 +23,8 @@ from typing import ( from pydantic import BaseModel +import labthings_fastapi as lt + T = TypeVar("T") P = ParamSpec("P") LockableClass = TypeVar("LockableClass") @@ -388,3 +392,43 @@ def resolve_path_from_dir(path: str, directory: str) -> str: if not os.path.isabs(path): path = os.path.join(directory, path) return os.path.normpath(path) + + +def coerce_thing_selector( + thing_mapping: Mapping[str, lt.Thing], + selected: Optional[str], + default: Optional[str] = None, +) -> Optional[str]: + """Return a valid selector for a mapping of things or None if empty. + + :param thing_mapping: A mapping of str -> ``Thing`` + :param selected: The key of the selected thing (or ``None``) + :param default: The default key to fallback to if selected is ``None`` or the key is + missing + :return: ``None`` if the mapping is empty, else a key that is in the mapping. Order + of preference is: selected, default, the first key. + """ + if not thing_mapping: + # Empty mapping + if selected is not None: + LOGGER.warning( + f"Could not select {selected} from Thing mapping as the mapping is empty." + ) + # The return is always None if the mapping is empty + return None + + if selected in thing_mapping: + # Selected exists, return it. + return selected + + if selected is not None: + LOGGER.warning(f"Could not select '{selected}' from Thing mapping") + + if default in thing_mapping: + return default + + if default is not None: + LOGGER.warning(f"Could not select default key '{default}' from Thing mapping") + + # Final option is to return the first key + return list(thing_mapping)[0] diff --git a/tests/hardware_specific_tests/picamera2/test_calibration.py b/tests/hardware_specific_tests/picamera2/test_calibration.py index d9181d90..6da9ee9b 100644 --- a/tests/hardware_specific_tests/picamera2/test_calibration.py +++ b/tests/hardware_specific_tests/picamera2/test_calibration.py @@ -19,7 +19,7 @@ def test_calibration(picamera_test_env): # Tuning should start the same as the server is loading with no settings. assert picamera_thing.default_tuning == picamera_thing.tuning # The background detector isn't ready as there is no background image. - assert not picamera_thing.active_detector.ready + assert not picamera_thing.background_detector.ready # Run full auto calibrate picamera_client.full_auto_calibrate() @@ -31,7 +31,7 @@ def test_calibration(picamera_test_env): # The default should be unchanged assert picamera_thing.default_tuning == original_default - assert picamera_thing.active_detector.ready + assert picamera_thing.background_detector.ready def test_tuning_is_persistent(): diff --git a/tests/unit_tests/test_simulated_camera.py b/tests/unit_tests/test_simulated_camera.py index db3d9d48..c558cba5 100644 --- a/tests/unit_tests/test_simulated_camera.py +++ b/tests/unit_tests/test_simulated_camera.py @@ -186,4 +186,4 @@ def test_simulation_cam_calibration(camera): assert camera.calibration_required camera.full_auto_calibrate() assert not camera.calibration_required - assert camera.active_detector.ready + assert camera.background_detector.ready diff --git a/tests/unit_tests/test_thing_selector.py b/tests/unit_tests/test_thing_selector.py new file mode 100644 index 00000000..36cf2767 --- /dev/null +++ b/tests/unit_tests/test_thing_selector.py @@ -0,0 +1,59 @@ +"""Test selector coercion logic for Thing mappings.""" + +import logging + +import pytest + +from openflexure_microscope_server.utilities import coerce_thing_selector + + +@pytest.mark.parametrize( + ("selected", "default", "warning_count"), + [ + ("a", "b", 1), + (None, "b", 0), + ("a", None, 1), + (None, None, 0), + ], +) +def test_coerce_with_empty_mapping(selected, default, warning_count, caplog): + """Test None always returned for empty mappings, check warnings when appropriate.""" + with caplog.at_level(logging.WARNING): + output = coerce_thing_selector( + thing_mapping={}, selected=selected, default=default + ) + assert output is None + assert len(caplog.records) == warning_count + + +@pytest.mark.parametrize( + ("selected", "default", "returned", "warning_count"), + [ + ("b", "b", "b", 0), + ("b", "a", "b", 0), + ("b", None, "b", 0), + ("b", "z", "b", 0), + (None, "b", "b", 0), + (None, "a", "a", 0), + (None, None, "a", 0), + (None, "z", "a", 1), + ("z", "b", "b", 1), + ("z", "a", "a", 1), + ("z", None, "a", 1), + ("z", "z", "a", 2), + ], +) +def test_coerce_with_populated_mapping( + selected, default, returned, warning_count, caplog +): + """Test coercion of selected key for a populated thing mapping. + + Check that warnings are raised when appropriate. + """ + thing_mapping = {"a": "Foo", "b": "Bar", "c": "FooBar"} + with caplog.at_level(logging.WARNING): + output = coerce_thing_selector( + thing_mapping=thing_mapping, selected=selected, default=default + ) + assert output == returned + assert len(caplog.records) == warning_count diff --git a/webapp/src/components/tabContentComponents/backgroundDetectComponents/paneBackgroundDetect.vue b/webapp/src/components/tabContentComponents/backgroundDetectComponents/paneBackgroundDetect.vue index 4b91f504..814ff9db 100644 --- a/webapp/src/components/tabContentComponents/backgroundDetectComponents/paneBackgroundDetect.vue +++ b/webapp/src/components/tabContentComponents/backgroundDetectComponents/paneBackgroundDetect.vue @@ -82,16 +82,21 @@ export default { this.modalNotify(`Current image is ${label} (${r.output[1]})`); }, readSettings: async function () { - this.backgroundDetectorName = await this.readThingProperty("camera", "detector_name"); - this.ready = await this.readThingProperty(this.backgroundDetectorName, "ready"); - this.backgroundDetectorSettings = await this.readThingProperty( - this.backgroundDetectorName, - "settings_ui", - ); - this.backgroundDetectorDisplayName = await this.readThingProperty( - this.backgroundDetectorName, - "display_name", + this.backgroundDetectorName = await this.readThingProperty( + "camera", + "background_detector_name", ); + if (this.backgroundDetectorName) { + this.ready = await this.readThingProperty(this.backgroundDetectorName, "ready"); + this.backgroundDetectorSettings = await this.readThingProperty( + this.backgroundDetectorName, + "settings_ui", + ); + this.backgroundDetectorDisplayName = await this.readThingProperty( + this.backgroundDetectorName, + "display_name", + ); + } }, }, };