[feat] Add correction of optical focus during ROA acquisition based o… #3258
Conversation
📝 WalkthroughWalkthroughAdds optional focus-mapping to FASTEM acquisition (new acquire(..., focus_mapping) parameter, AcquisitionTask focus_mapping constructor param, linalg import, runtime validation, and 3D stage target computation when enabled). Updates alignment run() to use the first entry of provided stage_pos for stage movement and to pass focus_pos into run_calibration. GUI: introduces a colour-blind-friendly yellow constant and adds nine focus_map calibrations per scintillator plus visibility toggles for those regions. Tests updated to pass the new focus_mapping argument where applicable. Sequence Diagram(s)sequenceDiagram
participant Caller
participant acquire
participant AcquisitionTask
participant move_stage
Caller->>acquire: acquire(..., focus_mapping=True)
activate acquire
acquire->>AcquisitionTask: __init__(..., focus_mapping=True)
activate AcquisitionTask
AcquisitionTask->>AcquisitionTask: _correct_focus = True
AcquisitionTask->>AcquisitionTask: _focus_plane = stage.metadata.MD_CALIB.focus_plane
deactivate AcquisitionTask
rect rgb(240,248,255)
Note over acquire,move_stage: per-tile acquisition run
acquire->>AcquisitionTask: run()
activate AcquisitionTask
AcquisitionTask->>AcquisitionTask: validate _focus_plane present
AcquisitionTask->>move_stage: move_stage_to_next_tile(tile_xy)
activate move_stage
alt focus mapping enabled
move_stage->>move_stage: z = linalg.get_z_pos_on_plane(plane_normal, gamma, x,y)
move_stage->>move_stage: target = (x,y,z)
else focus mapping disabled
move_stage->>move_stage: target = (x,y)
end
move_stage->>move_stage: stage.moveTo(target)
deactivate move_stage
deactivate AcquisitionTask
end
deactivate acquire
sequenceDiagram
participant Caller
participant AlignRun
participant Calibration
Caller->>AlignRun: run(stage_pos=[(x1,y1),...])
activate AlignRun
note right of AlignRun: when stage_pos provided, use stage_pos[0] as (x,y)
AlignRun->>AlignRun: stage_move = {'x': stage_pos[0][0], 'y': stage_pos[0][1]}
AlignRun->>Calibration: run_calibration(..., focus_pos=stage_move)
activate Calibration
Calibration-->>AlignRun: result
deactivate Calibration
deactivate AlignRun
sequenceDiagram
participant GUI
participant Model
GUI->>Model: initialize scintillator calibrations
activate Model
Model->>Model: add 3 base calibrations
Model->>Model: add focus_map_0..focus_map_8 (colour FG_COLOUR_BLIND_YELLOW)
deactivate Model
GUI->>GUI: on_visibility_btn toggle CALIBRATION_1
GUI->>GUI: also toggle visibility for focus_map_0..focus_map_8
Pre-merge checks and finishing touches✅ 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: 2
🧹 Nitpick comments (1)
src/odemis/acq/fastem.py (1)
163-164: Consider adding type annotation for consistency.The
focus_mappingparameter lacks a type annotation while other parameters likeacq_dwell_timehave explicit type hints. Adding: boolwould improve consistency.def acquire(roa, path, username, scanner, multibeam, descanner, detector, stage, scan_stage, ccd, beamshift, lens, se_detector, ebeam_focus, pre_calibrations=None, save_full_cells=False, settings_obs=None, - spot_grid_thresh=0.5, blank_beam=True, stop_acq_on_failure=True, acq_dwell_time: Optional[float] = None, - focus_mapping=False): + spot_grid_thresh=0.5, blank_beam=True, stop_acq_on_failure=True, acq_dwell_time: Optional[float] = None, + focus_mapping: bool = False):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/fastem.py(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
🧬 Code graph analysis (1)
src/odemis/acq/fastem.py (3)
src/odemis/util/__init__.py (1)
TimeoutError(666-667)src/odemis/model/_components.py (2)
model(570-571)moveAbsSync(852-859)src/odemis/util/linalg.py (1)
get_z_pos_on_plane(180-194)
🪛 Ruff (0.14.3)
src/odemis/acq/fastem.py
377-377: 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-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (5)
src/odemis/acq/fastem.py (5)
48-48: LGTM!The
linalgimport is correctly added to support the focus plane calculations introduced in this PR.
303-304: LGTM!Instance variables are properly initialized. The
_focus_planeis correctly set toNoneand populated later inrun()when focus mapping is enabled.
700-700: LGTM!The
moveAbsSync()call correctly uses thenew_posdictionary that contains either 2D (x, y) or 3D (x, y, z) coordinates depending on whether focus mapping is enabled. Assuming the validation concerns raised in previous comments are addressed, this implementation is sound.
694-694: LGTM!The logging statements appropriately distinguish between 3D positioning (with focus correction) and 2D positioning (without), which will be helpful for debugging and verifying the focus mapping feature is working as expected.
Also applies to: 697-697
217-217: LGTM!The
focus_mappingparameter is correctly propagated from theacquire()function through to theAcquisitionTaskconstructor, and the documentation is clear and consistent throughout.Also applies to: 236-236
| if self._correct_focus: | ||
| # The focus is corrected based on a plane fitted through focus points, during calibration 1 | ||
| # The metadata should look like: {'gamma': -4.29e-05, 'normal': (1.7e-16, -7.99e-17, -1)} | ||
| self._focus_plane = self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane", None) | ||
| if not self._focus_plane: | ||
| raise ValueError("Focus plane not found in stage MD_CALIB metadata, run Calibration 1.") |
There was a problem hiding this comment.
Validate the structure of focus_plane to prevent KeyErrors downstream.
The code validates the presence of focus_plane but not its structure. Lines 690 and 692 expect focus_plane to contain "gamma" and "normal" keys. If these keys are missing or malformed, a KeyError will occur during stage movement, which is harder to debug than an early validation error.
Apply this diff to add structure validation:
if self._correct_focus:
# The focus is corrected based on a plane fitted through focus points, during calibration 1
# The metadata should look like: {'gamma': -4.29e-05, 'normal': (1.7e-16, -7.99e-17, -1)}
self._focus_plane = self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane", None)
if not self._focus_plane:
raise ValueError("Focus plane not found in stage MD_CALIB metadata, run Calibration 1.")
+ if "gamma" not in self._focus_plane or "normal" not in self._focus_plane:
+ raise ValueError(f"Focus plane metadata is incomplete. Expected 'gamma' and 'normal' keys, "
+ f"but got: {self._focus_plane.keys()}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self._correct_focus: | |
| # The focus is corrected based on a plane fitted through focus points, during calibration 1 | |
| # The metadata should look like: {'gamma': -4.29e-05, 'normal': (1.7e-16, -7.99e-17, -1)} | |
| self._focus_plane = self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane", None) | |
| if not self._focus_plane: | |
| raise ValueError("Focus plane not found in stage MD_CALIB metadata, run Calibration 1.") | |
| if self._correct_focus: | |
| # The focus is corrected based on a plane fitted through focus points, during calibration 1 | |
| # The metadata should look like: {'gamma': -4.29e-05, 'normal': (1.7e-16, -7.99e-17, -1)} | |
| self._focus_plane = self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane", None) | |
| if not self._focus_plane: | |
| raise ValueError("Focus plane not found in stage MD_CALIB metadata, run Calibration 1.") | |
| if "gamma" not in self._focus_plane or "normal" not in self._focus_plane: | |
| raise ValueError(f"Focus plane metadata is incomplete. Expected 'gamma' and 'normal' keys, " | |
| f"but got: {self._focus_plane.keys()}") |
🧰 Tools
🪛 Ruff (0.14.3)
377-377: Avoid specifying long messages outside the exception class
(TRY003)
tepals
force-pushed
the
FVEM-26-optical-focus-mapping
branch
from
8fb3593 to
72590e9
Compare
|
@tepals the changes LGTM! A question: how will the |
yes that still needs to be implemented. I want to do some testing first, to see whether it makes sense to have this always enabled or have a checkbox in the GUI. |
…n focus plane AutofocusMultiprobe in fastem-calibrations has been updated to run in multiple positions and fit a plane through those positions. Based on this plane the focus is corrected during ROA acquisition
tepals
force-pushed
the
FVEM-26-optical-focus-mapping
branch
from
6e9b5e0 to
1991832
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/odemis/acq/fastem.py (1)
372-377: Add validation for focus_plane metadata structure.The code retrieves
focus_planefrom metadata but doesn't validate that it contains the required"gamma"and"normal"keys before they're accessed at lines 690 and 692. If these keys are missing, aKeyErrorwill occur during stage movement.Apply this diff:
if self._correct_focus: # The focus is corrected based on a plane fitted through focus points, during calibration 1 # The metadata should look like: {'gamma': -4.29e-05, 'normal': (1.7e-16, -7.99e-17, -1)} self._focus_plane = self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane", None) if not self._focus_plane: raise ValueError("Focus plane not found in stage MD_CALIB metadata, run Calibration 1.") + if "gamma" not in self._focus_plane or "normal" not in self._focus_plane: + raise ValueError(f"Focus plane metadata is incomplete. Expected 'gamma' and 'normal' keys, " + f"but got: {self._focus_plane.keys()}")
🧹 Nitpick comments (1)
src/odemis/gui/model/main_gui_data.py (1)
39-39: Fix import order.The import of
FG_COLOUR_BLIND_YELLOWshould come beforeconfto maintain alphabetical order within theodemis.guiimports.Apply this diff:
from odemis.gui import ( FG_COLOUR_BLIND_BLUE, FG_COLOUR_BLIND_ORANGE, FG_COLOUR_BLIND_PINK, - conf, FG_COLOUR_BLIND_YELLOW, + FG_COLOUR_BLIND_YELLOW, + conf, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/align/fastem.py(1 hunks)src/odemis/acq/fastem.py(9 hunks)src/odemis/acq/test/fastem_test.py(12 hunks)src/odemis/gui/__init__.py(1 hunks)src/odemis/gui/cont/acquisition/fastem_acq.py(2 hunks)src/odemis/gui/model/main_gui_data.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/acq/align/fastem.py
- src/odemis/acq/test/fastem_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/gui/cont/acquisition/fastem_acq.py
🧬 Code graph analysis (2)
src/odemis/gui/model/main_gui_data.py (1)
src/odemis/acq/fastem.py (2)
FastEMCalibration(93-111)FastEMROC(114-139)
src/odemis/acq/fastem.py (3)
src/odemis/util/__init__.py (1)
TimeoutError(666-667)src/odemis/model/_components.py (2)
model(570-571)moveAbsSync(852-859)src/odemis/util/linalg.py (1)
get_z_pos_on_plane(180-194)
🪛 Ruff (0.14.3)
src/odemis/acq/fastem.py
377-377: 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 (3)
src/odemis/gui/__init__.py (1)
51-51: LGTM!The new color constant follows the established naming pattern and is consistent with the other color-blind-friendly color definitions.
src/odemis/gui/cont/acquisition/fastem_acq.py (2)
1693-1695: Verify the visibility toggle behavior is consistent with user expectations.When toggling CALIBRATION_1 visibility, the code also toggles visibility for all 9 focus_map regions. Ensure this behavior is intuitive for users—they might expect to control the focus_map regions independently, or they should be informed that these are linked to CALIBRATION_1.
1952-1960: The verification confirms the code transformation is correct—no changes needed.The
align()function expectsstage_posto be a list and uses the first positionstage_pos[0]for stage movement viamoveAbsSync(), while passing the entirestage_poslist asfocus_posto calibration routines. The code at lines 1952-1960 that convertsstage_posfrom a tuple to a list and appends 9 focus_map positions aligns correctly with this interface. The comment in the code confirms the first position is always the region of calibration (ROC) position, establishing that order is preserved as expected.
| for i, focus_pos in enumerate(( | ||
| (-100, 10), | ||
| (-100, 100), | ||
| (-100, -100), | ||
| (10, -100), | ||
| (10, 100), | ||
| (10, 10), | ||
| (100, 10), | ||
| (100, 100), | ||
| (100, -100), | ||
| )): | ||
| calibration_name = f"focus_map_{i}" | ||
| calibration = FastEMCalibration(name=calibration_name) | ||
| number = 1 | ||
| colour = FG_COLOUR_BLIND_YELLOW | ||
| calibration.sequence.value = calib_1_calibrations | ||
| xmin = position[0] - 0.5 * sz[0] + focus_pos[0] * sz[0] | ||
| xmax = position[0] + 0.5 * sz[0] + focus_pos[0] * sz[0] | ||
| ymin = position[1] + 0.5 * sz[1] + focus_pos[1] * sz[1] | ||
| ymax = position[1] - 0.5 * sz[1] + focus_pos[1] * sz[1] | ||
| calibration.region = FastEMROC(name=str(number), | ||
| scintillator_number=int(scintillator_number), | ||
| coordinates=(xmin, ymin, xmax, ymax), colour=colour) | ||
| scintillator.calibrations[calibration_name] = calibration |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the focus_pos offsets and clarify the spatial pattern.
The 9 focus_pos tuples use hardcoded values like (-100, 10), (100, -100), etc., but there's no explanation of:
- What these values represent (they appear to be multipliers of the field size, e.g., -100 means -100× the field width).
- Why these specific positions were chosen.
- The spatial pattern they form (is it a 3×3 grid? The values don't suggest a regular grid).
Additionally, line 807 has inconsistent spacing: ymax = (extra space before =).
Apply this diff to add documentation and fix spacing:
+ # Create 9 focus_map calibrations distributed around the scintillator
+ # focus_pos values are multipliers of the field size (sz), forming a spatial grid
+ # for multi-point optical focus mapping during Calibration 1
for i, focus_pos in enumerate((
(-100, 10),
(-100, 100),
(-100, -100),
(10, -100),
(10, 100),
(10, 10),
(100, 10),
(100, 100),
(100, -100),
)):
calibration_name = f"focus_map_{i}"
calibration = FastEMCalibration(name=calibration_name)
number = 1
colour = FG_COLOUR_BLIND_YELLOW
calibration.sequence.value = calib_1_calibrations
xmin = position[0] - 0.5 * sz[0] + focus_pos[0] * sz[0]
xmax = position[0] + 0.5 * sz[0] + focus_pos[0] * sz[0]
ymin = position[1] + 0.5 * sz[1] + focus_pos[1] * sz[1]
- ymax = position[1] - 0.5 * sz[1] + focus_pos[1] * sz[1]
+ ymax = position[1] - 0.5 * sz[1] + focus_pos[1] * sz[1]
calibration.region = FastEMROC(name=str(number),
scintillator_number=int(scintillator_number),
coordinates=(xmin, ymin, xmax, ymax), colour=colour)
scintillator.calibrations[calibration_name] = calibrationConsider also defining the focus_pos pattern more explicitly or using a comprehension to generate a regular grid if that's the intent:
# Example for a regular 3x3 grid:
focus_positions = [(x, y) for x in (-100, 0, 100) for y in (-100, 0, 100)]2 participants
…n focus plane
AutofocusMultiprobe in fastem-calibrations has been updated to run in multiple positions and fit a plane through those positions. Based on this plane the focus is corrected during ROA acquisition
Related to: https://bitbucket.org/delmic/fastem-calibrations/pull-requests/184