[CSDM 1162] extend tiled acquisition plugin to support wafer acquisition workflow#3326
Conversation
📝 WalkthroughWalkthroughThe changes consolidate tile acquisition into a unified tiled-area workflow. A new weaving configuration system with options (Mean, Collage, etc.) is introduced alongside region-based field-of-view handling. The core TiledAcquisitionTask now accepts overlay stream support, explicit FoV parameters, and batch acquisition modes. GUI layer updates propagate stream panel options through controller constructors. The plugin's Sequence DiagramsequenceDiagram
actor User
participant TileAcqPlugin
participant AcquisitionDialog
participant TiledAcquisitionTask
participant StreamBar
participant Stitcher
User->>TileAcqPlugin: Configure weaver & region
TileAcqPlugin->>TileAcqPlugin: _on_weaver_change()
TileAcqPlugin->>TileAcqPlugin: _get_region(start_pos)
User->>AcquisitionDialog: Initiate tiled acquisition
AcquisitionDialog->>StreamBar: addStream(stream, sp_options)
StreamBar->>StreamBar: _add_stream(sp_options)
AcquisitionDialog->>TileAcqPlugin: acquire()
TileAcqPlugin->>TileAcqPlugin: _get_stitch_streams()
TileAcqPlugin->>TiledAcquisitionTask: acquireTiledArea(overlay_stream, sfov, batch_acquire_streams)
TiledAcquisitionTask->>TiledAcquisitionTask: guessSmallestFov(streams)
loop For each tile
TiledAcquisitionTask->>TiledAcquisitionTask: _acquireStreamsTile(batch mode)
end
alt Registrar & Weaver present
TiledAcquisitionTask->>Stitcher: Stitch tiles with weaver
Stitcher->>Stitcher: Apply weaving (Mean/Collage)
end
TiledAcquisitionTask-->>TileAcqPlugin: Return stitched result
TileAcqPlugin->>AcquisitionDialog: Export & display results
AcquisitionDialog-->>User: Show acquisition results
🚥 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
| def __init__(self, stream_bar, stream, tab_data_model, show_panel=True, view=None, | ||
| sb_ctrl=None): | ||
| sb_ctrl=None, sp_options=None): | ||
| """ | ||
| view (MicroscopeView or None): Link stream to a view. If view is None, the stream | ||
| will be linked to the focused view. Passing a view to the controller ensures | ||
| that the visibility button functions correctly when multiple views are present. | ||
| sb_ctrl (StreamBarController or None): the StreamBarController which (typically) | ||
| created this StreamController. Only needed for ROA repetition display. | ||
| sp_options: (int or None) combination of OPT_* values for the StreamPanel or None for default. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n src/odemis/gui/cont/stream.py | head -100Repository: delmic/odemis
Length of output: 4996
🏁 Script executed:
sed -n '69,200p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 5847
🏁 Script executed:
grep -n "def " src/odemis/gui/cont/stream.py | head -20Repository: delmic/odemis
Length of output: 940
🏁 Script executed:
sed -n '290,335p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 1912
🏁 Script executed:
grep -n ": " src/odemis/gui/cont/stream.py | grep "def " | head -10Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
rg "->|: int|: str|: bool|: \w+\[" src/odemis/gui/cont/stream.py | head -20Repository: delmic/odemis
Length of output: 461
🏁 Script executed:
rg '\-\>|: [a-zA-Z]' src/odemis/gui/cont/stream.py | head -20Repository: delmic/odemis
Length of output: 140
🏁 Script executed:
rg 'def .* ->' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 39
🏁 Script executed:
fd "\.py$" src/odemis/gui/cont | head -10Repository: delmic/odemis
Length of output: 512
🏁 Script executed:
rg 'def .+\(.*\):' src/odemis/gui/cont/acquisition/snapshot.py | head -5Repository: delmic/odemis
Length of output: 274
🏁 Script executed:
sed -n '1,100p' src/odemis/gui/cont/acquisition/snapshot.pyRepository: delmic/odemis
Length of output: 3626
🏁 Script executed:
sed -n '69,150p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 3539
🏁 Script executed:
sed -n '1,60p' src/odemis/gui/cont/stream.py | grep -E "import|from"Repository: delmic/odemis
Length of output: 1242
🏁 Script executed:
rg "class StreamBarController" src/Repository: delmic/odemis
Length of output: 125
🏁 Script executed:
grep -A 5 "class MicroscopeView" src/odemis/gui/util.py | head -10Repository: delmic/odemis
Length of output: 112
🏁 Script executed:
rg "class MicroscopeView" src/Repository: delmic/odemis
Length of output: 381
Add type hints to StreamController.__init__.
This method lacks type hints for all parameters and return type. Per coding guidelines, type hints are mandatory for Python files. The sp_options parameter should be annotated as int | None (or Optional[int]).
🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/stream.py` around lines 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.
plugins/tileacq.py
Outdated
| ) | ||
| except (ValueError, AttributeError): | ||
| # No streams or cannot compute FoV | ||
| tat = 1 |
There was a problem hiding this comment.
is this really a sensible default?
There was a problem hiding this comment.
Right, I will set it to 0
plugins/tileacq.py
Outdated
| self.nx = model.IntContinuous(5, (1, 1000), setter=self._set_nx) | ||
| self.ny = model.IntContinuous(5, (1, 1000), setter=self._set_ny) | ||
| self.overlap = model.FloatContinuous(20, (1, 80), unit="%") | ||
| self.overlap = model.FloatContinuous(20, (0, 80), unit="%") |
There was a problem hiding this comment.
you have to divide the overlap by 100 in a few places, would it make sense to already divide by 100 here?
There was a problem hiding this comment.
Yeah, I can change the unit from % to fraction
plugins/tileacq.py
Outdated
| overlay_stream = None | ||
| overlap_frac = self.overlap.value / 100 | ||
| stitch_ss = self._get_stitch_streams() | ||
| ss += stitch_ss |
There was a problem hiding this comment.
why not user ss.append(stich_ss) here?
There was a problem hiding this comment.
I think you meant ss.extend(stitch_ss)
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.
| def _on_weaver_change(self, weaver): | ||
| # For WEAVER_COLLAGE, use REGISTER_IDENTITY, which is the fastest and stiches the tiles based on the stage positions | ||
| if weaver == WEAVER_COLLAGE: | ||
| self.register = REGISTER_IDENTITY | ||
| else: | ||
| self.register = REGISTER_GLOBAL_SHIFT | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all references to WEAVER_COLLAGE_REVERSE and how it's used with registrars
rg -n "WEAVER_COLLAGE_REVERSE" --type=py -B2 -A2Repository: delmic/odemis
Length of output: 8528
🏁 Script executed:
#!/bin/bash
# Check the weaver constants definitions and their documentation
rg -n "WEAVER_COLLAGE|WEAVER_MEAN|REGISTER_IDENTITY|REGISTER_GLOBAL_SHIFT" plugins/tileacq.py | head -30Repository: delmic/odemis
Length of output: 989
🏁 Script executed:
#!/bin/bash
# Get context around the _on_weaver_change method and any related weaver handling
sed -n '240,280p' plugins/tileacq.pyRepository: delmic/odemis
Length of output: 1723
🏁 Script executed:
#!/bin/bash
# Find and examine the weaver implementations
fd -n "_weaver.py" --type fRepository: delmic/odemis
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Look at the CollageWeaver and CollageWeaverReverse class definitions
rg -n "class CollageWeaver|class CollageWeaverReverse" --type=py -A20Repository: delmic/odemis
Length of output: 3840
🏁 Script executed:
#!/bin/bash
# Check test case for COLLAGE_REVERSE to see what registrar is expected
sed -n '310,320p' src/odemis/acq/stitching/test/tiledacq_test.pyRepository: delmic/odemis
Length of output: 609
WEAVER_COLLAGE_REVERSE should also use REGISTER_IDENTITY for consistency.
Both CollageWeaver and CollageWeaverReverse place tiles at their center positions (as documented in the docstrings). Since WEAVER_COLLAGE_REVERSE is position-based like WEAVER_COLLAGE, it should use REGISTER_IDENTITY rather than falling through to REGISTER_GLOBAL_SHIFT. This is particularly important since WEAVER_COLLAGE_REVERSE is the default weaver for secom/delphi microscopes.
Proposed fix
def _on_weaver_change(self, weaver):
- # For WEAVER_COLLAGE, use REGISTER_IDENTITY, which is the fastest and stiches the tiles based on the stage positions
- if weaver == WEAVER_COLLAGE:
+ # For collage weavers, use REGISTER_IDENTITY, which is the fastest and stitches the tiles based on the stage positions
+ if weaver in (WEAVER_COLLAGE, WEAVER_COLLAGE_REVERSE):
self.register = REGISTER_IDENTITY
else:
self.register = REGISTER_GLOBAL_SHIFT🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 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.
| # Estimate overlay stream acquisition time | ||
| overlay_time = 0 | ||
| if self._overlay_stream is not None: | ||
| overlay_time = self._overlay_stream.estimateAcquisitionTime() | ||
|
|
||
| try: | ||
| # move_speed is a default speed but not an actual stage speed due to which | ||
| # extra time is added based on observed time taken to move stage from one tile position to another | ||
| move_time = max(self._guessSmallestFov(self._streams)) * (remaining - 1) / self._move_speed + 0.3 * remaining | ||
| move_time = max(self._sfov) * (remaining - 1) / self._move_speed + 0.3 * remaining | ||
| # current tile is part of remaining, so no need to move there | ||
| except ValueError: # no current streams | ||
| move_time = 0.5 | ||
|
|
||
| logging.info(f"The computed time in seconds for tiled acquisition for {remaining} tiles for move is {move_time}," | ||
| f" acquisition is {acq_time}") | ||
|
|
||
| return acq_time + move_time + stitch_time | ||
| return acq_time + move_time + stitch_time + overlay_time |
There was a problem hiding this comment.
Overlay time is underestimated — it should be multiplied by the number of tiles.
When batch_acquire_streams=True, the overlay stream is acquired on every tile (see _getTileDAs line 782-785), but overlay_time is only added once to the total estimate instead of being multiplied by remaining.
Proposed fix
# Estimate overlay stream acquisition time
overlay_time = 0
if self._overlay_stream is not None:
- overlay_time = self._overlay_stream.estimateAcquisitionTime()
+ overlay_time = self._overlay_stream.estimateAcquisitionTime() * remaining
🤖 Prompt for AI Agents
In `@src/odemis/acq/stitching/_tiledacq.py` around lines 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.
| def _acquireStreamsTile(self, i, ix, iy, streams): | ||
| """ | ||
| Calls acquire function and blocks until the data is returned. | ||
| :return DataArray: Acquired das for the current tile streams | ||
| """ | ||
| # Update the progress bar | ||
| self._future.set_progress(end=self.estimateTime(self._number_of_tiles - i) + time.time()) | ||
| # Acquire data array for passed stream | ||
| self._future.running_subf = acqmng.acquire(streams, self._settings_obs) | ||
| das, e = self._future.running_subf.result() # blocks until all the acquisitions are finished | ||
| if e: | ||
| logging.warning(f"Acquisition for tile {ix}x{iy}, streams partially failed: {e}") | ||
|
|
||
| if self._future._task_state == CANCELLED: | ||
| raise CancelledError() | ||
|
|
||
| return das # return all das for the tile |
There was a problem hiding this comment.
Docstring return type is incorrect.
The docstring says :return DataArray: but the method returns a list of DataArrays (line 771: return das).
Proposed fix
def _acquireStreamsTile(self, i, ix, iy, streams):
"""
Calls acquire function and blocks until the data is returned.
- :return DataArray: Acquired das for the current tile streams
+ :return: list of DataArrays acquired for the current tile streams
"""🤖 Prompt for AI Agents
In `@src/odemis/acq/stitching/_tiledacq.py` around lines 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.
|
|
||
| if self._batch_acquire_streams: | ||
| streams = [] | ||
| if self._overlay_stream: | ||
| streams.append(self._overlay_stream) | ||
| streams.extend(self._streams) | ||
| das = self._acquireStreamsTile(i, ix, iy, streams) | ||
| else: | ||
| 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) | ||
| das.append(da) | ||
|
|
||
| return das |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the _getTileDAs method and understand the context
sed -n '773,802p' src/odemis/acq/stitching/_tiledacq.pyRepository: delmic/odemis
Length of output: 1418
🏁 Script executed:
# Now let's look at the _sortDAs method to understand how it handles DAs
sed -n '488,548p' src/odemis/acq/stitching/_tiledacq.pyRepository: delmic/odemis
Length of output: 2977
🏁 Script executed:
# Let's find where _sortDAs is called (around line 857)
sed -n '850,870p' src/odemis/acq/stitching/_tiledacq.pyRepository: delmic/odemis
Length of output: 866
🏁 Script executed:
# Search for MD_ACQ_TYPE usage to understand when it's set
rg -n "MD_ACQ_TYPE" src/odemis/acq/stitching/_tiledacq.py -A 2 -B 2Repository: delmic/odemis
Length of output: 2176
🏁 Script executed:
# Let's also check what _acquireStreamsTile returns and how overlay_stream is handled
rg -n "_acquireStreamsTile\|_acquireStreamCompressedZStack\|_acquireStreamTile" src/odemis/acq/stitching/_tiledacq.py -B 2 -A 5 | head -100Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check the batch_acquire_streams and overlay_stream initialization
rg -n "batch_acquire_streams\|overlay_stream" src/odemis/acq/stitching/_tiledacq.py | head -20Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Search for _acquireStreamsTile implementation to see if it sets MD_ACQ_TYPE
rg -n "_acquireStreamsTile" src/odemis/acq/stitching/_tiledacq.py -B 5 -A 20 | head -80Repository: delmic/odemis
Length of output: 2581
🏁 Script executed:
# Check if overlay_stream is included elsewhere or if there's special handling
rg -n "overlay_stream" src/odemis/acq/stitching/_tiledacq.py -B 2 -A 2Repository: delmic/odemis
Length of output: 2675
🏁 Script executed:
# Check the full _acquireTiles method to understand the complete flow
sed -n '820,870p' src/odemis/acq/stitching/_tiledacq.pyRepository: delmic/odemis
Length of output: 2083
🏁 Script executed:
# Look for where overlay_stream is set/initialized
rg -n "self._overlay_stream\s*=" src/odemis/acq/stitching/_tiledacq.py -B 2 -A 2Repository: delmic/odemis
Length of output: 319
Overlay stream DA will cause KeyError at line 531 in _sortDAs.
When batch_acquire_streams=True with an overlay_stream, the overlay stream's DA is included in the returned das list (lines 782-785). However, _sortDAs is called with only self._streams (line 857), which doesn't include overlay_stream. The overlay DA won't match any stream in the search (lines 505-527), so MD_ACQ_TYPE won't be set. Line 531 then unconditionally accesses da.metadata[model.MD_ACQ_TYPE], raising KeyError.
Pass overlay_stream to _sortDAs if present in batch mode, or filter out overlay DAs before calling _sortDAs.
🤖 Prompt for AI Agents
In `@src/odemis/acq/stitching/_tiledacq.py` around lines 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.
3 participants
JIRA Tickets: