[CSDM-1150] implement deterministic automatic mirror alignment algorithm in the gui #3251
Conversation
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
afb0850 to
a821854
Compare
📝 WalkthroughWalkthroughAdds a Parabolic Mirror alignment feature: public function parabolic_mirror_alignment(...) returning a ProgressiveFuture, a cancellable ParabolicMirrorAlignmentTask class, and an AlignmentAxis dataclass. Implements probabilistic grid snapping, bounded 1D axis searches, two sequential 3D Nelder–Mead refinements over [l, s, z], custom optimizers, spot measurement and multiple scoring metrics, background image saving, progress reporting and cancellation handling. Also adds GUI controls/resources, documentation, and unit tests. Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant GUI as Sparc2AlignTab
participant API as parabolic_mirror_alignment()
participant Task as ParabolicMirrorAlignmentTask
participant HW as Mirror/Stage/CCD
participant Saver as ImageSavingThread
User->>GUI: Click "Auto align"
GUI->>API: parabolic_mirror_alignment(axes, ccd, ...)
API->>Task: create & submit task to executor
API-->>GUI: return ProgressiveFuture
GUI->>GUI: update button -> "Cancel"
Note over Task,HW: Stage 1 — per-axis 1D bounded searches
loop per axis
Task->>HW: moveAbs(axis position)
HW-->>Task: capture image
Task->>Task: compute _get_spot_measurement() and _get_score()
alt cancel requested
Task-->>Task: stop with CancelledError
end
end
Note over Task,HW: Stage 2 — two 3D Nelder–Mead refinements on [l,s,z]
loop Nelder–Mead iterations
Task->>HW: moveAbs([l,s,z])
HW-->>Task: capture image
Task->>Task: compute _get_spot_measurement() and _get_score()
Task->>Task: _callback_3d() (progress/stall detection)
alt stop_early or stagnation
Task->>Task: early stop
end
alt cancel requested
Task-->>Task: stop with CancelledError
end
end
opt save_images
Task->>Saver: enqueue image + metadata
Saver-->>Task: confirm save
end
Task-->>GUI: future completes (success | cancelled | error)
GUI->>GUI: _on_auto_align_done() -> reset UI
opt User cancels during run
User->>GUI: Click "Cancel"
GUI->>Task: cancel(future)
Task-->>Task: propagate cancellation
end
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/odemis/acq/align/test/mirror_test.py (1)
98-99: Minor inconsistency: int conversion with float formatting.Line 98 converts the intensity to
int, but line 99 logs it with float format:.1f. While this still works, it's slightly inconsistent.Consider either:
- Keep as float:
intensity = float(img.max())- Use integer format:
logging.info(f"Final intensity = {intensity:d}")src/odemis/gui/cont/tabs/sparc2_align_tab.py (2)
841-842: Remove commented debug code.These commented lines appear to be leftover test/debug code and should be removed to keep the codebase clean.
Apply this diff:
- # img = generate_raytraced_pattern(dz=0.005) - # self.tab_data_model.main.ccd.set_image(model.DataArray(img))
893-913: Consider adding user feedback on alignment completion.While the current logging is appropriate, users may benefit from visual feedback when the alignment succeeds or fails, especially if the window is not visible. Consider adding a popup notification for success/failure cases.
Example enhancement:
def _on_auto_align_done(self, f): """ Callback invoked when the auto-alignment task finishes. Retrieves the future result to surface any exceptions and logs the outcome. Always resets the auto-alignment button label back to "Run" (via wx.CallAfter) so the GUI returns to the idle state. :param f: (concurrent.futures.Future) Future returned by mirror_alignment (or a ProgressiveFuture). Calling f.result() will re-raise any exception that occurred during the task. """ try: f.result() logging.debug("Auto alignment successful") # Optional: notify user of success # wx.CallAfter(popup.show_message, self.main_frame, "Auto Alignment", # "Mirror alignment completed successfully.", level=logging.INFO) except CancelledError: logging.debug("Auto alignment cancelled") except Exception: logging.exception("Auto alignment failed") wx.CallAfter(popup.show_message, self.main_frame, "Auto Alignment Error", "Auto alignment failed. Check the logs for details.", level=logging.ERROR) finally: wx.CallAfter(self.btn_auto_align.SetLabel, "Run")src/odemis/acq/align/mirror.py (4)
126-164: Document magic numbers and add input validation.The spot measurement implementation has a few areas for improvement:
- The magic number
1.3on line 160 should be documented. Why 1.3 specifically?- The
corner_size = 5is hardcoded and may not work well for very small or very large images.- No validation that the image is 2D or has sufficient size.
Apply this diff:
def _get_spot_measurement(self) -> Tuple[int, int, float]: """ Measure spot quality from the current camera image. The method estimates background noise from small corner regions, computes a threshold and returns: - spot_area: number of pixels above noise threshold (int) - spot_intensity: peak pixel value (int) - noise: estimated background level (float) The method checks for cancellation before acquiring data. :raises CancelledError: if the task was cancelled. :returns: (spot_area, spot_intensity, noise) """ if self._cancelled: raise CancelledError("Alignment was cancelled by user") image = self._ccd.data.get(asap=False) + + # Validate image dimensions + if image.ndim != 2: + raise ValueError(f"Expected 2D image, got {image.ndim}D") + if min(image.shape) < 10: + raise ValueError(f"Image too small for alignment: {image.shape}") corner_size = 5 # Use a 5x5 pixel square from each corner top_left = image[:corner_size, :corner_size] top_right = image[:corner_size, -corner_size:] bottom_left = image[-corner_size:, :corner_size] bottom_right = image[-corner_size:, -corner_size:] all_corners = numpy.concatenate( ( top_left.ravel(), top_right.ravel(), bottom_left.ravel(), bottom_right.ravel(), ) ) - noise = 1.3 * numpy.median(all_corners) + # Use 1.3× median of corners as noise threshold to account for + # Poisson statistics and ensure robust background estimation + noise = 1.3 * numpy.median(all_corners) spot_area = numpy.count_nonzero(image > noise) spot_intensity = int(image.max()) return spot_area, spot_intensity, noise
166-223: Consider removing commented alternative implementation.The 58 lines of commented FWHM-based implementation should be removed to keep the codebase clean. Version control preserves this code if needed in the future. If this alternative approach is valuable for future experimentation, consider moving it to a separate experimental module instead.
479-521: Remove unnecessary bounds parameter from Nelder-Mead optimization.The
minimize()calls on lines 495 and 539 pass aboundsparameter to the Nelder-Mead method, but Nelder-Mead does not support bounds (they are silently ignored). While your_objective_lszhandles bounds via clipping internally, passing the unused parameter is confusing.Apply this diff to both Nelder-Mead calls (lines 495 and 539):
lsz_result = minimize( fun=self._objective_lsz, x0=initial_guess, args=(l_range, s_range, z_range, weight), method="Nelder-Mead", - bounds=(l_range, s_range, z_range), callback=self._callback_lsz, options={ "maxiter": max_iter, "adaptive": True, }, )
381-564: Add iteration budget validation between stages.The current implementation subtracts iterations used from
max_iterafter each stage (lines 424, 452, 480, 524), which could result in insufficient iterations for later stages or negative values. Consider adding checks to ensure a minimum iteration budget remains for each stage.Example improvement:
# After each stage max_iter -= l_result.nit if max_iter < 10: # Ensure minimum iterations for remaining stages logging.warning(f"Low iteration budget remaining: {max_iter}. Consider increasing max_iter.") max_iter = 10 # Set minimum for next stageThis ensures each stage gets a fair chance to converge even if earlier stages consumed most of the budget.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(3 hunks)src/odemis/gui/main_xrc.py(2 hunks)src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/odemis/acq/align/test/mirror_test.py (1)
src/odemis/acq/align/mirror.py (1)
mirror_alignment(37-69)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (2)
model(570-571)HwComponent(277-542)src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (3)
src/odemis/acq/align/mirror.py (2)
mirror_alignment(37-69)cancel(111-124)src/odemis/gui/comp/settings.py (1)
SettingsPanel(52-586)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)
🪛 GitHub Actions: Syntax checking
src/odemis/acq/align/test/mirror_test.py
[warning] 1-1: Non-runnable test: lacks an 'main' guard.
🪛 Ruff (0.14.1)
src/odemis/acq/align/test/mirror_test.py
72-72: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
73-73: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
74-74: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
src/odemis/acq/align/mirror.py
111-111: Unused method argument: future
(ARG002)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
285-285: Ambiguous variable name: l
(E741)
286-286: Ambiguous variable name: l
(E741)
316-316: Avoid specifying long messages outside the exception class
(TRY003)
333-333: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Ambiguous variable name: l
(E741)
350-350: Avoid specifying long messages outside the exception class
(TRY003)
359-359: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
369-369: Avoid specifying long messages outside the exception class
(TRY003)
372-372: Avoid specifying long messages outside the exception class
(TRY003)
374-374: Ambiguous variable name: l
(E741)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
870-870: Unused method argument: evt
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (8)
src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc (1)
1003-1006: LGTM!The empty panel placeholder is appropriate for the auto-alignment UI scaffolding. The panel will be populated programmatically as indicated in the enriched summary.
src/odemis/acq/align/test/mirror_test.py (1)
69-74: LGTM!Using a fixed random seed for reproducibility is good practice in tests. The static analysis warnings (S311) about cryptographic randomness are false positives since this is not a security context.
src/odemis/gui/main_xrc.py (2)
15652-15655: No issues found — panel is intentionally empty and populated dynamically.The
pnl_auto_alignpanel is correctly defined as empty in the XRC. At runtime (insparc2_align_tab.pyline 843), it is wrapped withSettingsPanelwhich dynamically adds the child controls (auto_align_rng,auto_align_max_iter,btn_auto_align). This is a valid design pattern.
1306-1306: Do not manually editmain_xrc.py—it is auto-generated by pywxrc.This file is automatically generated from XRC resource files and contains the marker "This file was automatically generated by pywxrc" at line 1. The source XRC file (
src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc) already contains thepnl_auto_alignpanel definition, so the manual changes tomain_xrc.pyshould be removed and the file should be regenerated using pywxrc instead. Manual edits will be lost on the next regeneration.⛔ Skipped due to learnings
Learnt from: K4rishma PR: delmic/odemis#3245 File: src/odemis/gui/main_xrc.py:11982-11982 Timestamp: 2025-10-20T10:00:09.185Z Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
843-869: LGTM! Auto-alignment panel setup is correct.The panel initialization follows the existing patterns in the codebase:
- Proper use of
SettingsPanelfor consistent UI- Appropriate field configurations with sensible defaults and ranges
- Correct initialization of
auto_align_futureusingInstantaneousFuture()(consistent with_autofocus_fat line 427)- Button binding follows standard event handling patterns
src/odemis/acq/align/mirror.py (3)
37-69: LGTM! Public API function is well-designed.The
mirror_alignmentfunction properly:
- Returns a
ProgressiveFuturewith cancellation support viatask_canceller- Uses
submitfto schedule on the single-worker executor- Has comprehensive docstring documenting parameters and return value
- Follows existing patterns in the codebase
72-124: LGTM! Class initialization and cancellation are correct.The
MirrorAlignmentTaskclass is properly structured:
- Clear docstring explaining the multi-stage alignment procedure
- Reasonable constants for spot quality metrics
- Clean initialization with all necessary state
- Proper cancellation support via the
_cancelledflagThe static analysis warning about the unused
futureparameter incancel()(line 111) is a false positive—this parameter is required by the task canceller interface expected byProgressiveFuture.
225-252: LGTM! Scoring function is mathematically sound.The
_get_scoremethod correctly combines normalized area and intensity metrics:
- Proper normalization to [0, 1] range with clamping
- Clear weight parameter for adjusting the optimization objective
- Returns a single scalar score for the optimizer to minimize
The implementation handles edge cases well (clamping ensures robustness to extreme values).
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
2 times, most recently
from
f2c1cd8 to
972c532
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/odemis/acq/align/test/mirror_test.py (2)
61-61: Docstring vs threshold mismatch: update to 80%.- """Require at least 90% of random misalignments to realign successfully.""" + """Require at least 80% of random misalignments to realign successfully."""
63-63: Fix range comment to match the value (±30 µm, not ±45 µm).- rng = 30e-6 # ±45 µm range + rng = 30e-6 # ±30 µm rangesrc/odemis/acq/align/mirror.py (1)
8-8: Fix module docstring to describe mirror alignment.Replace GUI-tab text with a concise module purpose.
-Handles the switch of the content of the main GUI tabs. +Mirror alignment algorithms for automatic calibration of mirror and stage actuators.src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
883-891: Wrap task start in try/except; validate inputs/hardware; set label only on success.Start-up can throw (invalid UI value, missing mirror/stage/ccd, immediate failure). Handle errors and keep UI state consistent.
Apply:
- self.auto_align_future = mirror_alignment( - self.tab_data_model.main.mirror, - self.tab_data_model.main.stage, - self.tab_data_model.main.ccd, - search_range=float(self.auto_align_rng.GetValue()), - max_iter=int(self.auto_align_max_iter.GetValue()), - ) - self.auto_align_future.add_done_callback(self._on_auto_align_done) - self.btn_auto_align.SetLabel("Cancel") + try: + rng = float(self.auto_align_rng.GetValue()) + iters = int(self.auto_align_max_iter.GetValue()) + main = self.tab_data_model.main + if not (main.mirror and main.stage and main.ccd): + raise RuntimeError("Required hardware (mirror, stage, or camera) is unavailable") + self.auto_align_future = mirror_alignment( + main.mirror, + main.stage, + main.ccd, + search_range=rng, + max_iter=iters, + ) + self.auto_align_future.add_done_callback(self._on_auto_align_done) + self.btn_auto_align.SetLabel("Cancel") + except Exception: + logging.exception("Failed to start auto alignment") + popup.show_message( + self.main_frame, + "Auto Alignment Error", + message="Failed to start auto alignment. Check the hardware status and inputs.", + level=logging.ERROR, + )
🧹 Nitpick comments (14)
src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc (1)
1003-1006: Make pnl_auto_align usable and consistent (add sizer, expand, theme, assign_var).As-is, the panel may collapse and can’t host children unless a sizer is set. Match siblings’ look-and-feel and ensure it expands.
Apply this diff:
- <object class="sizeritem"> - <object class="wxPanel" name="pnl_auto_align"> - </object> - </object> + <object class="sizeritem"> + <object class="wxPanel" name="pnl_auto_align"> + <object class="wxBoxSizer"> + <orient>wxVERTICAL</orient> + </object> + <fg>#E5E5E5</fg> + <bg>#444444</bg> + <XRCED> + <assign_var>1</assign_var> + </XRCED> + </object> + <flag>wxBOTTOM|wxEXPAND</flag> + <border>5</border> + </object>src/odemis/acq/align/test/mirror_test.py (5)
35-36: Build paths portably with os.path.join/normpath.String concatenation with “../..” is brittle; use join/normpath.
-CONFIG_PATH = os.path.dirname(odemis.__file__) + "/../../install/linux/usr/share/odemis/" -SPARC2_MIRROR_CONFIG = CONFIG_PATH + "sim/sparc2-mirror-alignment-sim.odm.yaml" +CONFIG_PATH = os.path.normpath( + os.path.join(os.path.dirname(odemis.__file__), "..", "..", "install", "linux", "usr", "share", "odemis") +) +SPARC2_MIRROR_CONFIG = os.path.join( + CONFIG_PATH, "sim", "sparc2-mirror-alignment-sim.odm.yaml" +)
69-75: Use a local PRNG and silence Ruff S311 in tests.Keep determinism without touching global random; silence non-crypto warning.
- random.seed(25) + rnd = random.Random(25) # deterministic PRNG for tests @@ - dl = random.uniform(-rng, rng) - ds = random.uniform(-rng, rng) - dz = random.uniform(-rng, rng) + dl = rnd.uniform(-rng, rng) # noqa: S311 + ds = rnd.uniform(-rng, rng) # noqa: S311 + dz = rnd.uniform(-rng, rng) # noqa: S311
84-84: Avoid hard sleeps; reduce or wait for readiness.Hard-coded 1s sleeps slow CI. Prefer waiting on device readiness or reduce to 0.1s if stability allows.
- time.sleep(1) + time.sleep(0.1) @@ - time.sleep(1) + time.sleep(0.1)Also applies to: 110-110
34-34: Don’t change the root logger level from a test module.This affects all tests’ logging. Either remove it or gate under main; alternatively use a module logger.
-logging.getLogger().setLevel(logging.DEBUG) +# Prefer configuring logging in the test runner, or gate under __main__ if needed.
56-59: Reset mirror l/s to a known baseline for independence between trials.setUp resets stage z and mirror_xy x/y only. Consider also resetting mirror l/s to a reference position to avoid drift across iterations.
Can you confirm whether MD_FAV_POS_ACTIVE includes "l"/"s"? If so, reset them here; otherwise, capture the initial mirror position in setUpClass and moveAbs to it in setUp.
src/odemis/gui/cont/tabs/sparc2_align_tab.py (2)
870-870: Silence ARG002 by discarding unused event argument.Rename the parameter to underscore.
- def _on_btn_auto_align(self, evt): + def _on_btn_auto_align(self, _):
905-913: Ignore stale callbacks to avoid racing UI updates after rapid re-runs.If a previous run finishes after a new one started, skip resetting the label.
- try: + # Ignore callbacks from stale runs (user may have started a new one) + if f is not self.auto_align_future: + return + try: f.result() logging.debug("Auto alignment successful")src/odemis/acq/align/mirror.py (6)
407-410: Clamp per-stage iteration budgets; avoid negative maxiter.Ensure each optimizer gets a positive maxiter and clamp the running budget after each stage.
l_result = minimize_scalar( fun=self._objective_l, bounds=bounds, method="bounded", options={ - "maxiter": max_iter, + "maxiter": max(1, max_iter), }, ) @@ - max_iter -= l_result.nit + max_iter = max(1, max_iter - l_result.nit) @@ s_result = minimize_scalar( fun=self._objective_s, bounds=bounds, method="bounded", options={ - "maxiter": max_iter, + "maxiter": max(1, max_iter), }, ) @@ - max_iter -= s_result.nit + max_iter = max(1, max_iter - s_result.nit) @@ z_result = minimize_scalar( fun=self._objective_z, bounds=bounds, method="bounded", options={ - "maxiter": max_iter, + "maxiter": max(1, max_iter), }, ) @@ - max_iter -= z_result.nit + max_iter = max(1, max_iter - z_result.nit) @@ - max_iter -= lsz_result.nit + max_iter = max(1, max_iter - lsz_result.nit)Also applies to: 436-438, 464-466, 424-424, 452-452, 480-480, 524-524
485-487: Intersect search windows with hardware axis ranges.Avoid commanding out-of-range moves if search_range exceeds physical bounds.
- l_range = (l0 - search_range, l0 + search_range) - s_range = (s0 - search_range, s0 + search_range) - z_range = (z0 - search_range, z0 + search_range) + l_hw = self._mirror.axes["l"].range + s_hw = self._mirror.axes["s"].range + z_hw = self._stage.axes["z"].range + l_range = (max(l_hw[0], l0 - search_range), min(l_hw[1], l0 + search_range)) + s_range = (max(s_hw[0], s0 - search_range), min(s_hw[1], s0 + search_range)) + z_range = (max(z_hw[0], z0 - search_range), min(z_hw[1], z0 + search_range))Apply to both refinement stages.
Also applies to: 529-531
111-111: Silence ARG002 by discarding unused parameter in cancel().Rename to underscore.
- def cancel(self, future): + def cancel(self, _):
142-142: Drop exception messages for CancelledError to satisfy TRY003.Keep raises terse; logging already provides context.
- raise CancelledError("Alignment was cancelled by user") + raise CancelledErrorAlso applies to: 280-280, 316-316, 333-333, 350-350, 369-369, 372-372
359-361: Replace EN DASH in docstring.Use a hyphen to avoid RUF002.
- Optimizer callback executed after each Nelder–Mead iteration. + Optimizer callback executed after each Nelder-Mead iteration.
63-69: Optional: add progress reporting to ProgressiveFuture.Currently no updates are sent. Consider passing the ProgressiveFuture into the task and calling set_progress() per stage/iteration for a responsive UI.
I can draft a small ProgressReporter abstraction to decouple MirrorAlignmentTask from futures if preferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(3 hunks)src/odemis/gui/main_xrc.py(2 hunks)src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
PR: delmic/odemis#3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/main_xrc.py
🧬 Code graph analysis (3)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (2)
model(570-571)HwComponent(277-542)src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (5)
src/odemis/acq/align/mirror.py (2)
mirror_alignment(37-69)cancel(111-124)src/odemis/acq/stream_settings.py (1)
StreamSettingsConfig(49-190)src/odemis/gui/comp/settings.py (1)
SettingsPanel(52-586)src/odemis/gui/conf/data.py (2)
get_hw_config(1243-1267)get_local_vas(1284-1311)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)
src/odemis/acq/align/test/mirror_test.py (1)
src/odemis/acq/align/mirror.py (1)
mirror_alignment(37-69)
🪛 Ruff (0.14.1)
src/odemis/acq/align/mirror.py
111-111: Unused method argument: future
(ARG002)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
285-285: Ambiguous variable name: l
(E741)
286-286: Ambiguous variable name: l
(E741)
316-316: Avoid specifying long messages outside the exception class
(TRY003)
333-333: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Ambiguous variable name: l
(E741)
350-350: Avoid specifying long messages outside the exception class
(TRY003)
359-359: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
369-369: Avoid specifying long messages outside the exception class
(TRY003)
372-372: Avoid specifying long messages outside the exception class
(TRY003)
374-374: Ambiguous variable name: l
(E741)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
870-870: Unused method argument: evt
(ARG002)
src/odemis/acq/align/test/mirror_test.py
72-72: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
73-73: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
74-74: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (4)
src/odemis/acq/align/test/mirror_test.py (1)
124-125: LGTM: main guard present.Direct execution is now supported.
src/odemis/gui/main_xrc.py (2)
1306-1306: LGTM!The control instantiation follows the established pattern and is correctly placed in the initialization sequence.
15652-15655: Verify the panel layout behavior.The empty panel definition and sizeritem without explicit flags (e.g.,
wxEXPAND, proportion) rely on defaults. Ensure the layout behaves as intended when the panel is populated with actual controls.src/odemis/acq/align/mirror.py (1)
495-506: The review comment is based on incorrect information and should be dismissed.SciPy's
minimizeaccepts aboundsargument whenmethod='Nelder-Mead', with bounds enforced by clipping simplex vertices. This contradicts the review's core claim that "Nelder-Mead does not support bounds."The current code at lines 495–506 and 539–550 is correct and valid. It properly uses
method="Nelder-Mead"with bounds, and the_objective_lszfunction additionally clips values (l, s, z) usingnumpy.clip, providing redundant but harmless bounds enforcement. The suggested switch to Powell is unnecessary and based on a misunderstanding of SciPy's API.Likely an incorrect or invalid review comment.
src/odemis/acq/align/mirror.py
Outdated
| self._mirror.moveAbs({"l": l, "s": s}).result() | ||
| self._stage.moveAbs({"z": z}).result() |
There was a problem hiding this comment.
Add movement timeouts to prevent hangs on hardware failures.
Blocking forever on .result() can freeze alignment.
- self._mirror.moveAbs({"l": l, "s": s}).result()
- self._stage.moveAbs({"z": z}).result()
+ self._mirror.moveAbs({"l": l, "s": s}).result(timeout=self._move_timeout_s)
+ self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s)
@@
- self._stage.moveAbs({"z": z}).result()
+ self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s)
@@
- self._mirror.moveAbs({"s": s}).result()
+ self._mirror.moveAbs({"s": s}).result(timeout=self._move_timeout_s)
@@
- self._mirror.moveAbs({"l": l}).result()
+ self._mirror.moveAbs({"l": l}).result(timeout=self._move_timeout_s)Initialize the timeout once:
self._last_intensity = 0
self._last_noise = 0
self._iteration_count = 0
+ self._move_timeout_s = 30.0 # configurable safety timeoutAlso applies to: 318-318, 335-335, 352-352
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around lines 290-291 (and also at lines 318,
335, 352), the calls self._mirror.moveAbs(...).result() and
self._stage.moveAbs(...).result() block indefinitely on hardware failure;
initialize a single timeout variable (e.g. timeout = SOME_SECONDS) once at the
start of the method and pass it into each .result(timeout) call so the calls
raise on timeout instead of hanging, and handle or propagate the timeout
exception appropriately (log and abort/cleanup the alignment operation).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/odemis/acq/align/test/mirror_test.py (1)
44-54: Add tearDownClass to stop the backend and prevent resource leaks.The test starts the backend in
setUpClassbut never stops it, risking resource leaks and cross-test interference. This issue was previously flagged and remains unaddressed.src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
870-892: Add error handling for alignment task initialization.The method should handle potential failures when starting the alignment task, such as invalid UI field values or unavailable hardware components. Without error handling, the button label and state could become inconsistent if initialization fails.
Consider wrapping the task creation in a try-except block:
def _on_btn_auto_align(self, evt): """ Handle the "Auto alignment" button click. Starts or cancels the mirror auto-alignment task. If an alignment is currently running this will cancel it. Otherwise it launches a new mirror alignment using the current values from the auto-alignment settings (search range and max iterations). The button label is updated to indicate the running/cancellable state. """ if self.auto_align_future.running(): self.auto_align_future.cancel() return - self.auto_align_future = mirror_alignment( - self.tab_data_model.main.mirror, - self.tab_data_model.main.stage, - self.tab_data_model.main.ccd, - search_range=float(self.auto_align_rng.GetValue()), - max_iter=int(self.auto_align_max_iter.GetValue()), - ) - self.auto_align_future.add_done_callback(self._on_auto_align_done) - self.btn_auto_align.SetLabel("Cancel") + try: + self.auto_align_future = mirror_alignment( + self.tab_data_model.main.mirror, + self.tab_data_model.main.stage, + self.tab_data_model.main.ccd, + search_range=float(self.auto_align_rng.GetValue()), + max_iter=int(self.auto_align_max_iter.GetValue()), + ) + self.auto_align_future.add_done_callback(self._on_auto_align_done) + self.btn_auto_align.SetLabel("Cancel") + except (ValueError, TypeError) as e: + logging.exception("Failed to start auto alignment") + popup.show_message( + self.main_frame, + "Auto Alignment Error", + message=f"Failed to start auto alignment: {e}", + level=logging.ERROR + ) + except Exception: + logging.exception("Failed to start auto alignment") + popup.show_message( + self.main_frame, + "Auto Alignment Error", + message="Failed to start auto alignment. Check the hardware status.", + level=logging.ERROR + )src/odemis/acq/align/mirror.py (2)
1-21: Add a module docstring describing the mirror alignment functionality.The module lacks a docstring explaining its purpose. Add a brief description of the mirror alignment algorithms and functionality provided by this module.
Apply this diff:
# -*- coding: utf-8 -*- """ @author: Nandish Patel Copyright © 2025 Nandish Patel, Delmic +Mirror alignment algorithms for automatic calibration of mirror and stage actuators. + +Provides the mirror_alignment() function and MirrorAlignmentTask class for performing +multi-stage optimization to align mirror (l, s) and stage (z) axes based on camera +image measurements. + This file is part of Odemis.
288-289: Add timeouts to prevent indefinite hangs on hardware failures.The calls to
moveAbs().result()block indefinitely if hardware fails or gets stuck. This issue was previously flagged and remains unaddressed. This can cause the entire alignment to hang without any user feedback or ability to recover.Initialize a timeout constant and apply it to all movement calls:
def __init__( self, mirror: model.HwComponent, stage: model.HwComponent, ccd: model.HwComponent, ): """ Initialize the alignment task. :param mirror: (model.HwComponent) Mirror actuator component (axes 'l' and 's'). :param stage: (model.HwComponent) Stage actuator component (axis 'z'). :param ccd: (model.HwComponent) Camera component providing image data. """ self._mirror = mirror self._stage = stage self._ccd = ccd self._cancelled = False self._last_area = 0 self._last_intensity = 0 self._last_noise = 0 self._iteration_count = 0 + self._move_timeout = 30.0 # seconds - safety timeout for hardware movesThen update all moveAbs calls:
- self._mirror.moveAbs({"l": l, "s": s}).result() - self._stage.moveAbs({"z": z}).result() + self._mirror.moveAbs({"l": l, "s": s}).result(timeout=self._move_timeout) + self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout)Apply the same pattern to
_objective_z(line 316),_objective_s(line 333), and_objective_l(line 350).
🧹 Nitpick comments (1)
src/odemis/gui/main_xrc.py (1)
15652-15655: Add sizer flags and traversal style so the panel is visible and resizes correctly.Without flags, an empty wxPanel can end up (0,0) and not expand; add wxEXPAND (and option if it should stretch) and enable keyboard traversal.
Apply:
<object class="sizeritem"> - <object class="wxPanel" name="pnl_auto_align"> - </object> + <object class="wxPanel" name="pnl_auto_align"> + <style>wxTAB_TRAVERSAL</style> + </object> + <flag>wxEXPAND</flag> + <!-- If this panel should take remaining space, also add: --> + <!-- <option>1</option> --> </object>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(3 hunks)src/odemis/gui/main_xrc.py(2 hunks)src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
PR: delmic/odemis#3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/main_xrc.py
🧬 Code graph analysis (3)
src/odemis/acq/align/test/mirror_test.py (2)
src/odemis/model/_components.py (2)
model(570-571)role(338-342)src/odemis/acq/align/mirror.py (1)
mirror_alignment(35-67)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (4)
src/odemis/acq/align/mirror.py (2)
mirror_alignment(35-67)cancel(109-122)src/odemis/gui/comp/settings.py (1)
SettingsPanel(52-586)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)src/odemis/driver/semcomedi.py (7)
cancel(2008-2009)cancel(2098-2142)cancel(2213-2214)cancel(2316-2332)cancel(2451-2492)cancel(2520-2521)CancelledError(112-116)
src/odemis/acq/align/mirror.py (1)
src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
🪛 Ruff (0.14.1)
src/odemis/acq/align/test/mirror_test.py
72-72: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
73-73: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
74-74: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
870-870: Unused method argument: evt
(ARG002)
src/odemis/acq/align/mirror.py
109-109: Unused method argument: future
(ARG002)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
283-283: Ambiguous variable name: l
(E741)
284-284: Ambiguous variable name: l
(E741)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
338-338: Ambiguous variable name: l
(E741)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
357-357: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
367-367: Avoid specifying long messages outside the exception class
(TRY003)
370-370: Avoid specifying long messages outside the exception class
(TRY003)
372-372: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (2)
src/odemis/gui/main_xrc.py (1)
1306-1306: Wiring LGTM; add a fast‑fail check to avoid silent None.Binding is correct. Consider asserting the control exists so layout regressions surface early.
Example:
assert self.pnl_auto_align is not None, "XRC: pnl_auto_align not found"src/odemis/acq/align/mirror.py (1)
479-523: Verify that the search range reduction strategy is appropriate.The search range is halved twice during the alignment procedure (lines 479 and 523), resulting in a final range that is 1/4 of the initial value. This aggressive reduction might cause the algorithm to fail if the initial alignment is significantly off or if the first stages don't converge well.
Consider whether this reduction schedule is based on empirical testing, or if a more conservative approach (e.g., reducing by a smaller factor like 1.5x or keeping the same range) might be more robust.
If the current schedule is empirically validated, consider documenting the rationale in comments:
# Halve search range for refinement stage (empirically determined for typical misalignments) search_range /= 2
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
2 times, most recently
from
a901b4d to
75646ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
869-891: Harden auto-alignment start: validate inputs, handle init failures, and disable inputs while running.Wrap task creation in try/except, validate UI values, set consistent state on failure, disable inputs on start. Also silence unused event arg. This mirrors a prior review note.
Apply this diff:
- def _on_btn_auto_align(self, evt): + def _on_btn_auto_align(self, _evt): @@ - if self.auto_align_future.running(): + if self.auto_align_future.running(): self.auto_align_future.cancel() return - self.auto_align_future = mirror_alignment( - self.tab_data_model.main.mirror, - self.tab_data_model.main.stage, - self.tab_data_model.main.ccd, - search_range=float(self.auto_align_rng.GetValue()), - max_iter=int(self.auto_align_max_iter.GetValue()), - ) - self.auto_align_future.add_done_callback(self._on_auto_align_done) - self.btn_auto_align.SetLabel("Cancel") + try: + rng = float(self.auto_align_rng.GetValue()) + iters = int(self.auto_align_max_iter.GetValue()) + if not (10e-6 <= rng <= 100e-6): + raise ValueError("Search range out of bounds") + if not (100 <= iters <= 500): + raise ValueError("Max iterations out of bounds") + self.auto_align_future = mirror_alignment( + self.tab_data_model.main.mirror, + self.tab_data_model.main.stage, + self.tab_data_model.main.ccd, + search_range=rng, + max_iter=iters, + ) + self.auto_align_future.add_done_callback(self._on_auto_align_done) + self.btn_auto_align.SetLabel("Cancel") + # Disable parameter inputs while running + self.auto_align_rng.Enable(False) + self.auto_align_max_iter.Enable(False) + except Exception: + logging.exception("Failed to start auto alignment") + self.auto_align_future = model.InstantaneousFuture() + popup.show_message( + self.main_frame, + "Auto Alignment Error", + message="Failed to start auto alignment. Check the hardware status and parameters.", + level=logging.ERROR, + ) + self.btn_auto_align.SetLabel("Run")
🧹 Nitpick comments (6)
src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc (1)
1003-1006: Add sizer flags for proper layout behavior.The empty placeholder panel lacks sizer flags, which may prevent it from displaying or allocating space correctly. Compare with other sizeritem elements in this file (e.g., lines 255-402), which typically include flags like
wxEXPANDand border specifications.Apply this diff to add layout flags:
<object class="sizeritem"> <object class="wxPanel" name="pnl_auto_align"> </object> + <flag>wxTOP|wxBOTTOM|wxEXPAND</flag> + <border>5</border> </object>Optionally, consider adding basic styling for consistency with other panels in the Mirror section:
<object class="sizeritem"> <object class="wxPanel" name="pnl_auto_align"> + <fg>#E5E5E5</fg> + <bg>#444444</bg> </object> <flag>wxTOP|wxBOTTOM|wxEXPAND</flag> <border>5</border> </object>src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
892-912: Guard against stale done-callback and re-enable inputs.If a new run starts before the previous callback fires, avoid resetting the new button state; also re-enable inputs.
Apply this diff:
def _on_auto_align_done(self, f): @@ - finally: - wx.CallAfter(self.btn_auto_align.SetLabel, "Run") + finally: + # Only reset UI if this callback matches the current future + if f is self.auto_align_future: + wx.CallAfter(self.btn_auto_align.SetLabel, "Run") + wx.CallAfter(self.auto_align_rng.Enable, True) + wx.CallAfter(self.auto_align_max_iter.Enable, True) + else: + logging.debug("Stale auto-align done callback ignored")src/odemis/acq/align/mirror.py (4)
164-221: Remove large commented-out alternative implementation.Dead/commented code adds noise. Keep it in git history.
Apply this diff:
- # def _get_spot_measurement(self) -> Tuple[float, float, float]: - # """ - # Measure the spot quality based on 2D FWHM and peak intensity, - # without assuming a single central peak. - # """ - # if self._cancelled: - # raise CancelledError("Alignment was cancelled by user") - # ... - # return float(spot_area), spot_intensity, float(noise)
109-121: Silence unused cancel() parameter.Rename to underscore to satisfy linters; behavior unchanged.
Apply this diff:
- def cancel(self, future): + def cancel(self, _future):
70-82: Minor docs: replace en dash and clarify bounds usage.Use a hyphen-minus in “Nelder-Mead” and (optionally) note bounds are enforced in the objective, not by the optimizer.
Apply this diff:
- - Two successive 3D Nelder–Mead refinements over [l, s, z] combining + - Two successive 3D Nelder-Mead refinements over [l, s, z] combining
1-5: E741 (ambiguous ‘l’) noise: file-level ignore.Axis name ‘l’ is part of the domain; prefer a file-level ignore to avoid disruptive renames.
Apply this diff:
# -*- coding: utf-8 -*- +# ruff: noqa=E741Also applies to: 283-286, 372-372
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(3 hunks)src/odemis/gui/main_xrc.py(2 hunks)src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
PR: delmic/odemis#3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/main_xrc.py
🧬 Code graph analysis (3)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (2)
model(570-571)HwComponent(277-542)src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (3)
src/odemis/acq/align/mirror.py (2)
mirror_alignment(35-67)cancel(109-122)src/odemis/gui/comp/settings.py (1)
SettingsPanel(52-586)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)
src/odemis/acq/align/test/mirror_test.py (2)
src/odemis/acq/align/mirror.py (1)
mirror_alignment(35-67)src/odemis/util/testing.py (1)
stop_backend(135-169)
🪛 Ruff (0.14.1)
src/odemis/acq/align/mirror.py
109-109: Unused method argument: future
(ARG002)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
283-283: Ambiguous variable name: l
(E741)
284-284: Ambiguous variable name: l
(E741)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
338-338: Ambiguous variable name: l
(E741)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
357-357: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
367-367: Avoid specifying long messages outside the exception class
(TRY003)
370-370: Avoid specifying long messages outside the exception class
(TRY003)
372-372: Ambiguous variable name: l
(E741)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
869-869: Unused method argument: evt
(ARG002)
src/odemis/acq/align/test/mirror_test.py
76-76: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
77-77: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
78-78: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (3)
src/odemis/gui/main_xrc.py (2)
1306-1306: Control reference properly utilized and integrated.The verification confirms that
pnl_auto_alignis correctly defined inmain_xrc.py(line 1306) and actively used insparc2_align_tab.py(line 843) as part of the SettingsPanel initialization. The widget is properly accessed through the panel hierarchy and integrated into the auto-alignment UI.
15652-15655: ****The empty
pnl_auto_alignpanel is intentional. The panel is auto-generated from XRC resources and is designed as a placeholder container. Insparc2_align_tab.py(line 843), aSettingsPanelinstance is created with this empty panel as its parent, andSettingsPanelpopulates it with controls at runtime. This is a legitimate dynamic UI pattern.Likely an incorrect or invalid review comment.
src/odemis/acq/align/test/mirror_test.py (1)
91-99: The referenced second location (lines 120-125) does not contain anyf.result()calls and does not apply to this review.Lines 120-125 contain success rate computation and a final assertion—no futures are awaited there. This location reference is incorrect. The primary suggestion for lines 91-99 is valid (adding a timeout to avoid indefinite hangs), but the "Also applies to" claim is factually wrong.
Additionally, the review overlooks two other unguarded
.result()calls on lines 85–86 (mirror and stage movement operations) that could also hang indefinitely and should receive the same timeout treatment for test resilience.Likely an incorrect or invalid review comment.
src/odemis/acq/align/mirror.py
Outdated
| self._mirror.moveAbs({"l": l, "s": s}).result() | ||
| self._stage.moveAbs({"z": z}).result() | ||
|
|
There was a problem hiding this comment.
Add movement timeouts to prevent hangs on hardware failures.
.result() without timeouts can block indefinitely and freeze alignment. Introduce a configurable timeout and use it for all movements.
Apply this diff:
@@
def __init__(
self,
mirror: model.HwComponent,
stage: model.HwComponent,
ccd: model.HwComponent,
):
@@
self._last_intensity = 0
self._last_noise = 0
self._iteration_count = 0
+ self._move_timeout_s = 30.0 # safety timeout for actuator moves
@@
- self._mirror.moveAbs({"l": l, "s": s}).result()
- self._stage.moveAbs({"z": z}).result()
+ self._mirror.moveAbs({"l": l, "s": s}).result(timeout=self._move_timeout_s)
+ self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s)
@@
- self._stage.moveAbs({"z": z}).result()
+ self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s)
@@
- self._mirror.moveAbs({"s": s}).result()
+ self._mirror.moveAbs({"s": s}).result(timeout=self._move_timeout_s)
@@
- self._mirror.moveAbs({"l": l}).result()
+ self._mirror.moveAbs({"l": l}).result(timeout=self._move_timeout_s)Also applies to: 316-316, 333-334, 350-350, 87-108
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around lines 288-290 (and also update lines
316, 333-334, 350, and 87-108), the current calls to .result() can block
indefinitely on hardware failures; add a configurable movement timeout (e.g.,
self.move_timeout set in the class constructor with a sensible default) and pass
that timeout to the movement wait/result calls (or wrap the future in a timed
wait) so all hardware moves use the timeout; update every move call in the file
to use this new timeout parameter and surface/configure it where the class is
instantiated.
75646ba to
f91a9a3
Compare
src/odemis/acq/align/mirror.py
Outdated
| z0 = self._stage.position.value["z"] | ||
| l_range = (l0 - search_range, l0 + search_range) | ||
| s_range = (s0 - search_range, s0 + search_range) | ||
| z_range = (z0 - search_range, z0 + search_range) |
There was a problem hiding this comment.
Be aware that as you update l0, s0, and z0 based on the intermediary new location, the range might actually get outside of the original bound. For instance, if you define the z range as -50->50 at init, and on the first z optimisation it find 50, now the z_range will be 25->75. That's bigger than the range given as input. => It might be safer to clip the new ranges to be within the original (large) range.
src/odemis/acq/align/mirror.py
Outdated
| REF_ALIGNED_AREA = 200 | ||
| MAX_MISALIGNED_AREA = 10000 | ||
| MAX_ALIGNED_INTENSITY = 50000 |
src/odemis/acq/align/mirror.py
Outdated
| - Two successive 3D Nelder-Mead refinements over [l, s, z] combining | ||
| normalized spot area and intensity into a single score. | ||
| """ | ||
| REF_ALIGNED_AREA = 200 |
There was a problem hiding this comment.
As discussed, for finding the good area size, it might be better to use a constant in meters, and convert it to pixel based on the pixel size of the image
src/odemis/acq/align/mirror.py
Outdated
|
|
||
| noise = 1.3 * numpy.median(all_corners) | ||
| spot_area = numpy.count_nonzero(image > noise) | ||
| spot_intensity = int(image.max()) |
There was a problem hiding this comment.
As discussed, it's little bit dangerous to take the max as-is. It's more reliable to take the Nth maximum value. You can use like 0.1% or even just the 10th pixel (which is what we use for the streak cam).
(BTW, I couldn't find example code that exactly does it).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/odemis/acq/align/mirror.py (4)
158-161: Use a robust intensity metric (Nth-max or high percentile) instead of raw max.Raw max is noise‑sensitive; prefer the 99.9th percentile or 10th highest pixel, as already suggested.
- spot_intensity = int(image.max()) + # Robust peak estimate (less sensitive to hot pixels) + spot_intensity = int(numpy.percentile(image, 99.9))Alternatively: use numpy.partition(image.ravel(), -10)[-10].
164-221: Remove large commented‑out alternative implementation.Keep history in VCS; don’t retain dead code in-tree.
- # def _get_spot_measurement(self) -> Tuple[float, float, float]: - # """ - # Measure the spot quality based on 2D FWHM and peak intensity, - # without assuming a single central peak. - # """ - # if self._cancelled: - # raise CancelledError("Alignment was cancelled by user") - # ... - # return float(spot_area), spot_intensity, float(noise)
100-109: Add movement timeouts to prevent hangs on hardware failures..result() without a timeout can block indefinitely if an actuator stalls; cancellation won’t help while blocked. Introduce a configurable timeout and use it for all moves.
@@ self._last_intensity = 0 self._last_noise = 0 self._iteration_count = 0 + # Safety timeout for actuator moves (seconds); tune per system if needed + self._move_timeout_s = 30.0 @@ - self._mirror.moveAbs({"l": l, "s": s}).result() - self._stage.moveAbs({"z": z}).result() + self._mirror.moveAbs({"l": l, "s": s}).result(timeout=self._move_timeout_s) + self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s) @@ - self._stage.moveAbs({"z": z}).result() + self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s) @@ - self._mirror.moveAbs({"s": s}).result() + self._mirror.moveAbs({"s": s}).result(timeout=self._move_timeout_s) @@ - self._mirror.moveAbs({"l": l}).result() + self._mirror.moveAbs({"l": l}).result(timeout=self._move_timeout_s)Optionally catch TimeoutError and abort gracefully with logging.
Also applies to: 288-289, 316-316, 333-334, 350-351
429-433: Clamp per-stage iteration budgets; never pass non-positive maxiter.Subtracting nit can drive max_iter ≤ 0 and some optimizers error on non-positive budgets. Clamp inputs to ≥1 and clamp the running budget after each stage.
@@ - max_iter -= l_result.nit - if max_iter < 1: + max_iter = max(0, int(max_iter) - int(getattr(l_result, "nit", 0))) + if max_iter < 1: return @@ - options={ - "maxiter": max_iter, - }, + options={"maxiter": max(1, int(max_iter))}, @@ - max_iter -= s_result.nit - if max_iter < 1: + max_iter = max(0, int(max_iter) - int(getattr(s_result, "nit", 0))) + if max_iter < 1: return @@ - options={ - "maxiter": max_iter, - }, + options={"maxiter": max(1, int(max_iter))}, @@ - max_iter -= z_result.nit - if max_iter < 1: + max_iter = max(0, int(max_iter) - int(getattr(z_result, "nit", 0))) + if max_iter < 1: return @@ - options={ - "maxiter": max_iter, - "adaptive": True, - }, + options={"maxiter": max(1, int(max_iter)), "adaptive": True}, @@ - max_iter -= lsz_result.nit - if max_iter < 1: + max_iter = max(0, int(max_iter) - int(getattr(lsz_result, "nit", 0))) + if max_iter < 1: return @@ - options={ - "maxiter": max_iter, - "adaptive": True, - }, + options={"maxiter": max(1, int(max_iter)), "adaptive": True},Also applies to: 439-446, 458-461, 467-474, 487-491, 514-526, 542-546, 569-580
🧹 Nitpick comments (10)
src/odemis/acq/align/mirror.py (6)
283-287: Avoid ambiguous variable name “l” (Ruff E741); rename locals while keeping axis key.Rename local scalar to ell/l_val; keep {"l": ...} key unchanged.
@@ - l, s, z = xk - l = float(numpy.clip(l, l_range[0], l_range[1]))) + l_val, s, z = xk + l_val = float(numpy.clip(l_val, l_range[0], l_range[1])) @@ - self._mirror.moveAbs({"l": l, "s": s}).result(timeout=self._move_timeout_s) + self._mirror.moveAbs({"l": l_val, "s": s}).result(timeout=self._move_timeout_s) @@ - def _objective_l(self, l): + def _objective_l(self, ell): @@ - self._mirror.moveAbs({"l": l}).result(timeout=self._move_timeout_s) + self._mirror.moveAbs({"l": ell}).result(timeout=self._move_timeout_s) @@ - l, s, z = xk + l_val, s, z = xk logging.debug( - f"Iter {self._iteration_count}: l={l:.6f} m, s={s:.6f} m, z={z:.6f} m | " + f"Iter {self._iteration_count}: l={l_val:.6f} m, s={s:.6f} m, z={z:.6f} m | "Also applies to: 338-353, 372-376
514-521: Nelder‑Mead ignores bounds; drop the unused bounds argument to avoid confusion.Bounds aren’t enforced for Nelder‑Mead; clipping in the objective already constrains the search.
- lsz_result = minimize( + lsz_result = minimize( fun=self._objective_lsz, x0=initial_guess, args=(l_range, s_range, z_range, weight), method="Nelder-Mead", - bounds=(l_range, s_range, z_range), callback=self._callback_lsz, options={"maxiter": max(1, int(max_iter)), "adaptive": True}, ) @@ - lsz_result = minimize( + lsz_result = minimize( fun=self._objective_lsz, x0=initial_guess, args=(l_range, s_range, z_range, weight), method="Nelder-Mead", - bounds=(l_range, s_range, z_range), callback=self._callback_lsz, options={"maxiter": max(1, int(max_iter)), "adaptive": True}, )Also applies to: 569-576
139-141: Silence TRY003: don’t attach long messages to CancelledError.Keep it simple; messages add little and trip linters.
- raise CancelledError("Alignment was cancelled by user") + raise CancelledError()Also applies to: 277-279, 313-315, 330-332, 347-349, 366-368
109-122: Unused parameter in cancel(); rename to _future to appease linters.No behavior change.
- def cancel(self, future): + def cancel(self, _future):
3-21: Add a one‑line module summary to the top docstring.Briefly state the module’s responsibility.
""" @author: Nandish Patel @@ -Odemis. If not, see http://www.gnu.org/licenses/. +Odemis. If not, see http://www.gnu.org/licenses/. + +Mirror alignment algorithms to automatically calibrate mirror (l, s) and stage (z) +using camera‑based spot metrics; includes a cancellable multi‑stage optimizer. """
355-361: Replace en dash with hyphen in docstring (“Nelder–Mead” → “Nelder-Mead”).Avoid RUF002.
- Optimizer callback executed after each Nelder–Mead iteration. + Optimizer callback executed after each Nelder-Mead iteration.src/odemis/acq/align/test/mirror_test.py (4)
62-64: Add timeouts to .result() in tests to avoid indefinite hangs.Keeps CI from stalling on simulator glitches.
- self.stage.moveAbs({"z": self.aligned_pos["z"]}).result() - self.mirror_xy.moveAbs({"x": self.aligned_pos["x"], "y": self.aligned_pos["y"]}).result() + self.stage.moveAbs({"z": self.aligned_pos["z"]}).result(timeout=60) + self.mirror_xy.moveAbs({"x": self.aligned_pos["x"], "y": self.aligned_pos["y"]}).result(timeout=60) @@ - self.mirror.moveRel({"l": dl, "s": ds}).result() - self.stage.moveRel({"z": dz}).result() + self.mirror.moveRel({"l": dl, "s": ds}).result(timeout=60) + self.stage.moveRel({"z": dz}).result(timeout=60) @@ - f.result() + f.result(timeout=180)Also applies to: 87-88, 93-95
114-115: Avoid calling setUp() directly; extract a small reset helper instead.Directly invoking setUp can surprise future maintainers. Move reset code into a private helper and call it.
- # Reset for next test - self.setUp() + # Reset for next test + self._reset_to_aligned()And add:
def _reset_to_aligned(self): self.stage.moveAbs({"z": self.aligned_pos["z"]}).result(timeout=60) self.mirror_xy.moveAbs({"x": self.aligned_pos["x"], "y": self.aligned_pos["y"]}).result(timeout=60)
74-79: Optional: silence S311 for non-crypto randomness in tests.If S311 is enforced, annotate to suppress.
- random.seed(25) + random.seed(25) # noqa: S311 - test determinism only @@ - dl = random.uniform(-rng, rng) - ds = random.uniform(-rng, rng) - dz = random.uniform(-rng, rng) + dl = random.uniform(-rng, rng) # noqa: S311 + ds = random.uniform(-rng, rng) # noqa: S311 + dz = random.uniform(-rng, rng) # noqa: S311
69-71: Consider making the success criterion relative to pre-align intensity.A fixed threshold can be brittle across simulators/cameras; comparing improvement or using a percentile of baseline intensities is more robust. I can draft this if desired.
Also applies to: 101-107, 117-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (2)
model(570-571)HwComponent(277-542)src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
src/odemis/acq/align/test/mirror_test.py (2)
src/odemis/acq/align/mirror.py (1)
mirror_alignment(35-67)src/odemis/util/testing.py (1)
stop_backend(135-169)
🪛 Ruff (0.14.1)
src/odemis/acq/align/mirror.py
109-109: Unused method argument: future
(ARG002)
140-140: Avoid specifying long messages outside the exception class
(TRY003)
278-278: Avoid specifying long messages outside the exception class
(TRY003)
281-281: Avoid specifying long messages outside the exception class
(TRY003)
283-283: Ambiguous variable name: l
(E741)
284-284: Ambiguous variable name: l
(E741)
314-314: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
338-338: Ambiguous variable name: l
(E741)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
357-357: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
367-367: Avoid specifying long messages outside the exception class
(TRY003)
370-370: Avoid specifying long messages outside the exception class
(TRY003)
372-372: Ambiguous variable name: l
(E741)
src/odemis/acq/align/test/mirror_test.py
77-77: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
78-78: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
79-79: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
6b75bdb to
30511bf
Compare
[update] address initial pr comments [update] changes after first system test - Introduced a stop_early flag which attempts to stop the Nelder-Mead stages early if progress stalls - updated weight for 2nd l, s, z alignment; More weight to spot pixel count than peak intensity - updated MIN_PIXEL_COUNT value to more realistic 25
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
30511bf to
1100f6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/odemis/acq/align/mirror.py (2)
178-235: Remove the commented-out alternative implementation.This 58-line block of commented-out FWHM-based code should be removed to keep the codebase clean. If this approach might be needed later, it can be preserved in version control history.
Apply this diff:
return spot_pixel_count, spot_intensity, noise - # def _get_spot_measurement(self) -> Tuple[float, float, float]: - # """ - # Measure the spot quality based on 2D FWHM and peak intensity, - # without assuming a single central peak. - # """ - # if self._cancelled: - # raise CancelledError("Alignment was cancelled by user") - - # image = self._ccd.data.get(asap=False) - - # # Estimate background noise from corners - # corner_size = 5 - # corners = numpy.concatenate([ - # image[:corner_size, :corner_size].ravel(), - # image[:corner_size, -corner_size:].ravel(), - # image[-corner_size:, :corner_size].ravel(), - # image[-corner_size:, -corner_size:].ravel(), - # ]) - # noise = 1.3 * numpy.median(corners) - - # # Compute half-max threshold - # peak = image.max() - # half_max = noise + 0.5 * (peak - noise) - - # # Create binary mask above half-maximum - # mask = image >= half_max - # if not numpy.any(mask): - # return 0.0, float(peak), float(noise) - - # # Compute FWHM along x and y (width of mask projection) - # # Project the mask along each axis - # proj_x = mask.any(axis=0) - # proj_y = mask.any(axis=1) - - # # FWHM = width (count of continuous True values) - # def contiguous_width(projection: numpy.ndarray) -> int: - # """Return width of the largest contiguous True region.""" - # if not numpy.any(projection): - # return 0 - # # Identify start and end of each contiguous True segment - # diff = numpy.diff(numpy.concatenate([[0], projection.view(numpy.int8), [0]])) - # starts = numpy.where(diff == 1)[0] - # ends = numpy.where(diff == -1)[0] - # widths = ends - starts - # return int(widths.max()) if len(widths) > 0 else 0 - - # fwhm_x = contiguous_width(proj_x) - # fwhm_y = contiguous_width(proj_y) - - # # Fallback to max if one axis fails - # if fwhm_x == 0 or fwhm_y == 0: - # fwhm_x = fwhm_y = max(fwhm_x, fwhm_y) - - # # Spot pixel count and intensity - # spot_pixel_count = fwhm_x * fwhm_y - # spot_intensity = int(peak) - - # return int(spot_pixel_count), spot_intensity, float(noise) - def _get_score(self, pixel_count: int, intensity: int, weight=0.5):
302-303: Add timeouts to hardware movement calls to prevent indefinite hangs.The
.result()calls on lines 302-303 (and similar calls at lines 331, 348, 365) block indefinitely if hardware fails or gets stuck. This can freeze the entire alignment procedure without user feedback.Add a configurable timeout attribute in
__init__:self._last_score = 0 self._last_xk = numpy.zeros(3) self._iteration_count = 0 self._stall_count = 0 + self._move_timeout_s = 30.0 # Safety timeout for actuator movesThen use it in all movement calls:
- self._mirror.moveAbs({"l": l, "s": s}).result() - self._stage.moveAbs({"z": z}).result() + self._mirror.moveAbs({"l": l, "s": s}).result(timeout=self._move_timeout_s) + self._stage.moveAbs({"z": z}).result(timeout=self._move_timeout_s)Apply the same pattern to lines 331, 348, and 365.
🧹 Nitpick comments (2)
src/odemis/acq/align/mirror.py (2)
174-174: Consider using the Nth highest pixel value instead of the absolute maximum.Using
image.max()directly can be unreliable due to hot pixels or noise spikes. A more robust approach is to use the Nth highest value (e.g., the 10th highest pixel or the 99.9th percentile).Based on learnings
Apply this diff:
noise = 1.3 * numpy.median(all_corners) spot_pixel_count = numpy.count_nonzero(image > noise) - spot_intensity = int(image.max()) + # Use Nth highest value for robustness against hot pixels + flat = image.ravel() + flat.sort() + spot_intensity = int(flat[-10]) # 10th highest pixelAlternatively, use
numpy.percentile(image, 99.9)for a percentile-based approach.
88-90: Consider defining spot size limits in physical units.The current pixel-based constants (MIN_PIXEL_COUNT, MAX_PIXEL_COUNT) are camera-dependent. For better robustness across different camera configurations, consider defining these in physical units (meters) and converting to pixels based on the camera's pixel size metadata.
Based on learnings
For example:
# Physical spot size limits MIN_SPOT_SIZE_M = 5e-6 # 5 µm MAX_SPOT_SIZE_M = 500e-6 # 500 µm # Convert to pixels in _get_spot_measurement using MD_PIXEL_SIZE pixel_size = self._ccd.pixelSize.value # or from metadata min_pixels = int((MIN_SPOT_SIZE_M / pixel_size[0]) ** 2) max_pixels = int((MAX_SPOT_SIZE_M / pixel_size[0]) ** 2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(3 hunks)src/odemis/gui/main_xrc.py(2 hunks)src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
PR: delmic/odemis#3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/main_xrc.py
🧬 Code graph analysis (3)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (5)
src/odemis/acq/align/mirror.py (2)
mirror_alignment(35-72)cancel(123-136)src/odemis/gui/comp/settings.py (1)
SettingsPanel(52-586)src/odemis/gui/util/widgets.py (2)
AxisConnector(131-256)ProgressiveFutureConnector(260-392)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)src/odemis/driver/andorcam3.py (1)
CancelledError(67-71)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (2)
model(570-571)HwComponent(277-542)src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
src/odemis/acq/align/test/mirror_test.py (2)
src/odemis/acq/align/mirror.py (1)
mirror_alignment(35-72)src/odemis/util/testing.py (1)
stop_backend(135-169)
🪛 Ruff (0.14.2)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
872-872: Unused method argument: evt
(ARG002)
src/odemis/acq/align/mirror.py
123-123: Unused method argument: future
(ARG002)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Avoid specifying long messages outside the exception class
(TRY003)
295-295: Avoid specifying long messages outside the exception class
(TRY003)
297-297: Ambiguous variable name: l
(E741)
298-298: Ambiguous variable name: l
(E741)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
353-353: Ambiguous variable name: l
(E741)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
382-382: Avoid specifying long messages outside the exception class
(TRY003)
385-385: Avoid specifying long messages outside the exception class
(TRY003)
394-394: Avoid specifying long messages outside the exception class
(TRY003)
397-397: Ambiguous variable name: l
(E741)
src/odemis/acq/align/test/mirror_test.py
77-77: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
78-78: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
79-79: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (2)
src/odemis/gui/main_xrc.py (2)
1306-1306: LGTM! Control registration follows the standard pattern.The registration of
pnl_auto_alignfollows the established XRC control pattern and is correctly placed in the initialization sequence.
15652-15655: LGTM! Empty panel serves as a placeholder container.The empty
pnl_auto_alignpanel definition is intentional and serves as a container that will be populated with content from the separate XRC layout file (panel_tab_sparc2_align.xrc), as indicated in the related changes.
…ment specific for SPARC
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
676bc81 to
352e6e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/odemis/acq/align/mirror.py (2)
687-688: Add movement timeouts to prevent indefinite hangs.Multiple past reviews flagged that calling
.result()without a timeout can hang indefinitely if hardware fails. This is still not addressed and is a critical reliability issue.Apply this diff:
+ # Timeout for hardware movements (typical moves complete in <10s) + MOVE_TIMEOUT = 30.0 # seconds + - self._mirror.moveAbs({"l": l, "s": s}).result() - self._stage.moveAbs({"z": z}).result() + try: + self._mirror.moveAbs({"l": l, "s": s}).result(timeout=MOVE_TIMEOUT) + self._stage.moveAbs({"z": z}).result(timeout=MOVE_TIMEOUT) + except TimeoutError: + raise CancelledError("Hardware movement timed out during alignment")Apply the same pattern to line 715 in
_objective_1d:- hw_comp.moveAbs({axis: pos}).result() + try: + hw_comp.moveAbs({axis: pos}).result(timeout=MOVE_TIMEOUT) + except TimeoutError: + raise CancelledError(f"Hardware movement timed out during {axis} alignment")Consider adding
MOVE_TIMEOUTas a class constant or__init__parameter for configurability.
559-559: Use a more robust peak intensity measurement.Taking
image.max()directly can be unreliable due to hot pixels or cosmic rays. A past reviewer suggested using the Nth maximum value (e.g., 10th pixel or 0.1 percentile) for better robustness.Consider using:
- spot_intensity = int(image.max()) + # Use 10th highest pixel to avoid hot pixels/cosmic rays + flat = numpy.sort(image.ravel()) + spot_intensity = int(flat[-10] if len(flat) >= 10 else flat[-1])Or use a percentile:
- spot_intensity = int(image.max()) + # Use 99.9th percentile to avoid outliers + spot_intensity = int(numpy.percentile(image, 99.9))
🧹 Nitpick comments (4)
src/odemis/acq/align/mirror.py (4)
85-85: Consider moving RNG initialization inside the function body.The static analysis correctly flags that calling
numpy.random.default_rng(7)in the default argument is evaluated once at function definition time, which can cause subtle issues if the function is called multiple times expecting independent generators.Refactor to:
def _custom_minimize_neldermead(func, x0, args=(), callback=None, maxiter=None, maxfev=None, disp=False, return_all=False, initial_simplex=None, xatol=1e-4, fatol=1e-3, adaptive=False, bounds=None, - min_step_size=None, rng=numpy.random.default_rng(7), + min_step_size=None, rng=None, **unknown_options): """ ... """ + if rng is None: + rng = numpy.random.default_rng(7)
435-438: Add physical units to all constants for clarity.The constants mix different unit systems without clear documentation. Based on past reviews and the context:
Based on past reviews
Apply this diff to clarify units:
- MIN_SEARCH_RANGE = 5e-6 # [μm] - MIN_PIXEL_COUNT = 25 # [px] - MAX_PIXEL_COUNT = 10000 # [px] - MAX_INTENSITY = 50000 # [a.u.] + MIN_SEARCH_RANGE = 5e-6 # [m] - minimum search range in meters + MIN_PIXEL_COUNT = 25 # [px] - minimum expected spot size in pixels + MAX_PIXEL_COUNT = 10000 # [px] - maximum expected spot size in pixels + MAX_INTENSITY = 50000 # [counts] - maximum expected camera intensity
563-620: Remove commented-out alternative implementation.This 58-line commented block was flagged in previous reviews and clutters the code. If needed later, it's preserved in version control.
Remove lines 563-620.
759-773: Document themin_step_sizeparameter in the docstring.The
align_mirrormethod accepts amin_step_sizeparameter with a default value of(1e-6, 1e-6, 1e-6), but it's not documented in the docstring. This makes it unclear what units are expected and what the parameter controls.Add to the docstring:
:param search_range: Initial half-range (metres) for coarse searches. :param max_iter: Total maximum iterations budget (shared between stages). + :param min_step_size: Tuple of minimum step sizes [m] for (l, s, z) axes. + Used by the Nelder-Mead optimizer to snap candidate + positions to a grid, preventing oscillation at the + hardware's discrete step resolution. + Defaults to (1e-6, 1e-6, 1e-6). """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/align/mirror.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (2)
model(570-571)HwComponent(277-542)src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
🪛 Ruff (0.14.3)
src/odemis/acq/align/mirror.py
85-85: Do not perform function call numpy.random.default_rng in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
155-155: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
165-165: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Avoid specifying long messages outside the exception class
(TRY003)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
471-471: Unused method argument: future
(ARG002)
507-507: Avoid specifying long messages outside the exception class
(TRY003)
677-677: Avoid specifying long messages outside the exception class
(TRY003)
680-680: Avoid specifying long messages outside the exception class
(TRY003)
682-682: Ambiguous variable name: l
(E741)
683-683: Ambiguous variable name: l
(E741)
713-713: Avoid specifying long messages outside the exception class
(TRY003)
737-737: Avoid specifying long messages outside the exception class
(TRY003)
740-740: Avoid specifying long messages outside the exception class
(TRY003)
749-749: Avoid specifying long messages outside the exception class
(TRY003)
752-752: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: lint
…d 3d; normalize intensity for 1d objective
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
055f434 to
9ae1420
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/odemis/acq/align/mirror.py (1)
847-848: Add timeout parameter to hardware movements.The
.result()calls on lines 847-848 can block indefinitely if hardware hangs. Despite past reviews indicating this was addressed, no timeout is visible in the current code.Apply this diff:
+ # Hardware movement timeout to prevent indefinite hangs + MOVE_TIMEOUT = 30.0 # seconds + - self._mirror.moveAbs({"l": l, "s": s}).result() - self._stage.moveAbs({"z": z}).result() + self._mirror.moveAbs({"l": l, "s": s}).result(timeout=MOVE_TIMEOUT) + self._stage.moveAbs({"z": z}).result(timeout=MOVE_TIMEOUT)Consider adding
MOVE_TIMEOUTas a class constant or__init__parameter for configurability.
🧹 Nitpick comments (5)
src/odemis/acq/align/test/mirror_test.py (2)
778-778: Consider using Nth maximum pixel value instead of absolute maximum.Taking the absolute maximum pixel value can be sensitive to hot pixels, noise spikes, or cosmic rays. A more robust approach is to use the Nth highest pixel value (e.g., 10th highest or top 0.1%).
Apply this diff:
- spot_intensity = int(image.max()) + # Use Nth maximum (10th highest pixel) for robustness against hot pixels + flat_image = image.ravel() + if len(flat_image) >= 10: + spot_intensity = int(numpy.partition(flat_image, -10)[-10]) + else: + spot_intensity = int(image.max())Based on past review comments.
690-703: Consider prefixing unused parameter with underscore or adding a comment.The
futureparameter is flagged as unused but may be required by thetask_cancellerprotocol. If it's part of the interface contract, consider either prefixing with_or adding a comment to clarify.- def cancel(self, future): + def cancel(self, _future): """ Request cancellation of the running task. - The method sets an internal flag which is checked throughout the task. - The future wrapper expects a boolean return value indicating whether the - cancellation request was accepted. + :param _future: (concurrent.futures.Future) The future requesting cancel (unused, required by protocol). - :param future: (concurrent.futures.Future) The future requesting cancel. :returns: bool True if the cancellation request was accepted. """src/odemis/acq/align/mirror.py (3)
600-642: Document units more explicitly in parameter descriptions.While the docstring mentions "(in metres)" for
search_range, themin_step_sizeparameter description should also explicitly state units for clarity.Apply this diff:
:param min_step_size : Tuple[float, float, float], optional - Per-axis minimum hardware step sizes (metres) for [l, s, z]. Used by + Per-axis minimum hardware step sizes in metres for [l, s, z]. Used by probabilistic snapping to round candidate positions to discrete actuator steps. Defaults to (1.5e-6, 1.5e-6, 2e-6).Based on learnings
658-661: Consider making constants configurable or document their derivation.The hardcoded constants (MIN_PIXEL_COUNT=25, MAX_PIXEL_COUNT=10000, MAX_INTENSITY=50000) may not be universally applicable across different camera sensors or optical configurations. Either make them configurable parameters or add comments explaining their derivation.
778-778: Replaceimage.max()with more robust Nth-maximum to avoid noise spikes.Using the absolute maximum pixel value can be sensitive to hot pixels or noise spikes. Consider using the Nth maximum (e.g., 10th highest pixel or 0.1% percentile) for more robust intensity measurement.
Apply this diff:
- spot_intensity = int(image.max()) + # Use Nth maximum (10th pixel) to avoid noise spikes / hot pixels + flat = image.ravel() + spot_intensity = int(numpy.partition(flat, -10)[-10])Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/odemis/acq/align/mirror.py (1)
src/odemis/model/_futures.py (2)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)
src/odemis/acq/align/test/mirror_test.py (2)
src/odemis/acq/align/mirror.py (5)
_custom_minimize_neldermead(299-597)_custom_minimize_scalar_bounded(131-296)_probabilistic_snap_to_grid(81-128)_wrap_closed_loop_function(44-78)parabolic_mirror_alignment(600-642)src/odemis/util/testing.py (1)
stop_backend(135-169)
🪛 Ruff (0.14.4)
src/odemis/acq/align/mirror.py
59-59: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Prefer TypeError exception for invalid type
(TRY004)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
173-174: Avoid specifying long messages outside the exception class
(TRY003)
176-176: Avoid specifying long messages outside the exception class
(TRY003)
207-207: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
253-253: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
373-373: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
383-383: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
418-418: Avoid specifying long messages outside the exception class
(TRY003)
441-441: Avoid specifying long messages outside the exception class
(TRY003)
443-443: Avoid specifying long messages outside the exception class
(TRY003)
690-690: Unused method argument: future
(ARG002)
726-726: Avoid specifying long messages outside the exception class
(TRY003)
837-837: Avoid specifying long messages outside the exception class
(TRY003)
840-840: Avoid specifying long messages outside the exception class
(TRY003)
842-842: Ambiguous variable name: l
(E741)
843-843: Ambiguous variable name: l
(E741)
881-881: Avoid specifying long messages outside the exception class
(TRY003)
908-908: Avoid specifying long messages outside the exception class
(TRY003)
911-911: Avoid specifying long messages outside the exception class
(TRY003)
920-920: Avoid specifying long messages outside the exception class
(TRY003)
923-923: Ambiguous variable name: l
(E741)
src/odemis/acq/align/test/mirror_test.py
91-91: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
92-92: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
318-318: Unused function argument: x
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (10)
src/odemis/acq/align/test/mirror_test.py (1)
1-515: Comprehensive test coverage for the mirror alignment module.The test suite provides excellent coverage with well-structured test cases:
- Probabilistic alignment success rate testing with realistic random misalignments
- Thorough testing of utility functions (snapping, wrapper, minimizers)
- Proper test isolation with setUp/tearDown methods
- Reproducible tests using fixed random seeds
The static analysis warnings (S311) about cryptographic randomness are false positives—the
randommodule is appropriate for generating test data.src/odemis/acq/align/mirror.py (9)
41-41: Verify single-threaded executor is intentional.The executor is limited to
max_workers=1, ensuring only one alignment task runs at a time. Confirm this is the intended design, as it prevents parallel alignments on different hardware.
44-78: LGTM!The closed-loop function wrapper correctly counts evaluations, enforces the maxfun limit, and validates the expected return signature.
81-128: LGTM!The probabilistic snapping approach elegantly handles discrete hardware steps by using the fractional part as rounding probability, which avoids deterministic oscillation.
131-296: LGTM!The custom bounded scalar minimizer properly integrates probabilistic snapping and closed-loop feedback while preserving the core Brent's algorithm logic.
299-597: LGTM!The custom Nelder-Mead implementation correctly applies probabilistic snapping at all simplex operations (reflection, expansion, contraction, shrink) and integrates closed-loop position feedback.
690-703: LGTM!The cancel method correctly implements the expected cancellation pattern for odemis tasks.
782-809: LGTM!The scoring function properly normalizes both metrics to [0,1] and combines them with a configurable weight, providing a flexible optimization target.
896-925: LGTM!The callback correctly implements early-stop detection by tracking stall count and raises StopIteration when optimization plateaus for more than 5 iterations.
1060-1071: LGTM!The refined search ranges are properly clamped to the original absolute bounds using
max(l_abs_bound[0], l0 - search_range)andmin(l_abs_bound[1], l0 + search_range), preventing range expansion beyond the initial limits.Also applies to: 1128-1139
src/odemis/acq/align/mirror.py
Outdated
| if self._cancelled: | ||
| raise CancelledError(f"1D alignment for {axis} was cancelled by user") | ||
|
|
||
| hw_comp.moveAbs({axis: pos}).result() |
There was a problem hiding this comment.
Add timeout parameter to hardware movement.
The .result() call can block indefinitely on hardware failures.
Apply this diff:
+ MOVE_TIMEOUT = 30.0
- hw_comp.moveAbs({axis: pos}).result()
+ hw_comp.moveAbs({axis: pos}).result(timeout=MOVE_TIMEOUT)🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around line 883, the hw_comp.moveAbs({axis:
pos}).result() call can block indefinitely; change it to call
.result(timeout=...) with a reasonable timeout value (or a timeout parameter
sourced from the surrounding function/config) and wrap the call in try/except to
handle concurrent.futures.TimeoutError (and other exceptions) — on timeout
log/raise a descriptive error and ensure any required hardware-safe shutdown or
rollback is performed. Use a clear timeout constant or function argument,
propagate failures appropriately, and avoid leaving the hardware in an
uncontrolled state.
- pixel count's range is calulcated using sensor pixel size, binning and slit width - intensity's range is found using the ccd image data type
…ibrated ebeam working distance value
…cess rate test case
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
9ae1420 to
15a9ac5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
876-878: Consider defensive metadata access.The initialization uses
mirror_md.get(model.MD_CALIB, {})but here direct accessmirror_md[model.MD_CALIB]is used. While the button is only shown when calibration exists, using consistent defensive access would be more robust against edge cases where metadata might be modified at runtime.mirror_md = self.tab_data_model.main.mirror.getMetadata() - calib = mirror_md[model.MD_CALIB] + calib = mirror_md.get(model.MD_CALIB, {}) + if not calib: + logging.warning("Mirror calibration data missing, cannot perform auto-alignment") + return min_step_size = calib["auto_align_min_step_size"]src/odemis/gui/main_xrc.py (1)
15559-15591: Confirm Z step size slider range and defaults align with alignment algorithm
lbl_step_size_zandslider_stageare wired correctly and hidden by default; however, please double‑check that theUnitFloatSlidersettings (value1e-6, min1e-7, max1e-3, unitm, log scale) match the expected step‑size limits for the underlying parabolic mirror alignment so users can’t pick invalid or misleading values.src/odemis/acq/align/mirror.py (3)
385-388: Add explicitstacklevel=2to warnings.The warnings should include
stacklevel=2so they point to the caller's code rather than this library code, making debugging easier.Apply this diff:
warnings.warn("ftol is deprecated for Nelder-Mead," " use fatol instead. If you specified both, only" " fatol is used.", - DeprecationWarning) + DeprecationWarning, stacklevel=2) @@ warnings.warn("xtol is deprecated for Nelder-Mead," " use xatol instead. If you specified both, only" " xatol is used.", - DeprecationWarning) + DeprecationWarning, stacklevel=2) @@ warnings.warn("Initial guess is not within the specified bounds", - OptimizeWarning, 3) + OptimizeWarning, stacklevel=3)Also applies to: 395-398, 432-433
952-953: Addstrict=Trueto zip() for robustness.The zip() calls should use
strict=True(available in Python 3.10+) to ensureaxesandxk(orinitial_guess) have matching lengths. Without it, mismatched lengths would silently truncate, potentially masking bugs.Apply this diff:
- for axis, value in zip(axes, xk): + for axis, value in zip(axes, xk, strict=True): axis.component.moveAbs({axis.name: value}).result() @@ - for axis, guess in zip(axes, initial_guess): + for axis, guess in zip(axes, initial_guess, strict=True): if axis.abs_bounds is None:Also applies to: 1072-1082
1221-1221: Minor style: add spaces around equality operator.For consistency with PEP 8, add spaces around the
==operator.Apply this diff:
- weight=0.4 if j==2 else 0.5, + weight=0.4 if j == 2 else 0.5,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(5 hunks)src/odemis/gui/doc/sparc2_mirror_auto_alignment.html(1 hunks)src/odemis/gui/main_xrc.py(5 hunks)src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/main_xrc.pysrc/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc
🧬 Code graph analysis (3)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (1)
src/odemis/acq/align/mirror.py (3)
parabolic_mirror_alignment(627-668)AlignmentAxis(612-625)cancel(722-735)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (3)
model(570-571)Actuator(773-965)HwComponent(277-542)src/odemis/model/_futures.py (2)
ProgressiveFuture(371-507)set_progress(427-444)
src/odemis/acq/align/test/mirror_test.py (1)
src/odemis/acq/align/mirror.py (6)
AlignmentAxis(612-625)_custom_minimize_neldermead(311-609)_custom_minimize_scalar_bounded(145-308)_probabilistic_snap_to_grid(95-142)_wrap_closed_loop_function(58-92)parabolic_mirror_alignment(627-668)
🪛 Ruff (0.14.5)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
862-862: Unused method argument: evt
(ARG002)
902-902: Unused method argument: start
(ARG002)
902-902: Unused method argument: end
(ARG002)
src/odemis/acq/align/mirror.py
73-73: Avoid specifying long messages outside the exception class
(TRY003)
80-80: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Prefer TypeError exception for invalid type
(TRY004)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
183-183: Avoid specifying long messages outside the exception class
(TRY003)
187-188: Avoid specifying long messages outside the exception class
(TRY003)
190-190: Avoid specifying long messages outside the exception class
(TRY003)
221-221: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
267-267: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
385-385: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
395-395: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
430-430: Avoid specifying long messages outside the exception class
(TRY003)
453-453: Avoid specifying long messages outside the exception class
(TRY003)
455-455: Avoid specifying long messages outside the exception class
(TRY003)
623-623: Avoid specifying long messages outside the exception class
(TRY003)
625-625: Avoid specifying long messages outside the exception class
(TRY003)
722-722: Unused method argument: future
(ARG002)
799-799: Avoid specifying long messages outside the exception class
(TRY003)
910-910: Avoid specifying long messages outside the exception class
(TRY003)
947-947: Avoid specifying long messages outside the exception class
(TRY003)
950-950: Avoid specifying long messages outside the exception class
(TRY003)
952-952: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
981-981: Avoid specifying long messages outside the exception class
(TRY003)
990-990: Avoid specifying long messages outside the exception class
(TRY003)
1072-1072: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1235-1235: Avoid specifying long messages outside the exception class
(TRY003)
src/odemis/acq/align/test/mirror_test.py
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
94-94: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
95-95: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
165-165: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
389-389: Unused function argument: x
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (22)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (5)
43-64: LGTM on imports and constant definition.The new imports for
parabolic_mirror_alignment,AlignmentAxis, andStreamSettingsConfigare correctly grouped. TheEBEAM_WD_LIMITconstant is appropriately defined at module level.
828-845: Well-structured conditional initialization.The auto-alignment UI is correctly hidden by default and only enabled when the required hardware components and calibration data are present. The check for
"auto_align_min_step_size"and"ebeam_working_distance"in calibration metadata ensures proper configuration before enabling the feature.
902-911: LGTM on progress update callback.The
startandendparameters are unused but required by the callback interface signature. The implementation correctly guards against division by zero withfuture.n_steps > 0and useswx.CallAfterfor thread-safe GUI updates.
913-934: LGTM on completion callback.The done callback properly handles all outcomes: success, cancellation, and failure with appropriate logging. The
finallyblock ensures the button label is always reset and the future is replaced with anInstantaneousFuture, maintaining consistent state.
1454-1456: LGTM on documentation integration.The new auto-alignment documentation page is correctly added for the
mirror-alignmode, following the existing pattern.src/odemis/gui/xmlh/resources/panel_tab_sparc2_align.xrc (4)
830-871: LGTM on auto-align UI controls.The auto-align button and progress gauge are correctly structured, hidden by default (
<hidden>1</hidden>), and follow the established patterns in this file. The grid bag sizer configuration properly positions these elements.
903-935: LGTM on Z-axis step size controls.The Z-axis step size label and slider follow the same pattern as the existing X,Y controls, with appropriate hidden defaults.
1071-1142: LGTM on Z-axis movement controls.The Z-axis buttons (
btn_p_stage_z,btn_m_stage_z) and labels (lbl_pz,lbl_mz) are correctly positioned in column 6 of the grid, maintaining separation from the X,Y controls while following the same design patterns. All elements are appropriately hidden by default.
875-876: LGTM on clarified label.Updating the label from "Step size" to "Step size (X, Y)" appropriately distinguishes this control from the new Z-axis step size control.
src/odemis/gui/main_xrc.py (4)
1297-1301: New control bindings for auto‑align and Z step size look correct
btn_auto_align,gauge_auto_align,lbl_step_size_z, andslider_stageare all bound viaxrc.XRCCTRLwith names that match the XRC definitions below; this wiring should work as expected.
1310-1313: Bindings for +/-Z labels and stage Z buttons are consistent with XRCThe new attributes
lbl_pz,btn_p_stage_z,btn_m_stage_z, andlbl_mzcorrectly mirror the corresponding XRCnamevalues and complete the existing mirror control set.
15486-15527: Auto‑align button + progress gauge layout is reasonable and self‑containedThe
wxGridBagSizerwithbtn_auto_alignandgauge_auto_align(range 100, smooth style, hidden by default) is set up cleanly and should integrate well with the surrounding layout; no structural or naming issues stand out.
15727-15797: +/–Z labels and stage Z buttons match existing axis control patternsThe new
lbl_pz/lbl_mzlabels andbtn_p_stage_z/btn_m_stage_zbuttons follow the same naming, styling, and grid positioning conventions as the existing mirror controls, and being hidden by default should keep the UI clean until Z control is relevant.src/odemis/acq/align/test/mirror_test.py (2)
81-218: LGTM!The alignment success rate tests are well-structured. The approach of applying random misalignments, running alignment, and verifying success rates provides good coverage. The different thresholds (80% for 3D, 90% for 1D) appropriately reflect the increased complexity of multi-axis alignment.
221-583: LGTM!The helper function tests provide excellent coverage:
- Probabilistic snapping tests verify both deterministic behavior (grid alignment) and stochastic properties (distribution)
- Wrapper tests thoroughly validate error handling and call counting
- Optimizer tests verify both snapping and non-snapping modes
The comprehensive test suite gives confidence in the underlying utilities.
src/odemis/acq/align/mirror.py (7)
611-626: LGTM!The AlignmentAxis validation properly ensures the axis exists on the component and the step size is sensible. The constraints are appropriate for mirror alignment scenarios.
627-669: LGTM!The entry point properly sets up the asynchronous alignment task with cancellation support and progress tracking. The pattern follows the codebase's ProgressiveFuture conventions.
784-869: LGTM with minor observation.The spot measurement pipeline is well-designed:
- Gaussian blur reduces noise before thresholding
- Otsu's method automatically adapts to varying brightness
- Morphological closing fills gaps in the detected region
- Fallback to corner-based noise estimation is good defensive programming
The TODO about CLAHE (lines 826-831) suggests ongoing experimentation, which is fine for now.
870-897: LGTM!The scoring function properly normalizes both metrics to [0,1], inverts intensity to match the lower-is-better convention, and combines them using the configurable weight. The clamping ensures edge cases are handled safely.
899-967: LGTM!The objective functions are well-designed:
- Closed-loop feedback via actual position return helps compensate for actuator imprecision
- Cancellation checks at entry prevent wasted work
- The 1D objective appropriately uses intensity-only for faster coarse alignment
- The ND objective uses the full weighted score for refined multi-axis optimization
- Input validation prevents dimension mismatches
969-995: LGTM!The callback implements sensible early stopping logic. Detecting 5 consecutive iterations without simplex movement prevents wasted function evaluations when the optimizer has plateaued. The StopIteration exception is properly caught downstream.
1124-1238: LGTM!The main alignment orchestration is well-structured:
- Clear separation between 1D and 3D paths
- Proper iteration budget management with early exit when exhausted
- Progressive range reduction (halving search range between stages) balances exploration and exploitation
- Image saving with metadata preservation aids debugging and analysis
- The weight adjustment to 0.4 in the final refinement slightly favors tighter pixel count, which is reasonable for final convergence
The algorithm design is sound and the implementation handles edge cases properly.
| <li>Verify the CCD has enough counts but is not saturating, adjust the exposure time and gain to obtain count | ||
| rates of X - Y.</li> |
There was a problem hiding this comment.
Placeholder text needs actual values.
The count rate range "X - Y" is a placeholder that should be replaced with actual recommended values before merging.
🤖 Prompt for AI Agents
In src/odemis/gui/doc/sparc2_mirror_auto_alignment.html around lines 10-11, the
instruction contains a placeholder "X - Y" for CCD count rate ranges; replace
this with the actual recommended count-rate lower and upper bounds (e.g., 10,000
- 30,000 ADU or the project-specific values) and ensure units are specified (ADU
or e-/s) and include guidance on acceptable tolerance; update the sentence to
read with the concrete numeric range and units so the document contains
actionable exposure/gain targets.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/odemis/acq/align/mirror.py (1)
383-393: Add stacklevel to warnings for better stack traces.The deprecation warnings should include
stacklevel=2to point to the caller's location rather than this function.Apply this diff:
if 'ftol' in unknown_options: warnings.warn("ftol is deprecated for Nelder-Mead," " use fatol instead. If you specified both, only" " fatol is used.", - DeprecationWarning) + DeprecationWarning, stacklevel=2) if (numpy.isclose(fatol, 1e-4) and not numpy.isclose(unknown_options['ftol'], 1e-4)): # only ftol was probably specified, use it. fatol = unknown_options['ftol'] unknown_options.pop('ftol') if 'xtol' in unknown_options: warnings.warn("xtol is deprecated for Nelder-Mead," " use xatol instead. If you specified both, only" " xatol is used.", - DeprecationWarning) + DeprecationWarning, stacklevel=2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/acq/align/test/mirror_test.py (1)
src/odemis/acq/align/mirror.py (5)
AlignmentAxis(609-622)_custom_minimize_scalar_bounded(144-306)_probabilistic_snap_to_grid(94-141)_wrap_closed_loop_function(57-91)parabolic_mirror_alignment(624-665)
🪛 Ruff (0.14.6)
src/odemis/acq/align/mirror.py
72-72: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Prefer TypeError exception for invalid type
(TRY004)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
181-181: Avoid specifying long messages outside the exception class
(TRY003)
185-186: Avoid specifying long messages outside the exception class
(TRY003)
188-188: Avoid specifying long messages outside the exception class
(TRY003)
219-219: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
265-265: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
383-383: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
393-393: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
427-427: Avoid specifying long messages outside the exception class
(TRY003)
450-450: Avoid specifying long messages outside the exception class
(TRY003)
452-452: Avoid specifying long messages outside the exception class
(TRY003)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
725-725: Unused method argument: future
(ARG002)
802-802: Avoid specifying long messages outside the exception class
(TRY003)
914-914: Avoid specifying long messages outside the exception class
(TRY003)
951-951: Avoid specifying long messages outside the exception class
(TRY003)
954-954: Avoid specifying long messages outside the exception class
(TRY003)
956-956: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
985-985: Avoid specifying long messages outside the exception class
(TRY003)
994-994: Avoid specifying long messages outside the exception class
(TRY003)
1079-1079: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1242-1242: Avoid specifying long messages outside the exception class
(TRY003)
src/odemis/acq/align/test/mirror_test.py
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
94-94: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
95-95: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
166-166: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
390-390: Unused function argument: x
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (8)
src/odemis/acq/align/mirror.py (3)
94-141: LGTM! Solid probabilistic snapping implementation.The probabilistic rounding logic correctly handles grid alignment while avoiding deterministic oscillation. The documentation with examples is excellent.
608-621: LGTM! Clean dataclass with appropriate validation.The
__post_init__validation ensures the axis exists on the component and the min_step_size is reasonable. Good defensive programming.
624-665: LGTM! Well-designed public API.The function properly creates a ProgressiveFuture with cancellation support and comprehensive documentation. The API is clean and follows odemis patterns.
src/odemis/acq/align/test/mirror_test.py (5)
53-79: LGTM! Proper test setup and teardown.The test class correctly starts the backend in
setUpClass, stops it intearDownClass, and resets positions before each test insetUp. This prevents resource leaks and cross-test interference.
80-219: LGTM! Comprehensive success rate tests.The tests properly validate alignment robustness by:
- Testing multiple random misalignments (10 iterations each)
- Using fixed random seed for reproducibility
- Setting reasonable success thresholds (80% for 3D, 90% for 1D)
- Logging failures for debugging
The S311 static analysis warnings about pseudo-random generators are false positives - cryptographic quality is not needed for test fixtures.
222-329: LGTM! Excellent test coverage for probabilistic snapping.The test suite thoroughly validates:
- Edge cases (None step size, zero/non-zero origins, varying step sizes)
- Statistical properties with appropriate tolerance (±5% for probability validation)
- Array input handling
The probabilistic distribution test (lines 289-315) is particularly well-designed, running 1000 trials to verify the expected 70/30 split.
331-479: LGTM! Thorough validation of wrapper function.The test suite comprehensively validates:
- Counter behavior and maxfun enforcement
- Return value validation (tuple structure, score scalar, position array)
- Argument passing and array copying
- Error conditions with clear assertions
The ARG001 warning at line 390 is a false positive - the
xparameter is required for the dummy function's signature.
481-584: LGTM! Good coverage of optimizer behavior.The tests validate key optimizer features:
- Bounds enforcement and initial guess clipping
- Grid snapping when enabled vs. continuous optimization when disabled
- Convergence to expected solutions
Using simple quadratic objectives makes the tests deterministic and easy to verify.
| for axis, value in zip(axes, xk): | ||
| axis.component.moveAbs({axis.name: value}).result() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add strict=True to zip() to catch length mismatches.
Without strict=True, if axes and xk have different lengths, the zip will silently truncate to the shorter length, potentially masking bugs. Although the code validates xk.size != len(axes) at line 953-954, adding strict=True provides additional safety.
Apply this diff:
- for axis, value in zip(axes, xk):
+ for axis, value in zip(axes, xk, strict=True):
axis.component.moveAbs({axis.name: value}).result()🧰 Tools
🪛 Ruff (0.14.6)
956-956: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around lines 956-957, the loop uses zip(axes,
xk) which will silently truncate if the sequences differ; add strict=True to the
zip call (i.e., use zip(axes, xk, strict=True)) so a ValueError is raised on
length mismatch, providing an extra safety check in addition to the existing
size validation. Ensure the runtime supports Python 3.10+ where zip(...,
strict=True) is available.
| f" with search range {search_range} m and max iter {max_iter}") | ||
| initial_guess = [axis.component.position.value[axis.name] for axis in axes] | ||
| bounds = [] | ||
| for axis, guess in zip(axes, initial_guess): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add strict=True to zip() for safety.
Although axes and initial_guess are constructed to match, adding strict=True provides explicit validation and better error messages if they ever diverge due to future refactoring.
Apply this diff:
- for axis, guess in zip(axes, initial_guess):
+ for axis, guess in zip(axes, initial_guess, strict=True):
if axis.abs_bounds is None:🧰 Tools
🪛 Ruff (0.14.6)
1079-1079: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around line 1079, the for-loop uses zip(axes,
initial_guess) without strict checking; modify the zip call to zip(axes,
initial_guess, strict=True) so the code raises a clear error if the iterables
ever differ in length, ensuring explicit validation during iteration.
4a3a362 to
43306f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/odemis/acq/align/mirror.py (1)
953-957: Usezip(..., strict=True)to catch length mismatches between axes and position vectors.Ruff already flagged these loops, and there’s an earlier review note on the same point. Using
strict=Truemakes mismatches betweenaxesand the corresponding coordinate vectors fail fast with a clearValueErrorinstead of silently truncating:- for axis, value in zip(axes, xk): + for axis, value in zip(axes, xk, strict=True): axis.component.moveAbs({axis.name: value}).result(timeout=self._move_timeout_s) @@ - for axis, guess in zip(axes, initial_guess): + for axis, guess in zip(axes, initial_guess, strict=True): if axis.abs_bounds is None:This provides an extra safety net if the construction of
axesand the initial guess vector ever diverge during future refactors. Please confirm the runtime is Python ≥ 3.10 wherestrictis available.Also applies to: 1079-1089
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (3)
model(570-571)Actuator(773-965)HwComponent(277-542)src/odemis/model/_futures.py (3)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)set_progress(427-444)
🪛 Ruff (0.14.6)
src/odemis/acq/align/mirror.py
72-72: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Prefer TypeError exception for invalid type
(TRY004)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Unused function argument: unknown_options
(ARG001)
181-181: Avoid specifying long messages outside the exception class
(TRY003)
185-186: Avoid specifying long messages outside the exception class
(TRY003)
188-188: Avoid specifying long messages outside the exception class
(TRY003)
219-219: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
265-265: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
383-383: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
393-393: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
427-427: Avoid specifying long messages outside the exception class
(TRY003)
450-450: Avoid specifying long messages outside the exception class
(TRY003)
452-452: Avoid specifying long messages outside the exception class
(TRY003)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
725-725: Unused method argument: future
(ARG002)
802-802: Avoid specifying long messages outside the exception class
(TRY003)
914-914: Avoid specifying long messages outside the exception class
(TRY003)
951-951: Avoid specifying long messages outside the exception class
(TRY003)
954-954: Avoid specifying long messages outside the exception class
(TRY003)
956-956: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
985-985: Avoid specifying long messages outside the exception class
(TRY003)
994-994: Avoid specifying long messages outside the exception class
(TRY003)
1079-1079: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1242-1242: Avoid specifying long messages outside the exception class
(TRY003)
src/odemis/acq/align/test/mirror_test.py
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
94-94: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
95-95: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
166-166: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
390-390: Unused function argument: x
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
src/odemis/acq/align/mirror.py
Outdated
| def _update_spot_measurement(self) -> Tuple[int, int]: | ||
| """ | ||
| Measure spot quality from the current camera image using contour detection. | ||
| The method: | ||
| 1. Pre-processes the image with Gaussian blur, CLAHE | ||
| 2. Uses Otsu thresholding to separate signal from background | ||
| 3. Applies morphological operations to clean up the binary mask | ||
| 4. Finds the largest contour which represents the spot | ||
| If no contours are found, falls back to returning the full image size | ||
| :raises CancelledError: if the task was cancelled. | ||
| """ | ||
| if self._cancelled: | ||
| raise CancelledError("Alignment was cancelled by user") | ||
|
|
||
| image = self._ccd.data.get(asap=False) | ||
| self._last_img = image # Store for potentially saving it at the end of the optimization | ||
|
|
||
| # Set pixel count range based on slit width and sensor properties | ||
| ccd_md = self._ccd.getMetadata() | ||
| input_slit_width = ccd_md.get(model.MD_INPUT_SLIT_WIDTH, DEFAULT_SLIT_WIDTH) | ||
| binning = ccd_md.get(model.MD_BINNING, DEFAULT_BINNING) | ||
| sensor_pixel_size = ccd_md.get(model.MD_SENSOR_PIXEL_SIZE, DEFAULT_SENSOR_PIXEL_SIZE) | ||
| sensor_pixel_size = (sensor_pixel_size[0] * binning[0], sensor_pixel_size[1] * binning[1]) | ||
| slit_width_px = (input_slit_width / sensor_pixel_size[0], input_slit_width / sensor_pixel_size[1]) | ||
| # The spot must at least cover the slit area | ||
| min_pixel_count = math.ceil(slit_width_px[0]) * math.ceil(slit_width_px[1]) | ||
| self.pixel_count.range = (min_pixel_count, self.MAX_PIXEL_COUNT) | ||
| # Set intensity range based on image data type | ||
| image_iinfo = numpy.iinfo(image.dtype) | ||
| self.intensity.range = (image_iinfo.min, image_iinfo.max) | ||
|
|
||
| # Slight Gaussian blur to reduce pixel noise | ||
| # Prevents threshold from overreacting | ||
| blurred = cv2.GaussianBlur(image, (7, 7), 0) | ||
| image_norm = cv2.normalize(blurred, None, 0, 255, cv2.NORM_MINMAX) | ||
| image_uint8 = image_norm.astype(numpy.uint8) | ||
|
|
||
| # TODO: CLAHE sometimes cannot help detect focussed spot? | ||
| # Use Contrast Limited Adaptive Histogram Equalization (CLAHE) | ||
| # This enhances local contrast without amplifying background noise across | ||
| # the entire image | ||
| # clahe = cv2.createCLAHE(clipLimit=2.0, tileGridSize=(8, 8)) | ||
| # enhanced_image = clahe.apply(image_uint8) | ||
|
|
||
| # Otsu thresholding automatically separates signal from background | ||
| # Adapts to changing brightness and exposure | ||
| _, binary_mask = cv2.threshold( | ||
| image_uint8, 0, 255, cv2.THRESH_BINARY + cv2.THRESH_OTSU | ||
| ) | ||
|
|
||
| # Morphological cleanup to fill small gaps | ||
| # Makes countour area continuous | ||
| kernel = numpy.ones((7, 7), numpy.uint8) | ||
| binary_mask = cv2.morphologyEx(binary_mask, cv2.MORPH_CLOSE, kernel) | ||
|
|
||
| # Contour detection | ||
| contours, _ = cv2.findContours(binary_mask, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE) | ||
|
|
||
| if contours: | ||
| main_contour = max(contours, key=cv2.contourArea) | ||
| count = self.pixel_count.clip(int(cv2.contourArea(main_contour))) | ||
| self.pixel_count.value = count | ||
| else: | ||
| logging.debug("No contours found, using fallback pixel count method") | ||
| corner_size = 5 # Use a 5x5 pixel square from each corner | ||
| top_left = image[:corner_size, :corner_size] | ||
| top_right = image[:corner_size, -corner_size:] | ||
| bottom_left = image[-corner_size:, :corner_size] | ||
| bottom_right = image[-corner_size:, -corner_size:] | ||
| all_corners = numpy.concatenate( | ||
| ( | ||
| top_left.ravel(), | ||
| top_right.ravel(), | ||
| bottom_left.ravel(), | ||
| bottom_right.ravel(), | ||
| ) | ||
| ) | ||
| noise = 1.3 * numpy.median(all_corners) | ||
| count = self.pixel_count.clip(numpy.count_nonzero(image > noise)) | ||
| self.pixel_count.value = count | ||
|
|
||
| intensity = self.intensity.clip(int(image.max())) | ||
| self.intensity.value = intensity | ||
|
|
||
| def _get_score(self, weight: float = 0.5): | ||
| """ | ||
| Compute a scalar score combining normalized pixel count and intensity. | ||
| The score is in [0, 1] where 0 is best. 'weight' controls the relative | ||
| importance of pixel count vs intensity (1.0 -> only pixel count, 0.0 -> only intensity). | ||
| :param weight: Weight given to spot pixel count in [0.0, 1.0] (default 0.5). | ||
| :returns: float | ||
| Normalized score (lower is better). | ||
| """ | ||
| # Normalize pixel count | ||
| # Score is 0 for best pixel count, 1 for worst | ||
| area_range = float(self.pixel_count.range[1] - self.pixel_count.range[0]) | ||
| norm_area = (self.pixel_count.value - self.pixel_count.range[0]) / area_range | ||
| norm_area = max(0.0, min(1.0, norm_area)) # Clamp to [0, 1] | ||
|
|
||
| # Normalize intensity | ||
| # Score is 0 for best intensity, 1 for worst | ||
| intensity_range = float(self.intensity.range[1] - self.intensity.range[0]) | ||
| norm_intensity = (self.intensity.value - self.intensity.range[0]) / intensity_range | ||
| norm_intensity = 1.0 - norm_intensity # Invert so higher intensity = lower score | ||
| norm_intensity = max(0.0, min(1.0, norm_intensity)) # Clamp to [0, 1] | ||
|
|
||
| # Calculate weighted score | ||
| # This is the single value the optimizer will try to minimize | ||
| score = (weight * norm_area) + ((1 - weight) * norm_intensity) | ||
| return score |
There was a problem hiding this comment.
Tighten _update_spot_measurement / _get_score API and guard against zero-width ranges.
Two issues here:
-
_update_spot_measurementis annotated as returningTuple[int, int]but never returns anything; callers use its side effects. Either return(self.pixel_count.value, self.intensity.value)or update the signature/docstring to reflectNonereturn to avoid confusion for readers and tools. -
_get_scoreassumes bothpixel_count.rangeandintensity.rangehave non-zero width. In extreme configurations (e.g. very large slit area vsMAX_PIXEL_COUNT, or degenerate intensity range) this would lead to aZeroDivisionError.
A defensive tweak keeps behavior while avoiding crashes:
- area_range = float(self.pixel_count.range[1] - self.pixel_count.range[0])
+ area_range = float(self.pixel_count.range[1] - self.pixel_count.range[0])
+ if area_range <= 0:
+ # Fallback to avoid division by zero; treat as single-bin range.
+ area_range = 1.0
@@
- intensity_range = float(self.intensity.range[1] - self.intensity.range[0])
+ intensity_range = float(self.intensity.range[1] - self.intensity.range[0])
+ if intensity_range <= 0:
+ intensity_range = 1.0And, if you keep _update_spot_measurement side-effect-only, align its signature:
- def _update_spot_measurement(self) -> Tuple[int, int]:
+ def _update_spot_measurement(self) -> None:🧰 Tools
🪛 Ruff (0.14.6)
802-802: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around lines 787 to 901,
_update_spot_measurement is annotated to return Tuple[int,int] but never returns
(it only sets self.pixel_count/self.intensity), and _get_score can divide by
zero if pixel_count.range or intensity.range has zero width; fix by either
returning (self.pixel_count.value, self.intensity.value) at the end of
_update_spot_measurement or changing its signature/docstring to indicate it
returns None (pick one consistent API and update callers/docs), and in
_get_score defensively handle zero-width ranges by checking range width before
division and using a safe fallback (e.g., set normalized component to 0.0 when
range width <= 0, then clamp and compute the weighted score) to avoid
ZeroDivisionError.
src/odemis/acq/align/mirror.py
Outdated
| def _objective_1d(self, pos: float, axis: AlignmentAxis): | ||
| """ | ||
| 1D objective for optimizing hardware component's axis by maximizing intensity. | ||
| :param pos: Candidate position. | ||
| :param axis: Alignment axis to optimize. | ||
| :raises CancelledError: if the task was cancelled. | ||
| :returns: float | ||
| Negative measured intensity (-intensity). | ||
| """ | ||
| if self._cancelled: | ||
| raise CancelledError(f"1D alignment for {axis} was cancelled by user") | ||
|
|
||
| axis.component.moveAbs({axis.name: pos}).result() | ||
| self._update_spot_measurement() | ||
|
|
||
| # Normalize intensity | ||
| # Score is 0 for best intensity, 1 for worst | ||
| norm_intensity = 1.0 - (self.intensity.value / self.intensity.range[1]) | ||
| norm_intensity = max(0.0, min(1.0, norm_intensity)) # Clamp to [0, 1] | ||
|
|
||
| # For closed-loop feedback to the optimizer | ||
| pos_actual = axis.component.position.value[axis.name] | ||
|
|
||
| return norm_intensity, pos_actual | ||
|
|
||
| def _objective_nd( | ||
| self, | ||
| xk: numpy.ndarray, | ||
| axes: List[AlignmentAxis], | ||
| weight: float, | ||
| ): | ||
| """ | ||
| Objective function for simultaneous optimization of multiple axes. | ||
| The optimizer will pass a candidate vector xk. The method | ||
| enforces the supplied bounds, moves the actuators to the candidate | ||
| position, measures the spot and returns a scalar score (lower is better). | ||
| :param xk: Candidate vector. | ||
| :param axes: List of alignment axes corresponding to xk. | ||
| :param weight: Weight used by _get_score to combine metrics. | ||
| :raises CancelledError: if the task was cancelled during evaluation. | ||
| :raises ValueError: if xk does not have 3 elements. | ||
| :returns: float | ||
| Computed score for the candidate position. | ||
| """ | ||
| if self._cancelled: | ||
| raise CancelledError("Nelder-Mead alignment was cancelled by user") | ||
|
|
||
| if xk.size != len(axes): | ||
| raise ValueError(f"Expected xk to have {len(axes)} elements, got {xk.size}") | ||
|
|
||
| for axis, value in zip(axes, xk): | ||
| axis.component.moveAbs({axis.name: value}).result() | ||
|
|
||
| self._update_spot_measurement() |
There was a problem hiding this comment.
I'll help you verify the review comment about missing timeouts in hardware moves. Let me start by exploring the relevant code in the mirror.py file.
<function_calls>
cat -n src/odemis/acq/align/mirror.py | head -1
</function_calls>
<function_calls>
wc -l src/odemis/acq/align/mirror.py
</function_calls>
Let me read the target lines and surrounding context:
<function_calls>
sed -n '890,970p' src/odemis/acq/align/mirror.py
</function_calls>
Now let me check the __init__ method and other relevant context:
<function_calls>
sed -n '1,150p' src/odemis/acq/align/mirror.py | grep -n "class|def init"
</function_calls>
Let me get a broader view of the class structure:
<function_calls>
rg -n "class|def init|def _objective_1d|def _objective_nd|def _update_spot_measurement" src/odemis/acq/align/mirror.py | head -20
</function_calls>
Let me read the init and _update_spot_measurement methods:
<function_calls>
sed -n '700,850p' src/odemis/acq/align/mirror.py
</function_calls>
Let me search for _update_spot_measurement:
<function_calls>
rg -n "_update_spot_measurement" src/odemis/acq/align/mirror.py -A 15
</function_calls>
Now let me check for timeout patterns elsewhere in the codebase:
<function_calls>
rg -n ".result(.*timeout" src/odemis/acq/align/mirror.py
</function_calls>
Let me search more broadly for timeout usage in the acq module:
<function_calls>
rg -n ".result(.*timeout" src/odemis/acq/ | head -20
</function_calls>
Let me check the Python version requirements and look for concurrent.futures usage:
<function_calls>
grep -r "python_requires|Programming Language :: Python" . --include="setup.py" --include="pyproject.toml" --include="setup.cfg" | head -10
</function_calls>
Let me check for imports and understand the Future type being used:
<function_calls>
sed -n '1,50p' src/odemis/acq/align/mirror.py
</function_calls>
Let me look at more of the file to understand the class and its initialization:
<function_calls>
sed -n '600,750p' src/odemis/acq/align/mirror.py
</function_calls>
Let me search for the class definition:
<function_calls>
rg -n "^class" src/odemis/acq/align/mirror.py
</function_calls>
Let me read from line 500 onwards to find the class and init:
<function_calls>
sed -n '500,700p' src/odemis/acq/align/mirror.py
</function_calls>
Let me search for where _save_thread is initialized (mentioned in the diff):
<function_calls>
rg -n "_save_thread" src/odemis/acq/align/mirror.py -B 10 -A 5 | head -50
</function_calls>
Now let me check the exact lines around the class initialization and the functions in question:
<function_calls>
sed -n '350,450p' src/odemis/acq/align/mirror.py
</function_calls>
Let me look at the beginning to find the class:
<function_calls>
sed -n '100,250p' src/odemis/acq/align/mirror.py
</function_calls>
Let me use ast-grep to find the class definition and init:
<function_calls>
ast-grep --pattern 'class $_ {
$$$
def init($$$) {
$$$
}
$$$
}'
</function_calls>
Let me try a simpler approach - search for the pattern of _cancelled which is mentioned in the functions:
<function_calls>
rg -n "self._cancelled" src/odemis/acq/align/mirror.py | head -10
</function_calls>
Let me check line 800-950 to get the broader context including the class:
<function_calls>
sed -n '800,950p' src/odemis/acq/align/mirror.py
</function_calls>
Let me search for the specific class containing these methods:
<function_calls>
sed -n '750,850p' src/odemis/acq/align/mirror.py
</function_calls>
Let me look for the class definition more carefully by searching backwards from line 903:
<function_calls>
sed -n '1,900p' src/odemis/acq/align/mirror.py | grep -n "^class" | tail -1
</function_calls>
Perfect! Let me read from line 320 onwards:
<function_calls>
sed -n '320,430p' src/odemis/acq/align/mirror.py
</function_calls>
Great! Now let me check the init method:
<function_calls>
sed -n '330,500p' src/odemis/acq/align/mirror.py
</function_calls>
Now let me verify if there are any timeout patterns or cancellation handling in concurrent.futures context:
<function_calls>
rg -n "concurrent.futures|TimeoutError" src/odemis/acq/align/mirror.py
</function_calls>
Let me check if moveAbs is defined in this module or imported:
<function_calls>
rg -n "moveAbs" src/odemis/acq/align/mirror.py | head -10
</function_calls>
Let me check the model module to understand moveAbs return type:
<function_calls>
find . -name "model.py" -path "/odemis/" | head -5
</function_calls>
Let me search for moveAbs in the odemis codebase:
<function_calls>
rg -l "def moveAbs" src/odemis/ | head -5
</function_calls>
Let me check one of those files:
<function_calls>
rg -n "def moveAbs" src/odemis/model/_hwcomponent.py -A 10 | head -30
</function_calls>
Now let me verify if .result() is a method that accepts timeout. Let me check the imports:
<function_calls>
grep -n "import" src/odemis/acq/align/mirror.py | head -20
</function_calls>
Let me look at the actual _update_spot_measurement implementation:
<function_calls>
rg -n "def _update_spot_measurement" src/odemis/acq/align/mirror.py -A 20
</function_calls>
Now let me check for the return type annotation on _update_spot_measurement:
<function_calls>
sed -n '850,900p' src/odemis/acq/align/mirror.py
</function_calls>
Let me look for Python version requirements:
<function_calls>
find . -name "setup.py" -o -name "pyproject.toml" | xargs grep -l "python" | head -3
</function_calls>
Let me check pyproject.toml:
<function_calls>
cat pyproject.toml 2>/dev/null || cat setup.py 2>/dev/null | grep -i python
</function_calls>
Let me find the project configuration:
<function_calls>
ls -la | grep -E "setup|pyproject|requirements"
</function_calls>
<function_calls>
cat setup.py 2>/dev/null | head -50
</function_calls>
Now let me check for zip() usage with strict parameter in the context mentioned:
<function_calls>
sed -n '903,960p' src/odemis/acq/align/mirror.py
</function_calls>
Let me look for the _run_nelder_mead and _run_scalar functions mentioned in the scratchpad:
<function_calls>
rg -n "def _run_nelder_mead|def _run_scalar" src/odemis/acq/align/mirror.py -A 10 | head -40
</function_calls>
Let me check the actual full context around the zip calls:
<function_calls>
sed -n '940,970p' src/odemis/acq/align/mirror.py
</function_calls>
Let me look at the full _objective_nd function:
<function_calls>
sed -n '927,980p' src/odemis/acq/align/mirror.py
</function_calls>
Now let me check if there are any existing timeout patterns in the codebase for .result() calls:
<function_calls>
rg -n ".result(timeout\s*=" src/odemis/ --type py | head -20
</function_calls>
Let me check what type of Future is used:
<function_calls>
sed -n '1,100p' src/odemis/acq/align/mirror.py
</function_calls>
Now let me verify by checking the actual parameters and structure. Let me look at the complete class to understand what's being passed:
<function_calls>
sed -n '320,380p' src/odemis/acq/align/mirror.py
</function_calls>
Perfect! Now let me summarize my findings by reviewing the actual code one more time to confirm:
<function_calls>
sed -n '903,980p' src/odemis/acq/align/mirror.py
</function_calls>
Let me check the return type annotation on _objective_1d:
<function_calls>
sed -n '903,925p' src/odemis/acq/align/mirror.py
</function_calls>
Let me check the docstring more carefully for _objective_1d:
<function_calls>
sed -n '903,930p' src/odemis/acq/align/mirror.py | cat -n
</function_calls>
I see - the docstring says :returns: float but the function returns a tuple (norm_intensity, pos_actual). Let me verify how this function is used:
<function_calls>
rg -n "_objective_1d" src/odemis/acq/align/mirror.py
</function_calls>
Let me check the _run_scalar function to see how _objective_1d is called:
<function_calls>
rg -n "def _run_scalar" src/odemis/acq/align/mirror.py -A 30
</function_calls>
Let me search for where these objective functions are actually used:
<function_calls>
rg -n "minimize_scalar|minimize.*_objective_1d" src/odemis/acq/align/mirror.py
</function_calls>
Let me look at run methods and check how objectives are passed:
<function_calls>
rg -n "_objective" src/odemis/acq/align/mirror.py
</function_calls>
Let me check the context where optimize functions are called:
<function_calls>
rg -n "scipy.optimize|minimize_scalar" src/odemis/acq/align/mirror.py -B 3 -A 3
</function_calls>
Let me search more broadly:
<function_calls>
sed -n '1000,1100p' src/odemis/acq/align/mirror.py
</function_calls>
Let me check if there are more functions after the snippet provided:
<function_calls>
sed -n '960,1050p' src/odemis/acq/align/mirror.py
</function_calls>
Now let me check the complete file structure to understand what happens after line 959:
<function_calls>
tail -100 src/odemis/acq/align/mirror.py | head -80
</function_calls>
Now let me verify the actual return statement by looking at the full _objective_1d function definition:
<function_calls>
sed -n '903,926p' src/odemis/acq/align/mirror.py | cat -n
</function_calls>
Let me check if there's a custom optimization method being used:
<function_calls>
grep -n "scipy" src/odemis/acq/align/mirror.py
</function_calls>
Let me check what imports are in the file:
<function_calls>
head -50 src/odemis/acq/align/mirror.py | grep -E "^import|^from"
</function_calls>
Now let me make sure I understand the return value issue. Let me see the exact return line:
<function_calls>
grep -n "return norm_intensity" src/odemis/acq/align/mirror.py
</function_calls>
Perfect! Now I have all the information needed. Let me create a final verification by checking if this is indeed the code state:
<function_calls>
python3 << 'EOF'
Verify the specific lines mentioned in the review
with open('src/odemis/acq/align/mirror.py', 'r') as f:
lines = f.readlines()
# Check line 917 (0-indexed: 916)
line_917 = lines[916].strip() if len(lines) > 916 else "NOT FOUND"
# Check line 946 (0-indexed: 945)
line_946 = lines[945].strip() if len(lines) > 945 else "NOT FOUND"
# Check line 924 (0-indexed: 923)
line_924 = lines[923].strip() if len(lines) > 923 else "NOT FOUND"
print(f"Line 917: {line_917}")
print(f"Line 924: {line_924}")
print(f"Line 946: {line_946}")
print()
# Check for timeout parameter in these lines
has_timeout_917 = "timeout" in line_917
has_timeout_946 = "timeout" in line_946
print(f"Line 917 has timeout parameter: {has_timeout_917}")
print(f"Line 946 has timeout parameter: {has_timeout_946}")
# Check return statement
returns_tuple = "," in line_924 and "return" in line_924
print(f"Line 924 returns tuple: {returns_tuple}")
EOF
</function_calls>
Based on my verification of the code, I can now provide the rewritten review comment:
Add timeout protection to hardware actuator moves to prevent indefinite thread blocking.
The _objective_1d and _objective_nd methods call moveAbs(...).result() without timeout (lines 917 and 946), which means a stuck or unresponsive actuator will block the worker thread indefinitely. This blocks cancellation—the CancelledError flag check at the function entry (lines 912, 939) cannot interrupt a blocked .result() call.
Introduce a per-task timeout attribute in __init__ and apply it consistently:
def __init__(
self,
axes: List[AlignmentAxis],
ccd: model.HwComponent,
future: model.ProgressiveFuture,
stop_early: bool,
save_images: bool,
):
self._save_thread = threading.Thread(target=self._saving_thread, daemon=True)
+ self._move_timeout_s = 30.0 # safety timeout for actuator moves (s)Then update the move calls:
- axis.component.moveAbs({axis.name: pos}).result()
+ axis.component.moveAbs({axis.name: pos}).result(timeout=self._move_timeout_s) for axis, value in zip(axes, xk):
- axis.component.moveAbs({axis.name: value}).result()
+ axis.component.moveAbs({axis.name: value}).result(timeout=self._move_timeout_s)Optionally catch concurrent.futures.TimeoutError around these calls to log and abort the alignment cleanly rather than propagating the exception.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
914-914: Avoid specifying long messages outside the exception class
(TRY003)
951-951: Avoid specifying long messages outside the exception class
(TRY003)
954-954: Avoid specifying long messages outside the exception class
(TRY003)
956-956: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py around lines 903-959 (notably the .result()
calls at ~917 and ~946), the code calls axis.component.moveAbs(...).result()
with no timeout which can block indefinitely; add a per-task/move timeout
attribute on the class (initialize in __init__), use that timeout when calling
.result(timeout=self._move_timeout) for every hardware move in _objective_1d and
_objective_nd, and wrap those .result(...) calls in a try/except catching
concurrent.futures.TimeoutError to log/abort the alignment cleanly (set
cancelled or raise a controlled error) so the worker thread cannot be blocked
forever.
| def test_copies_input_array(self): | ||
| """Wrapper should not modify the input array passed to objective.""" | ||
|
|
||
| def func_check_copy(x, original_id): | ||
| # Check that wrapped passes a copy, not the original | ||
| self.assertNotEqual(id(x), original_id[0]) | ||
| return 1.0, x | ||
|
|
||
| x_input = numpy.array([1.0, 2.0, 3.0]) | ||
| _, wrapped = _wrap_closed_loop_function( | ||
| func_check_copy, (([id(x_input)],),), 10 | ||
| ) | ||
|
|
||
| wrapped(x_input) | ||
|
|
There was a problem hiding this comment.
Fix test_copies_input_array so it actually checks copy semantics.
As written, original_id[0] inside func_check_copy ends up being a list rather than the id of the original array, so assertNotEqual(id(x), original_id[0]) will always pass, even if the wrapper stops copying the input.
You can simplify the test and make the assertion meaningful like this:
- def test_copies_input_array(self):
- """Wrapper should not modify the input array passed to objective."""
-
- def func_check_copy(x, original_id):
- # Check that wrapped passes a copy, not the original
- self.assertNotEqual(id(x), original_id[0])
- return 1.0, x
-
- x_input = numpy.array([1.0, 2.0, 3.0])
- _, wrapped = _wrap_closed_loop_function(
- func_check_copy, (([id(x_input)],),), 10
- )
-
- wrapped(x_input)
+ def test_copies_input_array(self):
+ """Wrapper should not pass the original array instance to the objective."""
+
+ def func_check_copy(x, original_id):
+ # Wrapped should pass a copy, not the original object
+ self.assertNotEqual(id(x), original_id)
+ return 1.0, x
+
+ x_input = numpy.array([1.0, 2.0, 3.0])
+ original_id = id(x_input)
+ _, wrapped = _wrap_closed_loop_function(
+ func_check_copy, (original_id,), 10
+ )
+
+ wrapped(x_input)This keeps the intent of the test while ensuring it will fail if _wrap_closed_loop_function ever stops copying its input.
🤖 Prompt for AI Agents
In src/odemis/acq/align/test/mirror_test.py around lines 465-479, fix
test_copies_input_array so it compares the wrapped call's parameter id against
the actual integer id of the original array (not a list containing it). Replace
the current pattern that passes ([id(x_input)],) with passing the raw id (or
capture original_id = id(x_input) in a closure) and change the assertion to
assertNotEqual(id(x), original_id) so the test will fail if the wrapper stops
copying the input.
43306f2 to
96d454f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/odemis/acq/align/mirror.py (2)
838-887: Clarify_update_spot_measurementreturn type and makeSpotrange updates saferThe method is annotated as returning
Tuple[int, int]but doesn’t return anything; it only updatesself.spotfields and_last_img. That mismatch can confuse readers and tools.Also, the dynamically set ranges can in principle collapse or invert (e.g.
min_pixel_count >= Spot.MAX_PIXEL_COUNT), which would makearea_rangeorintensity_rangezero/negative and crash_get_score.I’d recommend:
- Change the signature to reflect the side-effect-only behaviour:
- def _update_spot_measurement(self) -> Tuple[int, int]: + def _update_spot_measurement(self) -> None:
- Clamp the ranges defensively when you compute them (or in
_get_score) to avoid zero-width ranges, e.g.:- self.spot.pixel_count.range = (min_pixel_count, self.spot.MAX_PIXEL_COUNT) + hi = max(min_pixel_count + 1, self.spot.MAX_PIXEL_COUNT) + self.spot.pixel_count.range = (min_pixel_count, hi) @@ - self.spot.intensity.range = (int(image_iinfo.min * 0.1), image_iinfo.max) + lo = int(image_iinfo.min * 0.1) + hi = max(lo + 1, image_iinfo.max) + self.spot.intensity.range = (lo, hi)and/or add a small guard in
_get_scorebefore dividing by the range.This keeps the alignment from failing catastrophically on odd metadata configurations.
964-986: Harden_get_scoreagainst zero-width ranges
_get_scoreassumes bothpixel_count.rangeandintensity.rangehave non-zero width:area_range = float(self.spot.pixel_count.range[1] - self.spot.pixel_count.range[0]) intensity_range = float(self.spot.intensity.range[1] - self.spot.intensity.range[0])If those ever collapse (e.g. min == max after a dynamic update), this will raise
ZeroDivisionErrorand abort the alignment.Defensive guards keep behaviour while avoiding crashes, for example:
- area_range = float(self.spot.pixel_count.range[1] - self.spot.pixel_count.range[0]) + area_range = float(self.spot.pixel_count.range[1] - self.spot.pixel_count.range[0]) + if area_range <= 0: + area_range = 1.0 @@ - intensity_range = float(self.spot.intensity.range[1] - self.spot.intensity.range[0]) + intensity_range = float(self.spot.intensity.range[1] - self.spot.intensity.range[0]) + if intensity_range <= 0: + intensity_range = 1.0Given this runs inside a long background task, avoiding a hard crash here is important.
src/odemis/acq/align/test/mirror_test.py (1)
465-479: Fixtest_copies_input_arrayso it genuinely checks copy semanticsAs written,
original_id[0]is a list, not the id of the original array, soassertNotEqual(id(x), original_id[0])will always pass. This means the test won’t fail if_wrap_closed_loop_functionstops copying the input.You can simplify the API and make the assertion meaningful like this:
- def test_copies_input_array(self): - """Wrapper should not modify the input array passed to objective.""" - - def func_check_copy(x, original_id): - # Check that wrapped passes a copy, not the original - self.assertNotEqual(id(x), original_id[0]) - return 1.0, x - - x_input = numpy.array([1.0, 2.0, 3.0]) - _, wrapped = _wrap_closed_loop_function( - func_check_copy, (([id(x_input)],),), 10 - ) - - wrapped(x_input) + def test_copies_input_array(self): + """Wrapper should not pass the original array instance to the objective.""" + + def func_check_copy(x, original_id): + # Wrapped should pass a copy, not the original object + self.assertNotEqual(id(x), original_id) + return 1.0, x + + x_input = numpy.array([1.0, 2.0, 3.0]) + original_id = id(x_input) + _, wrapped = _wrap_closed_loop_function( + func_check_copy, (original_id,), 10 + ) + + wrapped(x_input)This keeps the intent but ensures the test will actually catch regressions.
🧹 Nitpick comments (5)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (2)
862-905: Tighten_on_btn_auto_alignUX and docsLogic is generally good (reset gauge, WD sanity check, axis wiring, task kickoff + callbacks), but two small improvements would help:
- The docstring still says it “uses the current values from the auto‑alignment settings (search range and max iterations)”, but the call to
parabolic_mirror_alignmentalways uses its defaults. Either pass the panel settings through or update the docstring to match the current behaviour.- Consider passing a parent (e.g.
self.main_frame) instead ofNonetowx.MessageDialogso the warning is properly modal to the main window.These are UX/documentation nits rather than functional bugs.
862-917: Optionally silence Ruff ARG002 by marking unused callback paramsRuff reports
evt,start, andendas unused. Since these signatures are dictated by wx /ProgressiveFuture, the simplest non-invasive fix—if you care about a clean lint run—is to rename them to_evt,_start,_endor add a small dummy use.Example:
- def _on_btn_auto_align(self, evt): + def _on_btn_auto_align(self, _evt): @@ - def _on_auto_align_update(self, future, start, end): + def _on_auto_align_update(self, future, _start, _end):This keeps behaviour unchanged while satisfying the linter.
src/odemis/acq/align/mirror.py (3)
753-754: Fix unit comment forMIN_SEARCH_RANGE
MIN_SEARCH_RANGE = 5e-6is expressed in metres, but the comment says# [μm]. That’s misleading since the rest of the code treats search ranges in metres.Consider clarifying, e.g.:
- MIN_SEARCH_RANGE = 5e-6 # [μm] + MIN_SEARCH_RANGE = 5e-6 # 5 µm, expressed in metres
988-1054: Keep objective docstrings in sync with closed-loop return shapeBoth
_objective_1dand_objective_ndnow follow the “closed-loop” contract ((score, actual_position)), which is correct for_wrap_closed_loop_functionand the custom optimizers, but the docstrings still say they return a scalar float.To avoid confusion for future maintainers, it would be good to update the docstrings (and type hints if you add them) to say they return
(score, pos_actual)/(score, xk_actual)rather than just a float.
1139-1213: Custom Nelder–Mead integration with snapping and early-stop looks solid but depends on SciPy internalsThe
_run_nelder_meadmethod is well-structured:
- Uses current hardware positions as
initial_guess.- Constructs bounded regions clipped by
axis.abs_bounds.- Passes per-axis
min_step_sizeand a seeded RNG into_custom_minimize_neldermeadfor deterministic snapping.- Uses
_callback_ndfor stall-based early stopping and cleans internal counters in thefinallyblock.However, this relies on a fork of SciPy’s internal Nelder–Mead (
_custom_minimize_neldermead) and private symbols like_status_message. That’s fine short term but couples you to SciPy’s internal API.Please double‑check that the SciPy version you depend on matches the internal signatures you’ve copied, and consider pinning SciPy (or documenting the required version) to avoid silent breakage on upgrades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)src/odemis/gui/cont/tabs/sparc2_align_tab.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/cont/tabs/sparc2_align_tab.pysrc/odemis/acq/align/mirror.py
🧬 Code graph analysis (3)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (3)
src/odemis/acq/align/mirror.py (3)
parabolic_mirror_alignment(694-735)AlignmentAxis(608-622)cancel(790-803)src/odemis/gui/conf/data.py (2)
get_hw_config(1243-1267)get_local_vas(1284-1311)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)
src/odemis/acq/align/mirror.py (1)
src/odemis/model/_futures.py (3)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)set_progress(427-444)
src/odemis/acq/align/test/mirror_test.py (4)
src/odemis/acq/align/mirror.py (6)
AlignmentAxis(608-622)_custom_minimize_neldermead(307-604)_custom_minimize_scalar_bounded(142-304)_probabilistic_snap_to_grid(92-139)_wrap_closed_loop_function(55-89)parabolic_mirror_alignment(694-735)src/odemis/util/testing.py (1)
stop_backend(135-169)src/odemis/util/img.py (1)
append(1086-1142)src/odemis/gui/model/tab_gui_data.py (1)
step(796-824)
🪛 Ruff (0.14.6)
src/odemis/gui/cont/tabs/sparc2_align_tab.py
862-862: Unused method argument: evt
(ARG002)
907-907: Unused method argument: start
(ARG002)
907-907: Unused method argument: end
(ARG002)
src/odemis/acq/align/mirror.py
70-70: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Prefer TypeError exception for invalid type
(TRY004)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Unused function argument: unknown_options
(ARG001)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
183-184: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
217-217: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
263-263: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
381-381: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
391-391: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
425-425: Avoid specifying long messages outside the exception class
(TRY003)
448-448: Avoid specifying long messages outside the exception class
(TRY003)
450-450: Avoid specifying long messages outside the exception class
(TRY003)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
790-790: Unused method argument: future
(ARG002)
867-867: Avoid specifying long messages outside the exception class
(TRY003)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1033-1033: Avoid specifying long messages outside the exception class
(TRY003)
1036-1036: Avoid specifying long messages outside the exception class
(TRY003)
1038-1038: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1068-1068: Avoid specifying long messages outside the exception class
(TRY003)
1077-1077: Avoid specifying long messages outside the exception class
(TRY003)
1163-1163: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1342-1342: Avoid specifying long messages outside the exception class
(TRY003)
src/odemis/acq/align/test/mirror_test.py
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
94-94: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
95-95: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
166-166: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
390-390: Unused function argument: x
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (11)
src/odemis/gui/cont/tabs/sparc2_align_tab.py (3)
828-846: Mirror auto-align enablement block looks soundConditionally enabling the auto-align UI only when mirror/stage/ccd/ebeam_focus exist and both
auto_align_min_step_sizeandebeam_working_distanceare present inMD_CALIBavoids runtime key errors and hides controls when calibration is unavailable. Initializing_mirror_auto_align_futurewithInstantaneousFuture()is a reasonable default for the “not running” state.
907-940: Progress and completion handling for auto-align looks correctUsing
add_update_callbackwithwx.CallAfterto update the gauge from the worker thread, and resetting_mirror_auto_align_futureback to anInstantaneousFutureplus restoring the button label in thefinallyblock ensures consistent UI state across success, cancellation, and failure. This is a clean integration withProgressiveFuture.
1456-1462: Mirror auto-alignment doc hook is consistentAdding
doc/sparc2_mirror_auto_alignment.htmlto the mirror-align branch matches the new feature and keeps the right-hand documentation panel in sync with the UI.src/odemis/acq/align/mirror.py (3)
1083-1137: Nice reuse of_custom_minimize_scalar_boundedfor 1D scans
_run_scalarwiring looks consistent: you pass_objective_1dplus(axis, objective_kwargs)viaargs, set bounds around the current position with optional absolute limits, and feedx0,min_step_size, andrnginto the custom method. Logging and exception handling (includingCancelledError) are reasonable, and you correctly subtractresult.nitfrom the remaining iteration budget inalign_mirror.No issues here from a correctness perspective.
1215-1345: Overall alignment flow is coherent; only minor nitsThe multi-stage orchestration in
align_mirroris coherent:
- Absolute bounds are set once from the initial positions and
search_range.- For 1D: a coarse scalar scan followed by a refined Nelder–Mead with reduced
search_range.- For 3D: three coarse 1D scans, then two full 3D Nelder–Meads with different quality metrics.
max_iteris treated as a global budget and decremented byresult.nitafter each stage, with guards to avoid calling optimizers with non-positive budgets.- Progress and image saving are kept in sync (
n_steps,current_step,_store_last_image_metadata, background saving thread with a sentinel).Two tiny observations:
- The extra
moveAbs(...).result()calls after_run_scalar/_run_nelder_meadare redundant in principle, since the objectives already move the hardware to the final evaluated position, but they are harmless and might be useful as a final “snap” to the reported optimum.axis.abs_boundsis mutated in-place here; that’s fine as long asAlignmentAxisinstances are not reused across unrelated alignments.Nothing blocking here; just highlighting behaviour for future maintainers.
1001-1046: Add movement timeouts to hardware moves to prevent indefinite blockingThe blocking calls to
.result()in_objective_1d()and_objective_nd()lack timeouts:axis.component.moveAbs({axis.name: pos}).result() ... for axis, value in zip(axes, xk): axis.component.moveAbs({axis.name: value}).result()If a stage/mirror move hangs, the alignment worker thread blocks indefinitely. The
_cancelledflag cannot interrupt an already-blocked.result()call. For long unattended alignments, this presents a reliability risk.Recommended approach: centralize a timeout constant and apply it consistently:
def __init__(...): self._save_thread = threading.Thread(target=self._saving_thread, daemon=True) + self._move_timeout_s = 30.0 @@ def _objective_1d(...): - axis.component.moveAbs({axis.name: pos}).result() + axis.component.moveAbs({axis.name: pos}).result(timeout=self._move_timeout_s) @@ def _objective_nd(...): - for axis, value in zip(axes, xk): - axis.component.moveAbs({axis.name: value}).result() + for axis, value in zip(axes, xk): + axis.component.moveAbs({axis.name: value}).result(timeout=self._move_timeout_s)Apply the same timeout to the final
moveAbs(...).result()call inalign_mirror(). Optionally catchTimeoutErrorand log or abort gracefully. Verify the timeout value matches expected move durations on your hardware.src/odemis/acq/align/test/mirror_test.py (5)
80-151: 3D alignment integration test is well-structuredThe 3D success-rate test exercises realistic misalignments over
l,s, andz, uses a fixed RNG seed, logs failures with context, and resets the hardware between runs viasetUp(). The 90% pass-rate threshold plus explicit intensity floor gives a clear safety margin without making the test flaky.No issues from a test-design standpoint.
153-219: 1D alignment integration test mirrors the 3D case appropriatelyThe 1D z-only test mirrors the 3D test structure (fixed seed, misalignment range, intensity threshold, per-run reset) and validates that the 1D alignment path behaves consistently.
Looks good and should catch regressions in the 1D flow.
222-329: Probabilistic grid snapping tests give good coverageThe
_probabilistic_snap_to_gridtests cover:
- Identity behaviour when
min_step_sizeisNone.- Zero/non-zero origin grids.
- Per-dimension step sizes.
- Approximate probability distribution with a reasonable tolerance.
- Array-of-points inputs.
This is a solid test suite for the snapping helper; no changes needed.
331-464:_wrap_closed_loop_functiontest suite is thoroughThe wrapper tests validate:
- Return shape (ncalls list and callable wrapper).
Nonebehaviour.- Call counting and
maxfunenforcement via_MaxFuncCallError.- Validation of return type/length, scalar score, and ndarray position.
- Propagation of extra args and complete tuple return.
These give good confidence in the wrapper’s behaviour.
481-583: Custom minimizer tests nicely isolate scalar vs Nelder–Mead behaviourThe tests for
_custom_minimize_scalar_boundedand_custom_minimize_neldermeadcheck:
- Respect for bounds and clipping of out-of-bounds initial guesses.
- Behaviour with and without snapping (
min_step_size/rng).- Convergence to known minima and to grid points when snapping is enabled.
This is a good level of coverage for the numerical helpers.
|
@tepals, @pieleric this PR is ready for review. NOTE: we must not merge this PR in This PR has the history of comments and separate commits as per the auto-mirror alignment development progress, so its easy to review here. Do you agree? Once the PR is reviewed, the core algorithm will be migrated and then I will push the changes accordingly and request for the final integration review. If you agree, please review until the second last commit d75c13c. The last commit 96d454f still needs to be tested on the system. |
nandishjpatel
force-pushed
the
CSDM-1150-implement-deterministic-automatic-mirror-alignment-algorithm-in-the-gui
branch
from
96d454f to
087846e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/odemis/acq/align/mirror.py (2)
1138-1212: Nelder–Mead runner and bounds handling are appropriate; considerzip(..., strict=True)for safety.The runner:
- logs axes, step sizes, and bounds
- derives
initial_guessfrom current positions- builds bounds intersected with
abs_boundsand standardized for Nelder–Mead- passes
maxiter, snapping parameters, RNG, and callback into_custom_minimize_neldermead- correctly resets internal iteration/stall state in
finallyTo make future refactors safer, you can add
strict=Trueto the zips where you assume matched lengths:- for axis, guess in zip(axes, initial_guess): + for axis, guess in zip(axes, initial_guess, strict=True):This will raise early if
axesandinitial_guessever diverge.
988-1055: Objective functions are consistent with the closed-loop optimizer expectations, but lack movement timeouts.The 1D and ND objectives correctly:
- check the cancellation flag early
- move actuators, then call
_update_spot_measurementand_get_score- return both score and “actual” positions for closed-loop optimization
- log target vs. actual positions in the ND case
However,
axis.component.moveAbs(...).result()is called without a timeout. If a move hangs (hardware failure, simulation stall), the worker thread can block indefinitely and neither cancellation nor progress reporting can recover.Consider introducing a per-task movement timeout in
__init__and applying it uniformly:class ParabolicMirrorAlignmentTask: @@ def __init__(...): @@ - self._save_thread = threading.Thread(target=self._saving_thread, daemon=True) + self._save_thread = threading.Thread(target=self._saving_thread, daemon=True) + self._move_timeout_s = 30.0 # safety timeout for actuator moves @@ - axis.component.moveAbs({axis.name: pos}).result() + axis.component.moveAbs({axis.name: pos}).result(timeout=self._move_timeout_s) @@ - for axis, value in zip(axes, xk): - axis.component.moveAbs({axis.name: value}).result() + for axis, value in zip(axes, xk): + axis.component.moveAbs({axis.name: value}).result(timeout=self._move_timeout_s)You can optionally wrap these in
try/exceptforTimeoutErrorto log and abort the alignment cleanly.src/odemis/acq/align/test/mirror_test.py (1)
465-479: Fixtest_copies_input_arrayso it actually validates copy semantics.As written,
original_idis passed in as a nested tuple/list andoriginal_id[0]is not the array id, soassertNotEqual(id(x), original_id[0])is effectively always true. The test will still pass even if_wrap_closed_loop_functionstops copying its input.Refactor to compare against the actual id of the original array:
def test_copies_input_array(self): - """Wrapper should not modify the input array passed to objective.""" - - def func_check_copy(x, original_id): - # Check that wrapped passes a copy, not the original - self.assertNotEqual(id(x), original_id[0]) - return 1.0, x - - x_input = numpy.array([1.0, 2.0, 3.0]) - _, wrapped = _wrap_closed_loop_function( - func_check_copy, (([id(x_input)],),), 10 - ) - - wrapped(x_input) + """Wrapper should not pass the original array instance to the objective.""" + + def func_check_copy(x, original_id): + # Wrapped should pass a copy, not the original + self.assertNotEqual(id(x), original_id) + return 1.0, x + + x_input = numpy.array([1.0, 2.0, 3.0]) + original_id = id(x_input) + _, wrapped = _wrap_closed_loop_function( + func_check_copy, (original_id,), 10 + ) + + wrapped(x_input)
🧹 Nitpick comments (5)
src/odemis/acq/align/mirror.py (5)
607-623: AlignmentAxis validation is appropriate but error types could be tightened (optional).Axis name membership and
min_step_sizerange checks are good guards against misconfiguration. If you want to align more with type‑semantics, usingTypeErrorfor a non‑Actuator component andValueErrorfor bad values would satisfy Ruff’s TRY004, but that’s optional.
625-665: Spot container matches the scoring needs, minor intensity-range comment mismatch.The three metrics (pixel_count, intensity, major_axis) and their units/initial ranges fit the later use in
_update_spot_measurementand_get_score. One nit:uint16_iinfo.minis 0, so “at least 10% of uint16 min” is misleading; if you intended a non-zero floor, consider basing it onmaxinstead.
790-819: Saving thread lifecycle is mostly safe; consider guarding join when thread is never started (optional).Currently
_stop_saving_threadunconditionally joins the thread; since you only call it whenself._save_imagesis true and you always start the thread in that branch, it’s fine. If future refactors ever change that ordering, a simpleif self._save_thread.is_alive():guard beforejoinwould make it more robust.
852-886: Spot measurement setup is reasonable; guard against extreme slit/sensor configs (optional).Deriving
pixel_count.rangefrom slit width and sensor pixel size, and tighteningintensity/major_axisranges per image, is a good response to prior feedback. In pathological configurations where the implied slit area exceedsMAX_PIXEL_COUNT,pixel_count.range[0]could exceedrange[1]; a simple clamp like:min_count = min(min_pixel_count, self.spot.MAX_PIXEL_COUNT - 1) self.spot.pixel_count.range = (min_count, self.spot.MAX_PIXEL_COUNT)would defensively avoid inverted ranges.
947-986: Harden_get_scoreagainst zero-width ranges to avoid potential division-by-zero.You assume
pixel_count.rangeandintensity.rangehave non-zero width. In unusual configurations (e.g., misconfigured ranges or degenerate images) those could collapse and raiseZeroDivisionError.A small defensive tweak keeps behavior while avoiding crashes:
- if quality_metric in (SpotQualityMetric.PIXEL_COUNT, SpotQualityMetric.PIXEL_COUNT_AND_INTENSITY): + if quality_metric in (SpotQualityMetric.PIXEL_COUNT, SpotQualityMetric.PIXEL_COUNT_AND_INTENSITY): # Normalize pixel count - area_range = float(self.spot.pixel_count.range[1] - self.spot.pixel_count.range[0]) + area_range = float(self.spot.pixel_count.range[1] - self.spot.pixel_count.range[0]) + if area_range <= 0: + area_range = 1.0 @@ - if quality_metric in (SpotQualityMetric.INTENSITY, SpotQualityMetric.PIXEL_COUNT_AND_INTENSITY): + if quality_metric in (SpotQualityMetric.INTENSITY, SpotQualityMetric.PIXEL_COUNT_AND_INTENSITY): # Normalize intensity - intensity_range = float(self.spot.intensity.range[1] - self.spot.intensity.range[0]) + intensity_range = float(self.spot.intensity.range[1] - self.spot.intensity.range[0]) + if intensity_range <= 0: + intensity_range = 1.0This keeps scores bounded without surprising behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/align/mirror.py(1 hunks)src/odemis/acq/align/test/mirror_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/acq/align/mirror.py
🧬 Code graph analysis (2)
src/odemis/acq/align/test/mirror_test.py (1)
src/odemis/acq/align/mirror.py (5)
AlignmentAxis(608-622)_custom_minimize_scalar_bounded(142-304)_probabilistic_snap_to_grid(92-139)_wrap_closed_loop_function(55-89)parabolic_mirror_alignment(694-735)
src/odemis/acq/align/mirror.py (2)
src/odemis/model/_components.py (3)
model(570-571)Actuator(773-965)HwComponent(277-542)src/odemis/model/_futures.py (2)
ProgressiveFuture(371-507)set_progress(427-444)
🪛 Ruff (0.14.8)
src/odemis/acq/align/test/mirror_test.py
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
94-94: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
95-95: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
166-166: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
390-390: Unused function argument: x
(ARG001)
src/odemis/acq/align/mirror.py
70-70: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Prefer TypeError exception for invalid type
(TRY004)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Unused function argument: unknown_options
(ARG001)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
183-184: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
217-217: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
263-263: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
381-381: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
391-391: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
425-425: Avoid specifying long messages outside the exception class
(TRY003)
448-448: Avoid specifying long messages outside the exception class
(TRY003)
450-450: Avoid specifying long messages outside the exception class
(TRY003)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
790-790: Unused method argument: future
(ARG002)
867-867: Avoid specifying long messages outside the exception class
(TRY003)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1033-1033: Avoid specifying long messages outside the exception class
(TRY003)
1036-1036: Avoid specifying long messages outside the exception class
(TRY003)
1038-1038: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1067-1067: Avoid specifying long messages outside the exception class
(TRY003)
1076-1076: Avoid specifying long messages outside the exception class
(TRY003)
1162-1162: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1341-1341: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (17)
src/odemis/acq/align/test/mirror_test.py (6)
53-152: Alignment success-rate tests look robust and representative.Backend lifecycle, random misalignment ranges, intensity thresholds, and pass‑rate assertions are consistent with the alignment API and simulate realistic misalignments. No changes needed here.
153-219: 1D alignment test mirrors the 3D flow appropriately.Same seeding, range, threshold and pass‑rate logic as the 3D test, but isolated to the z axis. This gives good coverage of the 1D branch of
parabolic_mirror_alignment. Looks good.
222-329: Probabilistic snapping tests are thorough and well structured.You cover:
- disabling via
min_step_size=None- zero and non‑zero origins
- per‑axis step sizes
- approximate distribution (with tolerance)
- array‑of‑points inputs
This gives solid confidence in
_probabilistic_snap_to_grid. No issues from my side.
331-464: Wrapper behavior tests are comprehensive and match the wrapper’s contract.The tests around
ncalls, maxfun enforcement, tuple and type validation, extra args, and return tuple semantics align well with_wrap_closed_loop_function’s implementation. They should catch most regressions in that helper.
481-546: Scalar bounded minimizer tests cover the key behaviors.You verify bounds respect, clipping of out‑of‑bounds initial guesses, and the impact of enabling/disabling probabilistic snapping. This matches the expected contract of
_custom_minimize_scalar_bounded. No additional changes needed.
548-584: Nelder–Mead custom minimizer tests exercise both continuous and snapped modes.The quadratic objective, disabled/enabled snapping, and assertions on convergence vs. grid alignment provide good coverage for
_custom_minimize_neldermead. Implementation and tests look aligned.src/odemis/acq/align/mirror.py (11)
55-89: Closed-loop wrapper is consistent with tests and intended contract.The wrapper:
- counts calls and enforces
maxfunvia_MaxFuncCallError- copies the input array before delegating
- validates
(score, position)types and shapes- returns the complete tuple for downstream use
This matches the dedicated tests in
TestWrapClosedLoopFunctionand is a reasonable abstraction.
92-140: Probabilistic grid snapping helper is well designed for actuator quantization.The implementation cleanly:
- supports scalar and array inputs with broadcasting
- injects a pluggable RNG for reproducibility
- probabilistically rounds based on the fractional grid index
- is a no-op when
min_step_size is NoneGiven it’s only used for hardware step snapping (not crypto), the RNG choice is appropriate despite the S311 linter warning.
142-305: Custom bounded scalar minimizer integration looks sound.The adaptation of SciPy’s bounded algorithm to:
- apply probabilistic snapping and clipping before each evaluation, and
- accept an objective returning
(fval, x_actual)is coherent and exercised by the dedicated tests. Bounds and
maxiterhandling match expectations, andOptimizeResultis populated consistently. No issues spotted here.
307-605: Custom Nelder–Mead with snapping and closed-loop objectives is consistent.You:
- wrap the objective with
_wrap_closed_loop_functionto track evaluations and enforcemaxfev- apply
_probabilistic_snap_to_gridand bounds clipping at every vertex update- preserve SciPy’s control flow, warnings, and
OptimizeResultfieldsThe logic aligns with the tests in
TestCustomMinimizeNelderMead. This part looks correct.
667-692: SpotQualityMetric enum clearly documents the scoring modes.Enum members and docstring match how
_get_scoreswitches onquality_metric, including the combined and major-axis modes. Looks good.
694-735: Publicparabolic_mirror_alignmentAPI is clean and integrates well with the executor.The function:
- constructs a
ProgressiveFuturewith progress fields- instantiates
ParabolicMirrorAlignmentTask- wires
task_canceller- submits
align_mirroron the dedicated executorOne minor behavior note: invalid axis counts (other than 1 or 3) are rejected later in
align_mirrorwith aValueError, which is acceptable as long as callers are aware.
738-789: Task initialization and background saving plumbing are solid.Storing axes/CCD/future, cancellation flag, iteration/stall tracking, reproducible RNG, and a dedicated save thread/queue is a good structure. The save thread only starts when
save_imagesis true and is joined infinally, which avoids leaks in the normal path.
837-851: Metadata enrichment for saved images is a nice touch.Embedding current axis positions into
MD_EXTRA_SETTINGSand stripping AR/spectrometry metadata to keep images viewable in the Odemis viewer is well thought-out. No concerns here.
887-945: Contour-based spot extraction and fallback look robust for noisy images.Blur → normalize → Otsu threshold → morphological close → largest-contour area and fitted ellipse major axis is a solid metric pipeline, and the corner-based fallback should still produce sensible counts when no contours are found.
Given tests validate scoring behavior rather than exact morphology details, this looks fine.
1056-1081: Nelder–Mead callback and early-stop logic look correct.The callback:
- re-checks cancellation
- detects stalls via repeated best vertex
- raises
StopIterationafter a configurable number of stalls- logs iterations and increments
self._iteration_count
_run_nelder_meadmapsStopIterationintoOptimizeResult.messageandnit, so this is wired through cleanly.
1082-1136: Scalar runner wiring is good.The integration with
_custom_minimize_scalar_boundedis logically consistent: you passfun=self._objective_1d,args=(axis, objective_kwargs), bounds derived from current position andabs_bounds, andmethod=_custom_minimize_scalar_boundedwith the extra options it expects. SciPy'sminimize_scalardoes accept callable methods (in addition to string method names), so this approach will work correctly.Likely an incorrect or invalid review comment.
| def align_mirror(self, search_range: float, max_iter: int): | ||
| """ | ||
| Execute the full alignment procedure. Can handle 1D or 3D optimizations. | ||
| 1D alignment (single axis): | ||
| - Run a single 1D bounded search to maximize intensity. | ||
| - Run a Nelder-Mead refinement over the axis, reducing the search range. | ||
| 3D alignment (three axes): | ||
| - Run independent 1D bounded searches for each axis to maximize intensity. | ||
| - Run two successive 3D Nelder-Mead refinements over all axes, reducing | ||
| the search range each time. | ||
| The method logs progress and handles CancelledError at each stage so a | ||
| cancellation request aborts the remaining steps cleanly. | ||
| :param search_range: Initial half-range (metres) for coarse searches. | ||
| :param max_iter: Total maximum iterations budget (shared between stages). | ||
| """ | ||
| # Define absolute bounds that should never be exceeded | ||
| for axis in self._axes: | ||
| x0 = axis.component.position.value[axis.name] | ||
| axis.abs_bounds = (x0 - search_range, x0 + search_range) | ||
|
|
||
| if self._save_images: | ||
| path = os.path.join(PATH_IMAGES, datetime.now().strftime("%Y%m%d-%H%M%S")) | ||
| os.makedirs(path, exist_ok=True) | ||
| self._save_thread.start() | ||
|
|
||
| try: | ||
| if len(self._axes) == 1: | ||
| self._future.n_steps = 2 | ||
| axis = self._axes[0] | ||
| # 1D coarse scan | ||
| start = time.time() | ||
| result = self._run_scalar( | ||
| axis, | ||
| search_range, | ||
| max_iter, | ||
| objective_kwargs={"quality_metric": SpotQualityMetric.INTENSITY}, | ||
| ) | ||
| axis.component.moveAbs({axis.name: result.x}).result() | ||
| end = time.time() | ||
| logging.debug(f"1D coarse scan for {axis.name} took {end - start:.2f} seconds") | ||
| if self._save_images: | ||
| self._store_last_image_metadata() | ||
| self._enqueue_save(os.path.join(path, f"1d_scan_coarse_{axis.name}.h5"), self._last_img) | ||
| self._future.current_step = 1 | ||
| self._future.set_progress() | ||
|
|
||
| max_iter -= result.nit | ||
| if max_iter < 1: | ||
| return | ||
|
|
||
| # Nelder-Mead refinement | ||
| search_range = self.MIN_SEARCH_RANGE | ||
| start = time.time() | ||
| result = self._run_nelder_mead( | ||
| [axis], | ||
| search_range, | ||
| max_iter, | ||
| objective_kwargs={"quality_metric": SpotQualityMetric.PIXEL_COUNT_AND_INTENSITY, "weight": 0.5}, | ||
| ) | ||
| axis.component.moveAbs({axis.name: result.x[0]}).result() | ||
| end = time.time() | ||
| logging.debug(f"1D Nelder-Mead refinement for {axis.name} took {end - start:.2f} seconds") | ||
| if self._save_images: | ||
| self._store_last_image_metadata() | ||
| self._enqueue_save(os.path.join(path, f"1d_scan_refinement_{axis.name}.h5"), self._last_img) | ||
| self._future.current_step = 2 | ||
| self._future.set_progress() | ||
| elif len(self._axes) == 3: | ||
| self._future.n_steps = 5 | ||
| # 1D coarse scans for each axis | ||
| for i, axis in enumerate(self._axes, start=1): | ||
| start = time.time() | ||
| result = self._run_scalar( | ||
| axis, | ||
| search_range, | ||
| max_iter, | ||
| objective_kwargs={"quality_metric": SpotQualityMetric.INTENSITY}, | ||
| ) | ||
| if axis.name == "z": | ||
| axis.component.moveAbs({axis.name: result.x}).result() | ||
| end = time.time() | ||
| logging.debug(f"1D coarse scan for {axis.name} took {end - start:.2f} seconds") | ||
|
|
||
| if self._save_images: | ||
| self._store_last_image_metadata() | ||
| self._enqueue_save(os.path.join(path, f"1d_scan_coarse_{axis.name}.h5"), self._last_img) | ||
| self._future.current_step = i | ||
| self._future.set_progress() | ||
| max_iter -= result.nit | ||
| if max_iter < 1: | ||
| return | ||
|
|
||
| # Two Nelder-Mead refinements over all axes | ||
| for j in range(1, 3): | ||
| search_range = max(self.MIN_SEARCH_RANGE, search_range / 2) | ||
| if j == 1: | ||
| objective_kwargs = {"quality_metric": SpotQualityMetric.MAJOR_AXIS} | ||
| else: | ||
| objective_kwargs = { | ||
| "quality_metric": SpotQualityMetric.PIXEL_COUNT_AND_INTENSITY, | ||
| "weight": 0.4 | ||
| } | ||
| start = time.time() | ||
| result = self._run_nelder_mead( | ||
| self._axes, | ||
| search_range, | ||
| max_iter, | ||
| objective_kwargs, | ||
| ) | ||
| for k, axis in enumerate(self._axes): | ||
| axis.component.moveAbs({axis.name: result.x[k]}).result() | ||
| end = time.time() | ||
| logging.debug(f"3D Nelder-Mead refinement {j} took {end - start:.2f} seconds") | ||
|
|
||
| if self._save_images: | ||
| self._store_last_image_metadata() | ||
| self._enqueue_save(os.path.join(path, f"3d_scan_refinement_{j}.h5"), self._last_img) | ||
| self._future.current_step = i + j | ||
| self._future.set_progress() | ||
| max_iter -= result.nit | ||
| if max_iter < 1: | ||
| return | ||
| else: | ||
| raise ValueError("Expected 1 or 3 alignment axes") | ||
| finally: | ||
| if self._save_images: | ||
| self._stop_saving_thread() |
There was a problem hiding this comment.
Overall alignment orchestration is well thought-out; reuse movement timeout and tighten zip.
Positives:
- Absolute per-axis bounds are frozen from the initial
search_range. - 1D and 3D branches share a consistent pattern: coarse 1D scans, then one (1D) or two (3D) Nelder–Mead refinements with decreasing
search_range. max_iteris treated as a shared budget and decremented byresult.nitafter each stage, with early returns when exhausted.- Progress fields (
n_steps,current_step) and image saving steps are updated coherently. - Final NM refinements update all axes using
result.x[k].
Concerns and small improvements:
- Hardware moves without timeout (same concern as objectives).
Final positioning moves use bare .result() and can hang indefinitely:
axis.component.moveAbs({axis.name: result.x}).result()
axis.component.moveAbs({axis.name: result.x[0]}).result()
...
axis.component.moveAbs({axis.name: result.x[k]}).result()Once you introduce _move_timeout_s as suggested earlier, please apply it here as well:
- axis.component.moveAbs({axis.name: result.x}).result()
+ axis.component.moveAbs({axis.name: result.x}).result(timeout=self._move_timeout_s)
@@
- axis.component.moveAbs({axis.name: result.x[0]}).result()
+ axis.component.moveAbs({axis.name: result.x[0]}).result(timeout=self._move_timeout_s)
@@
- if axis.name == "z":
- axis.component.moveAbs({axis.name: result.x}).result()
+ if axis.name == "z":
+ axis.component.moveAbs({axis.name: result.x}).result(timeout=self._move_timeout_s)
@@
- for k, axis in enumerate(self._axes):
- axis.component.moveAbs({axis.name: result.x[k]}).result()
+ for k, axis in enumerate(self._axes):
+ axis.component.moveAbs({axis.name: result.x[k]}).result(timeout=self._move_timeout_s)zipstrictness (minor).
You can also add strict=True to the 3D final‑move loop for extra safety:
- for k, axis in enumerate(self._axes):
- axis.component.moveAbs({axis.name: result.x[k]}).result(timeout=self._move_timeout_s)
+ for k, axis in enumerate(self._axes):
+ axis.component.moveAbs({axis.name: result.x[k]}).result(timeout=self._move_timeout_s)(or iterate over zip(self._axes, result.x, strict=True) instead of enumerate).
Overall structure is solid; addressing the timeout concern will significantly improve robustness against hardware issues.
🧰 Tools
🪛 Ruff (0.14.8)
1341-1341: Avoid specifying long messages outside the exception class
(TRY003)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/odemis/acq/align/mirror.py (1)
1046-1047: Add timeout to hardware movements in_objective_ndto prevent indefinite blocking.Line 1047 calls
moveAbs(...).result()in a loop without a timeout, which can block indefinitely if any actuator hangs. Apply the same timeout pattern as suggested for_objective_1d:for axis, value in zip(axes, xk): - axis.component.moveAbs({axis.name: value}).result() + axis.component.moveAbs({axis.name: value}).result(timeout=self._move_timeout_s)
🧹 Nitpick comments (4)
src/odemis/acq/align/mirror.py (4)
381-391: Consider addingstacklevel=2to deprecation warnings for clearer stack traces.The
warnings.warncalls on lines 381 and 391 lack an explicitstacklevelparameter. Settingstacklevel=2will point the warning to the caller of this function rather than this line, making it easier for users to identify where deprecated parameters were used.
948-994: Consider defensive checks for zero-width ranges in_get_score.If
self.spot.pixel_count.rangeorself.spot.intensity.rangeever has zero width (e.g., due to extreme configurations), lines 968 and 976 will divide by zero. While unlikely given the initialization inSpot.__init__and updates in_update_spot_measurement, a defensive check would prevent crashes:area_range = float(self.spot.pixel_count.range[1] - self.spot.pixel_count.range[0]) if area_range <= 0: area_range = 1.0 # Fallback to avoid division by zeroApply similarly to
intensity_rangeon line 976.
1170-1170: Consider addingstrict=Truetozip()for additional safety.Line 1170 uses
zip(axes, initial_guess)withoutstrict=True. Whileaxesandinitial_guessare constructed to match, addingstrict=Trueprovides explicit validation and clearer error messages if they ever diverge due to future refactoring:- for axis, guess in zip(axes, initial_guess): + for axis, guess in zip(axes, initial_guess, strict=True):This requires Python 3.10+, which appears to be supported based on the codebase patterns.
1046-1046: Consider addingstrict=Truetozip()for additional safety.Line 1046 uses
zip(axes, xk)withoutstrict=True. While the code validatesxk.size != len(axes)at line 1043, addingstrict=Trueprovides an additional layer of safety:- for axis, value in zip(axes, xk): + for axis, value in zip(axes, xk, strict=True):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/align/mirror.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/acq/align/mirror.py
🧬 Code graph analysis (1)
src/odemis/acq/align/mirror.py (1)
src/odemis/model/_futures.py (3)
CancellableThreadPoolExecutor(33-120)ProgressiveFuture(371-507)set_progress(427-444)
🪛 Ruff (0.14.8)
src/odemis/acq/align/mirror.py
70-70: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
84-84: Prefer TypeError exception for invalid type
(TRY004)
84-84: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Unused function argument: unknown_options
(ARG001)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
183-184: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
217-217: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
263-263: Consider (*fmin_data, step) instead of concatenation
Replace with (*fmin_data, step)
(RUF005)
381-381: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
391-391: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
425-425: Avoid specifying long messages outside the exception class
(TRY003)
448-448: Avoid specifying long messages outside the exception class
(TRY003)
450-450: Avoid specifying long messages outside the exception class
(TRY003)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
622-622: Avoid specifying long messages outside the exception class
(TRY003)
791-791: Unused method argument: future
(ARG002)
868-868: Avoid specifying long messages outside the exception class
(TRY003)
1007-1007: Avoid specifying long messages outside the exception class
(TRY003)
1041-1041: Avoid specifying long messages outside the exception class
(TRY003)
1044-1044: Avoid specifying long messages outside the exception class
(TRY003)
1046-1046: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1075-1075: Avoid specifying long messages outside the exception class
(TRY003)
1084-1084: Avoid specifying long messages outside the exception class
(TRY003)
1170-1170: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
1348-1348: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (3)
src/odemis/acq/align/mirror.py (3)
607-623: AlignmentAxis validation looks good.The
__post_init__validation properly checks that the axis exists on the component and thatmin_step_sizeis positive and within a reasonable bound (≤ 0.1 m). The upper bound serves as a safety check to catch configuration errors.
695-736: Public API design is clean and well-documented.The
parabolic_mirror_alignmentfunction provides a clear interface with sensible defaults. The returnedProgressiveFutureproperly supports cancellation and progress tracking through dynamically added attributes (n_steps,current_step).
1222-1351: Overall alignment orchestration is well-structured and comprehensive.The
align_mirrormethod implements a robust multi-stage alignment procedure:
- Properly sets absolute bounds to prevent exceeding initial search range
- Handles both 1D and 3D alignment cases with appropriate strategies
- Progressively reduces search range across refinement stages
- Shares iteration budget across stages with early returns when exhausted
- Properly updates progress tracking and saves intermediate images
- Uses appropriate quality metrics for different stages (intensity for coarse scans, major axis and combined metrics for refinements)
The logic is sound and well-organized.
Note: Apply the timeout fixes mentioned in previous comments to make this code production-ready.
| def _objective_1d(self, pos: float, axis: AlignmentAxis, objective_kwargs: dict) -> Tuple[float, float]: | ||
| """ | ||
| 1D objective for optimizing hardware component's axis by maximizing intensity. | ||
| :param pos: Candidate position. | ||
| :param axis: Alignment axis to optimize. | ||
| :raises CancelledError: if the task was cancelled. | ||
| :returns: Tuple[float, float] | ||
| Computed score for the candidate position and actual position after move. | ||
| """ | ||
| if self._cancelled: | ||
| raise CancelledError(f"1D alignment for {axis} was cancelled by user") | ||
|
|
||
| axis.component.moveAbs({axis.name: pos}).result() | ||
|
|
||
| self._update_spot_measurement() | ||
| score = self._get_score(**objective_kwargs) | ||
|
|
||
| # For closed-loop feedback to the optimizer | ||
| pos_actual = axis.component.position.value[axis.name] | ||
|
|
||
| return score, pos_actual |
There was a problem hiding this comment.
Add timeout to hardware movement in _objective_1d to prevent indefinite blocking.
Line 1009 calls moveAbs(...).result() without a timeout, which can block indefinitely if the actuator hangs or fails. This prevents cancellation from working properly and can freeze the alignment process.
Add a timeout attribute in __init__ and apply it here:
def __init__(
self,
axes: List[AlignmentAxis],
ccd: model.HwComponent,
future: model.ProgressiveFuture,
stop_early: bool,
save_images: bool,
):
...
self._save_thread = threading.Thread(target=self._saving_thread, daemon=True)
+ self._move_timeout_s = 30.0 # Safety timeout for actuator movesThen update the move call:
- axis.component.moveAbs({axis.name: pos}).result()
+ axis.component.moveAbs({axis.name: pos}).result(timeout=self._move_timeout_s)Apply the same timeout pattern to all other moveAbs(...).result() calls in _objective_nd (line 1047) and align_mirror (lines 1263, 1285, 1305).
Based on past review comments, this issue was supposedly addressed but the timeout is not present in the current code.
🧰 Tools
🪛 Ruff (0.14.8)
1007-1007: Avoid specifying long messages outside the exception class
(TRY003)
| max_iter, | ||
| objective_kwargs={"quality_metric": SpotQualityMetric.INTENSITY}, | ||
| ) | ||
| axis.component.moveAbs({axis.name: result.x}).result() |
There was a problem hiding this comment.
Add timeout to final positioning move.
Line 1263 performs a final positioning move after the 1D coarse scan without a timeout. Apply the timeout pattern:
- axis.component.moveAbs({axis.name: result.x}).result()
+ axis.component.moveAbs({axis.name: result.x}).result(timeout=self._move_timeout_s)Apply the same fix to lines 1285, 1305 as well.
Also applies to: 1285-1285, 1305-1305
🤖 Prompt for AI Agents
In src/odemis/acq/align/mirror.py lines 1263, 1285, 1305 the final
axis.component.moveAbs(...).result() calls lack a timeout; change each to use
the same timeout pattern used elsewhere in this file by replacing .result() with
.result(timeout=<existing_timeout_variable>) (e.g., self._move_timeout or
move_timeout depending on the variable used in this module) so the move will
raise/timeout consistently; apply the identical change at all three line
locations.
| return ncalls, function_wrapper | ||
|
|
||
|
|
||
| def _probabilistic_snap_to_grid(x, x0, min_step_size=None, rng=None): |
There was a problem hiding this comment.
can you add typing here or in the docstring?
| If 'min_step_size' is 'None' the input 'x' is returned unchanged. | ||
| :param x: Position(s) to snap. | ||
| :param x0: Grid origin. Broadcastable to 'x'. |
There was a problem hiding this comment.
do you mean broadcastable as in numpy broadcasting?
| rng=None, args=(), xatol=1e-5, maxiter=500, disp=0, | ||
| **unknown_options): | ||
| """ | ||
| Parameters |
There was a problem hiding this comment.
this function looks like it's based on/partly copied from scipy _minimize_scalar_bounded. Please add a comment to explain the difference and add a reference to scipy
| unit="a.u.", | ||
| ) | ||
| # Minimize ellipse major axis, penalize objective function by limting a reasonable max value | ||
| self.major_axis = model.IntContinuous(1, range=(1, 256), unit="px") |
There was a problem hiding this comment.
doesn't it make sense to derive the max value here from the dtype as well?
| refinement making use of spot fitted ellipse's major axis and second | ||
| combining normalized spot pixel count and intensity into a single score. | ||
| """ | ||
| MIN_SEARCH_RANGE = 5e-6 # [μm] |
|
|
||
| # Slight Gaussian blur to reduce pixel noise | ||
| # Prevents threshold from overreacting | ||
| blurred = cv2.GaussianBlur(image, (7, 7), 0) |
3 participants
GUI look:
