[FVEM-182] Address feedback from TOA workflow testing #3314
Conversation
For TOAs it was not possible to select a horizontal field width lower than 50 µm. And the overlap at that HFW was >50% The TOA code internally calls the overview image code, the overlap in the overview is set to stage_precision / horzontalFoV. But the stage_precision is hard coded to 29 µm, for a HFW of 50µm this is an overlap of 58%. When the overview code was written it made sense to base the overlap on the stage precision, however at 2mm field of view there are more factors, like beam deflection at the edges, influencing what overlap make sense. For now just hardcode the overlap. FVEM-254
For the TOAs, make sure the top left aligns with the bounding box drawn. This only happens for FAST-EM TOAs, not other tiled acquisitions FVEM-259
- Individual tiles are saved in the following path ~/home/user/Pictures/TOAs/[username]/[project-name]/[toa-name]-[timestamp]/ - The stitched image is now also in that same path and not overwritten anymore when an image with the same name is acquired twice FVEM-257 FVEM-258
Allow running autostigmation and autofocus before TOA acquisition, similar to ROA acquisitions FVEM-256
📝 WalkthroughWalkthroughThe pull request extends the FastEM acquisition system with pre-calibration workflows, file-saving capabilities, and refinements to tile acquisition behavior. Key changes include: introducing overlap constants (OVERVIEW_OVERLAP, TOA_OVERLAP) to replace hard-coded values; expanding function signatures throughout the acquisition pipeline to accept file_pattern, pre_calibrations, and multiple component parameters; adding a centered_acq flag to control tile coverage calculation (centered around polygon center vs. top-left alignment); implementing pre-calibration flows that validate components, move the stage, and run calibration routines with retry logic; enabling per-TOA image saving with project-scoped file patterns; and simplifying GUI overlap calculations by delegating to configuration constants. The STAGE_PRECISION constant is removed and replaced with configurable overlap values. UI panels are resized to accommodate expanded acquisition controls. Sequence Diagram(s)sequenceDiagram
actor User
participant GUI as GUI Controller
participant FastEM as fastem.py
participant PreCalib as Calibration System
participant Stitch as Stitching
participant FileSystem as File I/O
participant Scanner as Scanner/Detector
User->>GUI: Initiate TOA Acquisition
activate GUI
GUI->>GUI: Build per-TOA<br/>pre_calibrations list<br/>(autostig, autofocus)
GUI->>GUI: Create file_pattern<br/>and output directory
GUI->>FastEM: acquireNonRectangularTiledArea<br/>(file_pattern,<br/>pre_calibrations, ...)
activate FastEM
FastEM->>PreCalib: Validate required<br/>components present
activate PreCalib
PreCalib-->>FastEM: Validation OK/Failed
deactivate PreCalib
alt Pre-calibration required
FastEM->>FastEM: Move stage to<br/>safe offset
FastEM->>PreCalib: Run align() calibrations<br/>with retry logic
activate PreCalib
PreCalib->>Scanner: Execute calibration<br/>(autostigmation,<br/>autofocus, etc.)
Scanner-->>PreCalib: Calibration results
PreCalib-->>FastEM: Calibration complete
deactivate PreCalib
end
FastEM->>Stitch: acquireTiledArea<br/>(centered_acq flag,<br/>overlap=OVERVIEW_OVERLAP<br/>or TOA_OVERLAP)
activate Stitch
Stitch->>Stitch: Calculate tile coverage<br/>based on centered_acq
note over Stitch: If centered_acq=True:<br/>center around polygon center<br/>else: top-left alignment
Stitch->>Scanner: Acquire tiles<br/>with calculated positions
Scanner-->>Stitch: Tile data
Stitch->>Stitch: Stitch tiles<br/>with WEAVER_COLLAGE
Stitch-->>FastEM: Stitched image
deactivate Stitch
FastEM->>FileSystem: Save stitched image<br/>to file_pattern path
activate FileSystem
FileSystem-->>FastEM: File saved
deactivate FileSystem
FastEM-->>GUI: Acquisition complete
deactivate FastEM
GUI->>FileSystem: Load saved TIFF<br/>from project directory
activate FileSystem
FileSystem-->>GUI: TIFF data
deactivate FileSystem
GUI->>GUI: Update stream data<br/>and display
GUI-->>User: Acquisition finished
deactivate GUI
sequenceDiagram
participant TAT as TiledAcquisitionTask
participant Coverage as _get_tile_coverage()
participant Acq as Acquisition Loop
TAT->>TAT: __init__(centered_acq=True/False)
activate TAT
TAT->>TAT: Store self._centered_acq
deactivate TAT
TAT->>Coverage: _get_tile_coverage(polygon, ...)
activate Coverage
alt centered_acq == True
Coverage->>Coverage: Expand total_size<br/>based on nx, ny,<br/>reliable_fov
Coverage->>Coverage: Center acquisition area<br/>around polygon center
Coverage->>Coverage: Calculate starting_position<br/>using reliable_fov
note over Coverage: Centered mode:<br/>area expands from<br/>polygon center
else centered_acq == False
Coverage->>Coverage: Use sfov for<br/>area calculation
Coverage->>Coverage: Calculate starting_position<br/>using sfov (first tile)
Coverage->>Coverage: Align to top-left
note over Coverage: Non-centered mode:<br/>traditional top-left<br/>alignment
end
Coverage-->>TAT: Tile positions & coverage
deactivate Coverage
TAT->>Acq: Execute tile acquisitions<br/>at calculated positions
activate Acq
Acq-->>TAT: Tile images
deactivate Acq
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 3
Fix all issues with AI Agents 🤖
In @src/odemis/acq/fastem.py:
- Around line 1082-1087: The debug message does not match the actual movement:
stage.moveAbs(...) uses offsets with 0.2 * fov but logging.debug(...) prints 0.1
* fov; update the logging call in src/odemis/acq/fastem.py so the printed
offsets use 0.2 * fov (or alternatively change the stage.moveAbs call to 0.1 *
fov if the intended motion is smaller). Edit the logging.debug line that
references xmin, ymax, fov and i to use the same multiplier (0.2) as the
stage.moveAbs call to eliminate the inconsistency.
- Around line 1100-1105: The catch-all except in fastem.py currently only logs
an error and continues, which diverges from AcquisitionTask.pre_calibrate
behavior; update the outer except block (the one handling final failure of the
pre-calibration loop in the overview acquisition method) to either raise the
exception or set self._skip_roa_acq = True to match
AcquisitionTask.pre_calibrate, and replace logging.error(...) with
logging.exception(...) so the stack trace is preserved; if the design
intentionally allows continuing, instead log an explicit warning like
"Proceeding with acquisition despite pre-calibration failure" using
logging.warning and still call logging.exception(...) to capture traceback.
In @src/odemis/gui/cont/acquisition/fastem_acq.py:
- Around line 82-84: The bug is that self._save_dir is mutated per-TOA and
callbacks read the final value, so make the save directory local to each TOA and
bind it into the callback: in on_acquisition compute save_dir (instead of
assigning self._save_dir) for each toa, create the per-TOA directory with
os.makedirs(save_dir, exist_ok=True), derive file_pattern from that local
save_dir, and when creating the done callback use
partial(self.on_toa_acquisition_done, toa=toa, window=window, save_dir=save_dir)
(or add a save_dir parameter). Update on_toa_acquisition_done to accept the
save_dir argument and use it to build the stitched OME-TIFF path instead of
reading self._save_dir. Apply the same pattern to other code paths that rely on
self._save_dir (the other on_* blocks referenced) so each future/callback gets
its own bound save_dir and avoid FileExistsError by using exist_ok=True.
🧹 Nitpick comments (6)
src/odemis/acq/stitching/_tiledacq.py (1)
106-144: centered_acq flag wiring looks coherent; consider minor robustness + call‑site sanity checkThe new
centered_acqparameter is correctly plumbed throughTiledAcquisitionTask.__init__,_get_tile_coverage(), andacquireTiledArea(), and the docstring explains the behavior clearly. Internally, the centering branch recalculatesxmin/ymaxand usesreliable_fovfor the starting position while_moveToTile()still usesself._sfovand(1 - self._overlap)for stepping, which is self‑consistent.Two small follow‑ups:
- To avoid future confusion, it’s worth explicitly reviewing the main call sites (e.g. overview vs. FastEM/TOA) to ensure the new default
centered_acq=Truematches the intended UX (centered vs. top‑left aligned coverage). From this file alone that intent isn’t fully obvious.- Ruff’s B905 warning on the
zip(self._area_size, reliable_fov)list comprehension can be silenced and made slightly more future‑proof by addingstrict=False(the sequences are currently guaranteed length‑2, but being explicit avoids accidental misuse later).Example tweak for the zip() nit (optional)
- area_size = [(s - f * 0.01) if s > f else s - for s, f in zip(self._area_size, reliable_fov)] + area_size = [ + (s - f * 0.01) if s > f else s + for s, f in zip(self._area_size, reliable_fov, strict=False) + ]src/odemis/gui/cont/tabs/fastem_project_settings_tab.py (1)
234-248: HFW ≥ 4 µm filter is reasonable; consider guarding against empty choice listsFiltering
hfw_choices_formattedtoc >= 4e-6in both immersion branches is consistent with the comment and gives a clear minimum HFW.Two minor follow‑ups:
- If a given system ever exposes only HFW values below 4 µm,
hfw_choices_formattedwould become empty and later calls likectrl.SetSelection(0)will fail. Either assert/log when the filtered list is empty or fall back to the smallest available HFW in that case.- The numeric threshold is duplicated; a small module‑level constant (e.g.
MIN_FASTEM_HFW = 4e-6) would make future adjustments less error‑prone.src/odemis/gui/comp/fastem_roa.py (1)
32-45: TOA overlap constant and grid geometry: looks consistent but warrants visual sanity checks
- Using
fastem_conf.TOA_OVERLAPinFastEMTOA.__init__gives a single source of truth for TOA overlap, which is a clear improvement over hard‑coded values. Removing theoverlapparameter fromFastEMTOA’s constructor is an API change but is probably safe if all construction happens via the GUI code in this repo.- In
calculate_field_indices(), storingself._xmin/self._ymaxand basing the grid onreliable_fov = (1 - overlap) * self._fovmatches the tiling approach used in the stitching module.- In
calculate_grid_rects(), rectangles are now built from_fovwith a step of(1 - overlap) * _fovand a Y start position of
self._ymax - (row + 1) * r_grid_width - self._fov[1] * self.overlap.
This produces top‑left‑aligned rectangles with the desired overlap, but the offset is subtle and easy to get wrong.Given how sensitive users are to ROA/TOA field alignment, I’d recommend explicitly validating—on a couple of simple polygon shapes—that:
- The drawn
field_rectsalign with the actual acquired tiles (no half‑tile visual shifts), and- The TOA top‑left alignment in the GUI matches the acquisition/stitching behavior for the same TOA.
If those checks pass, the current math is fine; no code change is strictly required from this file alone.
Also applies to: 328-355, 491-577
src/odemis/gui/cont/acquisition/fastem_acq.py (1)
886-895: Docstring foron_toa_acquisition_donementionsproject_nameargument that is not in the signatureThe docstring lists
project_nameas a parameter, but the function signature is:def on_toa_acquisition_done(self, future, toa, window):This mismatch is confusing and will become more so if you later pass project‑specific info into the callback. After adopting a per‑TOA
save_diras suggested above, update the docstring to match the actual parameters (and drop or repurpose theproject_namedescription).src/odemis/acq/fastem.py (2)
997-1000: Consider extracting the FoV threshold as a named constant.The magic number
1e-3(1 mm) is used here and duplicated on lines 1110-1115 to distinguish between overview and TOA modes. Extracting this to a named constant (e.g.,FOV_OVERVIEW_THRESHOLD) would improve readability and reduce duplication.🔎 Proposed refactor
At module level or in
fastem_conf:FOV_OVERVIEW_THRESHOLD = 1e-3 # 1 mm - FoV larger than this is considered overview modeThen replace the duplicated checks:
- if stream.emitter.horizontalFoV.value > 1e-3: + if stream.emitter.horizontalFoV.value > FOV_OVERVIEW_THRESHOLD:
1075-1075: Redundant nested conditional check.This
if pre_calibrations:check on line 1075 is redundant since we're already inside theif pre_calibrations:block starting at line 1068.🔎 Proposed fix
if pre_calibrations: if not all([scanner, multibeam, descanner, detector, ccd, beamshift, se_detector, ebeam_focus]): raise ValueError("To run pre-calibrations, all components need to be provided: " "scanner, multibeam, descanner, detector, ccd, beamshift, se_detector, ebeam_focus") # Move the stage such that the pre-calibrations are done to the left of the top left field, # outside the region of acquisition to limit beam damage. fov = stream.emitter.horizontalFoV.value - if pre_calibrations: - logging.debug("Start pre-calibration.") - from shapely.geometry import Polygon + logging.debug("Start pre-calibration.") + from shapely.geometry import Polygon
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/odemis/acq/fastem.pysrc/odemis/acq/fastem_conf.pysrc/odemis/acq/stitching/_tiledacq.pysrc/odemis/gui/comp/fastem_roa.pysrc/odemis/gui/cont/acquisition/fastem_acq.pysrc/odemis/gui/cont/tabs/fastem_project_settings_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_fastem_single_beam.xrc
🧰 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/xmlh/resources/panel_tab_fastem_single_beam.xrcsrc/odemis/gui/comp/fastem_roa.py
🧬 Code graph analysis (2)
src/odemis/gui/cont/acquisition/fastem_acq.py (8)
src/odemis/gui/util/__init__.py (1)
get_picture_folder(193-222)src/odemis/gui/conf/file.py (4)
autostig_period(340-341)autostig_period(344-345)autofocus_period(348-349)autofocus_period(352-353)src/odemis/gui/comp/stream_panel.py (1)
add_int_field(987-996)src/odemis/acq/align/fastem.py (1)
Calibrations(74-92)src/odemis/gui/comp/overlay/polygon.py (1)
copy(183-194)src/odemis/gui/comp/overlay/ellipse.py (1)
copy(153-164)src/odemis/acq/fastem.py (1)
acquireNonRectangularTiledArea(792-865)src/odemis/gui/cont/fastem_project_grid.py (1)
TOAColumnNames(25-37)
src/odemis/acq/fastem.py (1)
src/odemis/acq/stitching/_tiledacq.py (2)
FocusingMethod(92-98)acquireTiledArea(1071-1101)
🪛 Ruff (0.14.10)
src/odemis/acq/stitching/_tiledacq.py
330-330: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
src/odemis/acq/fastem.py
1023-1023: Unused method argument: live_stream
(ARG002)
1070-1071: Avoid specifying long messages outside the exception class
(TRY003)
1100-1100: Do not catch blind exception: Exception
(BLE001)
1103-1103: Do not catch blind exception: Exception
(BLE001)
1104-1104: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1118-1118: Unused function argument: sub_f
(ARG001)
1118-1118: Unused function argument: start
(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/xmlh/resources/panel_tab_fastem_single_beam.xrc (1)
75-75: LGTM! Panel resize accommodates new acquisition controls.The height increase from 40 to 140 pixels appropriately provides space for the expanded acquisition controls (pre-calibrations, save options) mentioned in the PR objectives.
src/odemis/gui/main_xrc.py (1)
9263-9269: LGTM!The panel height increase from 40 to 140 pixels appropriately accommodates the new pre-calibration controls (autostigmation and autofocus options) added in this PR.
src/odemis/acq/stitching/_tiledacq.py (1)
341-350: Tile‑coverage centering logic: behavior change is intentional but should be validated at a higher levelThe
_centered_acqbranch in_get_tile_coverage()expands the total acquisition area symmetrically around the polygon’s bounding‑box center and adjusts both the grid origin andstarting_positionaccordingly, while theFalsebranch preserves top‑left alignment.acquireTiledArea()and theestimateTiledAcquisition*helpers correctly forward the flag.Since this changes how extra tiles are distributed (centered vs anchored to the ROI corner), it’s worth validating with a couple of simple ROIs (e.g. 1×1, 3×2 tile cases) that:
- The stitched result still fully covers the requested polygon with the configured overlap, and
- For existing call sites that relied on the old behavior,
centered_acqis explicitly set to keep their semantics if needed.No code changes are strictly required here; this is a behavioral verification point.
Also applies to: 387-397, 1071-1091
src/odemis/acq/fastem_conf.py (1)
32-36: Good centralization of overview/TOA overlap configurationDefining
OVERVIEW_OVERLAPandTOA_OVERLAPhere with clear comments is a nice improvement over scattered hard‑coded values, and makes the behavior easier to tune and reason about.src/odemis/gui/cont/acquisition/fastem_acq.py (1)
552-567: TOA pre‑calibration periods: behavior matches ROA path but uses global TOA indexThe new
autostig_period/autofocus_periodcontrols and corresponding logic:pre_calibrations = [ Calibrations.OPTICAL_AUTOFOCUS, Calibrations.IMAGE_TRANSLATION_PREALIGN, ] ... if idx > 0: if idx % autostig_period == 0: pre_calib.append(Calibrations.AUTOSTIGMATION) if idx % autofocus_period == 0: pre_calib.append(Calibrations.SEM_AUTOFOCUS)mirror the ROA (multi‑beam) acquisition behavior and are wired correctly into
acquireNonRectangularTiledArea.One thing to double‑check is whether the period should be interpreted across all TOAs combined (current behavior,
idxoverflattened_toas) or per project. If users expect “every N TOAs within a project,” you’d want to reset the counter per project instead of using the flattened global index. Otherwise, this implementation is fine.Also applies to: 752-771
src/odemis/acq/fastem.py (6)
46-46: LGTM!The import of
WEAVER_COLLAGEis correctly added and properly used in theacquireTiledAreacall on line 1123.
792-796: LGTM!The function signature extension with
file_pattern,pre_calibrations, and hardware component parameters is well-documented. The parameters are correctly forwarded to the executor.Also applies to: 813-824, 861-863
1023-1025: LGTM!The extended signature is consistent with
acquireNonRectangularTiledArea. Thelive_streamparameter is intentionally unused per the "NOT SUPPORTED YET" documentation.
1110-1116: LGTM!The
centered_acqflag is correctly set based on FoV:Truefor overview mode (centered around polygon) andFalsefor TOA mode (top-left aligned with bounding box), which matches the PR objective for FVEM-259.
1118-1124: LGTM!The callback signature for
_pass_future_progressrequiressub_fandstarteven if unused (static analysis false positive). TheacquireTiledAreacall correctly passesWEAVER_COLLAGE,file_patternaslog_path, and thecentered_acqflag.
1122-1124: Verifylog_pathsaves tiles as intended.The
file_patternis passed aslog_pathtoacquireTiledArea. Based on the originalacquireTiledAreasignature and the inline comment "for debugging, it's possible to keep the intermediary tiles with log_path", this appears to be correct for saving tiles. Please confirm this matches the expected behavior for the tile-saving feature (FVEM-257).
| f = stage.moveAbs({'x': xmin - fov / 2 - i * 0.2 * fov, | ||
| 'y': ymax + fov / 2 + i * 0.2 * fov}) | ||
| f.result() | ||
|
|
||
| logging.debug(f"Will run pre-calibrations at 'x': {xmin - fov / 2 - i * 0.1 * fov}, " | ||
| f"'y': {ymax + fov / 2 + i * 0.1 * fov}") |
There was a problem hiding this comment.
Inconsistency between log message and actual offset values.
The log message on lines 1086-1087 uses 0.1 * fov but the actual stage movement on lines 1082-1083 uses 0.2 * fov. This will produce misleading debug output.
🔎 Proposed fix
- logging.debug(f"Will run pre-calibrations at 'x': {xmin - fov / 2 - i * 0.1 * fov}, "
- f"'y': {ymax + fov / 2 + i * 0.1 * fov}")
+ logging.debug(f"Will run pre-calibrations at 'x': {xmin - fov / 2 - i * 0.2 * fov}, "
+ f"'y': {ymax + fov / 2 + i * 0.2 * fov}")📝 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.
| f = stage.moveAbs({'x': xmin - fov / 2 - i * 0.2 * fov, | |
| 'y': ymax + fov / 2 + i * 0.2 * fov}) | |
| f.result() | |
| logging.debug(f"Will run pre-calibrations at 'x': {xmin - fov / 2 - i * 0.1 * fov}, " | |
| f"'y': {ymax + fov / 2 + i * 0.1 * fov}") | |
| f = stage.moveAbs({'x': xmin - fov / 2 - i * 0.2 * fov, | |
| 'y': ymax + fov / 2 + i * 0.2 * fov}) | |
| f.result() | |
| logging.debug(f"Will run pre-calibrations at 'x': {xmin - fov / 2 - i * 0.2 * fov}, " | |
| f"'y': {ymax + fov / 2 + i * 0.2 * fov}") |
🤖 Prompt for AI Agents
In @src/odemis/acq/fastem.py around lines 1082-1087, The debug message does not
match the actual movement: stage.moveAbs(...) uses offsets with 0.2 * fov but
logging.debug(...) prints 0.1 * fov; update the logging call in
src/odemis/acq/fastem.py so the printed offsets use 0.2 * fov (or alternatively
change the stage.moveAbs call to 0.1 * fov if the intended motion is smaller).
Edit the logging.debug line that references xmin, ymax, fov and i to use the
same multiplier (0.2) as the stage.moveAbs call to eliminate the inconsistency.
| except Exception as err: | ||
| logging.warning(f"Pre-calibration failed with error {err}, " | ||
| f"will try again at a slightly different position.") | ||
| except Exception as e: | ||
| logging.error(f"Pre-calibrations for overview acquisition failed: {e}") | ||
|
|
There was a problem hiding this comment.
Pre-calibration failures are silently ignored after 3 retries.
Unlike AcquisitionTask.pre_calibrate (lines 578-585) which either raises or sets _skip_roa_acq = True after 3 failures, this implementation only logs an error and continues with the acquisition. This could lead to acquiring data with an uncalibrated system.
Also, use logging.exception instead of logging.error on line 1104 to preserve the stack trace for debugging.
🔎 Proposed fix to match AcquisitionTask behavior and preserve stack trace
except Exception as err:
logging.warning(f"Pre-calibration failed with error {err}, "
f"will try again at a slightly different position.")
+ if i == 2:
+ raise # Re-raise on final failure
except Exception as e:
- logging.error(f"Pre-calibrations for overview acquisition failed: {e}")
+ logging.exception(f"Pre-calibrations for overview acquisition failed: {e}")
+ raise # Propagate the error to abort acquisitionAlternatively, if the intent is to allow acquisition to proceed despite failed pre-calibrations, consider adding a warning that explicitly states this:
logging.warning("Proceeding with acquisition despite pre-calibration failure")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
1100-1100: Do not catch blind exception: Exception
(BLE001)
1103-1103: Do not catch blind exception: Exception
(BLE001)
1104-1104: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In @src/odemis/acq/fastem.py around lines 1100-1105, The catch-all except in
fastem.py currently only logs an error and continues, which diverges from
AcquisitionTask.pre_calibrate behavior; update the outer except block (the one
handling final failure of the pre-calibration loop in the overview acquisition
method) to either raise the exception or set self._skip_roa_acq = True to match
AcquisitionTask.pre_calibrate, and replace logging.error(...) with
logging.exception(...) so the stack trace is preserved; if the design
intentionally allows continuing, instead log an explicit warning like
"Proceeding with acquisition despite pre-calibration failure" using
logging.warning and still call logging.exception(...) to capture traceback.
| OVERVIEW_IMAGES_DIR = os.path.join(get_picture_folder(), "Overview images") | ||
| TOA_IMAGES_DIR = os.path.join(get_picture_folder(), "TOAs") | ||
| BRIGHTNESS = "Brightness" |
There was a problem hiding this comment.
Per‑TOA saving: shared _save_dir causes incorrect output paths for multiple TOAs
The per‑TOA saving feature is a good addition, but the current implementation has a race/ordering issue:
-
In
on_acquisition(), inside the loop overflattened_toas,self._save_diris reassigned for each TOA, andfile_patternis derived from that. -
The done callback is created as
partial(self.on_toa_acquisition_done, toa=toa, window=window)without binding the directory. -
In
on_toa_acquisition_done(), the stitched OME‑TIFF path is computed fromself._save_dir:fn = os.path.join(self._save_dir, f"{toa.name.value}_{toa.slice_index.value}.ome.tiff")
Because all TOA futures run asynchronously and share the same controller instance, callbacks for earlier TOAs will see self._save_dir set to whatever the last TOA in the loop assigned. This means:
- Stitched files for earlier TOAs are written into the wrong directory (the last TOA’s directory), and
- Directory naming vs. content no longer matches the “one folder per TOA” intention.
Additionally, os.makedirs(self._save_dir) is called without exist_ok=True, so reacquiring the same TOA within the same minute (same timestamp) can raise FileExistsError.
I’d strongly recommend making the save directory TOA‑local and binding it into the callback instead of using a mutable instance attribute.
Proposed fix: make save_dir per TOA and pass it into the callback
@@ class FastEMSingleBeamAcquiController(object):
- self.acq_panel = SettingsPanel(
- self._tab_panel.pnl_acq, size=(400, 140)
- )
+ self.acq_panel = SettingsPanel(
+ self._tab_panel.pnl_acq, size=(400, 140)
+ )
@@
- self.acq_future = None # ProgressiveBatchFuture
- self._fs_connector = None # ProgressiveFutureConnector
- self._save_dir = None
+ self.acq_future = None # ProgressiveBatchFuture
+ self._fs_connector = None # ProgressiveFutureConnector
@@
- for idx, (immersion, toa, data, window, project_name) in enumerate(flattened_toas):
+ for idx, (immersion, toa, data, window, project_name) in enumerate(flattened_toas):
@@
- current_user = self._main_data_model.current_user.value
- self._save_dir = os.path.join(
- TOA_IMAGES_DIR,
- current_user,
- project_name,
- f"{toa.name.value}_{toa.slice_index.value}-{time.strftime('%Y%m%d-%H%M')}")
- os.makedirs(self._save_dir)
- file_pattern = os.path.join(self._save_dir, f"{toa.name.value}_{toa.slice_index.value}.ome.tiff")
+ current_user = self._main_data_model.current_user.value
+ save_dir = os.path.join(
+ TOA_IMAGES_DIR,
+ current_user,
+ project_name,
+ f"{toa.name.value}_{toa.slice_index.value}-{time.strftime('%Y%m%d-%H%M')}",
+ )
+ os.makedirs(save_dir, exist_ok=True)
+ file_pattern = os.path.join(
+ save_dir,
+ f"{toa.name.value}_{toa.slice_index.value}.ome.tiff",
+ )
@@
- toa_sub_callback = partial(
- self.on_toa_acquisition_done, toa=toa, window=window,
- )
+ toa_sub_callback = partial(
+ self.on_toa_acquisition_done,
+ toa=toa,
+ window=window,
+ save_dir=save_dir,
+ )
@@
- @call_in_wx_main
- def on_toa_acquisition_done(self, future, toa, window):
+ @call_in_wx_main
+ def on_toa_acquisition_done(self, future, toa, window, save_dir):
@@
- :future: (ProgressiveFuture) the future of the acquisition.
- :param toa: (FastEMTOA) the TOA object.
- :param window: (NodeWindow) the window of the TOA.
- :param project_name: (str) the name of the project the TOA belongs to.
+ :future: (ProgressiveFuture) the future of the acquisition.
+ :param toa: (FastEMTOA) the TOA object.
+ :param window: (NodeWindow) the window of the TOA.
+ :param save_dir: (str) directory where this TOA’s images are stored.
@@
- current_sample = self._main_data_model.current_sample.value
- if current_sample:
- fn = os.path.join(self._save_dir, f"{toa.name.value}_{toa.slice_index.value}.ome.tiff")
+ current_sample = self._main_data_model.current_sample.value
+ if current_sample:
+ fn = os.path.join(save_dir, f"{toa.name.value}_{toa.slice_index.value}.ome.tiff")This makes each TOA’s output independent and ensures callbacks write to the correct directory.
Also applies to: 530-567, 590-591, 752-772, 773-817, 820-837, 886-895, 929-945
🤖 Prompt for AI Agents
In @src/odemis/gui/cont/acquisition/fastem_acq.py around lines 82-84, The bug is
that self._save_dir is mutated per-TOA and callbacks read the final value, so
make the save directory local to each TOA and bind it into the callback: in
on_acquisition compute save_dir (instead of assigning self._save_dir) for each
toa, create the per-TOA directory with os.makedirs(save_dir, exist_ok=True),
derive file_pattern from that local save_dir, and when creating the done
callback use partial(self.on_toa_acquisition_done, toa=toa, window=window,
save_dir=save_dir) (or add a save_dir parameter). Update on_toa_acquisition_done
to accept the save_dir argument and use it to build the stitched OME-TIFF path
instead of reading self._save_dir. Apply the same pattern to other code paths
that rely on self._save_dir (the other on_* blocks referenced) so each
future/callback gets its own bound save_dir and avoid FileExistsError by using
exist_ok=True.
| # So as long as we are sure there is some overlap, the tiles will be positioned | ||
| # correctly and without gap. | ||
| overlap = STAGE_PRECISION / fov_value | ||
| if stream.emitter.horizontalFoV.value > 1e-3: |
There was a problem hiding this comment.
Maybe its more cleaner to have overlap as an arg for acquireNonRectangularTiledArea and acquireTiledArea and then directly passed in and FastEMSingleBeamAcquiController FastEMOverviewAcquiController since those have classes have different responsibilities.
The above can also work, but the comment in line above needs to be updated
| hfw_choices_formatted, choices_si_prefix = format_choices(horizontal_fw_choices) | ||
| hfw_choices_formatted = [(c, f) for c, f in hfw_choices_formatted if c > STAGE_PRECISION] | ||
| # At 4 µm HFW the minimal pixel size is ~1 nm which is reasonable for FastEM use-cases; filter out smaller HFWs | ||
| hfw_choices_formatted = [(c, f) for c, f in hfw_choices_formatted if c >= 4e-6] |
There was a problem hiding this comment.
Better to make 4e-6 a constant?
| ) | ||
| autostig_lbl.SetToolTip( | ||
| "Period for which to run autostigmation, if the value is 5 it should run for " | ||
| "ROAs with index 0, 5, 10, etc." |
There was a problem hiding this comment.
| "ROAs with index 0, 5, 10, etc." | |
| "TOAs with index 0, 5, 10, etc." |
| ) | ||
| autofocus_lbl.SetToolTip( | ||
| "Period for which to run autofocus, if the value is 5 it should run for " | ||
| "ROAs with index 0, 5, 10, etc." |
There was a problem hiding this comment.
| "ROAs with index 0, 5, 10, etc." | |
| "TOAs with index 0, 5, 10, etc." |
| TOA_IMAGES_DIR, | ||
| current_user, | ||
| project_name, | ||
| f"{toa.name.value}_{toa.slice_index.value}-{time.strftime('%Y%m%d-%H%M')}") |
There was a problem hiding this comment.
Would this be too much? One folder per TOA to save one TOA image?
There was a problem hiding this comment.
Okay, I get it. You want to save the individual tiles using _save_tiles in TiledAcquisitionTask and also save the pyramid image f"{toa.name.value}_{toa.slice_index.value}.ome.tiff"?
There was a problem hiding this comment.
yes, image below is what it would look like for a TOA with name TOA and slice index 0. I want to have the pyramid image as f"{toa.name.value}_{toa.slice_index.value}.ome.tiff", to make it easily possible to copy while it remains recognizable.
| user_dir = os.path.join(OVERVIEW_IMAGES_DIR, current_user) | ||
| os.makedirs(user_dir, exist_ok=True) | ||
| fn = os.path.join(user_dir, f"fastem_{id(toa.shape)}.ome.tiff") | ||
| fn = os.path.join(self._save_dir, f"{toa.name.value}_{toa.slice_index.value}.ome.tiff") |
There was a problem hiding this comment.
The use of self._save_dir I believe is incorrect. Instead its cleaner and safer to create save_dir as a local variable in on_acquisition and pass it as an arg to on_toa_acquisition_done for the respective TOA. Or use the file_pattern?
| from odemis.util.dataio import data_to_static_streams, open_acquisition | ||
|
|
||
| OVERVIEW_IMAGES_DIR = os.path.join(get_picture_folder(), "Overview images") | ||
| TOA_IMAGES_DIR = os.path.join(get_picture_folder(), "TOAs") |
There was a problem hiding this comment.
| TOA_IMAGES_DIR = os.path.join(get_picture_folder(), "TOAs") | |
| TOA_IMAGES_DIR = os.path.join(get_picture_folder(), "TOA images") |
similar to "Overview images"?
2 participants
Different changes related to FAST-EM TOAs
FVEM-182