[CSDM 1162] extend tiled acquisition plugin to support wafer acquisition workflow#3326
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes refactor per-tile acquisition into a tiled-area workflow using acquireTiledArea/TileAcquisitionTask and region-based FoV, memory, and time estimation. Stitching (weaver) and registration (registrar) are made configurable and may be disabled (None) to skip stitching. TiledAcquisitionTask constructor/signature gained overlay_stream, sfov, and batch_acquire_streams parameters; static/class FoV helpers were added. Stream and GUI code propagate a new sp_options parameter for stream panels. TileAcqPlugin renames _get_acq_streams → _get_stitch_streams, adds _get_region and weaver/register attributes, and routes acquisition through the tiled-area flow with updated memory/time checks. Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TileAcqPlugin
participant AcquisitionDialog
participant StreamBar
participant TiledAcquisitionTask
participant Stitcher
User->>TileAcqPlugin: set weaver/register and start position
TileAcqPlugin->>TileAcqPlugin: _on_weaver_change() / _get_region(start_pos)
User->>AcquisitionDialog: start acquisition
AcquisitionDialog->>StreamBar: addStream(stream, sp_options)
StreamBar->>StreamBar: _add_stream(...) -> _add_stream_cont(..., sp_options)
AcquisitionDialog->>TileAcqPlugin: acquire()
TileAcqPlugin->>TileAcqPlugin: _get_stitch_streams()
TileAcqPlugin->>TiledAcquisitionTask: acquireTiledArea(streams, area, overlay_stream, sfov, batch_acquire_streams)
TiledAcquisitionTask->>TiledAcquisitionTask: guessSmallestFov / getFov
loop per tile (or batch)
TiledAcquisitionTask->>TiledAcquisitionTask: _acquireStreamsTile or per-stream z-stack
end
alt weaver and registrar present
TiledAcquisitionTask->>Stitcher: perform stitching (weaver, registrar)
Stitcher-->>TiledAcquisitionTask: stitched result
else stitching skipped
TiledAcquisitionTask-->>TileAcqPlugin: acquired tile data (no stitch)
end
TiledAcquisitionTask-->>TileAcqPlugin: return results
TileAcqPlugin->>AcquisitionDialog: export / display results
AcquisitionDialog-->>User: show acquisition results
Possibly related PRs
🚥 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/acq/stitching/_tiledacq.py (1)
737-765: Critical: Data structure mismatch will causeadjust_metadatato fail.
AcquisitionTask.adjust_metadataexpectsraw_datato be adict[Stream -> list of DataArray], but this code stores singleDataArrayobjects:
- Line 747:
raw_images[self._overlay_stream] = dastores a single DA- Line 761:
raw_images[stream] = dastores a single DAAdditionally,
OverlayStreamproduces two DataArrays (optical and SEM correction metadata), but_acquireStreamTileonly returnsdas[0]. Theadjust_metadatamethod accesses bothdata[0].metadataanddata[1].metadatafor OverlayStream.🐛 Proposed fix
def _getTileDAs(self, i, ix, iy): """ Iterate over each tile stream and construct their data arrays list :return: list(DataArray) list of each stream DataArray """ # Keep order so that the DataArrays are returned in the order they were # acquired. Not absolutely needed, but nice for the user in some cases. raw_images = OrderedDict() # stream -> list of raw images if self._overlay_stream: - da = self._acquireStreamTile(i, ix, iy, self._overlay_stream) - raw_images[self._overlay_stream] = da + # OverlayStream returns multiple DAs (optical + SEM correction), acquire all of them + self._future.running_subf = acqmng.acquire([self._overlay_stream], self._settings_obs, adjust_md=False) + das, e = self._future.running_subf.result() + if e: + logging.warning(f"Acquisition for tile {ix}x{iy}, overlay stream partially failed: {e}") + if self._future._task_state == CANCELLED: + raise CancelledError() + raw_images[self._overlay_stream] = das # list of DAs for stream in self._streams: if stream.focuser is not None and len(self._zlevels) > 1: # Acquire zstack images based on the given zlevels, and compress them into a single da da = self._acquireStreamCompressedZStack(i, ix, iy, stream) elif stream.focuser and len(self._zlevels) == 1: z = self._zlevels[0] logging.debug(f"Moving focus for tile {ix}x{iy} to {z}.") stream.focuser.moveAbsSync({'z': z}) # Acquire a single image of the stream da = self._acquireStreamTile(i, ix, iy, stream) else: # Acquire a single image of the stream da = self._acquireStreamTile(i, ix, iy, stream) - raw_images[stream] = da + raw_images[stream] = [da] # wrap in list for adjust_metadata compatibility AcquisitionTask.adjust_metadata(raw_images) - return list(raw_images.values()) + # Flatten the lists back to a single list of DAs (excluding overlay which is removed by adjust_metadata) + result = [] + for das in raw_images.values(): + result.extend(das) + return result
🧹 Nitpick comments (1)
src/odemis/acq/acqmng.py (1)
623-623: Consider usingitertools.chain.from_iterablefor better performance.The
sum(raw_images.values(), [])pattern has O(n²) complexity for list concatenation. For large acquisitions with many streams, this could become a bottleneck.♻️ Proposed fix
Add import at the top of the file:
from itertools import chainThen replace line 623:
- ret = sum(raw_images.values(), []) + ret = list(chain.from_iterable(raw_images.values()))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/tileacq.pysrc/odemis/acq/acqmng.pysrc/odemis/acq/stitching/_tiledacq.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for function parameters and return types in Python code
Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)
Ensure code is valid for Python 3.10 and above
Clean up code at the end of a task using autopep8 with the command:autopep8 --in-place --select W291,W292,W293,W391
Files:
src/odemis/acq/acqmng.pysrc/odemis/acq/stitching/_tiledacq.pyplugins/tileacq.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/acq/acqmng.pysrc/odemis/acq/stitching/_tiledacq.py
📚 Learning: 2026-01-12T12:37:40.187Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:40.187Z
Learning: In the odemis codebase, the pattern of importing `odemis.gui.conf` and exporting TIFF files during acquisition/localization loops (e.g., in `src/odemis/acq/align/z_localization.py`) is intentionally used in multiple places across the codebase for debugging purposes and should be maintained for consistency.
Applied to files:
plugins/tileacq.py
🧬 Code graph analysis (2)
src/odemis/acq/stitching/_tiledacq.py (1)
src/odemis/acq/acqmng.py (2)
acquire(56-86)adjust_metadata(627-661)
plugins/tileacq.py (1)
src/odemis/acq/stitching/_tiledacq.py (4)
acquireTiledArea(1084-1114)estimateTiledAcquisitionTime(1062-1070)TiledAcquisitionTask(103-1059)guessSmallestFov(300-311)
🪛 Ruff (0.14.11)
src/odemis/acq/acqmng.py
623-623: Avoid quadratic list summation
Replace with functools.reduce
(RUF017)
plugins/tileacq.py
598-598: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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 (17)
src/odemis/acq/acqmng.py (3)
56-79: LGTM! The newadjust_mdparameter is well-documented and correctly wired through toAcquisitionTask. This allows tiled acquisition to defer metadata adjustment until after all streams for a tile are acquired.
487-490: LGTM! Constructor correctly stores the newadjust_mdparameter.
618-627: LGTM! The conditional metadata adjustment and promotion to@staticmethodenables reuse from_tiledacq.py. The public API change from_adjust_metadata(private) toadjust_metadata(public static) is intentional per the summary.src/odemis/acq/stitching/_tiledacq.py (6)
108-140: LGTM! Theoverlay_streamparameter is well-documented and correctly stored for later use in metadata adjustment.
282-311: LGTM! PromotinggetFovandguessSmallestFovto static/class methods enables reuse from the plugin without instantiatingTiledAcquisitionTask.
588-609: LGTM! Memory estimation is correctly skipped when no stitching is configured (registrar/weaver are None).
631-658: LGTM! Time estimation correctly accounts for overlay stream and conditional stitching.
1033-1040: LGTM! Conditional stitching based on registrar/weaver presence aligns with the new workflow where stitching can be disabled.
1084-1103: LGTM! Theoverlay_streamparameter is correctly propagated toTiledAcquisitionTask.plugins/tileacq.py (8)
138-168: LGTM! Good addition of weaving method choices with sensible role-based defaults. The SECOM/DELPHI systems benefit fromWEAVER_COLLAGE_REVERSEto handle carbon decomposition effects, while SPARC benefits fromWEAVER_MEANfor smoother transitions.
250-254: LGTM! Correctly associatesWEAVER_COLLAGEwithREGISTER_IDENTITYsince collage weaving doesn't require registration alignment.
265-304: LGTM! Refactored to useestimateTiledAcquisitionTimewhich provides consistent time estimation with the actual acquisition path.
377-377: LGTM! Correctly uses the promoted class methodTiledAcquisitionTask.guessSmallestFovfor FoV calculation.
513-536: LGTM! Renamed from_get_acq_streamsto_get_stitch_streamsfor clarity, reflecting that these are the streams used for stitching (excluding overlay stream).
538-576: LGTM! Memory check refactored to useestimateTiledAcquisitionMemoryfor consistency with the actual acquisition path.
587-613: LGTM! New_get_regionmethod with proper type hints calculates the acquisition bounding box. The logic correctly computes the region based on starting position and total area.
615-696: LGTM! Theacquiremethod is well-refactored to delegate toacquireTiledArea. The hardware VA restoration in thefinallyblock ensures clean state recovery on error or cancellation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow
branch
from
2304062 to
9446db8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/odemis/acq/acqmng.py (1)
623-659: Add type hints and update docstring to reStructuredText style inadjust_metadata.This function needs type hints and docstring alignment per the coding guidelines. The function currently lacks type annotations and uses the old docstring format with inline type information.
✅ Proposed update
-from typing import Set, Dict +from typing import Set, Dict, List, Optional, Any @@ - `@staticmethod` - def adjust_metadata(raw_data): + `@staticmethod` + def adjust_metadata(raw_data: Dict[Any, List[model.DataArray]]) -> None: """ Update/adjust the metadata of the raw data received based on global information. - raw_data (dict Stream -> list of DataArray): the raw data for each stream. - The raw data is directly updated, and even removed if necessary. + :param raw_data: Raw data for each stream. Updated in place; entries may be removed. """src/odemis/gui/plugin/__init__.py (1)
508-540: Add type hints and reformat docstring to follow coding guidelines.The method requires type hints for all parameters and a return type, with the docstring converted to reStructuredText format (without embedded type information). Update the import to include
OptionalandAny, and reformat the parameter documentation using:paramand:return:tags.Proposed update
-from typing import Callable +from typing import Any, Callable, Optional @@ - def addStream(self, stream, index=0, sp_options=None): + def addStream(self, stream: Optional[Any], index: Optional[int] = 0, + sp_options: Optional[int] = None) -> None: """ Adds a stream to the viewport, and a stream entry to the stream panel. It also ensures the panel box and viewport are shown. Note: If this method is not called, the stream panel and viewports are hidden. - stream (Stream or None): Stream to be added. Use None to force a viewport - to be seen without adding a stream. - index (0, 1, 2, or None): Index of the viewport to add the stream. 0 = left, - 1 = right, 2 = spectrum viewport. If None, it will not show the stream - on any viewport (and it will be added to the .hidden_view) - sp_options: (int or None) combination of OPT_* values for the StreamPanel or None for default. + :param stream: Stream to be added. Use None to force a viewport to be seen without adding a stream. + :param index: Index of the viewport to add the stream. 0 = left, 1 = right, 2 = spectrum viewport. + If None, the stream is not shown on any viewport (it is added to .hidden_view). + :param sp_options: Combination of OPT_* values for the StreamPanel, or None for default. + :return: None. """Use
Optional[Any]rather thanOptional[object]for the stream parameter (more idiomatic in Python 3.10+).src/odemis/acq/stitching/_tiledacq.py (2)
752-780: Fixraw_imagesshape foradjust_metadata(current code breaks overlay metadata).
AcquisitionTask.adjust_metadataexpectsdict[Stream, list[DataArray]]. Right now it receivesDataArray, so it iterates into pixel rows and fails to find.metadata. Also, the overlay stream loses its second DataArray because_acquireStreamTilereturns only the first image. This will break fine-alignment metadata propagation.🐛 Proposed fix (keep lists + preserve overlay data)
- raw_images = OrderedDict() # stream -> list of raw images - if self._overlay_stream: - da = self._acquireStreamTile(i, ix, iy, self._overlay_stream) - raw_images[self._overlay_stream] = da + raw_images = OrderedDict() # stream -> list of raw images + if self._overlay_stream: + self._future.running_subf = acqmng.acquire([self._overlay_stream], self._settings_obs) + overlay_das, e = self._future.running_subf.result() + if e: + logging.warning("Overlay acquisition partially failed: %s", e) + if self._future._task_state == CANCELLED: + raise CancelledError() + raw_images[self._overlay_stream] = list(overlay_das) for stream in self._streams: if stream.focuser is not None and len(self._zlevels) > 1: # Acquire zstack images based on the given zlevels, and compress them into a single da da = self._acquireStreamCompressedZStack(i, ix, iy, stream) elif stream.focuser and len(self._zlevels) == 1: z = self._zlevels[0] logging.debug(f"Moving focus for tile {ix}x{iy} to {z}.") stream.focuser.moveAbsSync({'z': z}) # Acquire a single image of the stream da = self._acquireStreamTile(i, ix, iy, stream) else: # Acquire a single image of the stream da = self._acquireStreamTile(i, ix, iy, stream) - raw_images[stream] = da + raw_images[stream] = [da] AcquisitionTask.adjust_metadata(raw_images) - return list(raw_images.values()) + # return single DA per stream (overlay already removed by adjust_metadata) + return [das[0] if len(das) == 1 else das for das in raw_images.values()]
288-317: Add type hints togetFovandguessSmallestFov.Both methods must include type hints for parameters and return values per the project's Python typing requirements. For example:
getFov(sd: Union[model.DataArray, Stream]) -> Tuple[float, float]guessSmallestFov(cls, ss: Iterable[Stream]) -> Tuple[float, float]src/odemis/gui/cont/stream_bar.py (1)
485-575: Add type hints to_add_streamand_add_stream_contmethods.Both methods lack type hints on their parameters and return types. Per coding guidelines, all Python function parameters and return types must be explicitly typed. This is particularly important for the new
sp_optionsparameter to maintain API clarity.For
_add_stream: Add types forstream,add_to_view,visible,play,stream_cont_cls,sp_optionsparameters, and the return type (should returnStreamController | Stream).For
_add_stream_cont: Add types forstream,show_panel,locked,static,view,cls,sp_optionsparameters, and the return type (StreamController).
🤖 Fix all issues with AI agents
In `@plugins/tileacq.py`:
- Around line 251-256: Add explicit type annotations: annotate
_on_weaver_change(weaver) with a parameter type and return type (e.g., def
_on_weaver_change(self, weaver: Any) -> None:) and annotate _get_stitch_streams
with the concrete return type described in its docstring (for example def
_get_stitch_streams(self, ...) -> Sequence[Tuple[int, int]] or the precise
Sequence/Iterable/List of stream tuples the docstring documents). Import
required typing names (Any, Sequence, Tuple, etc.) at the top and ensure both
functions have the matching types consistent with their docstrings.
- Around line 592-614: The region is too large because totalArea.value (which
includes overlap) is being used directly to compute xmax/ymin while the FoV used
to calculate tiles should be the reliable (non-overlap) footprint; in
_get_region replace using self.totalArea.value[...] directly with a width/height
scaled to the reliable FoV (compute reliable_fov = (1-overlap_frac)*sfov as
already done, then width = self.totalArea.value[0] * (reliable_fov[0]/sfov[0])
and height = self.totalArea.value[1] * (reliable_fov[1]/sfov[1]) and use those
for xmax/xmin/ymin/ymax) so that TiledAcquisitionTask and its nx/ny calculation
produce the expected tile counts.
In `@src/odemis/gui/cont/stream.py`:
- Around line 69-78: Add missing type annotations to StreamController.__init__:
annotate all parameters (stream_bar, stream, tab_data_model, show_panel, view,
sb_ctrl, sp_options) and the return type as None. Use concrete types where
available (e.g., view: MicroscopeView | None, sb_ctrl: StreamBarController |
None) and annotate sp_options as int | None (or Optional[int]) per guidelines;
if some types are defined elsewhere, import or use forward references (quoted
names) to avoid circular imports. Ensure the signature in
src/odemis/gui/cont/stream.py for def __init__ includes these annotations and
keeps the existing docstring unchanged.
🧹 Nitpick comments (2)
src/odemis/acq/acqmng.py (1)
616-621: Avoid quadratic list concatenation forret.
sum(raw_images.values(), [])grows cost with each append; use a linear concat to reduce overhead for large acquisitions.♻️ Proposed fix
+import itertools @@ - ret = sum(raw_images.values(), []) + ret = list(itertools.chain.from_iterable(raw_images.values()))plugins/tileacq.py (1)
31-32: Avoid privateconcurrent.futures._baseimport.
Use the publicconcurrent.futures.CancelledErrorto prevent stdlib API breakage.🔧 Suggested change
-from concurrent.futures._base import CancelledError +from concurrent.futures import CancelledError
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@plugins/tileacq.py`:
- Around line 251-257: The method _on_weaver_change currently sets self.register
to REGISTER_IDENTITY only when weaver == WEAVER_COLLAGE and otherwise to
REGISTER_GLOBAL_SHIFT; update it to also treat WEAVER_COLLAGE_REVERSE as
position-based by checking if weaver is WEAVER_COLLAGE or WEAVER_COLLAGE_REVERSE
and setting self.register = REGISTER_IDENTITY in that case (leave the else
branch to set REGISTER_GLOBAL_SHIFT). This ensures CollageWeaver and
CollageWeaverReverse use REGISTER_IDENTITY consistently.
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 660-676: The overlay acquisition time is only added once but when
batch_acquire_streams=True the overlay stream is acquired on every tile; update
the calculation in the method using self._overlay_stream and its
estimateAcquisitionTime() so overlay_time is multiplied by remaining (e.g.,
overlay_time *= remaining) before returning; ensure you handle the case where
self._overlay_stream is None (leave overlay_time at 0) and keep existing logic
that computes move_time and acq_time unchanged; reference _getTileDAs behavior
(lines ~782-785) to justify multiplying by remaining.
- Around line 755-771: The docstring for _acquireStreamsTile incorrectly states
it returns a single DataArray while the function actually returns a list of
DataArrays (variable das); update the docstring return annotation and
description to indicate a list (e.g., List[DataArray] or sequence of DataArrays)
and explain that das is a collection of DataArrays for the tile so readers and
type-checkers correctly reflect the return type.
- Around line 779-802: The batch path appends the overlay DA into das (via
_acquireStreamsTile) but later _sortDAs is only called with self._streams,
causing overlay DAs to lack MD_ACQ_TYPE and raise KeyError; update the call site
that invokes _sortDAs to include self._overlay_stream when self._overlay_stream
is present (or else filter out overlay DAs from das before calling _sortDAs),
and ensure the overlay stream is passed into the same stream list used to
generate das so _sortDAs can set model.MD_ACQ_TYPE for those DAs; check/update
the code paths around _batch_acquire_streams, _acquireStreamsTile, and the
_sortDAs invocation to keep stream lists consistent.
0e9411c to
3d8f2d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/odemis/acq/stream/_helper.py (1)
672-681:⚠️ Potential issue | 🟠 MajorUse the Y-axis calibration when computing FoV height.
Lines 679-681 currently reuse the X-axis shape/FoV for both dimensions. On non-square scanners this miscomputes the Y FoV, which then skews tiled-area sizing and the new wafer acquisition estimates.
Suggested fix
- def guessFoV(self): + def guessFoV(self) -> Tuple[float, float]: """ Estimate the field-of-view based on the current settings. return (float, float): width, height in meters """ shape = self._scanner.shape pxs = self._scanner.pixelSize.value - full_fov = shape[0] * pxs[0], shape[0] * pxs[1] + full_fov = shape[0] * pxs[0], shape[1] * pxs[1] roi = self.roi.value - return full_fov[0] * (roi[2] - roi[0]), full_fov[0] * (roi[3] - roi[1]) + return full_fov[0] * (roi[2] - roi[0]), full_fov[1] * (roi[3] - roi[1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/stream/_helper.py` around lines 672 - 681, The guessFoV method uses the X-axis shape to compute both width and height, causing wrong Y FoV for non-square scanners; in guessFoV, compute full_fov as (shape[0] * pxs[0], shape[1] * pxs[1]) (use shape[1] for the Y axis) and return (full_fov[0] * (roi[2] - roi[0]), full_fov[1] * (roi[3] - roi[1])) so the height uses the Y-axis calibration (refer to guessFoV, full_fov, shape, pxs, and roi).plugins/tileacq.py (1)
185-198:⚠️ Potential issue | 🟡 MinorRefresh
expectedDurationwhen stitching settings change.
_update_exp_dur()now depends onoverlap,stitch,weaver, andregister, but this setup only wiresnx,ny, andfineAlignto it. Toggling stitching currently leaves the duration label stale until some unrelated setting changes.Suggested wiring
self.nx.subscribe(self._update_exp_dur) self.ny.subscribe(self._update_exp_dur) self.fineAlign.subscribe(self._update_exp_dur) + self.overlap.subscribe(self._update_exp_dur) self.nx.subscribe(self._update_total_area) self.ny.subscribe(self._update_total_area) self.overlap.subscribe(self._update_total_area) self.stitch.subscribe(self._on_stitch_change) + self.stitch.subscribe(self._update_exp_dur) @@ self.stitch.subscribe(self._memory_check) self.weaver.subscribe(self._memory_check) + self.weaver.subscribe(self._update_exp_dur) + self.register.subscribe(self._update_exp_dur)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/tileacq.py` around lines 185 - 198, The expectedDuration label can become stale because _update_exp_dur is only subscribed to nx, ny, and fineAlign but now reads overlap, stitch, weaver, and register; subscribe overlap, stitch, weaver, and register to self._update_exp_dur (in addition to the existing nx, ny, fineAlign subscriptions) so any change to those properties triggers _update_exp_dur and refreshes the duration display.
🧹 Nitpick comments (2)
src/odemis/acq/stream/_sync.py (1)
577-592: Type the newMultipleDetectorStream.guessFoV()helper.Please add
-> Tuple[float, float]here as well so the new tiled-acquisition helper follows the repository typing rule.As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Suggested fix
- def guessFoV(self): + def guessFoV(self) -> Tuple[float, float]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/stream/_sync.py` around lines 577 - 592, The method guessFoV needs an explicit return type hint—update its signature to include -> Tuple[float, float> (i.e. def guessFoV(self) -> Tuple[float, float]:) and ensure Tuple is imported from typing at the top of the module; keep the implementation and exception handling as-is in MultipleDetectorStream.guessFoV so the tiled-acquisition helper conforms to the repo typing rule.src/odemis/acq/stream/_helper.py (1)
1892-1898: Add the return annotation to this new helper.Please type this as returning
Tuple[float, float]so the new FoV helper matches the repository’s Python typing rule.As per coding guidelines
**/*.py: Always use type hints for function parameters and return types in Python code.Suggested fix
- def guessFoV(self): + def guessFoV(self) -> Tuple[float, float]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/stream/_helper.py` around lines 1892 - 1898, The function guessFoV currently lacks a return type annotation; update its signature to include a return type of Tuple[float, float> (i.e., def guessFoV(self) -> Tuple[float, float]:) so it matches the repo typing rule, and add an import for Tuple from typing if not already present; keep the implementation that computes tuple(s * p for s, p in zip(self._emitter.shape, self._emitter.pixelSize.value)) unchanged and reference self._emitter.shape and self._emitter.pixelSize.value when locating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/tileacq.py`:
- Around line 641-672: The issue is that self._ovrl_stream is appended to ss
when fineAlign is enabled but the tiled acquisition is started with stitch_ss,
so the overlay stream is never acquired; update the call to acquireTiledArea
(and any associated pre-acquisition emitter handling) to use the actual stream
list that will be acquired (ss) instead of stitch_ss, or else ensure stitch_ss
is extended to include self._ovrl_stream when self.fineAlign.value and
_can_fine_align(...) are true so that the overlay stream and its emitter setup
are included in the tiled task.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 565-575: The _add_stream_cont function unconditionally passes
sp_options to the controller constructor which breaks subclasses (e.g.,
FastEMStreamController.__init__ doesn't accept sp_options); update
_add_stream_cont to only pass sp_options when it's provided and the target class
constructor accepts that keyword (use inspect.signature(cls.__init__) or
hasattr/try/except to detect acceptance) so StreamController and subclasses are
constructed without causing a TypeError; ensure references to _add_stream_cont,
cls (the controller class), and FastEMStreamController.__init__ are used to
locate and update the logic.
In `@src/odemis/gui/cont/stream.py`:
- Around line 87-104: The code currently augments the caller-supplied sp_options
mask unconditionally; change it so the extra capability bits (OPT_BTN_TINT,
OPT_NO_COLORMAPS, OPT_FIT_RGB, OPT_NAME_EDIT, OPT_BTN_PEAK) are only added when
sp_options is None (i.e. when you set the default mask). Concretely: keep the
initial if sp_options is None: sp_options = OPT_BTN_REMOVE | OPT_BTN_SHOW |
OPT_BTN_UPDATE, and move the subsequent tint/colormap/spectrum/name/peak
conditional ORs for stream (checks using hasattr(stream, "tint"),
isinstance(..., acqstream.RGBStream), isinstance(..., acqstream.SpectrumStream)
and isinstance(..., acqstream.FluoStream), and hasattr(stream, "peak_method"))
inside that same sp_options is None branch so callers that pass an explicit
sp_options mask are not augmented.
---
Outside diff comments:
In `@plugins/tileacq.py`:
- Around line 185-198: The expectedDuration label can become stale because
_update_exp_dur is only subscribed to nx, ny, and fineAlign but now reads
overlap, stitch, weaver, and register; subscribe overlap, stitch, weaver, and
register to self._update_exp_dur (in addition to the existing nx, ny, fineAlign
subscriptions) so any change to those properties triggers _update_exp_dur and
refreshes the duration display.
In `@src/odemis/acq/stream/_helper.py`:
- Around line 672-681: The guessFoV method uses the X-axis shape to compute both
width and height, causing wrong Y FoV for non-square scanners; in guessFoV,
compute full_fov as (shape[0] * pxs[0], shape[1] * pxs[1]) (use shape[1] for the
Y axis) and return (full_fov[0] * (roi[2] - roi[0]), full_fov[1] * (roi[3] -
roi[1])) so the height uses the Y-axis calibration (refer to guessFoV, full_fov,
shape, pxs, and roi).
---
Nitpick comments:
In `@src/odemis/acq/stream/_helper.py`:
- Around line 1892-1898: The function guessFoV currently lacks a return type
annotation; update its signature to include a return type of Tuple[float, float>
(i.e., def guessFoV(self) -> Tuple[float, float]:) so it matches the repo typing
rule, and add an import for Tuple from typing if not already present; keep the
implementation that computes tuple(s * p for s, p in zip(self._emitter.shape,
self._emitter.pixelSize.value)) unchanged and reference self._emitter.shape and
self._emitter.pixelSize.value when locating the code.
In `@src/odemis/acq/stream/_sync.py`:
- Around line 577-592: The method guessFoV needs an explicit return type
hint—update its signature to include -> Tuple[float, float> (i.e. def
guessFoV(self) -> Tuple[float, float]:) and ensure Tuple is imported from typing
at the top of the module; keep the implementation and exception handling as-is
in MultipleDetectorStream.guessFoV so the tiled-acquisition helper conforms to
the repo typing rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0cefc52-bec1-42b5-9b4f-1e374acb85f4
📒 Files selected for processing (7)
plugins/tileacq.pysrc/odemis/acq/stitching/_tiledacq.pysrc/odemis/acq/stream/_helper.pysrc/odemis/acq/stream/_sync.pysrc/odemis/gui/cont/stream.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/plugin/__init__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/plugin/init.py
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow
branch
from
3d8f2d7 to
bea5e52
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/acq/stitching/_tiledacq.py (1)
282-311: 🛠️ Refactor suggestion | 🟠 MajorAdd type hints to the newly introduced helper APIs.
getFov,guessSmallestFov, and_acquireStreamsTilewere added without parameter/return annotations. This file already imports the needed typing primitives, so these should be typed before merge.Example annotation pass
`@staticmethod` - def getFov(sd): + def getFov(sd: Union[Stream, DataArray]) -> Tuple[float, float]: """ sd (Stream or DataArray): If it's a stream, it must be a live stream, and the FoV will be estimated based on the settings. return (float, float): width, height in m """ `@classmethod` - def guessSmallestFov(cls, ss): + def guessSmallestFov( + cls, ss: List[Union[Stream, DataArray]] + ) -> Tuple[float, float]: """ Return (float, float): smallest width and smallest height of all the FoV Note: they are not necessarily from the same FoV. raise ValueError: If no stream selected """ - def _acquireStreamsTile(self, i, ix, iy, streams): + def _acquireStreamsTile( + self, i: int, ix: int, iy: int, streams: List[Stream] + ) -> List[DataArray]: """ Calls acquire function and blocks until the data is returned. :return: list(DataArray) acquired das for the current tile streams """As per coding guidelines, "Always use type hints for function parameters and return types in Python code".
Also applies to: 741-757
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/stitching/_tiledacq.py` around lines 282 - 311, The new helper functions getFov, guessSmallestFov and _acquireStreamsTile lack type annotations; update their signatures to include parameter and return type hints using the typing primitives already imported (e.g., Stream, model.DataArray unions, Sequence/Iterable, Tuple[float,float], and appropriate Optional/Any where needed) so callers and linters can validate types; specifically annotate getFov(sd) to accept Union[model.DataArray, Stream] and return Tuple[float, float], annotate guessSmallestFov(cls, ss) to accept an Iterable[Stream] (or Sequence[Union[model.DataArray, Stream]] if applicable) and return Tuple[float, float], and add proper parameter/return annotations to _acquireStreamsTile consistent with how it’s used elsewhere.
🧹 Nitpick comments (3)
src/odemis/acq/stream/_helper.py (1)
1892-1898: Add return type hint for consistency.Per coding guidelines, type hints should be added for function parameters and return types. The method is missing a return type annotation.
Proposed fix
- def guessFoV(self): + def guessFoV(self) -> Tuple[float, float]: """ Estimate the field-of-view based on the current settings. :return: (float, float): width, height in meters """ # Calculate FoV based on the emitter's shape and pixel size return tuple(s * p for s, p in zip(self._emitter.shape, self._emitter.pixelSize.value))Note: You'll need to import
Tuplefromtypingif not already imported (it appears to be imported at line 27).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/stream/_helper.py` around lines 1892 - 1898, The method guessFoV is missing a return type annotation; add a return type hint like -> Tuple[float, float] to the guessFoV signature and ensure Tuple is imported from typing (or use a suitable existing alias), keeping the implementation unchanged (it returns tuple(s * p for s, p in zip(self._emitter.shape, self._emitter.pixelSize.value))).plugins/tileacq.py (2)
311-314: Consider logging the exception for debugging.When
ValueErrororAttributeErroris caught, only a debug message is logged without the exception details. This could make debugging harder when unexpected errors occur.Proposed improvement
except (ValueError, AttributeError): # No streams or cannot compute FoV - logging.debug("Cannot compute expected acquisition duration") + logging.debug("Cannot compute expected acquisition duration", exc_info=True) tat = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/tileacq.py` around lines 311 - 314, The except block that currently swallows ValueError/AttributeError and sets tat=0 should capture the exception and include its details in the log so debugging is easier; change the handler to "except (ValueError, AttributeError) as e:" and call logging.exception("Cannot compute expected acquisition duration") or logging.debug("Cannot compute expected acquisition duration", exc_info=True) before leaving tat = 0 so the original error context is preserved.
696-705: Potential race condition withos.path.getctime.When finding the last tile file,
os.path.getctimereturns the file creation time, but on some Unix systems this returns the last metadata change time (ctime), not creation time. Consider usingos.path.getmtimefor more consistent behavior across platforms.Consider using modification time
if files: - last_tile_fn = max(files, key=os.path.getctime) + last_tile_fn = max(files, key=os.path.getmtime)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/tileacq.py` around lines 696 - 705, The code uses os.path.getctime to pick the latest tile file which is inconsistent across platforms; change the selector to use os.path.getmtime when computing last_tile_fn (i.e., replace the key=os.path.getctime in the max(files, ...) call) so the most recently modified file is chosen before calling self.showAcquisition(last_tile_fn); keep the rest of the files collection logic (basename/extension matching) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/tileacq.py`:
- Around line 605-625: The callers (e.g., _update_exp_dur) expect _get_region to
possibly return None, but _get_region is annotated to always return
Tuple[float,float,float,float] and instead lets ValueError from
_guess_smallest_fov propagate; change _get_region to return
Optional[Tuple[float,float,float,float]]: catch ValueError (or other expected
errors) around the call to _guess_smallest_fov inside _get_region, log or record
the error as appropriate, and return None on failure; update the function
signature/type annotation to Optional[...] so callers that check "region is
None" are correct and no further changes to callers are needed.
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 123-124: The constructor (__init__) currently permits
half-configured stitching by allowing registrar or weaver to be None
independently; change __init__ to validate the pair so that either both
registrar and weaver are provided or both are None, and raise a ValueError with
a clear message if only one is set. Locate the registration/weaving guard logic
(references: __init__, any methods named _stitch, stitch, or the
registrar/weaver attributes used later in the class) and remove/adjust the
current "if either is None then skip" behavior so it relies on the validated
invariant; apply the same pair validation to the other related blocks mentioned
(the similar guards around the ranges noted) to ensure consistent behavior
across the class. Ensure tests or callers expecting skipped stitching still work
by explicitly requiring callers to pass both None for "no stitching."
In `@src/odemis/acq/stream/_sync.py`:
- Around line 577-592: The guessFoV method should not swallow AttributeError
from a substream implementation; instead, before calling s.guessFoV(), check the
attribute (e.g., hasattr(s, "guessFoV") or getattr(s, "guessFoV", None)) and
only call it if present, and only catch NotImplementedError from the call to
treat the stream as unsupported; remove AttributeError from the except clause.
Also add the missing return type hint to guessFoV (e.g., -> Tuple[float, float])
and ensure typing.Tuple is imported if needed.
---
Outside diff comments:
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 282-311: The new helper functions getFov, guessSmallestFov and
_acquireStreamsTile lack type annotations; update their signatures to include
parameter and return type hints using the typing primitives already imported
(e.g., Stream, model.DataArray unions, Sequence/Iterable, Tuple[float,float],
and appropriate Optional/Any where needed) so callers and linters can validate
types; specifically annotate getFov(sd) to accept Union[model.DataArray, Stream]
and return Tuple[float, float], annotate guessSmallestFov(cls, ss) to accept an
Iterable[Stream] (or Sequence[Union[model.DataArray, Stream]] if applicable) and
return Tuple[float, float], and add proper parameter/return annotations to
_acquireStreamsTile consistent with how it’s used elsewhere.
---
Nitpick comments:
In `@plugins/tileacq.py`:
- Around line 311-314: The except block that currently swallows
ValueError/AttributeError and sets tat=0 should capture the exception and
include its details in the log so debugging is easier; change the handler to
"except (ValueError, AttributeError) as e:" and call logging.exception("Cannot
compute expected acquisition duration") or logging.debug("Cannot compute
expected acquisition duration", exc_info=True) before leaving tat = 0 so the
original error context is preserved.
- Around line 696-705: The code uses os.path.getctime to pick the latest tile
file which is inconsistent across platforms; change the selector to use
os.path.getmtime when computing last_tile_fn (i.e., replace the
key=os.path.getctime in the max(files, ...) call) so the most recently modified
file is chosen before calling self.showAcquisition(last_tile_fn); keep the rest
of the files collection logic (basename/extension matching) intact.
In `@src/odemis/acq/stream/_helper.py`:
- Around line 1892-1898: The method guessFoV is missing a return type
annotation; add a return type hint like -> Tuple[float, float] to the guessFoV
signature and ensure Tuple is imported from typing (or use a suitable existing
alias), keeping the implementation unchanged (it returns tuple(s * p for s, p in
zip(self._emitter.shape, self._emitter.pixelSize.value))).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3116757a-a788-453b-a8ad-5f0959873312
📒 Files selected for processing (4)
plugins/tileacq.pysrc/odemis/acq/stitching/_tiledacq.pysrc/odemis/acq/stream/_helper.pysrc/odemis/acq/stream/_sync.py
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow
branch
from
bea5e52 to
4edcd0c
Compare
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow
branch
from
4edcd0c to
895cd23
Compare
There was a problem hiding this comment.
Pull request overview
Extends the tiled acquisition plugin and underlying stitching/acquisition utilities to better support wafer acquisition workflows, including optional “acquire tiles without stitching”, improved FoV handling, and more controllable stream-panel UI options in plugin dialogs.
Changes:
- Add a “hidden” stream view to acquisition dialogs and allow plugins to pass
sp_optionsto customize StreamPanel controls. - Enhance tiled acquisition task utilities (FoV helpers, time/memory estimation) and allow disabling stitching by passing
registrar=Noneandweaver=None. - Update TileAcq plugin UI/behavior: expose weaver/registrar choices, hide them when stitching is off, and switch acquisition execution to
acquireTiledArea().
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/gui/plugin/init.py | Adds hidden_view to the dialog model and forwards StreamPanel option flags (sp_options) when adding streams. |
| src/odemis/gui/cont/stream_bar.py | Plumbs sp_options through StreamBarController into StreamController creation. |
| src/odemis/gui/cont/stream.py | Allows StreamController to accept caller-provided StreamPanel option flags instead of always using defaults. |
| src/odemis/acq/stream/_sync.py | Exposes .guessFoV and .focuser for multi-detector streams by delegating from substreams. |
| src/odemis/acq/stream/_helper.py | Adds guessFoV() for OverlayStream to support FoV-based computations. |
| src/odemis/acq/stitching/_tiledacq.py | Refactors FoV helpers, makes stitching optional, and changes tile acquisition behavior when no z-stack is used. |
| plugins/tileacq.py | Updates the Tile Acquisition plugin to support stitch on/off + registrar/weaver selection and uses acquireTiledArea() for execution/estimation. |
You can also share your feedback on Copilot code review. Take the survey.
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow
branch
from
895cd23 to
6a02e09
Compare
JIRA Tickets: