From 797f3bcb6018d91fb8d885941ca86e3d53c1d86f Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Fri, 11 Apr 2025 00:43:51 +0100
Subject: [PATCH 01/11] Collecting scan parameters into dictionary that will
 become a dataclass

---
 .../things/smart_scan.py                      | 258 +++++++++++-------
 1 file changed, 156 insertions(+), 102 deletions(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index fbf116e5..8c33822a 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -203,6 +203,8 @@ class SmartScanThing(Thing):
         self._ongoing_scan_name: Optional[str] = None
         self._starting_position: Optional[Mapping[str, int]] = None
         self._scan_images_taken: Optional[int] = None
+        # TODO Scan data is a dict during refactoring, should become a dataclass
+        self._scan_data: Optional[dict] = None
 
     @thing_action
     def sample_scan(
@@ -224,7 +226,6 @@ class SmartScanThing(Thing):
         background_detect Thing).
         """
 
-        started_scan = False
         got_lock = self._scan_lock.acquire(timeout=0.1)
         if not got_lock:
             raise RuntimeError("Trying to run scan while scan is already running!")
@@ -240,20 +241,21 @@ class SmartScanThing(Thing):
         self._background_detect = background_detect
         self._scan_images_taken = 0
 
+        # Don't set self._scan_data dictionary. This is done at the start of _run_scan
+
         try:
-            self._check_background_is_set()
+            self._check_background_and_csm_set()
             self._ongoing_scan_name = self._get_unique_scan_name_and_dir(scan_name)
-            overlap = self.overlap
             # record starting position so we can return there
             self._starting_position = self._stage.position
-            started_scan = True
-            self._run_scan(scan_name, overlap)
+            self._run_scan()
         except Exception as e:
-            if started_scan:
+            # If _scan_data is set then scan started
+            if self._scan_data is not None:
                 self._return_to_starting_position()
                 if not isinstance(e, NotEnoughFreeSpaceError):
                     # Don't stich if drive is full (already logged)
-                    self._perform_final_stitch(overlap)
+                    self._perform_final_stitch()
             # Error must be raised so UI gives correct output
             raise e
         finally:
@@ -268,14 +270,24 @@ class SmartScanThing(Thing):
             self._background_detect = None
             self._ongoing_scan_name = None
             self._scan_images_taken = None
+            self._scan_data = None
             self._scan_lock.release()
 
     @_scan_running
-    def _check_background_is_set(self):
-        """Before starting a scan check that we've got a background set
+    def _check_background_and_csm_set(self):
+        """Before starting a scan check that background and camera-stage-mapping are set
 
-        Raise error if it is not set but background detect is being used.
+        Raise error if:
+          - background is to be skipped but is not set
+          -  camera stage mapping is not set
+
+        Raise warning if not using background detect that scan will go on until max steps reached
         """
+        if self._csm.image_resolution is None:
+            raise RuntimeError(
+                "Camera-stage mapping is not calibrated. This is required before "
+                "scans can be carried out."
+            )
 
         if self.skip_background:
             if not self._background_detect.background_distributions:
@@ -367,9 +379,11 @@ class SmartScanThing(Thing):
             if not os.path.exists(trial_dir):
                 os.makedirs(trial_dir)
                 # If we made the directory this is the scan name
-                # Save it as the most latest scan (this persists as a
+                # Save the scan name as the latest scan (this persists as a
                 # property after the scan finishes)
                 self._latest_scan_name = trial_unique_scan_name
+                # Create images directory and
+                os.mkdir(self.images_dir_for_scan(trial_unique_scan_name))
                 # Return the scan name
                 return trial_unique_scan_name
         raise FileExistsError("Could not create a new scan folder: all names in use!")
@@ -401,90 +415,113 @@ class SmartScanThing(Thing):
         return loc + [z]
 
     @_scan_running
-    def _run_scan(self, scan_name, overlap):
+    def _take_test_image_to_calc_displacement(self, overlap):
+        """
+        Take a test image and use camera stage mapping to calculate x and y displacement
+
+        Return (dx, dy) - the x and y displacments in steps
+        """
+        test_jpg = self._cam.grab_jpeg()
+        test_image = np.array(Image.open(test_jpg.open()))
+
+        test_image_res = list(test_image.shape[:2])
+        csm_image_res = [int(i) for i in self._csm.image_resolution]
+
+        if test_image_res != csm_image_res:
+            raise RuntimeError(
+                "Cannot start scan as it is set up to capture with a resolution that "
+                "has not been mapped.\n"
+                f"Scan resolution: {test_image_res}\n"
+                f"camera-stage-mapping resolution {csm_image_res}."
+            )
+
+        # get displacement matrix. note it is for (y, x) not (x, y) coordinates
+        csm_disp_matrix = self._csm.image_to_stage_displacement_matrix
+
+        # Calculate displacements in image coordinates
+        dx_img = test_image.shape[1] * (1 - overlap)
+        dy_img = test_image.shape[0] * (1 - overlap)
+
+        # Calculate discplacents in steps as vectors using a dot product with the matrix
+        dx_vec = np.dot(np.array([0, dx_img]), csm_disp_matrix)
+        dy_vec = np.dot(np.array([dy_img, 0]), csm_disp_matrix)
+
+        # Assume no rotation or skew and take only the aligned axis of vector.
+        # Cooerce to positive integer
+        dx = int(np.abs(dx_vec[0]))
+        dy = int(np.abs(dy_vec[1]))
+
+        return dx, dy
+
+    @_scan_running
+    def _set_scan_data(self):
+        """
+        This sets the self._scan_data dictionary. This needs to become a
+        dataclass.
+        """
+        overlap = self.overlap
+        dx, dy = self._take_test_image_to_calc_displacement(overlap)
+        self._scan_logger.info(
+            f"Based on an overlap of {overlap}, we will make steps of {dx}, {dy}"
+        )
+
+        if self.autofocus_dz == 0:
+            self._scan_logger.info("Running scan without autofocus")
+        elif self.autofocus_dz <= 200:
+            self._scan_logger.warning(
+                f"Your dz range is {self.autofocus_dz} steps, which is too short to "
+                "attempt to focus. Running without autofocus"
+            )
+
+        # Fix scan parameters in case UI is updates during scan.
+        self._scan_data = {
+            "scan_name": self._ongoing_scan_name,
+            "overlap": overlap,
+            "max_dist": self.max_range,
+            "dx": dx,
+            "dy": dy,
+            "autofocus_dz": self.autofocus_dz,
+            "start_time": time.strftime("%H_%M_%S-%d_%m_%Y"),
+            "skip_background": self.skip_background,
+            "stitch_automatically": self.stitch_automatically,
+        }
+
+    @_scan_running
+    def _save_scan_inputs_jons(self):
+        # This should be a method of the scan_data dataclass
+        data = {
+            "scan_name": self._ongoing_scan_name,
+            "overlap": self._scan_data["overlap"],
+            "autofocus range": self._scan_data["autofocus_dz"],
+            "dx": self._scan_data["dx"],
+            "dy": self._scan_data["dy"],
+            "start time": self._scan_data["start_time"],
+            "skipping background": self._scan_data["skip_background"],
+        }
+
+        scan_inputs_fname = os.path.join(
+            self._ongoing_scan_images_dir, "scan_inputs.json"
+        )
+        with open(scan_inputs_fname, "w", encoding="utf-8") as f:
+            json.dump(data, f, ensure_ascii=False, indent=4)
+
+    @_scan_running
+    def _run_scan(self):
         # Define these variables so we can use them in the finally: block
         # (after testing they are not None)
         scan_successful = True
         capture_thread = None
 
         try:
-            os.mkdir(self._ongoing_scan_images_dir)
-
-            self._scan_logger.info(f"Saving images to {self._ongoing_scan_images_dir}")
-
-            max_dist = self.max_range
-
-            if self.autofocus_dz == 0:
-                self._scan_logger.info("Running scan without autofocus")
-            elif self.autofocus_dz <= 200:
-                self._scan_logger.warning(
-                    f"Your dz range is {self.autofocus_dz} steps, which is too short to attempt to focus. Running without autofocus"
-                )
-
-            names = []
-            positions = []
-
-            r = self._cam.grab_jpeg()
-            arr = np.array(Image.open(r.open()))
-            if self._csm.image_resolution is None:
-                raise RuntimeError(
-                    "Camera-stage mapping is not calibrated. This is required before "
-                    "scans can be carried out."
-                )
-            if list(arr.shape[:2]) != [int(i) for i in self._csm.image_resolution]:
-                self._scan_logger.error(
-                    f"Images are, by default, {arr.shape[:2]}, but the CSM was "
-                    f"calibrated at {self._csm.image_resolution}."
-                )
-
-            # Here, we calculate the x and y step size based on the desired overlap
-            # TODO: Consider using CSM calibration size instead
-            # TODO: generalise to have 2D displacements for x and y (as the
-            # camera and stage may not be aligned).
-            csm_disp_matrix = self._csm.image_to_stage_displacement_matrix
-
-            dx = int(
-                np.abs(
-                    np.dot(
-                        np.array([0, arr.shape[1] * (1 - overlap)]), csm_disp_matrix
-                    )[0]
-                )
-            )
-            dy = int(
-                np.abs(
-                    np.dot(
-                        np.array([arr.shape[0] * (1 - overlap), 0]), csm_disp_matrix
-                    )[1]
-                )
-            )
-
-            self._scan_logger.info(
-                f"Based on an overlap of {overlap}, we will make steps of {dx}, {dy}"
-            )
+            self._set_scan_data()
+            self._save_scan_inputs_jons()
 
             # construct a 2D scan path
             path = [[self._stage.position["x"], self._stage.position["y"]]]
-
-            focused_path = []  # This holds a list of all points where focus succeeded
-            true_path = []  # This holds a list of all points visited
-
-            start_time = time.strftime("%H_%M_%S-%d_%m_%Y")
-
-            data = {
-                "scan_name": scan_name,
-                "overlap": overlap,
-                "autofocus range": self.autofocus_dz,
-                "dx": dx,
-                "dy": dy,
-                "start time": start_time,
-                "skipping background": self.skip_background,
-            }
-
-            scan_inputs_fname = os.path.join(
-                self._ongoing_scan_images_dir, "scan_inputs.json"
-            )
-            with open(scan_inputs_fname, "w", encoding="utf-8") as f:
-                json.dump(data, f, ensure_ascii=False, indent=4)
+            # This holds a list of all points where focus succeeded
+            focused_path = []
+            # This holds a list of all points visited
+            true_path = []
 
             if self._scan_images_taken != 0:
                 raise RuntimeError(
@@ -499,14 +536,14 @@ class SmartScanThing(Thing):
                 if self._scan_images_taken > 3:
                     if not self._preview_stitch_running():
                         self._preview_stitch_start()
-                    if self.stitch_automatically:
+                    if self._scan_data["stitch_automatically"]:
                         if not self._correlate_running():
-                            self._correlate_start(overlap=overlap)
+                            self._correlate_start(overlap=self._scan_data["overlap"])
 
                 ensure_free_disk_space(self._ongoing_scan_dir)
 
                 # Check if the image is background
-                if self.skip_background:
+                if self._scan_data["skip_background"]:
                     image_is_sample = self._background_detect.image_is_sample()
                 else:
                     image_is_sample = True
@@ -519,10 +556,22 @@ class SmartScanThing(Thing):
                 else:
                     # if not, it's sample. run an autofocus and use the updated height
                     new_pos = [
-                        [self._stage.position["x"] - dx, self._stage.position["y"]],
-                        [self._stage.position["x"] + dx, self._stage.position["y"]],
-                        [self._stage.position["x"], self._stage.position["y"] - dy],
-                        [self._stage.position["x"], self._stage.position["y"] + dy],
+                        [
+                            self._stage.position["x"] - self._scan_data["dx"],
+                            self._stage.position["y"],
+                        ],
+                        [
+                            self._stage.position["x"] + self._scan_data["dx"],
+                            self._stage.position["y"],
+                        ],
+                        [
+                            self._stage.position["x"],
+                            self._stage.position["y"] - self._scan_data["dy"],
+                        ],
+                        [
+                            self._stage.position["x"],
+                            self._stage.position["y"] + self._scan_data["dy"],
+                        ],
                     ]
                     for pos in new_pos:
                         if (
@@ -619,16 +668,16 @@ class SmartScanThing(Thing):
                     capture_thread.start()
                     acquired.wait()  # wait until the image is acquired
 
-                    positions.append(loc[:2])
-                    names.append(name)
-
                 # add the current position to the list of all positions visited
                 true_path.append(loc)
 
                 temp_path = []
 
                 for i in path:
-                    if distance_to_site(i, true_path[0][:2]) < max_dist:
+                    if (
+                        distance_to_site(i, true_path[0][:2])
+                        < self._scan_data["max_dist"]
+                    ):
                         temp_path.append(i)
                     else:
                         self._scan_logger.info(
@@ -638,7 +687,12 @@ class SmartScanThing(Thing):
                 path = sorted(
                     path,
                     key=lambda x: (
-                        steps_from_centre(x, true_path[0][:2], dx, dy),
+                        steps_from_centre(
+                            x,
+                            true_path[0][:2],
+                            self._scan_data["dx"],
+                            self._scan_data["dy"],
+                        ),
                         distance_to_site(loc[:2], x),
                     ),
                 )
@@ -686,7 +740,7 @@ class SmartScanThing(Thing):
         # This is what happens if the scan completes sucessfully or the
         # user cancels it.
         self._return_to_starting_position()
-        self._perform_final_stitch(overlap)
+        self._perform_final_stitch()
 
     @_scan_running
     def _return_to_starting_position(self):
@@ -697,7 +751,7 @@ class SmartScanThing(Thing):
             )
 
     @_scan_running
-    def _perform_final_stitch(self, overlap):
+    def _perform_final_stitch(self):
         """Perform final stitch of the data"""
 
         if self._scan_images_taken <= 3:
@@ -714,12 +768,12 @@ class SmartScanThing(Thing):
         self._preview_stitch_wait()
         self._correlate_wait()
         try:
-            if self.stitch_automatically:
+            if self._scan_data["stitch_automatically"]:
                 self._scan_logger.info("Stitching final image (may take some time)...")
                 self.stitch_scan(
                     logger=self._scan_logger,
                     scan_name=self._ongoing_scan_name,
-                    overlap=overlap,
+                    overlap=self._scan_data["overlap"],
                 )
         except SubprocessError as e:
             self._scan_logger.error(f"Stitching failed: {e}", exc_info=e)

From 2c171026beab548e4db9ea83a3b843942193eb0a Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Fri, 11 Apr 2025 11:37:04 +0100
Subject: [PATCH 02/11] Still tidying the start end end of the scan code

---
 .../things/smart_scan.py                      | 61 ++++++++++++-------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 8c33822a..df0ab80b 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -128,11 +128,21 @@ class NotEnoughFreeSpaceError(IOError):
 
 
 def ensure_free_disk_space(path: str, min_space: int = 500000000) -> None:
-    """Raise an exception if we are running out of disk space"""
+    """
+    Raise an exception if we are running out of disk space
+
+    Args:
+       path =  path to a location on the disk you want to check
+       min_space [int] = the minimum space required in bytes
+           default = 500,000,000 (500MiB)
+
+    Raises:
+        NotEnoughFreeSpaceError is the
+    """
     du = shutil.disk_usage(path)
     if du.free < min_space:
         raise NotEnoughFreeSpaceError(
-            "There is not enough free disk space to continue."
+            "There is not enough free disk space to continue. "
             f"(Required: {min_space}, {du})."
         )
 
@@ -442,7 +452,7 @@ class SmartScanThing(Thing):
         dx_img = test_image.shape[1] * (1 - overlap)
         dy_img = test_image.shape[0] * (1 - overlap)
 
-        # Calculate discplacents in steps as vectors using a dot product with the matrix
+        # Calculate displacements in steps as vectors using a dot product with the matrix
         dx_vec = np.dot(np.array([0, dx_img]), csm_disp_matrix)
         dy_vec = np.dot(np.array([dy_img, 0]), csm_disp_matrix)
 
@@ -505,16 +515,34 @@ class SmartScanThing(Thing):
         with open(scan_inputs_fname, "w", encoding="utf-8") as f:
             json.dump(data, f, ensure_ascii=False, indent=4)
 
+    @_scan_running
+    def _manage_stitching_threads(self):
+        """
+        Manage the stitching threads starting them if the are needed
+        and not running.
+        """
+        if self._scan_images_taken > 3:
+            if not self._preview_stitch_running():
+                self._preview_stitch_start()
+            if self._scan_data["stitch_automatically"]:
+                if not self._correlate_running():
+                    self._correlate_start(overlap=self._scan_data["overlap"])
+
     @_scan_running
     def _run_scan(self):
-        # Define these variables so we can use them in the finally: block
-        # (after testing they are not None)
+        # Uset to check if finally was reached via exeption (except
+        # cancel by user.
         scan_successful = True
+
         capture_thread = None
 
         try:
             self._set_scan_data()
             self._save_scan_inputs_jons()
+            if self._scan_images_taken != 0:
+                raise RuntimeError(
+                    "_scan_images_taken should be zero before starting scanning"
+                )
 
             # construct a 2D scan path
             path = [[self._stage.position["x"], self._stage.position["y"]]]
@@ -523,24 +551,13 @@ class SmartScanThing(Thing):
             # This holds a list of all points visited
             true_path = []
 
-            if self._scan_images_taken != 0:
-                raise RuntimeError(
-                    "_scan_images_taken should be zero before starting scanning"
-                )
             # At the start of the loop, we simultaneously capture an image and move to the next scan point.
             # We skip capturing on the first run, because we've not focused yet - and also we skip capturing if
             # it looks like background.
             while len(path) > 0:
-                loc = self._move_to_next_point(path=path, focused_path=focused_path)
-
-                if self._scan_images_taken > 3:
-                    if not self._preview_stitch_running():
-                        self._preview_stitch_start()
-                    if self._scan_data["stitch_automatically"]:
-                        if not self._correlate_running():
-                            self._correlate_start(overlap=self._scan_data["overlap"])
-
                 ensure_free_disk_space(self._ongoing_scan_dir)
+                self._manage_stitching_threads()
+                loc = self._move_to_next_point(path=path, focused_path=focused_path)
 
                 # Check if the image is background
                 if self._scan_data["skip_background"]:
@@ -726,13 +743,13 @@ class SmartScanThing(Thing):
                 try:
                     capture_thread.join()
                 except Exception as e:
-                    # If the thread has already ended due to an exception we will
+                    # If the scan has already ended due to an exception we will
                     # ignore any exceptions, but if it appeared to be successful
-                    # we will log an error.
+                    # we will log the error.
                     if scan_successful:
                         self._scan_logger.error(
-                            "The scan appears to have started successfully, however the final capture raised"
-                            f"the following error: {e}."
+                            "The scan appears to have started successfully, however "
+                            f"the final capture raised the following error: {e}."
                             "Attempting to stitch and archive images.",
                             exc_info=e,
                         )

From cddb49c9f707db7ee5f5f954a587a30ea405eaa0 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Fri, 11 Apr 2025 14:36:55 +0100
Subject: [PATCH 03/11] Clean up code as I remove bare excepts

---
 .../things/smart_scan.py                      | 104 +++++++++++-------
 1 file changed, 67 insertions(+), 37 deletions(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index df0ab80b..a69a4db1 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -1,5 +1,3 @@
-# ruff: noqa: E722
-
 import shutil
 import zipfile
 import threading
@@ -44,25 +42,25 @@ BackgroundDep = direct_thing_client_dependency(
 )
 
 
-def closest(current, focused_path):
+def closest(current: tuple[int, int], focused_path: list[tuple[int, int]]) -> int:
     """Finds the index of the closest x-y position in a list from the current position,
     with ties split by the later element in the list (most recently taken)
 
-    must be float64 to deal with the huge numbers involved!"""
+    must be float64 (double precision) to deal with the huge numbers involved!"""
 
     current_pos = np.array(current[:2], dtype="float64")
-    path_pos = np.asarray(focused_path, dtype="float64").T[:2].T
+    path_pos = np.asarray(focused_path, dtype="float64")[:, :2]
 
-    dist_2 = np.sqrt(
-        np.sum((path_pos - current_pos) ** 2, axis=1, dtype="float64"), dtype="float64"
-    )
-    min_dist = np.argmin(dist_2)
-    mask = np.where(dist_2 == dist_2[min_dist], 1, 0)
-    try:
-        closest = np.max(np.nonzero(mask))
-    except:
-        closest = 0
-    return closest
+    # Use linalg.norm to calculate the direct distance bweween the points
+    # Note linalg.norm always used float64
+    dists = np.linalg.norm((path_pos - current_pos), axis=1)
+
+    # Get indicies of all mimuma.
+    # Note np.where always returns a tuple of arrays, hence the trailing [0]
+    indicies = np.where(dists == np.min(dists))[0]
+
+    # Return the last index
+    return indicies[-1]
 
 
 def unpack_autofocus(scan_data):
@@ -857,7 +855,9 @@ class SmartScanThing(Thing):
                     metadata
                 ).encode("utf-8")
                 piexif.insert(piexif.dump(exif_dict), jpeg_path)
-            except:
+            except:  # noqa: E722
+                # We need to capture any exeption as there are many reasons metadata
+                # might not be added. We wern rather than log the error.
                 self._scan_logger.warning(f"Failed to add metadata to {jpeg_path}")
         except Exception as e:
             raise IOError(f"An error occurred while saving {jpeg_path}") from e
@@ -1134,30 +1134,45 @@ class SmartScanThing(Thing):
         logger: InvocationLogger,
         cmd: list[str],
     ) -> CompletedProcess:
-        """Run a  subprocess and log any output"""
+        """
+        Run a  subprocess and log any output
+
+        Raises:
+            ChildProcessError if exit code is not zero
+        """
         logger.info(f"Running command in subprocess: `{' '.join(cmd)}`")
 
-        p = Popen(cmd, stdout=PIPE, stderr=STDOUT, bufsize=1, universal_newlines=True)
-        os.set_blocking(p.stdout.fileno(), False)
+        def log_buffer(buffer):
+            """A short internal function to read everything in the buffer to
+            a multiline string and log"""
+            lines = []
+            while line := buffer.readline():
+                lines.append(line)
+            if lines:
+                logger.info("".join(lines))
+
+        # Run the command piping stdout into the process for reading and
+        # forwarding the stdrerr to stdout
+        process = Popen(
+            cmd, stdout=PIPE, stderr=STDOUT, bufsize=1, universal_newlines=True
+        )
+        # Stop opening pipe blocking writing to it
+        os.set_blocking(process.stdout.fileno(), False)
         logger.info(time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(time.time())))
-        while p.poll() is None:
-            try:
-                output = p.stdout.readline()
-                if output != "" and output is not None:
-                    logger.info(output)
-            except:
-                pass
 
-        for line in p.stdout:
-            try:
-                output = p.stdout.readline()
-                if output != "" and output is not None:
-                    logger.info(output)
-            except:
-                pass
+        # Poll returns None while running, will return the error code when finnished
+        while process.poll() is None:
+            log_buffer(process.stdout)
+            # Once buffer is clear sleep for 0.2s before trying again.
+            time.sleep(0.2)
 
-        logger.info("Stitching complete")
-        return p
+        # Print everything in the buffer when program finishes
+        log_buffer(process.stdout)
+
+        if process.poll() == 0:
+            logger.info("Stitching complete")
+        else:
+            raise ChildProcessError(f"Subprocess {cmd[0]} exited with an error.")
 
     @thing_action
     def stitch_scan(
@@ -1171,7 +1186,9 @@ class SmartScanThing(Thing):
         Note that as this is a thing_action it needs the logger passed as
         a variable if called from another thing action
         """
+        json_fname = "scan_inputs.json"
         images_folder = self.images_dir_for_scan(scan_name=scan_name)
+        json_fpath = os.path.join(images_folder, json_fname)
 
         if self.stitch_tiff:
             tiff_arg = "--stitch_tiff"
@@ -1180,11 +1197,24 @@ class SmartScanThing(Thing):
 
         if overlap == 0.0:
             try:
-                with open(os.path.join(images_folder, "scan_inputs.json")) as data_file:
+                with open(json_fpath, "r", encoding="utf-8") as data_file:
                     data_loaded = json.load(data_file)
                     logger.info(data_loaded)
                 overlap = data_loaded["overlap"]
-            except:
+            except (json.decoder.JSONDecodeError, FileNotFoundError, TypeError):
+                # As there is no schema or pydantic model this should handle
+                # The file not being there, it not being json in the file,
+                # or the imported data not being indexable
+                logger.warning(
+                    f"Couldn't read scan data, is {json_fname} missing or corrupt? "
+                    "Attempting stitch with overlap value of 0.1"
+                )
+                overlap = 0.1
+            except KeyError:
+                logger.warning(
+                    "Value for overlap not found in scan data. "
+                    "Attempting stitch with overlap value of 0.1"
+                )
                 overlap = 0.1
         self.run_subprocess(
             logger,

From 1f66e8bc7a5ee002fed5025f3bdf93c5c08bbc02 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Fri, 11 Apr 2025 21:13:05 +0100
Subject: [PATCH 04/11] Enormous refactor of the scan code, was hard to find an
 intermediate working place to commit

---
 .../scan_planners.py                          | 298 ++++++++++++
 .../things/smart_scan.py                      | 452 ++++++++----------
 2 files changed, 510 insertions(+), 240 deletions(-)
 create mode 100644 src/openflexure_microscope_server/scan_planners.py

diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py
new file mode 100644
index 00000000..c6ae8501
--- /dev/null
+++ b/src/openflexure_microscope_server/scan_planners.py
@@ -0,0 +1,298 @@
+"""
+This module contains functionality for planning a scan route
+
+A scan route can be planned by a ScanPlanner class currently there
+is only one type the SmartSpiral. More can be added using by
+subclassing the ScanPlanner
+"""
+
+from typing import TypeAlias, Optional
+import logging
+
+import numpy as np
+
+LOGGER = logging.getLogger(__name__)
+
+XYPos: TypeAlias = tuple[int, int]
+XYZPos: TypeAlias = tuple[int, int, int]
+XYPosList: TypeAlias = list[XYPos]
+XYZPosList: TypeAlias = list[XYZPos]
+
+
+class ScanPlanner:
+    """
+    A base class for a scan planner.
+
+    This should never be used directly for a scan, it should be subclassed.
+    Each subclass should implmenet the methods with NotImplementedError
+    set.
+    """
+
+    def __init__(self, intial_position: XYPos, planner_settings: Optional[dict] = None):
+        self._initial_position = tuple(intial_position)
+
+        self._parse(planner_settings)
+
+        # The remaining (x,y) locations to scan
+        # (This was `path` before refactoring from the long `sample_scan` code)
+        self._remaining_locations: XYPosList = self._intial_location_list()
+
+        # This holds a list of all (x,y,z) locations where images were taken
+        # this may not be equivalent to the x,y poistions ins self._path_history
+        # if background detect is used
+        # (This was not used in the `sample_scan` code)
+        self._imaged_locations: XYZPosList = []
+
+        # This holds a list of all (x,y,z) locations where autofocus was successful
+        # (This was `focused_path` before refactoring from the long `sample_scan` code)
+        self._focused_locations: XYZPosList = []
+
+        # This holds a list of all  x,y locations visited in order since the start
+        # (This was `true_path` before refactoring from the long `sample_scan` code
+        # previously it had  z set, but if we don't take an image, not z is needed and
+        # it slows other checks)
+        self._path_history: XYPosList = []
+
+    @property
+    def scan_complete(self) -> bool:
+        """
+        Return True if there are no locations left to scan.
+        """
+        return not (self._remaining_locations)
+
+    def _parse(self, planner_settings: Optional[dict] = None) -> None:
+        """
+        Parse any settings sent to this planner and store them if needed.
+        """
+        raise NotImplementedError("Did you call the ScanPlanner base class?")
+
+    def _intial_location_list(self) -> XYPosList:
+        """
+        Called on initalisation. Sets the initial list of locations for this scan planner
+
+        For a simple grid scan/snake scan this would be all locations to move to
+        """
+        raise NotImplementedError("Did you call the ScanPlanner base class?")
+
+    def position_visited(self, position: XYPos) -> bool:
+        """
+        Return True if input xy position has been visited before
+        """
+        # Ensure tuple for correct matching!
+        return tuple(position) in self._path_history
+
+    def position_planned(self, position: XYPos) -> bool:
+        """
+        Return True if input xy position is planned
+        """
+        # Ensure tuple for correct matching!
+        return tuple(position) in self._remaining_locations
+
+    def get_next_location_and_z_estimate(self) -> tuple[XYPos, Optional[int]]:
+        """
+        Return the next location to scan, and the estimated z-position
+        for this location.
+
+        Note z-position may be None! This indicates that the current z, position
+        should be used.
+        """
+        if self.scan_complete:
+            raise RuntimeError("Can't get next position, scan is complete")
+
+        next_location = self._remaining_locations[0]
+
+        # If focussed locations exist return closest location, favouring most recent
+        if self._focused_locations:
+            z = self.closest_focus_site(next_location)[2]
+        else:
+            z = None
+        return next_location, z
+
+    def closest_focus_site(self, xy_pos: XYPos) -> XYZPos:
+        """
+        Return the xyz position of the closest site where focus was achieved
+        to the input xy_position, with the most recently taken image returned in
+        the case of a tie
+
+        Returns None if there if no focussed locations are present
+        """
+        if not self._focused_locations:
+            return None
+
+        # must be float64 (double precision) to deal with the huge numbers involved!
+        current_pos = np.array(xy_pos, dtype="float64")
+        path_pos = np.asarray(self._focused_locations, dtype="float64")[:, :2]
+
+        # Use linalg.norm to calculate the direct distance bweween the points
+        # Note linalg.norm always used float64
+        dists = np.linalg.norm((path_pos - current_pos), axis=1)
+
+        # Get indicies of all mimuma.
+        # Note np.where always returns a tuple of arrays, hence the trailing [0]
+        indicies = np.where(dists == np.min(dists))[0]
+
+        # The last index is most recent
+        return self._focused_locations[indicies[-1]]
+
+    def mark_location_visited(
+        self, xyz_pos: XYPos, imaged: bool, focused: bool
+    ) -> None:
+        """
+        Mark the location as visited
+
+        Args:
+            xyz_pos: the x_y poistion
+            imaged: true if an image was taken, false if not (due to background detect)
+            focused: true if autofocus completed successfully
+        """
+        # ensure is tuple!
+        xyz_pos = tuple(xyz_pos)
+        xy_pos = xyz_pos[:2]
+
+        # Remove the expected position from the remaining locations list
+        # and check it's correct
+        expected_pos = tuple(self._remaining_locations.pop(0))
+        if xy_pos != expected_pos:
+            raise RuntimeError("Wrong scan location visited!")
+
+        # Append xy position for path_history
+        self._path_history.append(xy_pos)
+        # And full x,y,z for imaged and foucsed if appropriate
+        if imaged:
+            self._imaged_locations.append(xyz_pos)
+        if focused:
+            self._focused_locations.append(xyz_pos)
+
+
+class SmartSpiral(ScanPlanner):
+    """
+    This is a smart spiral scan that spirals out from the centre.
+
+    Each time and image is taken the four neighbouring images are added
+    to the list of poisitions to image (unless they are already listed or
+    tried). However if a location is not imaged due no sample being detected
+    then neibouring positions are not imaged.
+
+    The next image taken is the closes to the centre (considering the largest
+    of vertical or horizontal distance), ties are broken by the distance from
+    the current position.
+    """
+
+    _max_dist: int = 0
+    _dx: int = 0
+    _dy: int = 0
+
+    def _parse(self, planner_settings: Optional[dict] = None) -> None:
+        """
+        Parse SmartSpiral Settings. This should be a dictionary
+
+        "dx" - the movement size in x
+        "dy" - the movement size in y
+        "max_dist" - The maximum distance to a location can be from the centre.
+        """
+
+        expected_keys = ["max_dist", "dx", "dy"]
+        invalid_msg = "SmartSpiral requires a planner_settings dictionary with keys: "
+        if not planner_settings:
+            raise ValueError(invalid_msg + ",".join(expected_keys))
+        if not all(keys in planner_settings for keys in expected_keys):
+            raise ValueError(invalid_msg + ",".join(expected_keys))
+
+        self._dx = int(planner_settings["dx"])
+        self._dy = int(planner_settings["dy"])
+        self._max_dist = int(planner_settings["max_dist"])
+
+    def _intial_location_list(self) -> XYPosList:
+        """
+        Called on initalisation. Sets the initial list of locations for this scan planner
+
+        For smart spiral this is just the first point
+        """
+        return [self._initial_position]
+
+    def mark_location_visited(
+        self, xyz_pos: XYPos, imaged: bool = True, focused: bool = True
+    ) -> None:
+        """
+        Mark the location as visited. Adjust extra poisitons accordingly
+
+        Args:
+            xyz_pos: the x_y poistion
+            imaged: true if an image was taken, false if not (due to background detect)
+            focused: true if autofocus completed successfully
+        """
+        # First call the base class to update the positions
+        super().mark_location_visited(xyz_pos, imaged, focused)
+
+        xy_pos = tuple(xyz_pos[:2])
+        if imaged:
+            self._add_surrounding_positions(xy_pos)
+        self._re_sort_remaining_locations(xy_pos)
+
+    def _add_surrounding_positions(self, xy_pos: XYPos) -> None:
+        """
+        This adds the surrounding (4 point connectivity) poistions
+        to the remaining locations if they are not too far away or
+        or already planned or already visited
+        """
+        new_positions = [
+            (xy_pos[0] - self._dx, xy_pos[1]),
+            (xy_pos[0] + self._dx, xy_pos[1]),
+            (xy_pos[0], xy_pos[1] - self._dy),
+            (xy_pos[0], xy_pos[1] + self._dy),
+        ]
+
+        for new_pos in new_positions:
+            # Skip position if already planned or fixited
+            if self.position_planned(new_pos) or self.position_visited(new_pos):
+                continue
+
+            dist = distance_between(new_pos, self._initial_position)
+            if dist > self._max_dist:
+                LOGGER.debug("Rejected moving to %s as it is out of range", new_pos)
+                continue
+            self._remaining_locations.append(new_pos)
+
+    def _re_sort_remaining_locations(self, current_pos: XYPos) -> None:
+        """
+        Sort the remaining positions besed on the current location
+        """
+
+        # Defined rather than use a lambda for readability
+        def sort_key(pos):
+            return self.moves_from_centre(pos), distance_between(current_pos, pos)
+
+        self._remaining_locations.sort(key=sort_key)
+
+    def moves_from_centre(
+        self,
+        xy_pos: XYPos,
+    ) -> float:
+        """
+        Return the number of moves from the centre in the x or y direction
+        whichever is largest
+
+        Args:
+        xy_pos: the position
+
+        Note this has been renamed from `steps_from_centre` as that implied
+        stepper motor steps not number of moves in a scan
+        """
+        move_size = np.array([self._dx, self._dy])
+        starting_pos = np.array(self._initial_position, dtype="float64")
+        current_pos = np.array(xy_pos, dtype="float64")
+
+        displacement_in_moves = (current_pos - starting_pos) / move_size
+
+        return np.max(np.abs(displacement_in_moves))
+
+
+def distance_between(current_pos: XYPos, next_pos: XYPos) -> float:
+    """
+    Calculate the distance between the two xy positions
+
+    This was previously called `distance_to_site`
+    """
+    next_pos = np.array(next_pos, dtype="float64")
+    current_pos = np.array(current_pos, dtype="float64")
+    return np.linalg.norm(next_pos - current_pos)
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index a69a4db1..da404f63 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -33,7 +33,7 @@ from openflexure_microscope_server.utilities import ErrorCapturingThread
 from openflexure_microscope_server.things.autofocus import AutofocusThing
 from openflexure_microscope_server.things.camera_stage_mapping import CameraStageMapper
 from openflexure_microscope_server.things.background_detect import BackgroundDetectThing
-
+from openflexure_microscope_server import scan_planners
 
 CSMDep = direct_thing_client_dependency(CameraStageMapper, "/camera_stage_mapping/")
 AutofocusDep = direct_thing_client_dependency(AutofocusThing, "/autofocus/")
@@ -42,27 +42,6 @@ BackgroundDep = direct_thing_client_dependency(
 )
 
 
-def closest(current: tuple[int, int], focused_path: list[tuple[int, int]]) -> int:
-    """Finds the index of the closest x-y position in a list from the current position,
-    with ties split by the later element in the list (most recently taken)
-
-    must be float64 (double precision) to deal with the huge numbers involved!"""
-
-    current_pos = np.array(current[:2], dtype="float64")
-    path_pos = np.asarray(focused_path, dtype="float64")[:, :2]
-
-    # Use linalg.norm to calculate the direct distance bweween the points
-    # Note linalg.norm always used float64
-    dists = np.linalg.norm((path_pos - current_pos), axis=1)
-
-    # Get indicies of all mimuma.
-    # Note np.where always returns a tuple of arrays, hence the trailing [0]
-    indicies = np.where(dists == np.min(dists))[0]
-
-    # Return the last index
-    return indicies[-1]
-
-
 def unpack_autofocus(scan_data):
     """Extract z, sharpness data from a move_and_measure call
 
@@ -87,7 +66,7 @@ def unpack_autofocus(scan_data):
     return jpeg_heights[turning[0] : turning[1]], jpeg_sizes_mb[turning[0] : turning[1]]
 
 
-def limit_focus_change(prev_pos, prev_z, new_pos, new_z, limit):
+def focus_change_acceptable(prev_pos, prev_z, new_pos, new_z, fractional_limit):
     # limit is the largest ratio of change in z to change in xy that's allowed
 
     prev_xy = np.asarray(prev_pos, dtype="float64")
@@ -102,23 +81,9 @@ def limit_focus_change(prev_pos, prev_z, new_pos, new_z, limit):
     else:
         movement_ratio = np.divide(focus_change, dist, dtype="float64")
 
-    if movement_ratio > limit:
-        return "reject"
-    else:
-        return "accept"
-
-
-def steps_from_centre(current_loc, starting_loc, dx, dy):
-    step_size = np.array([dx, dy])
-    return np.max(np.abs(np.divide(np.subtract(current_loc, starting_loc), step_size)))
-
-
-def distance_to_site(current_pos, next_pos):
-    next_pos = np.array(next_pos, dtype="float64")
-    current_pos = np.array(current_pos, dtype="float64")
-    return np.sqrt(
-        (next_pos[1] - current_pos[1]) ** 2 + (next_pos[0] - current_pos[0]) ** 2
-    )
+    if movement_ratio > fractional_limit:
+        return False
+    return True
 
 
 class NotEnoughFreeSpaceError(IOError):
@@ -398,29 +363,27 @@ class SmartScanThing(Thing):
 
     @_scan_running
     def _move_to_next_point(
-        self,
-        path: list[list[int]],
-        focused_path: list[list[int]],
-    ) -> list[int]:
-        """Remove the first point from the path, and move there.
+        self, next_point: tuple[int, int], z_estimate: Optional[int] = None
+    ) -> tuple[int, int, int]:
+        """Move to the next position (half an autofocus move below estimated z)
 
-        This will move to the next XY position in `path`, taking the `z` value
-        either from the current z value of the stage, or from `focused_path`.
+        Moves the stage to the next poistion. If no z_estimate is given then
+        the current stage position is used.
 
-        Returns the point we have moved to.
+        Returns the (x,y,z) with the chosen z_estimate
         """
-        loc = [path[0][0], path[0][1]]
-        path.remove(path[0])
-        if len(focused_path) > 1:
-            z_index = closest(loc, focused_path)
-            z = int(focused_path[z_index][2])
-        else:
-            z = self._stage.position["z"]
-        self._scan_logger.info(f"Moving to {loc}")
+
+        if z_estimate is None:
+            z_estimate = self._stage.position["z"]
+
+        self._scan_logger.info(f"Moving to {next_point}")
         self._stage.move_absolute(
-            x=int(loc[0]), y=int(loc[1]), z=z - self.autofocus_dz / 2
+            x=next_point[0],
+            y=next_point[1],
+            z=z_estimate - self._scan_data["autofocus_dz"] / 2,
         )
-        return loc + [z]
+
+        return (next_point[0], next_point[1], z_estimate)
 
     @_scan_running
     def _take_test_image_to_calc_displacement(self, overlap):
@@ -473,13 +436,15 @@ class SmartScanThing(Thing):
             f"Based on an overlap of {overlap}, we will make steps of {dx}, {dy}"
         )
 
-        if self.autofocus_dz == 0:
+        autofocus_dz = self.autofocus_dz
+        if autofocus_dz == 0:
             self._scan_logger.info("Running scan without autofocus")
-        elif self.autofocus_dz <= 200:
+        elif autofocus_dz <= 200:
             self._scan_logger.warning(
-                f"Your dz range is {self.autofocus_dz} steps, which is too short to "
+                f"Your autofocus range is {autofocus_dz} steps, which is too short to "
                 "attempt to focus. Running without autofocus"
             )
+            autofocus_dz = 0
 
         # Fix scan parameters in case UI is updates during scan.
         self._scan_data = {
@@ -488,7 +453,8 @@ class SmartScanThing(Thing):
             "max_dist": self.max_range,
             "dx": dx,
             "dy": dy,
-            "autofocus_dz": self.autofocus_dz,
+            "autofocus_dz": autofocus_dz,
+            "autofocus_on": bool(autofocus_dz),
             "start_time": time.strftime("%H_%M_%S-%d_%m_%Y"),
             "skip_background": self.skip_background,
             "stitch_automatically": self.stitch_automatically,
@@ -497,6 +463,7 @@ class SmartScanThing(Thing):
     @_scan_running
     def _save_scan_inputs_jons(self):
         # This should be a method of the scan_data dataclass
+
         data = {
             "scan_name": self._ongoing_scan_name,
             "overlap": self._scan_data["overlap"],
@@ -538,184 +505,11 @@ class SmartScanThing(Thing):
             self._set_scan_data()
             self._save_scan_inputs_jons()
             if self._scan_images_taken != 0:
-                raise RuntimeError(
-                    "_scan_images_taken should be zero before starting scanning"
-                )
+                msg = "_scan_images_taken should be zero before starting scanning"
+                raise RuntimeError(msg)
 
-            # construct a 2D scan path
-            path = [[self._stage.position["x"], self._stage.position["y"]]]
-            # This holds a list of all points where focus succeeded
-            focused_path = []
-            # This holds a list of all points visited
-            true_path = []
-
-            # At the start of the loop, we simultaneously capture an image and move to the next scan point.
-            # We skip capturing on the first run, because we've not focused yet - and also we skip capturing if
-            # it looks like background.
-            while len(path) > 0:
-                ensure_free_disk_space(self._ongoing_scan_dir)
-                self._manage_stitching_threads()
-                loc = self._move_to_next_point(path=path, focused_path=focused_path)
-
-                # Check if the image is background
-                if self._scan_data["skip_background"]:
-                    image_is_sample = self._background_detect.image_is_sample()
-                else:
-                    image_is_sample = True
-
-                # if more than 92% of the image is background, treat it as background and continue
-                if not image_is_sample:
-                    self._scan_logger.info(
-                        f"Skipping {self._stage.position} as it is {round(self._background_detect.background_fraction(), 0)}% background."
-                    )
-                else:
-                    # if not, it's sample. run an autofocus and use the updated height
-                    new_pos = [
-                        [
-                            self._stage.position["x"] - self._scan_data["dx"],
-                            self._stage.position["y"],
-                        ],
-                        [
-                            self._stage.position["x"] + self._scan_data["dx"],
-                            self._stage.position["y"],
-                        ],
-                        [
-                            self._stage.position["x"],
-                            self._stage.position["y"] - self._scan_data["dy"],
-                        ],
-                        [
-                            self._stage.position["x"],
-                            self._stage.position["y"] + self._scan_data["dy"],
-                        ],
-                    ]
-                    for pos in new_pos:
-                        if (
-                            pos not in [sublist[:2] for sublist in true_path]
-                            and pos not in path
-                        ):
-                            path.append(pos)
-
-                    attempts = 0
-                    if self.autofocus_dz > 200:
-                        while True:
-                            jpeg_zs, jpeg_sizes = self._autofocus.looping_autofocus(
-                                dz=self.autofocus_dz, start="base"
-                            )
-                            current_height = self._stage.position["z"]
-                            time.sleep(0.2)
-                            autofocus_success = self._autofocus.verify_focus_sharpness(
-                                sweep_sizes=jpeg_sizes, camera=CamDep, threshold=0.92
-                            )
-                            self._scan_logger.info(
-                                f"We just tested the focus! Result was {autofocus_success}"
-                            )
-
-                            if autofocus_success:
-                                # if there have been successful autofocuses in this scan, find the closest one in x-y
-                                # test if the change in z between them exceeds a ratio (indicating a failed autofocus)
-                                if len(focused_path) > 0:
-                                    nearest_focused_site = focused_path[
-                                        closest(loc, focused_path)
-                                    ]
-                                    result = limit_focus_change(
-                                        nearest_focused_site[0:2],
-                                        nearest_focused_site[-1],
-                                        loc[0:2],
-                                        current_height,
-                                        0.5,
-                                    )
-
-                                # if there haven't been any previous autofocuses, we have to assume this one worked
-                                else:
-                                    result = "accept"
-                            else:
-                                result = "reject"
-
-                            # if the autofocus worked, add the current position to the list of successful locations
-                            if result == "accept":
-                                loc = list(self._stage.position.values())
-                                focused_path.append(loc)
-                                break
-                            if attempts >= 3:
-                                self._scan_logger.warning(
-                                    "Could not autofocus after 3 attempts."
-                                )
-                                break
-                            # if the autofocus was rejected, we return to the height of the closest successful autofocus. not perfect, but better than wandering out of focus
-                            self._scan_logger.info(
-                                "The focus has shifted further than we expect: retrying."
-                            )
-                            self._stage.move_absolute(z=int(loc[2]))
-                            attempts += 1
-
-                    # Acquire the image in a thread, and continue once it's acquired (i.e. leave saving in the background)
-                    if capture_thread:  # wait for the previous capture to be saved, i.e. don't leave more than one image saving in the background
-                        wait_start = time.time()
-                        thread_was_alive = capture_thread.is_alive()
-
-                        # If the capture thread has thrown an exception it will be raised when join is called,
-                        # this will cause the scan to end. If we want to retry captures at a later date
-                        # this is where we will need to catch the IOError or CaptureError from the thread.
-                        capture_thread.join()
-                        time.sleep(0.2)
-                        if thread_was_alive:
-                            wait_time = time.time() - wait_start
-                            self._scan_logger.info(
-                                f"Waited {wait_time:.1f}s for the previous capture to finish saving."
-                            )
-
-                        # increment capure counter as thread has completed
-                        self._scan_images_taken += 1
-                    acquired = Event()
-                    name = f"image_{loc[0]}_{loc[1]}.jpg"
-                    jpeg_path = os.path.join(self._ongoing_scan_images_dir, name)
-                    time.sleep(0.2)
-
-                    # Use ErrorCapturingThread intead of Thread. This will raise errors in the calling
-                    # thread only when join() is called, allowing us to handle this appropriately.
-                    capture_thread = ErrorCapturingThread(
-                        target=self._capture_and_save,
-                        kwargs={
-                            "acquired": acquired,
-                            "jpeg_path": jpeg_path,
-                        },
-                    )
-                    capture_thread.start()
-                    acquired.wait()  # wait until the image is acquired
-
-                # add the current position to the list of all positions visited
-                true_path.append(loc)
-
-                temp_path = []
-
-                for i in path:
-                    if (
-                        distance_to_site(i, true_path[0][:2])
-                        < self._scan_data["max_dist"]
-                    ):
-                        temp_path.append(i)
-                    else:
-                        self._scan_logger.info(
-                            f"Rejected moving to {i} as it is out of range"
-                        )
-                path = temp_path.copy()
-                path = sorted(
-                    path,
-                    key=lambda x: (
-                        steps_from_centre(
-                            x,
-                            true_path[0][:2],
-                            self._scan_data["dx"],
-                            self._scan_data["dy"],
-                        ),
-                        distance_to_site(loc[:2], x),
-                    ),
-                )
-                self.create_zip_of_scan(
-                    logger=self._scan_logger,
-                    scan_name=self._ongoing_scan_name,
-                    download_zip=False,
-                )
+            # This is the main loop of the scan!
+            self._main_scan_loop()
 
         except InvocationCancelledError:
             scan_successful = False
@@ -752,11 +546,189 @@ class SmartScanThing(Thing):
                             exc_info=e,
                         )
 
-        # This is what happens if the scan completes sucessfully or the
+        # This is what happens if the scan completes successfully or the
         # user cancels it.
         self._return_to_starting_position()
         self._perform_final_stitch()
 
+    @_scan_running
+    def _main_scan_loop(self):
+        planner_settings = {
+            "dx": self._scan_data["dx"],
+            "dy": self._scan_data["dy"],
+            "max_dist": self._scan_data["max_dist"],
+        }
+        route_planner = scan_planners.SmartSpiral(
+            intial_position=(self._stage.position["x"], self._stage.position["y"]),
+            planner_settings=planner_settings,
+        )
+
+        capture_thread = None
+
+        # At the start of the loop, we simultaneously capture an image and move to the next scan point.
+        # We skip capturing on the first run, because we've not focused yet - and also we skip capturing if
+        # it looks like background.
+        while not route_planner.scan_complete:
+            ensure_free_disk_space(self._ongoing_scan_dir)
+            self._manage_stitching_threads()
+
+            next_pos_xy, z_est = route_planner.get_next_location_and_z_estimate()
+            new_pos_xyz = self._move_to_next_point(next_pos_xy, z_est)
+
+            capture_image = True
+            # If skipping background, take and image to check if is background
+            if self._scan_data["skip_background"]:
+                capture_image = self._background_detect.image_is_sample()
+
+            if not capture_image:
+                route_planner.mark_location_visited(
+                    new_pos_xyz, imaged=False, focused=False
+                )
+                # Background franction is actually a percentage
+                back_perc = round(self._background_detect.background_fraction(), 0)
+                msg = f"Skipping {new_pos_xyz} as it is {back_perc}% background."
+                self._scan_logger.info(msg)
+                continue
+
+            focused = False
+            if self._scan_data["autofocus_on"]:
+                closest_xyz = route_planner.closest_focus_site(new_pos_xyz[:2])
+                focused = self._try_autofocus(new_pos_xyz, closest_xyz)
+
+            route_planner.mark_location_visited(
+                new_pos_xyz, imaged=True, focused=focused
+            )
+
+            # wait for the previous capture to be saved, i.e. don't leave more than one image saving in the background
+            if capture_thread:
+                self._wait_for_capture_thread(capture_thread)
+                # increment capure counter as thread has completed
+                self._scan_images_taken += 1
+                # Add it to the incremental zip
+                self.create_zip_of_scan(
+                    logger=self._scan_logger,
+                    scan_name=self._ongoing_scan_name,
+                    download_zip=False,
+                )
+
+            name = f"image_{new_pos_xyz[0]}_{new_pos_xyz[1]}.jpg"
+            jpeg_path = os.path.join(self._ongoing_scan_images_dir, name)
+            capture_thread, acquired = self._start_capture_thread(jpeg_path)
+            # wait until the image is acquired
+            acquired.wait()
+
+    @_scan_running
+    def _try_autofocus(
+        self,
+        this_xyz: tuple[int, int, int],
+        closest_xyz: Optional[tuple[int, int, int]],
+    ) -> bool:
+        """
+        Try to perform autofocus and return boolean for if successful
+
+        Args:
+            this_xyz is the current x,y,z position.
+            closest_xyz is the (x, y, z) coordinates of the closest position, this is None
+                if no previous images have been taken or in focus
+
+        Return True on successful autofocus.
+        Return False if failed after 3 tries - the position will be the initial estimate
+        """
+        attempts = 0
+
+        while attempts < 3:
+            attempts += 1
+
+            # Base on first run otherwise we move to estimated_z
+            start = "base" if attempts == 0 else "centre"
+
+            _, jpeg_sizes = self._autofocus.looping_autofocus(
+                dz=self._scan_data["autofocus_dz"], start=start
+            )
+            current_height = self._stage.position["z"]
+            time.sleep(0.2)
+            autofocus_sharp_enough = self._autofocus.verify_focus_sharpness(
+                sweep_sizes=jpeg_sizes, camera=CamDep, threshold=0.92
+            )
+
+            # Not sharp enough, nobe to start and try again
+            if not autofocus_sharp_enough:
+                self._stage.move_absolute(z=this_xyz[2])
+                continue
+
+            # No previous positions to compare against return success
+            if closest_xyz is None:
+                return True
+
+            # Check the change in z-position is acceptable
+            # If the z change compared to the closest focused image exceeds
+            # a given fraction of the xy displacement this indicates failure
+            success = focus_change_acceptable(
+                prev_pos=closest_xyz[:2],
+                prev_z=closest_xyz[2],
+                new_pos=this_xyz[:2],
+                new_z=current_height,
+                fractional_limit=0.5,
+            )
+            # No focus change acceptable return success
+            if success:
+                return True
+
+            # Shifted to far move to start and try again
+            self._stage.move_absolute(z=this_xyz[2])
+            self._scan_logger.info(
+                "The focus has shifted further than we expect: retrying."
+            )
+
+        self._scan_logger.warning("Could not autofocus after 3 attempts.")
+        return False
+
+    @_scan_running
+    def _wait_for_capture_thread(self, capture_thread: ErrorCapturingThread) -> None:
+        """
+        Wait for the capture thread to be complete.
+        """
+        wait_start = time.time()
+        thread_was_alive = capture_thread.is_alive()
+
+        # If the capture thread has thrown an exception it will be raised
+        # when join is called, this will cause the scan to end. If we want
+        # to retry captures at a later date this is where we will need to
+        # catch the IOError or CaptureError from the thread.
+        capture_thread.join()
+        time.sleep(0.2)
+        if thread_was_alive:
+            wait_time = time.time() - wait_start
+            self._scan_logger.info(
+                f"Waited {wait_time:.1f}s for the previous capture to finish saving."
+            )
+
+    @_scan_running
+    def _start_capture_thread(
+        self, jpeg_path: str
+    ) -> tuple[ErrorCapturingThread, Event]:
+        """
+        Start the capture thread.
+
+        Args:
+           jpeg_path, the path to save the image once aquired
+
+        Return the thread and an event that will be set when the image is aquired
+        """
+        acquired = Event()
+        time.sleep(0.2)
+
+        # Acquire the image in a thread, and continue once it's acquired
+        # (i.e. leave saving in the background) Use ErrorCapturingThread
+        # intead of Thread. This will raise errors in the calling thread
+        # only when join() is called, allowing us to handle this appropriately.
+        capture_thread = ErrorCapturingThread(
+            target=self._capture_and_save,
+            kwargs={"acquired": acquired, "jpeg_path": jpeg_path},
+        )
+        capture_thread.start()
+        return capture_thread, acquired
+
     @_scan_running
     def _return_to_starting_position(self):
         self._scan_logger.info("Returning to starting position.")

From 842e196f9b18f7188fc4b5517a18cefb59c63a11 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sat, 12 Apr 2025 01:03:08 +0100
Subject: [PATCH 05/11] Add some basic tests, and do minor fixes based on tests
 and MyPy

---
 .../scan_planners.py                          |  54 ++++++--
 tests/test_scan_planners.py                   | 116 ++++++++++++++++++
 2 files changed, 158 insertions(+), 12 deletions(-)
 create mode 100644 tests/test_scan_planners.py

diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py
index c6ae8501..cecdc654 100644
--- a/src/openflexure_microscope_server/scan_planners.py
+++ b/src/openflexure_microscope_server/scan_planners.py
@@ -19,6 +19,32 @@ XYPosList: TypeAlias = list[XYPos]
 XYZPosList: TypeAlias = list[XYZPos]
 
 
+def enforce_xy_tuple(value: XYPos) -> XYPos:
+    """
+    Used for enfocring that an input is a tuple and is the correct length
+    """
+    if not isinstance(value, (list, tuple)):
+        raise ValueError("2 value tuple expected")
+    if not len(value) == 2:
+        raise ValueError("2 value tuple expected")
+    if isinstance(value, list):
+        return tuple(value)
+    return value
+
+
+def enforce_xyz_tuple(value: XYZPos) -> XYZPos:
+    """
+    Used for enfocring that an input is a tuple and is the correct length
+    """
+    if not isinstance(value, (list, tuple)):
+        raise ValueError("3 value tuple expected")
+    if not len(value) == 3:
+        raise ValueError("3 value tuple expected")
+    if isinstance(value, list):
+        return tuple(value)
+    return value
+
+
 class ScanPlanner:
     """
     A base class for a scan planner.
@@ -29,7 +55,7 @@ class ScanPlanner:
     """
 
     def __init__(self, intial_position: XYPos, planner_settings: Optional[dict] = None):
-        self._initial_position = tuple(intial_position)
+        self._initial_position = enforce_xy_tuple(intial_position)
 
         self._parse(planner_settings)
 
@@ -102,13 +128,15 @@ class ScanPlanner:
         next_location = self._remaining_locations[0]
 
         # If focussed locations exist return closest location, favouring most recent
-        if self._focused_locations:
-            z = self.closest_focus_site(next_location)[2]
-        else:
+        closest_pos = self.closest_focus_site(next_location)
+        if closest_pos is None:
             z = None
+        else:
+            z = closest_pos[2]
+
         return next_location, z
 
-    def closest_focus_site(self, xy_pos: XYPos) -> XYZPos:
+    def closest_focus_site(self, xy_pos: XYPos) -> Optional[XYZPos]:
         """
         Return the xyz position of the closest site where focus was achieved
         to the input xy_position, with the most recently taken image returned in
@@ -135,7 +163,7 @@ class ScanPlanner:
         return self._focused_locations[indicies[-1]]
 
     def mark_location_visited(
-        self, xyz_pos: XYPos, imaged: bool, focused: bool
+        self, xyz_pos: XYZPos, imaged: bool, focused: bool
     ) -> None:
         """
         Mark the location as visited
@@ -146,7 +174,7 @@ class ScanPlanner:
             focused: true if autofocus completed successfully
         """
         # ensure is tuple!
-        xyz_pos = tuple(xyz_pos)
+        xyz_pos = enforce_xyz_tuple(xyz_pos)
         xy_pos = xyz_pos[:2]
 
         # Remove the expected position from the remaining locations list
@@ -196,7 +224,7 @@ class SmartSpiral(ScanPlanner):
         if not planner_settings:
             raise ValueError(invalid_msg + ",".join(expected_keys))
         if not all(keys in planner_settings for keys in expected_keys):
-            raise ValueError(invalid_msg + ",".join(expected_keys))
+            raise KeyError(invalid_msg + ",".join(expected_keys))
 
         self._dx = int(planner_settings["dx"])
         self._dy = int(planner_settings["dy"])
@@ -211,7 +239,7 @@ class SmartSpiral(ScanPlanner):
         return [self._initial_position]
 
     def mark_location_visited(
-        self, xyz_pos: XYPos, imaged: bool = True, focused: bool = True
+        self, xyz_pos: XYZPos, imaged: bool = True, focused: bool = True
     ) -> None:
         """
         Mark the location as visited. Adjust extra poisitons accordingly
@@ -224,7 +252,7 @@ class SmartSpiral(ScanPlanner):
         # First call the base class to update the positions
         super().mark_location_visited(xyz_pos, imaged, focused)
 
-        xy_pos = tuple(xyz_pos[:2])
+        xy_pos = enforce_xy_tuple(xyz_pos[:2])
         if imaged:
             self._add_surrounding_positions(xy_pos)
         self._re_sort_remaining_locations(xy_pos)
@@ -287,7 +315,9 @@ class SmartSpiral(ScanPlanner):
         return np.max(np.abs(displacement_in_moves))
 
 
-def distance_between(current_pos: XYPos, next_pos: XYPos) -> float:
+def distance_between(
+    current_pos: XYPos | np.ndarray, next_pos: XYPos | np.ndarray
+) -> float:
     """
     Calculate the distance between the two xy positions
 
@@ -295,4 +325,4 @@ def distance_between(current_pos: XYPos, next_pos: XYPos) -> float:
     """
     next_pos = np.array(next_pos, dtype="float64")
     current_pos = np.array(current_pos, dtype="float64")
-    return np.linalg.norm(next_pos - current_pos)
+    return float(np.linalg.norm(next_pos - current_pos))
diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py
new file mode 100644
index 00000000..373415d0
--- /dev/null
+++ b/tests/test_scan_planners.py
@@ -0,0 +1,116 @@
+import pytest
+from openflexure_microscope_server import scan_planners
+from copy import copy
+
+
+def test_enforce_xy_tuple():
+    bad_len_vals = [[1], [], (1,), (2, 4, 4), [1, 4, 5, 7]]
+    bad_type_vals = ["hi", 1, {"this": "that"}, {1, 2}]
+    for value in bad_len_vals + bad_type_vals:
+        with pytest.raises(ValueError):
+            scan_planners.enforce_xy_tuple(value)
+
+    assert (1, 6) == scan_planners.enforce_xy_tuple((1, 6))
+    assert (1, 6) == scan_planners.enforce_xy_tuple([1, 6])
+
+
+def test_enforce_xyz_tuple():
+    bad_len_vals = [[1], [], (1,), (2, 4), [1, 4, 5, 7]]
+    bad_type_vals = ["hi!", 1, {"this": "that"}, {1, 2, 3}]
+    for value in bad_len_vals + bad_type_vals:
+        with pytest.raises(ValueError):
+            scan_planners.enforce_xyz_tuple(value)
+
+    assert (1, 6, 2) == scan_planners.enforce_xyz_tuple((1, 6, 2))
+    assert (1, 6, 6) == scan_planners.enforce_xyz_tuple([1, 6, 6])
+
+
+def test_v_basic_smart_spiral():
+    intial_position = (100, 50)
+    planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
+    planner = scan_planners.SmartSpiral(
+        intial_position=intial_position, planner_settings=planner_settings
+    )
+    # Create a planner it shouldn't start complete
+    assert not planner.scan_complete
+    # When we start it should want to stay in the inital pos and have
+    # no z_estimate
+    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
+    assert xy_pos == intial_position
+    assert z_pos is None
+
+    # Try to make imaged with only xy_position
+    with pytest.raises(ValueError):
+        planner.mark_location_visited(xy_pos, imaged=False, focused=False)
+    # scan still not complete
+    assert not planner.scan_complete
+    # if we mark this position as visited but not imaged
+    planner.mark_location_visited(
+        (xy_pos[0], xy_pos[1], 10), imaged=False, focused=False
+    )
+    # scan is now complete
+    assert planner.scan_complete
+
+
+def test_bad_smart_spiral_settings():
+    intial_position = (100, 50)
+    planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
+    with pytest.raises(KeyError):
+        keys = ["dx", "dy", "max_dist"]
+        for delkey in keys:
+            bad_planner_settings = copy(planner_settings)
+            del bad_planner_settings[delkey]
+            scan_planners.SmartSpiral(
+                intial_position=intial_position, planner_settings=bad_planner_settings
+            )
+    with pytest.raises(ValueError):
+        keys = ["dx", "dy", "max_dist"]
+        for badkey in keys:
+            bad_planner_settings = copy(planner_settings)
+            bad_planner_settings[badkey] = "I can't be converted to an int"
+            scan_planners.SmartSpiral(
+                intial_position=intial_position, planner_settings=bad_planner_settings
+            )
+
+
+def test__smart_spiral_next_pos():
+    intial_position = (100, 50)
+    planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
+    planner = scan_planners.SmartSpiral(
+        intial_position=intial_position, planner_settings=planner_settings
+    )
+    # Create a planner it shouldn't start complete
+    assert not planner.scan_complete
+    # When we start it should want to stay in the inital pos and have
+    # no z_estimate
+    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
+    assert xy_pos == intial_position
+    assert z_pos is None
+    z_focus = 10
+    xyz_pos = (xy_pos[0], xy_pos[1], z_focus)
+    # if we mark this position as visited and imaged
+    planner.mark_location_visited(xyz_pos, imaged=True, focused=True)
+
+    # scan is not complete
+    assert not planner.scan_complete
+
+    # Lists should have updated to add the position to histories
+    assert xyz_pos in planner._imaged_locations
+    assert xyz_pos in planner._focused_locations
+    assert xy_pos in planner._path_history
+
+    # remove from planned
+    assert xy_pos not in planner._remaining_locations
+
+    # Add 4 new points to planned
+    assert (100, 0) in planner._remaining_locations
+    assert (100, 100) in planner._remaining_locations
+    assert (150, 50) in planner._remaining_locations
+    assert (50, 50) in planner._remaining_locations
+    assert len(planner._remaining_locations) == 4
+
+    # Now the next location is update
+    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
+    # move in negative x-dir first
+    assert xy_pos == (50, 50)
+    assert z_pos is z_focus

From 91073535a863bdf595e080b3fa55cafe5ca8c752 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 13 Apr 2025 12:52:05 +0100
Subject: [PATCH 06/11] More tests for scan planners

---
 .../scan_planners.py                          |  28 ++-
 tests/test_scan_planners.py                   | 192 +++++++++++++++---
 2 files changed, 186 insertions(+), 34 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py
index cecdc654..24326e8d 100644
--- a/src/openflexure_microscope_server/scan_planners.py
+++ b/src/openflexure_microscope_server/scan_planners.py
@@ -50,13 +50,28 @@ class ScanPlanner:
     A base class for a scan planner.
 
     This should never be used directly for a scan, it should be subclassed.
-    Each subclass should implmenet the methods with NotImplementedError
-    set.
+
+    Each subclass should implement at least the methods with NotImplementedError
+    set:
+    * _parse() - to parse the planner_settings dictionary, saving values to class
+        variables
+    * _intial_location_list() - Sets the list of locations for the scan to follow
+
+    For a simple scan pattern this should be sufficent. For more complex ones that
+    dynanically adjust the path it is suggested to override `mark_location_visited()`
+    calling `super().mark_location_visited()` at the start of the method so that all
+    locations are adjusted.
+
+    When subclassing be sure to use enforce_xy_tuple and enforce_xyz_tuple on any user
+    data before
     """
 
     def __init__(self, intial_position: XYPos, planner_settings: Optional[dict] = None):
-        self._initial_position = enforce_xy_tuple(intial_position)
+        """
+        Set up lists for the path planning, and scan history.
+        """
 
+        self._initial_position = enforce_xy_tuple(intial_position)
         self._parse(planner_settings)
 
         # The remaining (x,y) locations to scan
@@ -84,7 +99,7 @@ class ScanPlanner:
         """
         Return True if there are no locations left to scan.
         """
-        return not (self._remaining_locations)
+        return not self._remaining_locations
 
     def _parse(self, planner_settings: Optional[dict] = None) -> None:
         """
@@ -96,7 +111,10 @@ class ScanPlanner:
         """
         Called on initalisation. Sets the initial list of locations for this scan planner
 
-        For a simple grid scan/snake scan this would be all locations to move to
+        For a simple grid scan/snake scan this would be all locations to move to.
+
+        Note for implementation that this _must_ contain (x,y) tuples, not [x, y]
+        lists or matching errors could occur.
         """
         raise NotImplementedError("Did you call the ScanPlanner base class?")
 
diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py
index 373415d0..a626dfcc 100644
--- a/tests/test_scan_planners.py
+++ b/tests/test_scan_planners.py
@@ -25,6 +25,12 @@ def test_enforce_xyz_tuple():
     assert (1, 6, 6) == scan_planners.enforce_xyz_tuple([1, 6, 6])
 
 
+def test_base_class_not_implemented():
+    intial_position = (100, 50)
+    with pytest.raises(NotImplementedError):
+        scan_planners.ScanPlanner(intial_position=intial_position)
+
+
 def test_v_basic_smart_spiral():
     intial_position = (100, 50)
     planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
@@ -51,56 +57,78 @@ def test_v_basic_smart_spiral():
     # scan is now complete
     assert planner.scan_complete
 
+    # if scan is complete, asking for the next location returns an error
+    with pytest.raises(RuntimeError):
+        planner.get_next_location_and_z_estimate()
+
 
 def test_bad_smart_spiral_settings():
     intial_position = (100, 50)
-    planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
-    with pytest.raises(KeyError):
-        keys = ["dx", "dy", "max_dist"]
-        for delkey in keys:
-            bad_planner_settings = copy(planner_settings)
-            del bad_planner_settings[delkey]
-            scan_planners.SmartSpiral(
-                intial_position=intial_position, planner_settings=bad_planner_settings
-            )
+
+    # Class init should raise error if no planner_settings dictionary set
     with pytest.raises(ValueError):
-        keys = ["dx", "dy", "max_dist"]
-        for badkey in keys:
-            bad_planner_settings = copy(planner_settings)
-            bad_planner_settings[badkey] = "I can't be converted to an int"
+        scan_planners.SmartSpiral(intial_position=intial_position)
+
+    planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
+    keys = ["dx", "dy", "max_dist"]
+
+    # Class init should raise error if planner_settings is missing any key
+    for delkey in keys:
+        bad_planner_settings = copy(planner_settings)
+        del bad_planner_settings[delkey]
+        with pytest.raises(KeyError):
+            scan_planners.SmartSpiral(
+                intial_position=intial_position, planner_settings=bad_planner_settings
+            )
+
+    # Class init should raise error if planner_settings if any value can't be cast
+    # to int
+    keys = ["dx", "dy", "max_dist"]
+    for badkey in keys:
+        bad_planner_settings = copy(planner_settings)
+        bad_planner_settings[badkey] = "I can't be converted to an int"
+        with pytest.raises(ValueError):
             scan_planners.SmartSpiral(
                 intial_position=intial_position, planner_settings=bad_planner_settings
             )
 
 
-def test__smart_spiral_next_pos():
+def test_smart_spiral_first_few_pos():
+    """
+    This test is  VERY long, not really a "unit". It checks step-by-step
+    that data is added correctly for the first few postions in a scan.
+
+    This should catch basic caseses of if the algorithm is updated.
+    """
     intial_position = (100, 50)
     planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
+    # Create a planner
     planner = scan_planners.SmartSpiral(
         intial_position=intial_position, planner_settings=planner_settings
     )
-    # Create a planner it shouldn't start complete
+    # it shouldn't start complete
     assert not planner.scan_complete
     # When we start it should want to stay in the inital pos and have
     # no z_estimate
-    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
-    assert xy_pos == intial_position
-    assert z_pos is None
+    xy_pos1, z_pos1 = planner.get_next_location_and_z_estimate()
+    assert xy_pos1 == intial_position
+    assert z_pos1 is None
+    # Set a focus value
     z_focus = 10
-    xyz_pos = (xy_pos[0], xy_pos[1], z_focus)
-    # if we mark this position as visited and imaged
-    planner.mark_location_visited(xyz_pos, imaged=True, focused=True)
+    xyz_pos1 = (xy_pos1[0], xy_pos1[1], z_focus)
+    # if we mark this position as visited, imaged, and focused
+    planner.mark_location_visited(xyz_pos1, imaged=True, focused=True)
 
     # scan is not complete
     assert not planner.scan_complete
 
     # Lists should have updated to add the position to histories
-    assert xyz_pos in planner._imaged_locations
-    assert xyz_pos in planner._focused_locations
-    assert xy_pos in planner._path_history
+    assert xyz_pos1 in planner._imaged_locations
+    assert xyz_pos1 in planner._focused_locations
+    assert xy_pos1 in planner._path_history
 
     # remove from planned
-    assert xy_pos not in planner._remaining_locations
+    assert xy_pos1 not in planner._remaining_locations
 
     # Add 4 new points to planned
     assert (100, 0) in planner._remaining_locations
@@ -109,8 +137,114 @@ def test__smart_spiral_next_pos():
     assert (50, 50) in planner._remaining_locations
     assert len(planner._remaining_locations) == 4
 
-    # Now the next location is update
-    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
+    # Now the next location is updated
+    xy_pos2, z_pos2 = planner.get_next_location_and_z_estimate()
     # move in negative x-dir first
-    assert xy_pos == (50, 50)
-    assert z_pos is z_focus
+    assert xy_pos2 == (50, 50)
+    # Focus set from closest point
+    assert z_pos2 is z_focus
+
+    xyz_pos2 = (xy_pos2[0], xy_pos2[1], z_focus)
+    # if we mark this position as visited, imaged, and NOT focused
+    planner.mark_location_visited(xyz_pos2, imaged=True, focused=False)
+
+    # Check this position remove from planned
+    assert xy_pos2 not in planner._remaining_locations
+    # Check original position not re-added
+    assert xy_pos1 not in planner._remaining_locations
+
+    # 3 old points still planned
+    assert (100, 0) in planner._remaining_locations
+    assert (100, 100) in planner._remaining_locations
+    assert (150, 50) in planner._remaining_locations
+    # Add 3 new points to planned
+    assert (0, 50) in planner._remaining_locations
+    assert (50, 100) in planner._remaining_locations
+    assert (50, 0) in planner._remaining_locations
+    assert len(planner._remaining_locations) == 6
+
+    # Now the next location is updated
+    xy_pos3, z_pos3 = planner.get_next_location_and_z_estimate()
+    # move in negative y-dir first next
+    assert xy_pos3 == (50, 0)
+    # Focus still set as before
+    assert z_pos3 is z_focus
+    # Check that the closest focus site to pos3 is pos 1 as
+    # pos 2 is not focussed
+    assert planner.closest_focus_site(xy_pos3) == xyz_pos1
+
+    new_z_focus = 20
+    xyz_pos3 = (xy_pos3[0], xy_pos3[1], new_z_focus)
+    # Finally check that if this is focused...
+    planner.mark_location_visited(xyz_pos3, imaged=True, focused=True)
+    # ... then the new 4th point ...
+    xy_pos4, z_pos4 = planner.get_next_location_and_z_estimate()
+    # ...(100, 0)...
+    assert xy_pos4 == (100, 0)
+    # ... and it should get its focus from the most recent point as this was focussed
+    assert z_pos4 is new_z_focus
+    assert planner.closest_focus_site(xy_pos4) == xyz_pos3
+
+
+def test_scan_stops_on_max_dist():
+    intial_position = (0, 0)
+    planner_settings = {"dx": 100, "dy": 100, "max_dist": 1000}
+    # Create a planner
+    planner = scan_planners.SmartSpiral(
+        intial_position=intial_position, planner_settings=planner_settings
+    )
+    while not planner.scan_complete:
+        xy_pos, _ = planner.get_next_location_and_z_estimate()
+        xyz_pos = (xy_pos[0], xy_pos[1], 0)
+        planner.mark_location_visited(xyz_pos, imaged=True, focused=True)
+
+    # Estimate of radius = 10 scans, pir^2 = 314 images. In reality it works out as
+    # 317
+    assert len(planner._path_history) == 317
+
+
+def test_mark_wrong_location():
+    """
+    This test is  VERY long, not really a "unit". It checks step-by-step
+    that data is added correctly for the first few postions in a scan.
+
+    This should catch basic caseses of if the algorithm is updated.
+    """
+    intial_position = (100, 50)
+    planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
+    # Create a planner
+    planner = scan_planners.SmartSpiral(
+        intial_position=intial_position, planner_settings=planner_settings
+    )
+
+    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
+    # Trying to mark the wrong location as visited raises error
+    wrong_xyz_pos = (xy_pos[0] + 1, xy_pos[1], 0)
+    with pytest.raises(RuntimeError):
+        planner.mark_location_visited(wrong_xyz_pos, imaged=True, focused=True)
+
+
+def test_closest_focus_wth_large_numbers():
+    """
+    The number of steps gets very large in reality runs some tests to check
+    that everything works well with huge numbers of steps
+    """
+    intial_position = (0, 0)
+    # Set this up, but we won't use the settings
+    planner_settings = {"dx": 10000, "dy": 10000, "max_dist": 100000}
+    # Create a planner
+    planner = scan_planners.SmartSpiral(
+        intial_position=intial_position, planner_settings=planner_settings
+    )
+    # Directly overwrite the private focussed locations list for test
+
+    # For two points 1m points away it should choose the last as they are equal
+    planner._focused_locations = [(1000000, 0, 0), (0, 1000000, 0)]
+    assert planner.closest_focus_site((0, 0)) == (0, 1000000, 0)
+    # Try similar
+    planner._focused_locations = [(1234567, 0, 0), (-1234567, 0, 0)]
+    assert planner.closest_focus_site((0, 0)) == (-1234567, 0, 0)
+
+    # Make the first point 1 step closer
+    planner._focused_locations = [(1234566, 0, 0), (-1234567, 0, 0)]
+    assert planner.closest_focus_site((0, 0)) == (1234566, 0, 0)

From a0b6dbdfcefeb9cd54a533628dfef5b3765ecc71 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 13 Apr 2025 17:05:28 +0100
Subject: [PATCH 07/11] Add weay to access a copy of the data from a
 ScanPlanner

---
 .../scan_planners.py                          | 31 +++++++++++++-
 tests/test_scan_planners.py                   | 40 +++++++++----------
 2 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py
index 24326e8d..d6d94e6a 100644
--- a/src/openflexure_microscope_server/scan_planners.py
+++ b/src/openflexure_microscope_server/scan_planners.py
@@ -8,6 +8,7 @@ subclassing the ScanPlanner
 
 from typing import TypeAlias, Optional
 import logging
+from copy import copy
 
 import numpy as np
 
@@ -101,6 +102,34 @@ class ScanPlanner:
         """
         return not self._remaining_locations
 
+    @property
+    def remaining_locations(self) -> XYPosList:
+        """
+        Property to access a copy of the remaining_locations
+        """
+        return copy(self._remaining_locations)
+
+    @property
+    def imaged_locations(self) -> XYZPosList:
+        """
+        Property to access a copy of the imaged_locations
+        """
+        return copy(self._imaged_locations)
+
+    @property
+    def focused_locations(self) -> XYZPosList:
+        """
+        Property to access a copy of the focused_locations
+        """
+        return copy(self._focused_locations)
+
+    @property
+    def path_history(self) -> XYPosList:
+        """
+        Property to access a copy of the path_history
+        """
+        return copy(self._path_history)
+
     def _parse(self, planner_settings: Optional[dict] = None) -> None:
         """
         Parse any settings sent to this planner and store them if needed.
@@ -167,7 +196,7 @@ class ScanPlanner:
 
         # must be float64 (double precision) to deal with the huge numbers involved!
         current_pos = np.array(xy_pos, dtype="float64")
-        path_pos = np.asarray(self._focused_locations, dtype="float64")[:, :2]
+        path_pos = np.array(self._focused_locations, dtype="float64")[:, :2]
 
         # Use linalg.norm to calculate the direct distance bweween the points
         # Note linalg.norm always used float64
diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py
index a626dfcc..3cb3e566 100644
--- a/tests/test_scan_planners.py
+++ b/tests/test_scan_planners.py
@@ -123,19 +123,19 @@ def test_smart_spiral_first_few_pos():
     assert not planner.scan_complete
 
     # Lists should have updated to add the position to histories
-    assert xyz_pos1 in planner._imaged_locations
-    assert xyz_pos1 in planner._focused_locations
-    assert xy_pos1 in planner._path_history
+    assert xyz_pos1 in planner.imaged_locations
+    assert xyz_pos1 in planner.focused_locations
+    assert xy_pos1 in planner.path_history
 
     # remove from planned
-    assert xy_pos1 not in planner._remaining_locations
+    assert xy_pos1 not in planner.remaining_locations
 
     # Add 4 new points to planned
-    assert (100, 0) in planner._remaining_locations
-    assert (100, 100) in planner._remaining_locations
-    assert (150, 50) in planner._remaining_locations
-    assert (50, 50) in planner._remaining_locations
-    assert len(planner._remaining_locations) == 4
+    assert (100, 0) in planner.remaining_locations
+    assert (100, 100) in planner.remaining_locations
+    assert (150, 50) in planner.remaining_locations
+    assert (50, 50) in planner.remaining_locations
+    assert len(planner.remaining_locations) == 4
 
     # Now the next location is updated
     xy_pos2, z_pos2 = planner.get_next_location_and_z_estimate()
@@ -149,19 +149,19 @@ def test_smart_spiral_first_few_pos():
     planner.mark_location_visited(xyz_pos2, imaged=True, focused=False)
 
     # Check this position remove from planned
-    assert xy_pos2 not in planner._remaining_locations
+    assert xy_pos2 not in planner.remaining_locations
     # Check original position not re-added
-    assert xy_pos1 not in planner._remaining_locations
+    assert xy_pos1 not in planner.remaining_locations
 
     # 3 old points still planned
-    assert (100, 0) in planner._remaining_locations
-    assert (100, 100) in planner._remaining_locations
-    assert (150, 50) in planner._remaining_locations
+    assert (100, 0) in planner.remaining_locations
+    assert (100, 100) in planner.remaining_locations
+    assert (150, 50) in planner.remaining_locations
     # Add 3 new points to planned
-    assert (0, 50) in planner._remaining_locations
-    assert (50, 100) in planner._remaining_locations
-    assert (50, 0) in planner._remaining_locations
-    assert len(planner._remaining_locations) == 6
+    assert (0, 50) in planner.remaining_locations
+    assert (50, 100) in planner.remaining_locations
+    assert (50, 0) in planner.remaining_locations
+    assert len(planner.remaining_locations) == 6
 
     # Now the next location is updated
     xy_pos3, z_pos3 = planner.get_next_location_and_z_estimate()
@@ -200,7 +200,7 @@ def test_scan_stops_on_max_dist():
 
     # Estimate of radius = 10 scans, pir^2 = 314 images. In reality it works out as
     # 317
-    assert len(planner._path_history) == 317
+    assert len(planner.path_history) == 317
 
 
 def test_mark_wrong_location():
@@ -217,7 +217,7 @@ def test_mark_wrong_location():
         intial_position=intial_position, planner_settings=planner_settings
     )
 
-    xy_pos, z_pos = planner.get_next_location_and_z_estimate()
+    xy_pos, _ = planner.get_next_location_and_z_estimate()
     # Trying to mark the wrong location as visited raises error
     wrong_xyz_pos = (xy_pos[0] + 1, xy_pos[1], 0)
     with pytest.raises(RuntimeError):

From 2ced8486959eaef44b1b8054a180f14114dcdd1c Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Sun, 13 Apr 2025 18:01:06 +0100
Subject: [PATCH 08/11] Add functionality for testing the scan planner on a
 given sample shape

---
 .gitignore                               |   4 +
 pyproject.toml                           |   3 +
 tests/__init__.py                        |   0
 tests/test_scan_planners.py              |  13 +-
 tests/utilities/__init__.py              |   3 +
 tests/utilities/example_smart_spiral.pkl | Bin 0 -> 13513 bytes
 tests/utilities/scan_test_helpers.py     | 196 +++++++++++++++++++++++
 7 files changed, 217 insertions(+), 2 deletions(-)
 create mode 100644 tests/__init__.py
 create mode 100644 tests/utilities/__init__.py
 create mode 100644 tests/utilities/example_smart_spiral.pkl
 create mode 100644 tests/utilities/scan_test_helpers.py

diff --git a/.gitignore b/.gitignore
index 02c38a88..4f6925c9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -84,3 +84,7 @@ openflexure_microscope/cobertura.xml
 
 # web app build
 /src/openflexure_microscope_server/static/
+
+# Files created by test utilities
+/tests/utilities/*.pstats
+/tests/utilities/*.png
diff --git a/pyproject.toml b/pyproject.toml
index 747efe26..53ec72fe 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -35,6 +35,7 @@ dev = [
   "mypy-gitlab-code-quality",
 #  "pytest-gitlab-code-quality", # pytest version constraint clashes with labthings
   "pytest",
+  "matplotlib~=3.10"
 ]
 pi = [
   "labthings-picamera2 == 0.0.2-dev0",
@@ -81,6 +82,8 @@ addopts = [
 
 norecursedirs = [".git", "build", "node_modules"]
 
+pythonpath = ["."]
+
 [tool.ruff.format]
 # Use native line endings for all files
 line-ending = "native"
diff --git a/tests/__init__.py b/tests/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py
index 3cb3e566..e4699d91 100644
--- a/tests/test_scan_planners.py
+++ b/tests/test_scan_planners.py
@@ -1,7 +1,9 @@
 import pytest
-from openflexure_microscope_server import scan_planners
 from copy import copy
 
+from openflexure_microscope_server import scan_planners
+from .utilities import scan_test_helpers
+
 
 def test_enforce_xy_tuple():
     bad_len_vals = [[1], [], (1,), (2, 4, 4), [1, 4, 5, 7]]
@@ -186,7 +188,7 @@ def test_smart_spiral_first_few_pos():
     assert planner.closest_focus_site(xy_pos4) == xyz_pos3
 
 
-def test_scan_stops_on_max_dist():
+def test_smart_spiral_stops_on_max_dist():
     intial_position = (0, 0)
     planner_settings = {"dx": 100, "dy": 100, "max_dist": 1000}
     # Create a planner
@@ -248,3 +250,10 @@ def test_closest_focus_wth_large_numbers():
     # Make the first point 1 step closer
     planner._focused_locations = [(1234566, 0, 0), (-1234567, 0, 0)]
     assert planner.closest_focus_site((0, 0)) == (1234566, 0, 0)
+
+
+def test_example_smart_spiral():
+    _, planner = scan_test_helpers.example_smart_spiral()
+    expected_planner = scan_test_helpers.get_expected_result_for_example_smart_spiral()
+    assert planner.path_history == expected_planner.path_history
+    assert planner.imaged_locations == expected_planner.imaged_locations
diff --git a/tests/utilities/__init__.py b/tests/utilities/__init__.py
new file mode 100644
index 00000000..275ec31c
--- /dev/null
+++ b/tests/utilities/__init__.py
@@ -0,0 +1,3 @@
+"""
+This directory contains utitlities that help with testing and debugging
+"""
diff --git a/tests/utilities/example_smart_spiral.pkl b/tests/utilities/example_smart_spiral.pkl
new file mode 100644
index 0000000000000000000000000000000000000000..5d5727d87df59a640d40cb8366f60a0e22d91b1f
GIT binary patch
literal 13513
zcmZo*ox0D20StPy^9xe*(sEKON{dqCb2F2R@{5!63sU2YQ;W({i}Z?<6Z7H=auW0M
zQj3bG^l%5~CKi<h7i1PC=1iG9MYC~A?G%k3!T8L)%#zH+ocMzL;>?oF{JbgN4BiZF
zQ+k-=Q!1wT?qLEkD!~lS_}s*b_>|1zk||yb+87ynMB<B5a}zW3GV{{obMliDLE4L_
z#7+U3nwyxOnt~##0W#Q|p?!*%%isV1K^!O(%5R_I%>ZZm?qNb^Lsa|jVe;L>1Tw;#
z0m=X?g0VY({r`{5hUoGF34_&vbV7I_Autc*6tED8=gk0Tg6xMW@~vP+h*z+}jq<Hv
zh1lR*!3wn&!~%)7g8~dJ5Aqe10oDj(gA9kU&wT&?AB7F^q!&mWVgr~7)(>(gga<MK
z!Gox6p8|F%SOHiL>_3>CHv?P(<Ug1&$bWFQ2?s)r2?y9TkU_pC9B}J>O*kN~@-^Xr
z`P+m8>H&}#NWOgvC@i3?JxpLd;IM!QfenKSf-QluK@Nej1-}0OkHUrotru7v8b%;C
z#5G`Hu(==yLwN8o0x5%g3?c^-0;>dB2^IqLKw1zyaQMMgdNaT!K;Z`y28AD-ZNh<2
z^MMP64YCJhh3^M0u$N(MkR4Dq#3#NVxM1P>feRL{AGn}F0h0Z|1=e7~;R|I|u)<h-
zn4m0g1}Fm@CNMV0doVW0K`=JR12A^Tr~m&^*w8owi$mi8%!bARhz)VM7l;YA59B@w
z4`d^P2eK2v1KEn;fi#18AYrg(Fb|{!!2=nG;DN#p&hut~GePkKGY%9#aJC5tLd^#*
z1bYoH3LE4CkmbH>c;U$g#0J?869?H1WkY=LyM`AQZ)<pA@wSE+7H?~Kp@9lgw}uz2
z^#hkLlx4yJV^y%iSbLbDEN=!V101$6HpnqBHYku_Y>?w&Y>>xc>>2O>|3_g%5|0;H
z92&=9HZ+dGY-k*V*brZWm4n>@@&SYgatVS5atne7at(q9au0$BF|~aPIPAejf#pD2
z5Im4^2p-5P1P>GkaGo~<oC%75nBk!KhqFyM5NbYfA=qnp5$p^B6gJGsz8M0D{G1_x
zNEaCbi2R%(01q193;{@p`DO^f(nW><EL~&>z|uvA04!Z(2tcC;qywb8eF`Y8Kv^HS
zz$Ug&0hO^(u?kk0*d8V*%bNkp06PT61_c|84GK^g8x+(qHprteHYmVg>^pD%|3_g%
zlB^e49GZr}Y-kz+v!Q7S%!Z~R5E~LAV69;Hf#Vw_0S`im9LS9bIgl$6JdisPJdjHf
zJdjaf9!MB$6qpCng5ZISL-0UWA$XvGLh!)p3udM_16%@>zF@+j^aW>|a3Iut;6kw1
z@FLh50tmK>FbW&wYLJ6{RfNHL0>%cp7{&&<7sdv;7RCm-70QN$vagCTEZwOH!_u9K
zFf84v2*c8yiZCqQsR%<O8l+c67;IdIfG?D_h8M>Azy)KOaKKo5n4m0g1}Fm@!!S0;
zsW3Lk3otgwYcMt_nqX{DP{7!r;DxajUj6@%!iHo8FR(Z?WrEqzlnG`-Qzn=VO_^Xe
zG-ZO=kT3%q19msa?+_ly<p>_g?Fb&o^#~rw{Rke&2M8X>4+tK_#`Y=T*auq!mIG-)
z@Ib~Pcp$3~JdiUGJdhI+JWxu2^Sl}0Oi-$YSqw_GaJC5tLd^#*1bYoHf}J6NV5<lt
z*bhWe*f4MUJ`hEeq7Ot7sri8@q7;1~iYP@Nh{B5ikY11<KtbaBKok-)z7IrUsri8@
zEHyt6g{9^PqOjEbKopjmABe(I^8-<6k^z|lGO>LMD1}2=83JHiAf+u->;o56%$otq
z0H+EV8x-I$HYh4!Y)~Y@*r4cxu|W|FV}pVS#s)<JjGglQ|9=!VB-?p`#i6Mj%!a0N
zFdLf6!E9(M2eYB69L$EMau6F5mS7{n{s9Fsga`5wf(P;wf(P;yf(P;!f(P;$f(P;&
zm<RG0*kCXZB#hvJv><pO;}ATMRR|u)83-Q8i3lDjg}`~<3~(kWl|#G=?t~!3O*jzj
z4_pZL8eRlDLjb{65k|1*NPu%H$TPlkBoHZkjszlQ&yhf+>^TyMls!iRk+SDVz=Pd)
zjszq&eCJ5OQuZ7PSjwIw0ZZ9)Bw#6fjsz@a&yj$o>^Ty!ls!iRns`9wfy{270!r;r
zmWnXgzV<1gwgpsd4KGy8n*quII}XMM1viWhiU=4R6h$yLDDq%zP_)9>pa_StK}i9|
z21N{v4N~K^;t6OlpnVEBND(}cxd<M}Mg$M!76cFEN(2v_C}3uKGr%Q4i2^1JD*E7T
z6Apx$4_pX#gf!Si;0TY9MnrOiG$N8Cq!E!EA&rRS2x)LQLiIu;IYJs1$q~}9NRE((
zMRJ5RERrLnVUZjm4U6OmX;>sjNW&sILK+&5z7f*C5z=7G=19O;4@6-s6=4`FLjcBF
z!wY43Ge8+&2gBH)Ace6(femAWq5;MRMG%Y)j(CU@z_|n_4vu)JI4HoOUIYg+)Z-8~
zw6FrJffiOEHYDW0O2F;|`4Pecxe&nvxe>twxe~<lX7Ck}12MpQVQi327#pMy#s=wv
zvB7?W>IM4^$_6D^m>RI(pyD9CUT+>k6EetD1P^2%f(K5XFqPg6a0yWIgbDkK$RU!O
zh#VrhiO3<6n}{4DxrxYueF*X@*mY0_EV+ru!IGPZ95mE@MdYAy0b)U_6A%kh$$(gp
zst3e^)`uc;u;eBp2aR8lEF=U#Nf64KBLQ{;r0ju;sR%>G!0z<Dpa60q*!?gzD9m7N
zP-w!~;Fy4_2gd}I4UP#Y8ypi5w(kW6nA<KWz}$O50hVPhD8RDO1qE2vx}X5dZWk0_
zVRAtM7ETuwU}1Mb0T!Mht3V#`y`TX04~z|pX&4*i9~c|tA27Q|84-Uy%3%M)#6jr-
z%7!F*-yUU{eLc!BH}ojO+|{EDb8C+>%>6yeFhBJu!~EH!4D)-BGBiBEZuad_23ZGo
z1B?xFGmH&#GmH(61E_j%96;INIDoQ2?)UXjLzoF=gUy7p!Dd3)U^Ag?u$fRc*h~o9
z*Fz2F9}hK{A3fAy{`OFVg@uP2EPOoFU||L_0^t}IO)wkm7$_U;7$_U;7$_U;7$_So
zen1<n7Zka^5O!*hP<&c`a%nO80Cxso27iV?hG2$JhH!>RhG>RZhIocVhGd3RhIEEZ
zhHQphhJ1!XhGK?NhH{2VhH8ddhI)oZhGvFVhIWQdhHi#lhJJ=YhGB+LhH-{ThG~Xb
zhIxiXhGm9ThINKbhHZvjhJA)ZhGT|PhI58XhHHjfhI@ubhG&LXhIfWfhHr*nhJQvt
zMqoxzMsP+*MrcM@MtDX<Mr1}*Ms!9@Mr=l0Mtnv>Mq)-%Msh|<MruY{MtVj@MrKA<
zMs`L{Ms7x4Mt(*?Mqx%#MsY?-MrlS_MtMd>MrB4-Ms-F_Mr}r2Mtw#@Mq@@(Msr3>
zMr%e}Mtep_MrTG>Mt4R}MsG%6Mt{bHjENbOGA3tC$(WikEn|AdjEtEXvodC9%*mLW
zF)w3&#)6E68H+L&XDrEBnz1ZndB%#2l^LrtR%fiqSevmfV|~VkjExzaGB#&y$=I5)
zEn|Dej*OidyE1lX?8(@hu`gqP#(|838HX|sXB^2mnsF@Sc*cp0lNqNnPG_9SIGb@U
z<9x=2jEfnUGA?Ia$+((vE#rE|jf|Taw=!;L+{w6`aWCV3#)FK98ILj^XFSPxn(-{-
zdB%&3ml>}zUT3_?c$@Jq<9)`5jE@<gGCpT~$@rS_E#rH}kBpxgzcPMj{K@#6@h{_l
z76T&#Llz^5U;+`$Ac6%%u!0CS5Wx;2I6wp^h~NSd+#rGnMDT(LJ`lkVA_PE$AcznG
z5yBus1Vo5}2r&>L4k9E#gd~WN0uj<6LIy<0f(SVfArB%HK!hTQPy!LkAVLL1sDcPJ
z5TOntG(dzVh|mHN+8{y)MCgJDJrJP}A`C!;A&4*n5yl|G1VosE2s0324k9c-ge8cu
z0uk0A!Ujaxf(SbhVGkl4K!hWRZ~_s|Ai@PixPk~b5aA9YJV1mei0}dt-XOvUMEHUT
zKM>&$A_71}AczP85y2oL1Vn^_h%gWl4k98zL?noa0uj+5A_hdnf`~W}5f35~Ktv*l
zNCFYbAR+}sq=JYv5RncdGC)Koh{ysF*&reZMC5{qJP?r&A__o6A&4jf5yc>)1VogA
zh%yjS4k9W*L?wu*0uj|9q6S3Nf`~d0Q4b;-Ktv;mXaW(<Afg3Cw1S8>5YY}IIzU7x
zi0A?l-5{a|MD&7)J`m9lA|`-{i6CMUh?oo_rhtg4AYvMbm<}RlfQXqOVit&)4I<`%
zh`AtQ9*CF^A{Ky%g&<-Ph*%6FmVk(*AYvJaSPmjqfQXeKViky34I<Wnh_xVM9f(*D
zA~t}CjUZwZh}aAwwt$GOAYvPc*bX9gfQX$SVi$<m4I=h{h`k_UABflwA`XCvgCOD%
zh&T))j(~`xAmSK^I1VCCfQXYI;uMHD4I<8fh_fK#9Edm%A})Z4iy-0>h`0<Qu7HTE
zAmSQ`xDFz2fQXwQ;ueUw4I=J<h`S)-9*DRPA|8N<haln+h<FSlo`8s_AmSN_cn%_7
zfQXkM;uVN^4I<uvh_@i(9f)`jB0hkKk09a`i1-X5zJQ3YAmST{_zog|fQX+U;unbc
z4I=)4h`%7>ABgx5A{dxJ<sTCRLuwCid_iJKMtnwQaY=qr<&@Yd;3?iV*aR<>0d8-!
z!6tXxKr^FIA#i047VzD}1X2N}z>*LaxY7klf!isdX=yM6tfCD%)d^AqvkAh6PUC_N
z2CHua%|#-yK#di!7|2A>G$qIi5M99v*5nJKy&1q1*e(bQWIBWeZb*O>f!lKsw}Qk#
z=7EL4y4t2d*dTo{Hb@oB<xn=rHDHCH5P&j3-2sR+sILlTL8oThK;!=~-$18j!D1lW
zK~n=@-+(7%!NR^K9AM8vXz)}l$R;os>`n*^<Vpw&WF3SBZX|)UgZm&5zk|g<R)EDo
zc7hpTliH?0*dViDY>;U%Hb^flB%o}NQm`gi974pQQ?20WfJ%cx0wM{TjE1tHQ><-M
zK$EHvt3WJ}sSq(x=z>`wmx88FK~4wJpy&h9An$?b4_uJY0@L8>RIuf~AU=3173x5+
z066#|ERdxT7RX5u7P#RE(g^OyLBbL&2C@_)266_N1#%ad0SiHpLa_O5&}lJ{4KOyy
z4j3Dx5EimfHb^PhFjyLcii1J{Dh{0_1$z!E1PWP*LQu#;S<uPRHqZ<dOx+q@SX@A-
zMIo}#&|bp}^#ZuD4weFE4bT)8$m1Xy9NOTCP>B2*UU202f@$y+C?u_d`QYhKa9Ra%
z!G?nsf<p(w0(lt10)-5O1@Zud1#ZlP^@3U87y+|D&HxYZK+-f=3>+gM7C1&g46u!D
z(2*&KC&8ManH0hX*$YzxvKht(832nZC>x{{tO=F}pyD8-VdCJJf`~(>Ga)er5dy^&
zlm(s01P32fGiZVuDhr*$Yy*w<!D0_OeF+f*1ssG0vK_(#*$rlaLI5;O2nrGq4URqV
zge6!SoXA1qpr8fO83K^l1k>PYN=RM;^TAV;U{Cmh_~7YDSY84NKw=Zj0yzq-2%O9y
zEKrz2SRk)KSm0hFSO=H|at37F2^_;<F>nlnSs=$lWI^5mvq1g>Ghi_cQV4cO8+1Yj
z<PsPg6m2jz$TcuF$UQJN$N*T(L)jpuU`t?m6e<of8YT`3dl(xO`cO7>q7fWuP$5vv
zLks}rQ78*K!3Yj~m^$d>VjF0F116>-3`-%<X+?-E#ABeTMv$8zVjvemSRnU+S)kAY
zO_qRy4@83k6hwo97({~t9Ylko0Yrl$21Kg}LsAQv22UM=6RR(n51uxJ<aIC~Jb4Jt
z>mV*TUcd^$u>xU%90_5891dZD0vW;r#Rh~03LFRvJTe3}56l8N3_Mo_DILIKAjd<*
zK;D6{Kpup!Kwbv3Kw$u8fL+!$1;T~|7K9CQ9ZVeLJ{TM1LKqw5Mi?99N*EjD5?D%t
zvO!A02E)n_s5r=Im^jEv7#rji7#kFGP&Rb(50a7~Mu1Whlm(sk14j!~1E>sv%0egn
zz-a&`3!Ut11I;YJQWtch4<ZKfE@%P|<Wh(j$ej=t$dwQl$c<nYC}cs?6ri96(V)Nw
z(V*x6(Vz$d(cshtp3nnp14l4O{DCOgfxciGJdp>^4iG+g5)V=qf#vrwfyKd7dayDG
zBmqffU>3-+U`3EJ2rLHj6hsUZEf5wc{va$+a6wq$$qld$FbkaWz$|de1G7Nh0M9Cc
z3sDFQ<W&d@<ar1S6dYg{D15*SSjqz#19orQ6o@#;#V|G`&>`X=SHr|X?uM~JE{Cx}
zZil5pkioE22xWt`fenMDLZ~>%8kji9dKeoNzc4l^hM{ce<QyawLW}^VLMRJ5EeA=3
zP%-F~9Jm018UiY(pt27{VPbP6U?~$iLDx0~Ha!PnK>`6J268h*4CG=63*=r13*=fb
z3l!d<2{cdyfM`&ZfM`%8foO1g1W&|)m4hoJkT@u^K{P1(LG&C6NIC`6;K?_zXMDkY
z@WdO~!(cvm`VE#IK?=a}3zh=6H6SdI2Ounvrywj)1VLD!ScR}aaSCC9f(y(7t(*m$
z3lf2b9XQ~?vnjAh2Ti%aDlsS<WD`sc$bJ|b6ofD~C=g+6P%uK-(8)JQq(h7YMLLuP
zopu9<0aOF1=z+>+2*AWtgkh`*X;^^+otkT#;tQRagRnsEgRnrZgRnqugRnp@1GB&p
z44#YwbHOb@(EJ5Bg2B^p5OGjW1&M?5E{Fz4FnIb6A`c2rka&bNBq4xl@U$DGKmzl@
zQ*Mv~3C#b%<qM+0({8W`2MItT9Lxea9;^sb0D;9o(Et$xM>tpv9N}OVIKsg!Q1n97
zfdUd7_2A%x6i;9=kT)P=Adf*<Ag@ALpwtavfr10V0_6Y*3zR3oEKnGN8L&bLWHi_h
zZBroPAYZ`PkcfeZgM0!L2l)lY2Kfe@RKU&$Pu;=7M??<VoACv+Kqf<2Aafxskf{(B
z$V><eWFmwG4j-^yaQJ{(pvg8!5e#O5eG6uReG6uReG6iNeG6j1d<$knk`{Od51dKc
zra;&rSHW@=NFgjoLD?Xuf)&D=g-~&jw_xHRqhV}Nx`nc#^M&AGf(n6h6vO~fvk=Pq
zzy*^Pk%Kk-pmU0CQ$Vwe5GR6IAX_0~AUh!}kd0s#$lstjK2Sh_Xi(6AXi#8*Xi#DS
z(V*l5qCp7?M1ztThz1vh;CVx^ap0H*iGv~llykr|c)k!)RD${7xk5-$3Ffci1xF&N
z1OxHC8GJ7&z)}X71#&n<4CEOI3*<ov3l!WC7C2>qb%0X_m<3K5U=}!KfLNfGE0}&D
z3f2cI%fb8zX$W6L4nkj0fH(|1<=8d_H1!BB62Q}q5I#5{z*CJ7J~*Jj(~J;4I3U4O
zj1WFJph44%U@kPZfu<QjeuRjD{0L!z{0L!z{0K=D2KWE}2Q{L-APlexFgC~x7#m~?
zj14je%J%J11{XSQQ$Q?`A_xnl6~cn14bZF~SOC(&2hUJK_+T%AXD1<iu$RFzlMp^Q
zNWim_5I#7_dX&Lw1MFPi9%Wc|2eUw~fQW${3Sogf3}JyI6|4>%sbCg3Qb8<l28be%
zqkTQpV8()3U}M27u(4nk*jO+NY%G`sHWthR8w+B2Gk^^P%};_|@1X`RUO{}Y@4)kx
zZBsyeu<yZhmJmKTbinhK5I#7<L35R0F4+GN7Bs{?)SyL+FK9}#ZHh08CM3jJG$HjC
zizaw<2_z2AjVzkrz72>E%8g(XeOWYNUI(+lUI(+lUI(+lUI(+lUI(+lUI(+lVh6OL
jjX+;83#=E+0-FbBfm{ud1qV4otVR!}7sN^}P0|AZUrHwK

literal 0
HcmV?d00001

diff --git a/tests/utilities/scan_test_helpers.py b/tests/utilities/scan_test_helpers.py
new file mode 100644
index 00000000..82fc84bd
--- /dev/null
+++ b/tests/utilities/scan_test_helpers.py
@@ -0,0 +1,196 @@
+import os
+import pickle
+
+import numpy as np
+from scipy import interpolate
+from matplotlib import pyplot as plt
+from matplotlib.path import Path as MatPath
+from matplotlib.patches import PathPatch
+from matplotlib.figure import Figure
+
+from openflexure_microscope_server import scan_planners
+
+THIS_DIR = os.path.dirname(os.path.realpath(__file__))
+
+
+class FakeSample:
+    """
+    A fake sample to test scan algorithms. The sample is able to return
+    whether a given position is sample, no image associated with the sample
+    """
+
+    def __init__(self, xy_points: list[tuple[int, int]]):
+        """
+        Create the sample from a spline interpolation around
+        the given points.
+        """
+        self._sample_perimeter = interp_closed_path(xy_points, 500)
+
+    def is_sample(self, pos: tuple[int, int], im_size: tuple[int, int]) -> bool:
+        """
+        Return whether an image at a given location with a given image size
+        is on the sample
+
+        This doesn't check the entire image feild as this is designed to be used
+        where the fake sample is much larger than the image and has smooth edges
+        It just checks the 4 corners
+        """
+        img_corners = [
+            (pos[0] + im_size[0], pos[1] + im_size[1]),
+            (pos[0] + im_size[0], pos[1] - im_size[1]),
+            (pos[0] - im_size[0], pos[1] + im_size[1]),
+            (pos[0] - im_size[0], pos[1] - im_size[1]),
+        ]
+        return any(
+            self._sample_perimeter.contains_point(corner) for corner in img_corners
+        )
+
+    @property
+    def patch(self) -> PathPatch:
+        """
+        The sample as a matplotlib patch fro plotting
+        """
+        patch = PathPatch(self._sample_perimeter)
+        patch.set(color=(1.0, 0.8, 1.0, 1.0))
+        return patch
+
+
+def visualise_scan(sample: FakeSample, planner: scan_planners.ScanPlanner) -> Figure:
+    """
+    For a given sample and scanner object return a matplotlib figure of the scan
+    """
+    fig, ax = plt.subplots(figsize=(8, 8))
+    ax.add_artist(sample.patch)
+    xh, yh = zip(*planner._path_history)
+    xi, yi, _ = zip(*planner._imaged_locations)
+
+    # convert history to numpy array so can calculate quiver arrows
+    xh = np.array(xh)
+    yh = np.array(yh)
+
+    plt.quiver(
+        xh[:-1],
+        yh[:-1],
+        xh[1:] - xh[:-1],
+        yh[1:] - yh[:-1],
+        scale_units="xy",
+        angles="xy",
+        scale=1,
+    )
+    plt.plot(xh, yh, "r.")
+    plt.plot(xi, yi, "g*")
+    ax.axis("equal")
+    return fig
+
+
+def interp_closed_path(xy_points: list[tuple[int, int]], n_points: int) -> MatPath:
+    """
+    Given a lists of xy_points interpolate an n_point closed curve. This can be used
+    to creat an arbitrary sample shape plan a scan.
+
+    Modified from:
+    https://stackoverflow.com/questions/33962717/interpolating-a-closed-curve-using-scipy
+    """
+
+    # Use zip to seperate x and y points into tuples
+    x, y = zip(*xy_points)
+
+    # Append first point and convert to array
+    x = np.array(x + (x[0],))
+    y = np.array(y + (y[0],))
+
+    # fit splines to x=f(u) and y=g(u), treating both as periodic. also note that s=0
+    # is needed in order to force the spline fit to pass through all the input points.
+    spline_data, _ = interpolate.splprep([x, y], s=0, per=True)
+
+    # evaluate the spline
+    xi, yi = interpolate.splev(np.linspace(0, 1, n_points), spline_data)
+
+    # Convert to a matplotlib closed path
+    path_points = [[xp, yp] for xp, yp in (zip(xi, yi))]
+    return MatPath(path_points, closed=True)
+
+
+def example_smart_spiral() -> tuple[FakeSample, scan_planners.ScanPlanner]:
+    """
+    Run an example scan and return the sample scanned and the planner object
+    after scan is complete
+    """
+    xy_sample_points = [
+        (-5000, -5000),
+        (-2000, 10000),
+        (1000, 2000),
+        (6000, 7000),
+        (9000, 2000),
+    ]
+    sample = FakeSample(xy_sample_points)
+    img_size = (1000, 1000)
+    intial_position = (0, 0)
+    planner_settings = {"dx": 700, "dy": 700, "max_dist": 100000}
+    planner = scan_planners.SmartSpiral(
+        intial_position=intial_position, planner_settings=planner_settings
+    )
+
+    while not planner.scan_complete:
+        xy_pos, _ = planner.get_next_location_and_z_estimate()
+        xyz_pos = (xy_pos[0], xy_pos[1], 0)
+        imaged = sample.is_sample(xy_pos, img_size)
+        planner.mark_location_visited(xyz_pos, imaged=imaged, focused=imaged)
+    return sample, planner
+
+
+def profile_and_save_plot_for_example_smart_spiral():
+    """
+    Run the example scan and save a plot and the profile data
+    Also print the cumulative stats
+
+
+    This runs if you run this file directly
+    """
+    import pstats
+    import cProfile
+
+    profiler = cProfile.Profile()
+    sample, planner = profiler.runcall(example_smart_spiral)
+
+    stats_fname = os.path.join(THIS_DIR, "scan_example_stats.pstats")
+    profiler.dump_stats(stats_fname)
+
+    png_fname = os.path.join(THIS_DIR, "scan_example_plot.png")
+    fig = visualise_scan(sample, planner)
+    fig.savefig(png_fname, dpi=200)
+
+    run_stats = pstats.Stats(stats_fname)
+    run_stats.strip_dirs()
+    run_stats.sort_stats("cumulative")
+    run_stats.print_stats("scan_planners.py")
+
+
+def update_example_smart_spiral_pickle():
+    """
+    Pickle the ScanPlanner for the example_smart_spiral(),
+    this is done so the history can be compared by testing to check
+    the algorithm is unchanged.
+
+    If the algorithm is purposefully changed then this will need to be
+    run to update the pickle for the test to pass.
+    """
+    pkl_fname = os.path.join(THIS_DIR, "example_smart_spiral.pkl")
+    with open(pkl_fname, "wb") as pkl_file_obj:
+        _, planner = example_smart_spiral()
+        pickle.dump(planner, pkl_file_obj, pickle.HIGHEST_PROTOCOL)
+
+
+def get_expected_result_for_example_smart_spiral():
+    """
+    Return the expected ScanPlanner object for the example_smart_spiral(),
+    this is pickled, so that it can be committed.
+    """
+    pkl_fname = os.path.join(THIS_DIR, "example_smart_spiral.pkl")
+    with open(pkl_fname, "rb") as pkl_file_obj:
+        planner = pickle.load(pkl_file_obj)
+    return planner
+
+
+if __name__ == "__main__":
+    profile_and_save_plot_for_example_smart_spiral()

From 7f46bc2d9b55e4098a2b5b6de47c1b6c2004bf15 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 14 Apr 2025 00:03:39 +0100
Subject: [PATCH 09/11] Fixing capture_thread variable and autofocus logic

---
 .../things/smart_scan.py                      | 42 +++++++++----------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index da404f63..0c327b0d 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -175,6 +175,7 @@ class SmartScanThing(Thing):
         self._background_detect: Optional[BackgroundDep] = None
         self._ongoing_scan_name: Optional[str] = None
         self._starting_position: Optional[Mapping[str, int]] = None
+        self._capture_thread: Optional[ErrorCapturingThread] = None
         self._scan_images_taken: Optional[int] = None
         # TODO Scan data is a dict during refactoring, should become a dataclass
         self._scan_data: Optional[dict] = None
@@ -212,6 +213,7 @@ class SmartScanThing(Thing):
         self._metadata_getter = metadata_getter
         self._csm = csm
         self._background_detect = background_detect
+        self._capture_thread = None
         self._scan_images_taken = 0
 
         # Don't set self._scan_data dictionary. This is done at the start of _run_scan
@@ -241,6 +243,7 @@ class SmartScanThing(Thing):
             self._metadata_getter = None
             self._csm = None
             self._background_detect = None
+            self._capture_thread = None
             self._ongoing_scan_name = None
             self._scan_images_taken = None
             self._scan_data = None
@@ -499,8 +502,6 @@ class SmartScanThing(Thing):
         # cancel by user.
         scan_successful = True
 
-        capture_thread = None
-
         try:
             self._set_scan_data()
             self._save_scan_inputs_jons()
@@ -530,10 +531,10 @@ class SmartScanThing(Thing):
             )
             raise e
         finally:
-            if capture_thread:
+            if self._capture_thread:
                 # If the capture thread had an error we capture it here
                 try:
-                    capture_thread.join()
+                    self._capture_thread.join()
                 except Exception as e:
                     # If the scan has already ended due to an exception we will
                     # ignore any exceptions, but if it appeared to be successful
@@ -563,8 +564,6 @@ class SmartScanThing(Thing):
             planner_settings=planner_settings,
         )
 
-        capture_thread = None
-
         # At the start of the loop, we simultaneously capture an image and move to the next scan point.
         # We skip capturing on the first run, because we've not focused yet - and also we skip capturing if
         # it looks like background.
@@ -600,8 +599,8 @@ class SmartScanThing(Thing):
             )
 
             # wait for the previous capture to be saved, i.e. don't leave more than one image saving in the background
-            if capture_thread:
-                self._wait_for_capture_thread(capture_thread)
+            if self._capture_thread:
+                self._wait_for_capture_thread()
                 # increment capure counter as thread has completed
                 self._scan_images_taken += 1
                 # Add it to the incremental zip
@@ -613,7 +612,7 @@ class SmartScanThing(Thing):
 
             name = f"image_{new_pos_xyz[0]}_{new_pos_xyz[1]}.jpg"
             jpeg_path = os.path.join(self._ongoing_scan_images_dir, name)
-            capture_thread, acquired = self._start_capture_thread(jpeg_path)
+            self._capture_thread, acquired = self._start_capture_thread(jpeg_path)
             # wait until the image is acquired
             acquired.wait()
 
@@ -635,25 +634,23 @@ class SmartScanThing(Thing):
         Return False if failed after 3 tries - the position will be the initial estimate
         """
         attempts = 0
+        max_attempts = 3
+        dz = self._scan_data["autofocus_dz"]
 
-        while attempts < 3:
+        while attempts < max_attempts:
             attempts += 1
 
-            # Base on first run otherwise we move to estimated_z
-            start = "base" if attempts == 0 else "centre"
-
-            _, jpeg_sizes = self._autofocus.looping_autofocus(
-                dz=self._scan_data["autofocus_dz"], start=start
-            )
+            _, jpeg_sizes = self._autofocus.looping_autofocus(dz=dz, start="base")
             current_height = self._stage.position["z"]
             time.sleep(0.2)
             autofocus_sharp_enough = self._autofocus.verify_focus_sharpness(
                 sweep_sizes=jpeg_sizes, camera=CamDep, threshold=0.92
             )
 
-            # Not sharp enough, nobe to start and try again
+            # Not sharp enough, go to start and try again
             if not autofocus_sharp_enough:
-                self._stage.move_absolute(z=this_xyz[2])
+                z = this_xyz[2] - dz / 2 if attempts < max_attempts else this_xyz[2]
+                self._stage.move_absolute(z=z)
                 continue
 
             # No previous positions to compare against return success
@@ -675,7 +672,8 @@ class SmartScanThing(Thing):
                 return True
 
             # Shifted to far move to start and try again
-            self._stage.move_absolute(z=this_xyz[2])
+            z = this_xyz[2] - dz / 2 if attempts < max_attempts else this_xyz[2]
+            self._stage.move_absolute(z=z)
             self._scan_logger.info(
                 "The focus has shifted further than we expect: retrying."
             )
@@ -684,18 +682,18 @@ class SmartScanThing(Thing):
         return False
 
     @_scan_running
-    def _wait_for_capture_thread(self, capture_thread: ErrorCapturingThread) -> None:
+    def _wait_for_capture_thread(self) -> None:
         """
         Wait for the capture thread to be complete.
         """
         wait_start = time.time()
-        thread_was_alive = capture_thread.is_alive()
+        thread_was_alive = self._capture_thread.is_alive()
 
         # If the capture thread has thrown an exception it will be raised
         # when join is called, this will cause the scan to end. If we want
         # to retry captures at a later date this is where we will need to
         # catch the IOError or CaptureError from the thread.
-        capture_thread.join()
+        self._capture_thread.join()
         time.sleep(0.2)
         if thread_was_alive:
             wait_time = time.time() - wait_start

From bd411db5ea4a6d3bd5fe3b37fbd62692a29185cb Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 14 Apr 2025 09:25:01 +0100
Subject: [PATCH 10/11] Relax line length when linting (increased checker)

---
 ruff-increased-checking.toml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ruff-increased-checking.toml b/ruff-increased-checking.toml
index 8de47661..9c4ffb8e 100644
--- a/ruff-increased-checking.toml
+++ b/ruff-increased-checking.toml
@@ -35,3 +35,8 @@ select = [
            # also "D" but this even warns about docstring punctuation. Perhaps a step too
            # far?
 ]
+
+# Line length is set to 88 for ruff. This is what the formatter aims for
+# some lines are not formatted to 88 if the formatter can't easily break
+# them. Allow up to 99 beofre throwing a long line error
+line-length = 99

From 06632f195e14fc5501f0d9168eed22583134e2c3 Mon Sep 17 00:00:00 2001
From: Julian Stirling <julian@julianstirling.co.uk>
Date: Mon, 14 Apr 2025 15:46:27 +0000
Subject: [PATCH 11/11] Spelling corrections and docstring improvements

Co-authored by Joe Knapper <jaknapper@hotmail.com>
---
 ruff-increased-checking.toml                  |  2 +-
 .../scan_planners.py                          | 26 +++++++++----------
 .../things/smart_scan.py                      | 22 +++++++++-------
 tests/test_scan_planners.py                   |  6 ++---
 tests/utilities/scan_test_helpers.py          |  6 ++---
 5 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/ruff-increased-checking.toml b/ruff-increased-checking.toml
index 9c4ffb8e..118cfbff 100644
--- a/ruff-increased-checking.toml
+++ b/ruff-increased-checking.toml
@@ -38,5 +38,5 @@ select = [
 
 # Line length is set to 88 for ruff. This is what the formatter aims for
 # some lines are not formatted to 88 if the formatter can't easily break
-# them. Allow up to 99 beofre throwing a long line error
+# them. Allow up to 99 before throwing a long line error
 line-length = 99
diff --git a/src/openflexure_microscope_server/scan_planners.py b/src/openflexure_microscope_server/scan_planners.py
index d6d94e6a..75ffd0fb 100644
--- a/src/openflexure_microscope_server/scan_planners.py
+++ b/src/openflexure_microscope_server/scan_planners.py
@@ -22,7 +22,7 @@ XYZPosList: TypeAlias = list[XYZPos]
 
 def enforce_xy_tuple(value: XYPos) -> XYPos:
     """
-    Used for enfocring that an input is a tuple and is the correct length
+    Used for enforcing that an input is a tuple and is the correct length
     """
     if not isinstance(value, (list, tuple)):
         raise ValueError("2 value tuple expected")
@@ -35,7 +35,7 @@ def enforce_xy_tuple(value: XYPos) -> XYPos:
 
 def enforce_xyz_tuple(value: XYZPos) -> XYZPos:
     """
-    Used for enfocring that an input is a tuple and is the correct length
+    Used for enforcing that an input is a tuple and is the correct length
     """
     if not isinstance(value, (list, tuple)):
         raise ValueError("3 value tuple expected")
@@ -64,7 +64,7 @@ class ScanPlanner:
     locations are adjusted.
 
     When subclassing be sure to use enforce_xy_tuple and enforce_xyz_tuple on any user
-    data before
+    data before running
     """
 
     def __init__(self, intial_position: XYPos, planner_settings: Optional[dict] = None):
@@ -199,15 +199,15 @@ class ScanPlanner:
         path_pos = np.array(self._focused_locations, dtype="float64")[:, :2]
 
         # Use linalg.norm to calculate the direct distance bweween the points
-        # Note linalg.norm always used float64
+        # Note linalg.norm always uses float64
         dists = np.linalg.norm((path_pos - current_pos), axis=1)
 
-        # Get indicies of all mimuma.
+        # Get indices of all minima.
         # Note np.where always returns a tuple of arrays, hence the trailing [0]
-        indicies = np.where(dists == np.min(dists))[0]
+        indices = np.where(dists == np.min(dists))[0]
 
         # The last index is most recent
-        return self._focused_locations[indicies[-1]]
+        return self._focused_locations[indices[-1]]
 
     def mark_location_visited(
         self, xyz_pos: XYZPos, imaged: bool, focused: bool
@@ -216,7 +216,7 @@ class ScanPlanner:
         Mark the location as visited
 
         Args:
-            xyz_pos: the x_y poistion
+            xyz_pos: the x_y_z position
             imaged: true if an image was taken, false if not (due to background detect)
             focused: true if autofocus completed successfully
         """
@@ -289,10 +289,10 @@ class SmartSpiral(ScanPlanner):
         self, xyz_pos: XYZPos, imaged: bool = True, focused: bool = True
     ) -> None:
         """
-        Mark the location as visited. Adjust extra poisitons accordingly
+        Mark the location as visited. Adjust extra positions accordingly
 
         Args:
-            xyz_pos: the x_y poistion
+            xyz_pos: the x_y_z position
             imaged: true if an image was taken, false if not (due to background detect)
             focused: true if autofocus completed successfully
         """
@@ -306,9 +306,9 @@ class SmartSpiral(ScanPlanner):
 
     def _add_surrounding_positions(self, xy_pos: XYPos) -> None:
         """
-        This adds the surrounding (4 point connectivity) poistions
+        This adds the surrounding (4 point connectivity) positions
         to the remaining locations if they are not too far away or
-        or already planned or already visited
+        already planned or already visited
         """
         new_positions = [
             (xy_pos[0] - self._dx, xy_pos[1]),
@@ -318,7 +318,7 @@ class SmartSpiral(ScanPlanner):
         ]
 
         for new_pos in new_positions:
-            # Skip position if already planned or fixited
+            # Skip position if already planned or visited
             if self.position_planned(new_pos) or self.position_visited(new_pos):
                 continue
 
diff --git a/src/openflexure_microscope_server/things/smart_scan.py b/src/openflexure_microscope_server/things/smart_scan.py
index 0c327b0d..5d853b69 100644
--- a/src/openflexure_microscope_server/things/smart_scan.py
+++ b/src/openflexure_microscope_server/things/smart_scan.py
@@ -67,7 +67,11 @@ def unpack_autofocus(scan_data):
 
 
 def focus_change_acceptable(prev_pos, prev_z, new_pos, new_z, fractional_limit):
-    # limit is the largest ratio of change in z to change in xy that's allowed
+    """Return true if the change in z-position is small enough.
+
+    A large change in z-position in an indication of focus failure.
+    fractional_limit is the largest ratio of change in z to change in xy that's allowed
+    """
 
     prev_xy = np.asarray(prev_pos, dtype="float64")
     new_xy = np.asarray(new_pos, dtype="float64")
@@ -100,7 +104,7 @@ def ensure_free_disk_space(path: str, min_space: int = 500000000) -> None:
            default = 500,000,000 (500MiB)
 
     Raises:
-        NotEnoughFreeSpaceError is the
+        NotEnoughFreeSpaceError if the remaining storage is below min_space
     """
     du = shutil.disk_usage(path)
     if du.free < min_space:
@@ -449,7 +453,7 @@ class SmartScanThing(Thing):
             )
             autofocus_dz = 0
 
-        # Fix scan parameters in case UI is updates during scan.
+        # Fix scan parameters in case UI is updated during scan.
         self._scan_data = {
             "scan_name": self._ongoing_scan_name,
             "overlap": overlap,
@@ -464,7 +468,7 @@ class SmartScanThing(Thing):
         }
 
     @_scan_running
-    def _save_scan_inputs_jons(self):
+    def _save_scan_inputs_json(self):
         # This should be a method of the scan_data dataclass
 
         data = {
@@ -504,7 +508,7 @@ class SmartScanThing(Thing):
 
         try:
             self._set_scan_data()
-            self._save_scan_inputs_jons()
+            self._save_scan_inputs_json()
             if self._scan_images_taken != 0:
                 msg = "_scan_images_taken should be zero before starting scanning"
                 raise RuntimeError(msg)
@@ -575,7 +579,7 @@ class SmartScanThing(Thing):
             new_pos_xyz = self._move_to_next_point(next_pos_xy, z_est)
 
             capture_image = True
-            # If skipping background, take and image to check if is background
+            # If skipping background, take an image to check if it is background
             if self._scan_data["skip_background"]:
                 capture_image = self._background_detect.image_is_sample()
 
@@ -583,7 +587,7 @@ class SmartScanThing(Thing):
                 route_planner.mark_location_visited(
                     new_pos_xyz, imaged=False, focused=False
                 )
-                # Background franction is actually a percentage
+                # Background fraction is actually a percentage
                 back_perc = round(self._background_detect.background_fraction(), 0)
                 msg = f"Skipping {new_pos_xyz} as it is {back_perc}% background."
                 self._scan_logger.info(msg)
@@ -826,8 +830,8 @@ class SmartScanThing(Thing):
                 ).encode("utf-8")
                 piexif.insert(piexif.dump(exif_dict), jpeg_path)
             except:  # noqa: E722
-                # We need to capture any exeption as there are many reasons metadata
-                # might not be added. We wern rather than log the error.
+                # We need to capture any exception as there are many reasons metadata
+                # might not be added. We warn rather than log the error.
                 self._scan_logger.warning(f"Failed to add metadata to {jpeg_path}")
         except Exception as e:
             raise IOError(f"An error occurred while saving {jpeg_path}") from e
diff --git a/tests/test_scan_planners.py b/tests/test_scan_planners.py
index e4699d91..da5ff961 100644
--- a/tests/test_scan_planners.py
+++ b/tests/test_scan_planners.py
@@ -39,7 +39,7 @@ def test_v_basic_smart_spiral():
     planner = scan_planners.SmartSpiral(
         intial_position=intial_position, planner_settings=planner_settings
     )
-    # Create a planner it shouldn't start complete
+    # Create a planner. It shouldn't be complete.
     assert not planner.scan_complete
     # When we start it should want to stay in the inital pos and have
     # no z_estimate
@@ -47,7 +47,7 @@ def test_v_basic_smart_spiral():
     assert xy_pos == intial_position
     assert z_pos is None
 
-    # Try to make imaged with only xy_position
+    # Try to mark location as imaged with only xy_position
     with pytest.raises(ValueError):
         planner.mark_location_visited(xy_pos, imaged=False, focused=False)
     # scan still not complete
@@ -100,7 +100,7 @@ def test_smart_spiral_first_few_pos():
     This test is  VERY long, not really a "unit". It checks step-by-step
     that data is added correctly for the first few postions in a scan.
 
-    This should catch basic caseses of if the algorithm is updated.
+    This should catch basic cases of if the algorithm is updated.
     """
     intial_position = (100, 50)
     planner_settings = {"dx": 50, "dy": 50, "max_dist": 10000}
diff --git a/tests/utilities/scan_test_helpers.py b/tests/utilities/scan_test_helpers.py
index 82fc84bd..6901361a 100644
--- a/tests/utilities/scan_test_helpers.py
+++ b/tests/utilities/scan_test_helpers.py
@@ -31,7 +31,7 @@ class FakeSample:
         Return whether an image at a given location with a given image size
         is on the sample
 
-        This doesn't check the entire image feild as this is designed to be used
+        This doesn't check the entire image field as this is designed to be used
         where the fake sample is much larger than the image and has smooth edges
         It just checks the 4 corners
         """
@@ -48,7 +48,7 @@ class FakeSample:
     @property
     def patch(self) -> PathPatch:
         """
-        The sample as a matplotlib patch fro plotting
+        The sample as a matplotlib patch for plotting
         """
         patch = PathPatch(self._sample_perimeter)
         patch.set(color=(1.0, 0.8, 1.0, 1.0))
@@ -86,7 +86,7 @@ def visualise_scan(sample: FakeSample, planner: scan_planners.ScanPlanner) -> Fi
 def interp_closed_path(xy_points: list[tuple[int, int]], n_points: int) -> MatPath:
     """
     Given a lists of xy_points interpolate an n_point closed curve. This can be used
-    to creat an arbitrary sample shape plan a scan.
+    to create an arbitrary sample shape plan a scan.
 
     Modified from:
     https://stackoverflow.com/questions/33962717/interpolating-a-closed-curve-using-scipy