From 4c557612fb0cd8471b552ccf3d915e1234d2f1ad Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Mon, 12 Jan 2026 17:41:51 +0000 Subject: [PATCH] Apply suggestions from code review of branch fast-sim Co-authored-by: Richard Bowman Co-authored-by: Joe Knapper --- .../things/camera/simulation.py | 29 ++++++++++++------- tests/unit_tests/test_simulated_camera.py | 12 ++++---- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/openflexure_microscope_server/things/camera/simulation.py b/src/openflexure_microscope_server/things/camera/simulation.py index f48217e0..09ffe905 100644 --- a/src/openflexure_microscope_server/things/camera/simulation.py +++ b/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: - if COLOUR_LIST_REGEX.match(value) is None: - self.logger.warning(f"{value} is not a valid colour string.") + def _set_colour(self, colour_value: str) -> None: + if COLOUR_LIST_REGEX.match(colour_value) is None: + 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 - # sprite is on the canvas. For a non-repeating image just clip to the - # first or last pixel. + # Rather than use a modulo for the index list, as above when wrapping, + # this uses np.clip to coerce all out of bound indices to repeat the + # 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) diff --git a/tests/unit_tests/test_simulated_camera.py b/tests/unit_tests/test_simulated_camera.py index 3572aedb..b9243f4a 100644 --- a/tests/unit_tests/test_simulated_camera.py +++ b/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 - # an incorrect colour. Trying 100 times for each colour_str as the returned colour + 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 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)