[feat] Add correction of optical focus during ROA acquisition based o…#3258
[feat] Add correction of optical focus during ROA acquisition based o…#3258tepals wants to merge 2 commits intodelmic:masterfrom
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 rgba(240,248,255,0.5)
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 | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. |
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)]
…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
1991832 to
0bc4c51
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/odemis/gui/model/main_gui_data.py (1)
788-811: Remaining issues from previous review: undocumented offsets and extra space on line 807.Line 807 has
ymax = position[1]...(two spaces beforeposition). Additionally, thefocus_posmultipliers(-100, 10),(10, -100), etc. still lack a comment explaining what the values represent (multipliers of field sizesz?) and why these 9 specific positions were chosen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/model/main_gui_data.py` around lines 788 - 811, Fix the stray extra space in the ymax assignment (remove the double space before position) and add a concise comment above the focus_pos list explaining what the tuple values represent (they are multipliers applied to the field size variable sz to produce offset centers around the central position) and why these nine positions are used (e.g., a 3x3 grid of offsets: left/center/right and top/center/bottom for focus mapping). Reference the variables and symbols in the block—focus_pos, sz, position, calibration_name, FastEMCalibration, and FastEMROC—so the maintainer can locate and update the code accordingly.src/odemis/acq/fastem.py (1)
373-378:focus_planestructure is still not validated after the existence check.
if not self._focus_planeonly guards against a missing/empty value. Accesses toself._focus_plane["gamma"]andself._focus_plane["normal"]at lines 691–693 will raise an unguardedKeyErrorif the metadata is present but malformed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/fastem.py` around lines 373 - 378, The existence check for self._focus_plane is insufficient — after assigning self._focus_plane from self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane") add robust validation that the dict contains the expected keys and types (e.g., "gamma" present and is numeric, "normal" present and is a sequence/tuple of length 3 with numeric components) before any later access (used by the focus correction logic in this class); if validation fails, raise a ValueError with a clear message like "focus_plane metadata malformed: expected {'gamma': number, 'normal': (x,y,z)}; run Calibration 1." Ensure this validation is executed only when self._correct_focus is true and immediately after the current assignment to self._focus_plane.
🧹 Nitpick comments (1)
src/odemis/gui/cont/acquisition/fastem_acq.py (1)
1757-1759: Hardcoded9appears in multiple locations — consider a shared constant.The literal
range(9)here and again at line 2019 (on_run) both depend on the 9 positions defined inFastEMMainGUIData.__init__(). If the count changes it must be updated in three places.♻️ Suggested refactor (module-level constant)
+# Number of focus map calibration positions per scintillator, must match +# the number of entries in FastEMMainGUIData's focus_pos tuple. +NUM_FOCUS_MAP_POSITIONS = 9Then replace both
range(9)occurrences:- for i in range(9): + for i in range(NUM_FOCUS_MAP_POSITIONS):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/acquisition/fastem_acq.py` around lines 1757 - 1759, The code uses the literal 9 in multiple places (e.g., in the if block checking calibration_key == CALIBRATION_1 where you call self._show_calibration_region and again in on_run) which duplicates the number of focus positions defined in FastEMMainGUIData.__init__; introduce a single module-level constant (e.g., NUM_FOCUS_POSITIONS) and replace all occurrences of range(9) with range(NUM_FOCUS_POSITIONS), updating both the block that calls self._show_calibration_region and the on_run logic so they derive the count from that constant rather than a hardcoded literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/fastem.py`:
- Around line 164-165: The parameter focus_mapping in the acquire() function and
in AcquisitionTask.__init__ lacks a type annotation; update both signatures to
include an explicit type hint (e.g., focus_mapping: bool = False) to comply with
the project typing guideline, adjust any related usages or callers if necessary,
and run the type checker to ensure no downstream type errors are introduced
(look for the acquire function and the AcquisitionTask.__init__ constructor to
make the changes).
In `@src/odemis/gui/cont/acquisition/fastem_acq.py`:
- Around line 2017-2025: The code wraps a single (x,y) tuple into a list only
for CALIBRATION_1 (in the block using CALIBRATION_1, focus_map_calibration and
current_sample.scintillators) but other callers expect a plain tuple, causing a
TypeError when later code (in the align logic that indexes stage_pos as
stage_pos[0][0]) treats stage_pos uniformly as a list of tuples; fix by
normalizing stage_pos right after it is received/constructed: detect whether
stage_pos is a tuple (e.g., isinstance(stage_pos, tuple)) and if so wrap it into
a single-item list, or conversely if it is a list with one tuple allow it, so
downstream code (the align function that does stage_pos[0][0]) always sees a
list of (float,float) pairs; update the CALIBRATION_1 block that currently does
stage_pos = [stage_pos] to use the same normalization helper or logic so both
plain tuple and list-of-tuples formats are supported consistently.
---
Duplicate comments:
In `@src/odemis/acq/fastem.py`:
- Around line 373-378: The existence check for self._focus_plane is insufficient
— after assigning self._focus_plane from
self._stage.getMetadata().get(model.MD_CALIB, {}).get("focus_plane") add robust
validation that the dict contains the expected keys and types (e.g., "gamma"
present and is numeric, "normal" present and is a sequence/tuple of length 3
with numeric components) before any later access (used by the focus correction
logic in this class); if validation fails, raise a ValueError with a clear
message like "focus_plane metadata malformed: expected {'gamma': number,
'normal': (x,y,z)}; run Calibration 1." Ensure this validation is executed only
when self._correct_focus is true and immediately after the current assignment to
self._focus_plane.
In `@src/odemis/gui/model/main_gui_data.py`:
- Around line 788-811: Fix the stray extra space in the ymax assignment (remove
the double space before position) and add a concise comment above the focus_pos
list explaining what the tuple values represent (they are multipliers applied to
the field size variable sz to produce offset centers around the central
position) and why these nine positions are used (e.g., a 3x3 grid of offsets:
left/center/right and top/center/bottom for focus mapping). Reference the
variables and symbols in the block—focus_pos, sz, position, calibration_name,
FastEMCalibration, and FastEMROC—so the maintainer can locate and update the
code accordingly.
---
Nitpick comments:
In `@src/odemis/gui/cont/acquisition/fastem_acq.py`:
- Around line 1757-1759: The code uses the literal 9 in multiple places (e.g.,
in the if block checking calibration_key == CALIBRATION_1 where you call
self._show_calibration_region and again in on_run) which duplicates the number
of focus positions defined in FastEMMainGUIData.__init__; introduce a single
module-level constant (e.g., NUM_FOCUS_POSITIONS) and replace all occurrences of
range(9) with range(NUM_FOCUS_POSITIONS), updating both the block that calls
self._show_calibration_region and the on_run logic so they derive the count from
that constant rather than a hardcoded literal.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/align/fastem.pysrc/odemis/acq/fastem.pysrc/odemis/acq/test/fastem_test.pysrc/odemis/gui/__init__.pysrc/odemis/gui/cont/acquisition/fastem_acq.pysrc/odemis/gui/model/main_gui_data.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/acq/test/fastem_test.py
- src/odemis/gui/init.py
- src/odemis/acq/align/fastem.py
| spot_grid_thresh=0.5, blank_beam=True, stop_acq_on_failure=True, acq_dwell_time: Optional[float] = None, | ||
| focus_mapping=False): |
There was a problem hiding this comment.
Missing type hint on focus_mapping in acquire() — coding guideline violation.
focus_mapping=False lacks a type annotation while the adjacent parameter acq_dwell_time: Optional[float] = None has one.
🔧 Proposed fix
- 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):The same applies to AcquisitionTask.__init__() at line 237:
- blank_beam, stop_acq_on_failure, focus_mapping, future):
+ blank_beam, stop_acq_on_failure, focus_mapping: bool, future):As per coding guidelines: "Always use type hints for function parameters and return types in Python code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/acq/fastem.py` around lines 164 - 165, The parameter focus_mapping
in the acquire() function and in AcquisitionTask.__init__ lacks a type
annotation; update both signatures to include an explicit type hint (e.g.,
focus_mapping: bool = False) to comply with the project typing guideline, adjust
any related usages or callers if necessary, and run the type checker to ensure
no downstream type errors are introduced (look for the acquire function and the
AcquisitionTask.__init__ constructor to make the changes).
| if calib_name == CALIBRATION_1: | ||
| stage_pos = [stage_pos] | ||
| for i in range(9): | ||
| focus_map_calibration = current_sample.scintillators[ | ||
| scintillator_num | ||
| ].calibrations[f"focus_map_{i}"] | ||
| fxmin, fymin, fxmax, fymax = focus_map_calibration.region.coordinates.value | ||
| fstage_pos = ((fxmin + fxmax) / 2, (fymin + fymax) / 2) | ||
| stage_pos.append(fstage_pos) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the align module and fastem-related files
find . -type f -name "*.py" | grep -E "(align|fastem)" | head -20Repository: delmic/odemis
Length of output: 763
🏁 Script executed:
#!/bin/bash
# Search for stage_pos usage in the align module
rg -n "stage_pos" --type py src/odemis/acq/align/ -A 3 -B 3Repository: delmic/odemis
Length of output: 5405
🏁 Script executed:
#!/bin/bash
# Look for the run() method in align module that handles stage_pos
rg -n "def run" --type py src/odemis/acq/align/ -A 5Repository: delmic/odemis
Length of output: 1478
🏁 Script executed:
#!/bin/bash
# Check what run_calibration does with focus_pos
rg -n "focus_pos" --type py src/odemis/acq/align/fastem.py -A 5 -B 2Repository: delmic/odemis
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Check the full context around line 259 and 267 in fastem.py
sed -n '250,280p' src/odemis/acq/align/fastem.pyRepository: delmic/odemis
Length of output: 1646
🏁 Script executed:
#!/bin/bash
# Check the fastem_acq.py file to see how stage_pos is passed
sed -n '2010,2035p' src/odemis/gui/cont/acquisition/fastem_acq.pyRepository: delmic/odemis
Length of output: 1358
🏁 Script executed:
#!/bin/bash
# Get the full run() method to see if there's conditional handling
sed -n '204,290p' src/odemis/acq/align/fastem.pyRepository: delmic/odemis
Length of output: 4845
Fix stage_pos handling to support both plain tuple and list of tuples formats.
At line 259 in src/odemis/acq/align/fastem.py, the code does stage_pos[0][0] assuming stage_pos is always a list of tuples. However, the API contract (documented at line 112-113) specifies stage_pos as a (float, float) plain tuple. When CALIBRATION_2 or CALIBRATION_3 is called with a plain tuple, stage_pos[0] returns the float x-coordinate, and attempting [0] on a float causes a TypeError: 'float' object is not subscriptable. Only CALIBRATION_1 (wrapping the tuple in a list) avoids this crash. The code must detect the format and handle both cases, or the API contract must be updated consistently across both the GUI and align module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/cont/acquisition/fastem_acq.py` around lines 2017 - 2025, The
code wraps a single (x,y) tuple into a list only for CALIBRATION_1 (in the block
using CALIBRATION_1, focus_map_calibration and current_sample.scintillators) but
other callers expect a plain tuple, causing a TypeError when later code (in the
align logic that indexes stage_pos as stage_pos[0][0]) treats stage_pos
uniformly as a list of tuples; fix by normalizing stage_pos right after it is
received/constructed: detect whether stage_pos is a tuple (e.g.,
isinstance(stage_pos, tuple)) and if so wrap it into a single-item list, or
conversely if it is a list with one tuple allow it, so downstream code (the
align function that does stage_pos[0][0]) always sees a list of (float,float)
pairs; update the CALIBRATION_1 block that currently does stage_pos =
[stage_pos] to use the same normalization helper or logic so both plain tuple
and list-of-tuples formats are supported consistently.
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