Apply suggestions from code review of branch fast-sim

Co-authored-by: Richard Bowman <richard.bowman@cantab.net>
Co-authored-by: Joe Knapper <joe.knapper@glasgow.ac.uk>

src/openflexure_microscope_server/things/camera/simulation.py

@@ -46,16 +46,17 @@ RNG = np.random.default_rng()
 
 DOWNSAMPLE = 2
 # Upsample for sprites and then downsample to create sharp edges for each sprite
-# as these are small and calculated once there is almos no performance penalty
+# as these are small and calculated once there is almost no performance penalty
 # for a nice gain in quality.
 SPRITE_UPSAMPLE = 4
 
 # A list of 6 digit hex colour codes separated by ;. Allow a trailing ;
+# For example, OpenFlexure pink would be #C5247F;
 COLOUR_LIST_REGEX = re.compile(
     r"^\s*(#[0-9a-fA-F]{6})\s*(?:;\s*(#[0-9a-fA-F]{6})\s*)*;?\s*$"
 )
+# regex to separate R, G and B from a 6 digit hex code with preceding #
 COLOUR_REGEX = re.compile(r"^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$")
-# regex to capture R, G and B
 
 
 @overload
@@ -159,16 +160,21 @@ class SimulatedCamera(BaseCamera):
 
     @lt.property
     def colour(self) -> str:
-        """The number of colour of the blobs."""
+        """The colour of the blobs as a HTML hex string.
+
+        The string can either be a single colour (e.g. "#c5247f") or a list of
+        colours separated by semicolons (e.g. "#c5247f; #b937b9"). Additional
+        spaces are allowed between colours.
+        """
         return self._colour
 
     @colour.setter
-    def _set_colour(self, value: str) -> None:
+    def _set_colour(self, colour_value: str) -> None:
-        if COLOUR_LIST_REGEX.match(value) is None:
+        if COLOUR_LIST_REGEX.match(colour_value) is None:
-            self.logger.warning(f"{value} is not a valid colour string.")
+            self.logger.warning(f"{colour_value} is not a valid colour string.")
             return
 
-        self._colour = value
+        self._colour = colour_value
         if self._capture_enabled:
             self.generate_canvas()
 
@@ -210,7 +216,7 @@ class SimulatedCamera(BaseCamera):
             sprite_pil = sprite_pil.resize(
                 (self.glyph_size, self.glyph_size), Image.Resampling.BILINEAR
             )
-            # Concert back and ensure all edges are zero as these are repeated at sample
+            # Convert back and ensure all edges are zero as these are repeated at sample
             # edge
             sprite = np.array(sprite_pil)
             sprite[0, :] = 0
@@ -311,9 +317,10 @@ class SimulatedCamera(BaseCamera):
             x_indices = x_indices % canvas_width
             y_indices = y_indices % canvas_height
         else:
-            # No sprites are placed right at the edge of the image so that the whole
+            # Rather than use a modulo for the index list, as above when wrapping,
-            # sprite is on the canvas. For a non-repeating image just clip to the
+            # this uses np.clip to coerce all out of bound indices to repeat the
-            # first or last pixel.
+            # first or last pixel in the canvas. This works because no sprite touches
+            # the very edge of the canvas (to prevent partial sprites).
             x_indices = np.clip(x_indices, 0, canvas_width - 1)
             y_indices = np.clip(y_indices, 0, canvas_height - 1)
 

tests/unit_tests/test_simulated_camera.py

@@ -77,13 +77,13 @@ def all_colours_present(
         if col_tuple not in colours:
             raise ValueError("Unexpected colour returned.")
         found[colours.index(col_tuple)] = True
-        # Don't exit early or we don't confrm that extra colours are not returned.
+        # Don't exit early or we don't confirm that extra colours are not returned.
     return all(found)
 
 
 def test_colour_str_to_colour():
     """Test colour_str_to_colour with some basic predefined test cases."""
-    # A basic test
+    # A basic test to convert a single str to the expected colour
     assert simulation.colour_str_to_colour("#123456") == (0x12, 0x34, 0x56)
     # A basic test with a trailing semicolon and surrounding spacing
     assert simulation.colour_str_to_colour("  #123456 ; ") == (0x12, 0x34, 0x56)
@@ -109,8 +109,8 @@ def test_colour_str_to_colour():
 def test_colour_list_regex(colour_str):
     """Check that anything matching the regex doesn't error when generating colours.
 
-    This will error if colour_str when splot into individual colours produces
+    This will error if splitting colour_str into individual colours produces
-    # an incorrect colour. Trying 100 times for each colour_str as the returned colour
+    an incorrect colour. Trying 100 times for each colour_str as the returned colour
     is randomised. Hypothesis will try to create the strings that match the regex but
     break the test.
     """
@@ -119,7 +119,7 @@ def test_colour_list_regex(colour_str):
 
 
 def test_canvas_regeneration(camera, caplog):
-    """Check canvas is regenerated if blob density of colour are changed."""
+    """Check canvas is regenerated if blob density or colour are changed."""
     cached_canvas = camera.canvas
     original_colour = camera.colour
 
@@ -172,7 +172,7 @@ def test_infinite_sample(camera, stage):
     # Turn on repeating
     camera.repeating = True
     time.sleep(0.2)  # Ensure frame regenerates
-    # Sample is now infitine, so not all background
+    # Sample is now infinite, so not all background
     assert not np.all(camera.capture_array() == simulation.BG_COLOR)