feat: improve laser autofocus with robust spot detection and error handling #101

hongquanli · 2025-02-11T10:42:48Z

Major changes:

Add comprehensive error handling and logging to laser autofocus
Implement robust spot detection with multiple detection modes
Add type hints and docstrings throughout
Add class constants for configuration values
Add laser autofocus testing and plotting capabilities

Technical improvements:

Move SpotDetectionMode from utils.py to _def.py
Add image cross correlation-based error detection
Improve defensive programming practices
Remove obsolete variables

- Add type hints and comprehensive docstrings - Improve error handling and logging

hongquanli · 2025-02-11T10:43:24Z

(some unrelated changes due to linting; tried to revert in cursor but was not successful)

ianohara · 2025-02-13T22:35:19Z

(some unrelated changes due to linting; tried to revert in cursor but was not successful)

For linting, it should always be safe (and acceptable) to just run:

black --config pyproject.toml .

from the software/ repo.

If we all get in the habit of doing that, and don't merge PRs if the linting is failing, then the lint changes in a PR will only pertain to code changed in the PR.

ianohara

I left a bunch of small comments, but overall it looks great in terms of accomplishing proper error handling in the autofocus code.

Before merging, it'd be good to rename the title to something that matches the intent of the PR a bit more closely. EG: autofocus: add error checking and logging throughout

ianohara · 2025-02-13T22:41:24Z

software/control/core/core.py

        self.microcontroller.turn_off_AF_laser()
        self.microcontroller.wait_till_operation_is_completed()

+        # Calculate conversion factor
        if x1 - x0 == 0:


Does this do weird things if the spot detection coincidentally gives us two equal x spots in a real scenario? Or is that literally impossible in the non-sim case?

ianohara · 2025-02-13T22:41:27Z

software/control/core/core.py

        else:
-            # calculate the conversion factor
            self.pixel_to_um = 6.0 / (x1 - x0)


Where does this 6 come from? Is this something we should grab from the camera instead of having it hard coded?

It was arbitrary. It should be large so that the conversion calculation is robust but not too large to be outside the "capture" range.

Gotcha. In that case, pulling it out as a class constant (see example below) and leaving a constant explaining it would be helpful.

ianohara · 2025-02-13T22:44:02Z

software/control/core/core.py

+            return False
+
+        self._log.debug(f"Final displacement: {final_displacement:.1f} μm")
+        return abs(final_displacement - target_um) < 1.0  # Success if within 1 μm


It'd be good to break this 1.0 out to a class constant for clarity.

Any suggestion where to specify these? Maybe another configuration objective (also for the 6 um)?

I'd start by just defining it at the class level as a constant. That'll make it easy to see and pull out into a config object later if we want. EG:

class LaserAutofocusController(QObject): DISPLACEMENT_SUCCESS_WINDOW_UM = 1

or something. Then, later, if we decide it should be a more configurable value on a per-call basis we can add it as an argument to the method and let the caller pull it from whatever config they're using.

ianohara · 2025-02-13T22:45:57Z

software/control/core/core.py

+                    self.camera.send_trigger()
+                elif self.liveController.trigger_mode == TriggerMode.HARDWARE:
+                    # self.microcontroller.send_hardware_trigger(control_illumination=True,illumination_on_time_us=self.camera.exposure_time*1000)
+                    pass  # to edit


Should we make the HARDWARE case return None for now? I think if we don't, and we get here in HARDWARE mode, the read_frame might hang below.

ianohara · 2025-02-13T22:47:57Z

software/control/core/core.py

+                continue
+
+        # Check if we got enough successful detections
+        if successful_detections == 0:


From a "defensive programming" perspective, I'd normally make this an if successful_detections <= 0 check instead of ==. Then it acts as a shield against accidentally making successful_detections negative somehow (it's obvious that never happens now, but you never know with refactoring...)

ianohara · 2025-02-13T22:52:39Z

software/control/utils.py

+    MULTI_SECOND_RIGHT: In multi-spot case, use spot immediately left of rightmost spot
+    """
+
+    SINGLE = auto()


I didn't know about auto()! Nice! It looks like there's a pretty easy way to have auto generate string values for you ( see https://docs.python.org/3/library/enum.html#enum.Enum._generate_next_value_ ).

The one downside of auto() in its int form I can think of is if we are using pydantic and serializing Enum. If someone comes and adds an entry between e.g. DUAL_RIGHT and DUAL_LEFT, and we're using ints from auto() for enum values, then all the entries after DUAL_RIGHT will get incremented. Then, any pydantic models we've serialized to file (in json, xml, or other format) will be deserialized incorrectly!

If you use the _generate_next_value_ --> name value instead, though, we're resilient to re-ordering but would fail on re-naming. This is a pretty common problem with serialization formats. Things like protobuf handle it by separating name and serialization identifier, and forcing you to manually allocate/manage them. Here's the protobuf bit on this: https://protobuf.dev/programming-guides/proto3/#deleting

Any suggestion what we should use here?

We can make our own helper to use the names as values. For instance (in ipython):

In [16]: import enum In [17]: class NameEnum(enum.Enum): ...: def _generate_next_value_(name, start, count, last_values): ...: return name ...: In [18]: class SpotDetectionMode(NameEnum): ...: SINGLE = enum.auto() ...: DUAL_RIGHT = enum.auto() ...: ETC = enum.auto() ...: In [19]: SpotDetectionMode.SINGLE.value Out[19]: 'SINGLE'

Then anyone can use NameEnum for this case.

ianohara · 2025-02-13T22:57:01Z

software/control/utils.py

+
+def find_spot_location(
+    image: np.ndarray, mode: SpotDetectionMode = SpotDetectionMode.SINGLE, params: Optional[dict] = None
+) -> Optional[Tuple[float, float]]:


It'd be nice to make the params and result here pydantic models. That way we aren't reliant on raw string indexing.

That'd also let you define the param defaults in the same location as the model definition.

Sounds good

ianohara · 2025-02-13T23:08:24Z

software/control/utils.py

+
+    try:
+        # Get the y position of the spots
+        intensity_profile = np.sum(image, axis=1)


I think when I was testing laser autofocus, the dots were at visibly different y positions in the image. I think this will have trouble with that case (we're more likely to end up having the spot that doesn't get the y selection here too close to the edge in the check below).

I guess now that you've plumped all the error checking through we'll be able to see the failure mode better, so we can see if it's an issue or not.

Great point. We can adjust the camera orientation so that the spot only move around x. I'll add this one to linear.app so that we can get back to it later.

ianohara · 2025-02-13T23:11:41Z

software/control/utils.py

+        # Calculate centroid in window around selected peak
+        return _calculate_spot_centroid(cropped_image, peak_x, peak_y, p)
+
+    except Exception as e:


I think the intent of this is to catch the errors you've thrown above and convert them to None, but probably not to catch other miscellaneous exceptions. If so, it'd be better to just catch ValueError. But if we do want something like internal np errors getting caught here (if those are thrown, it probably indicates some error we don't know how to handle), this works.

I guess its a decision on our part of whether or not we're okay with squashing unpredicted errors or not.

ianohara · 2025-02-13T23:14:37Z

software/control/utils.py

+        return None
+
+
+def _calculate_spot_centroid(cropped_image: np.ndarray, peak_x: int, peak_y: int, params: dict) -> Tuple[float, float]:


The functions in this file are great candidates for unit tests. Even simple ones like making your own 5x5 cropped image with a known center and passing it in.

…lass constants

…end on _def.py

hongquanli added 3 commits February 10, 2025 22:14

refactor: improve laser autofocus controller

d7fab36

- Add type hints and comprehensive docstrings - Improve error handling and logging

feat: add robust spot detection

7acebb1

bug fix

a4834a8

ianohara reviewed Feb 13, 2025

View reviewed changes

hongquanli changed the title ~~Improve laser autofocus~~ laser autofocus: update spot finding, add error checking and logging throughout Feb 15, 2025

hongquanli added 5 commits February 14, 2025 22:24

move SpotDetectionMode from utils.py to _def.py

ae2b34e

add image cross correlation-based error detection and handling; add c…

e76b14d

…lass constants

bug fix

33acc24

add laser af testing (including plotting)

1dbca04

be more defensive by changing == 0 to <= 0

efc4acf

hongquanli marked this pull request as ready for review February 15, 2025 07:31

remove obsolete variables

f7c8352

hongquanli changed the title ~~laser autofocus: update spot finding, add error checking and logging throughout~~ feat: improve laser autofocus with robust spot detection and error handling Feb 15, 2025

hongquanli and others added 2 commits February 14, 2025 23:59

move SpotDetectionMode back to utils.py so that utils.py does not dep…

da01667

…end on _def.py

Merge branch 'master' into improve-laser-autofocus

d0eb313

hongquanli merged commit f0c5e17 into master Feb 15, 2025
0 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve laser autofocus with robust spot detection and error handling #101

feat: improve laser autofocus with robust spot detection and error handling #101

hongquanli commented Feb 11, 2025 •

edited

Loading

hongquanli commented Feb 11, 2025

ianohara commented Feb 13, 2025

ianohara left a comment

ianohara Feb 13, 2025

ianohara Feb 13, 2025

hongquanli Feb 14, 2025

ianohara Feb 14, 2025

ianohara Feb 13, 2025

hongquanli Feb 14, 2025

ianohara Feb 14, 2025

ianohara Feb 13, 2025

hongquanli Feb 14, 2025

ianohara Feb 13, 2025

ianohara Feb 13, 2025

hongquanli Feb 14, 2025

ianohara Feb 14, 2025

ianohara Feb 13, 2025

hongquanli Feb 14, 2025

ianohara Feb 13, 2025

hongquanli Feb 14, 2025

ianohara Feb 13, 2025

ianohara Feb 13, 2025

		return None


		def _calculate_spot_centroid(cropped_image: np.ndarray, peak_x: int, peak_y: int, params: dict) -> Tuple[float, float]:

feat: improve laser autofocus with robust spot detection and error handling #101

feat: improve laser autofocus with robust spot detection and error handling #101

Conversation

hongquanli commented Feb 11, 2025 • edited Loading

hongquanli commented Feb 11, 2025

ianohara commented Feb 13, 2025

ianohara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongquanli commented Feb 11, 2025 •

edited

Loading