Apply suggestions from code review of branch camera_modes

Co-authored-by: Joe Knapper <joe.knapper@glasgow.ac.uk>

src/openflexure_microscope_server/things/__init__.py

@@ -68,9 +68,9 @@ class OFMThing(lt.Thing):
         """Create a ``RelativeDataPath`` object with this Thing set as the saving Thing.
 
         :param path: The relative path within the data directory of this Thing's data
-            dir that the data shudl be saved.
+            dir that the data should be saved to.
         :param absolute: Set to True if the current path is absolute. A relative path
-            will be returned. An validation error will be raised if the absolute path
+            will be returned. A validation error will be raised if the absolute path
             is not within the data directory.
 
         :return: A ``RelativeDataPath`` object with the saving Thing already set.

src/openflexure_microscope_server/things/camera/__init__.py

@@ -230,8 +230,10 @@ class BaseCamera(OFMThing, ABC):
         # would be ideal.
         self._default_background_detector = "bg_channel_deviations_luv"
         self._background_detector_name: Optional[str] = None
+        self._framerate_monitor_running = False
 
-        if "default" and "full_resolution" not in self.streaming_modes:
+        required_modes = ("default", "full_resolution")
+        if not all(mode in self.streaming_modes for mode in required_modes):
             raise KeyError(
                 f"Camera {type(self).__name__} doesn't define both a 'default' and a "
                 "'full_resolution' streaming mode."
@@ -559,7 +561,7 @@ class BaseCamera(OFMThing, ABC):
     def capture_downsampled_array(self) -> NDArray:
         """Acquire one image from the camera, downsample, and return as an array.
 
-        * The array is downsamples by the thing property `downsampled_array_factor`.
+        * The array is downsampled by the thing property `downsampled_array_factor`.
         * The default capture array arguments are used.
 
         This method provides the interface expected by the camera_stage_mapping.
@@ -718,7 +720,7 @@ class BaseCamera(OFMThing, ABC):
         """Capture a PIL image from the camera.
 
         This unlike the ``grab_*`` methods this may pause the stream or temporarily
-        switch streaming mode to capute the image if required by the mode.
+        switch streaming mode to capture the image if required by the mode.
         """
 
     @lt.action

src/openflexure_microscope_server/things/camera/opencv.py

@@ -149,7 +149,7 @@ class OpenCVCamera(BaseCamera):
             raise NotImplementedError(
                 "OpenCV camera camera doesn't support raw capture."
             )
-        # Warn if the capture mode is incorrect, but don't read the cooerced value as
+        # Warn if the capture mode is incorrect, but don't read the coerced value as
         # this camera only supports one mode.
         self._validate_capture_mode(capture_mode)
         ret, frame = self.cap.read()
@@ -159,7 +159,7 @@ class OpenCVCamera(BaseCamera):
 
     def _capture_image(self, capture_mode: str = "standard") -> Image.Image:
         """Acquire one image from the camera and return as a PIL image."""
-        # Warn if the capture mode is incorrect, but don't read the cooerced value as
+        # Warn if the capture mode is incorrect, but don't read the coerced value as
         # this camera only supports one mode.
         self._validate_capture_mode(capture_mode)
         return Image.fromarray(self.capture_as_array())

src/openflexure_microscope_server/things/camera/picamera.py

@@ -536,8 +536,8 @@ class StreamingPiCamera2(BaseCamera, ABC):
         If the camera is already in the correct mode, the stream isn't paused and
         this is the same as using ``self._streaming_picamera()``.
 
-        Otherwise, pause stream, and switch switch mode. Mode is reset and stream
+        Otherwise, pause stream, and switch mode. Mode is reset and stream
-        restarts stream after the context manager closes.
+        restarts after the context manager closes.
         """
         required_streaming_mode = capture_mode_info.streaming_mode
         if (
@@ -568,7 +568,7 @@ class StreamingPiCamera2(BaseCamera, ABC):
         """Acquire one image from the camera and return it as a PIL Image.
 
         :param capture_mode: The capture mode to use. See the description field of each
-            mode for more detail in ``capture_modes`` for more detail.
+            mode in ``capture_modes`` for more detail.
 
         :raises TimeoutError: if this time is exceeded during capture.
         """
@@ -594,13 +594,13 @@ class StreamingPiCamera2(BaseCamera, ABC):
 
         :param capture_mode: (Optional) The name of the capture mode as defined by the
             camera.
-        :param raw: Whether to capture RAW data. Capturing RAW data may infore some
+        :param raw: Whether to capture RAW data. Capturing RAW data may ignore some
             of the camera mode settings.
 
         :raises TimeoutError: if this time is exceeded during capture.
         """
         if raw:
-            # Raw cannot used _capture_image.
+            # Raw cannot use _capture_image.
             capture_mode = self._validate_capture_mode(capture_mode)
             capture_mode_info = self.capture_modes[capture_mode]
 

src/openflexure_microscope_server/things/camera/simulation.py

@@ -496,10 +496,8 @@ class SimulatedCamera(BaseCamera):
             Setting this to True will result in an error.
         """
         if raw is True:
-            raise NotImplementedError(
+            raise NotImplementedError("Simulation camera doesn't support raw capture.")
-                "Simulation camera camera doesn't support raw capture."
+        # Warn if the capture mode is incorrect, but don't read the coerced value as
-            )
-        # Warn if the capture mode is incorrect, but don't read the cooerced value as
         # this camera only supports one mode.
         self._validate_capture_mode(capture_mode)
         return np.array(self.generate_frame())
@@ -509,7 +507,7 @@ class SimulatedCamera(BaseCamera):
 
         It is used for capture to memory.
         """
-        # Warn if the capture mode is incorrect, but don't read the cooerced value as
+        # Warn if the capture mode is incorrect, but don't read the coerced value as
         # this camera only supports one mode.
         self._validate_capture_mode(capture_mode)
         return self.generate_frame()

src/openflexure_microscope_server/things/scan_workflows.py

@@ -129,7 +129,7 @@ class ScanWorkflow(Generic[SettingModelType], lt.Thing):
         """Return the save resolution as determined by a test image."""
         # Capture an example image.
         image = self._cam._capture_image(capture_mode=self.capture_mode)
-        # Check side to create a unit faction for downsampling.
+        # Check size to create a unit faction for downsampling.
         return image.size
 
     def pre_scan_routine(self, settings: SettingModelType) -> None:

tests/hardware_specific_tests/picamera2/test_acquisition.py

@@ -52,9 +52,9 @@ def test_format(picamera_client):
         image_format="png",
         retain_image=True,
     )
-    jpeg_capture = Image.open(blob.open())
+    png_capture = Image.open(blob.open())
-    jpeg_capture.verify()
+    png_capture.verify()
-    assert jpeg_capture.format == "PNG"
+    assert png_capture.format == "PNG"
 
 
 def test_standard_capture_size(picamera_client):

tests/integration_tests/test_actions.py

@@ -53,7 +53,7 @@ def test_grab_jpeg(simulation_test_env):
 def test_capture_and_metadata(simulation_test_env, image_format, caplog):
     """Capture an image and check a attributes.
 
-    - Check that the position is encoded into the image metadata.
+    - Check that the position is encoded into the image metadata
     - Check the dimensions
     - Check the format
     """

tests/unit_tests/test_base_camera.py

@@ -86,7 +86,7 @@ class MemorySaveTestCase:
 
 
 SAVE_TEST_CASES = [
-    # Default test case is a jpeg, ckec it works with all extensions.
+    # Default test case is a jpeg, check it works with all extensions.
     MemorySaveTestCase("foobar.jpeg"),
     MemorySaveTestCase("foobar.jpg"),
     MemorySaveTestCase("foobar.JPEG"),
@@ -103,7 +103,7 @@ SAVE_TEST_CASES = [
 
 @pytest.mark.parametrize("test_case", SAVE_TEST_CASES)
 def test_save_from_memory(test_case, test_env, mocker):
-    """Check the correct timage is retrieved and saved with correct settings."""
+    """Check the correct image is retrieved and saved with correct settings."""
     camera = test_env.get_thing_by_type(SimulatedCamera)
     camera._memory_buffer = mocker.Mock()
     camera._add_metadata_to_capture = mocker.Mock()

tests/unit_tests/test_camera_buffer.py

@@ -90,7 +90,7 @@ def test_get_two_images():
     )
     returned_image1, _, _ = mem_buf.get_image(buffer_id1)
     returned_image2, _, _ = mem_buf.get_image(buffer_id2)
-    # It they the same images
+    # Assert they are the same images
     assert misc_image1 is returned_image1
     assert misc_image2 is returned_image2
     # They are removed from memory
@@ -134,7 +134,7 @@ def test_buffer_size_changing():
     with pytest.raises(NoImageInMemoryError):
         mem_buf.get_image(buffer_id2)
     returned_image3, _, _ = mem_buf.get_image(buffer_id3)
-    # Image 3 the expected image
+    # Image 3 is the expected image
     assert misc_image3 is returned_image3
 
 
@@ -226,7 +226,7 @@ def test_get_metadata_too():
 
 
 def test_mode_is_returned():
-    """Capture 10 images with metadata and check metadata is returned as expected."""
+    """Check that the correct mode name is returned with the image from the buffer."""
     mem_buf = CameraMemoryBuffer()
     misc_image = random_image()
     buffer_id = mem_buf.add_image(misc_image, random_metadata(), "standard")

tests/unit_tests/test_ofm_thing.py

@@ -92,7 +92,7 @@ def test_saving_thing_propagates_on_join():
     path2 = path.join("bar")
     assert not path2.save_location_set
 
-    # Set thing and check joiend path is as expected
+    # Set thing and check joined path is as expected
     path2.set_saving_thing(thing)
     assert path2.save_location_set
     assert path2.abs_data_path == os.path.join("data", "thing", "foo", "bar")