[CSDM-1162] extend tiled acquisition plugin to support wafer acquisition workflow v2#3344
Draft
nandishjpatel wants to merge 6 commits intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR introduces tile-based acquisition capabilities to the SPARC2 system. It updates a configuration value in the mirror alignment simulator, adds comprehensive tile acquisition support to the TileAcqPlugin including tile mode selection (STANDARD or CUSTOM), tile loading from TSV files, weaver-based stitching controls, and per-tile z-position alignment. The TiledAcquisitionTask is refactored to support optional overlay streams and batch acquisition modes, with FoV-related methods converted to static/class methods. GUI controllers are enhanced with configurable stream panel options propagation, and stream infrastructure receives a focuser attribute for use during tiled acquisitions. Sequence Diagram(s)sequenceDiagram
actor User
participant AcquisitionDialog
participant TileAcqPlugin
participant _executor as Task Executor
participant TiledAcquisitionTask
participant Registrar/Weaver
User->>AcquisitionDialog: Configure tile mode to CUSTOM
AcquisitionDialog->>TileAcqPlugin: tile_mode changed
TileAcqPlugin->>TileAcqPlugin: _on_tile_mode_change()
User->>AcquisitionDialog: Load tile positions from TSV
AcquisitionDialog->>TileAcqPlugin: tiles_file set
TileAcqPlugin->>TileAcqPlugin: _on_tile_file_change(filepath)
TileAcqPlugin->>TileAcqPlugin: Parse TSV, populate tile_map
TileAcqPlugin->>TileAcqPlugin: _update_total_area, _update_exp_dur
User->>AcquisitionDialog: Select weaver (Mean/Collage)
AcquisitionDialog->>TileAcqPlugin: weaver changed
TileAcqPlugin->>TileAcqPlugin: _on_weaver_change(), set registrar
User->>AcquisitionDialog: Start acquisition
AcquisitionDialog->>TileAcqPlugin: acquire()
TileAcqPlugin->>TileAcqPlugin: Load tile_map from tiles_file
TileAcqPlugin->>TileAcqPlugin: _generate_triangulation_points()
loop For each tile position
TileAcqPlugin->>TileAcqPlugin: _perform_z_survey_point(tile_pos)
TileAcqPlugin->>_executor: Submit per-tile acquisition task
_executor->>TiledAcquisitionTask: run()
TiledAcquisitionTask->>TiledAcquisitionTask: Acquire streams for tile
TiledAcquisitionTask-->>_executor: Return tile data
end
TileAcqPlugin->>TileAcqPlugin: _get_stitch_streams()
TileAcqPlugin->>Registrar/Weaver: Stitch tile results
Registrar/Weaver-->>TileAcqPlugin: Stitched output
TileAcqPlugin-->>AcquisitionDialog: Acquisition complete
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/odemis/acq/stitching/_tiledacq.py (3)
752-780:⚠️ Potential issue | 🟠 MajorFix
adjust_metadatainput shape and preserve overlay corrections.Line 759-778 stores a single
DataArrayper stream (and uses_acquireStreamTilefor the overlay), butAcquisitionTask.adjust_metadataexpects lists of DataArrays and the OverlayStream needs both optical+SEM DAs. As written, it indexes into array rows, leading to incorrect metadata or errors and discarding the second overlay DA.🐛 Suggested fix
- 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( + f"Overlay acquisition for tile {ix}x{iy} partially failed: {e}" + ) + if self._future._task_state == CANCELLED: + raise CancelledError() + raw_images[self._overlay_stream] = overlay_das ... - raw_images[stream] = da + raw_images[stream] = [da] ... - return list(raw_images.values()) + return [d for das in raw_images.values() for d in das]
108-139:⚠️ Potential issue | 🟠 MajorAdd type hints and update docstring format for the
__init__signature.The entire
__init__method signature (lines 108-110) lacks type hints for all parameters and return type. Additionally, the docstring uses(type)notation instead of reST format without type information. The coding guidelines require type hints on all function parameters/returns and reST-style docstrings without embedded type info.Update the signature to include type hints for all parameters (including the new
overlay_streamandsfovparams) and a-> Nonereturn type. Remove type information from the docstring entries, keeping only descriptions of what each parameter does.Example for the new params
- def __init__(self, streams, stage, region, overlap, settings_obs=None, log_path=None, future=None, zlevels=None, - registrar=REGISTER_GLOBAL_SHIFT, weaver=WEAVER_MEAN, focusing_method=FocusingMethod.NONE, - focus_points=None, focus_range=None, centered_acq=True, overlay_stream=None, sfov=None): + def __init__(self, streams, stage, region, overlap, settings_obs=None, log_path=None, future=None, zlevels=None, + registrar=REGISTER_GLOBAL_SHIFT, weaver=WEAVER_MEAN, focusing_method=FocusingMethod.NONE, + focus_points=None, focus_range=None, centered_acq=True, + overlay_stream: Optional[Stream] = None, + sfov: Optional[Tuple[float, float]] = None) -> None:
288-318:⚠️ Potential issue | 🔴 CriticalAdd type hints and convert docstrings to reST style for helper methods.
Lines 288-318 and 1002-1029 add
getFov,guessSmallestFov, andstitchTileswithout type hints and with docstrings that embed type information. Please add type hints to parameters and return types, and convert docstrings to reST:param/:return:style without embedded types.🔧 Example for `getFov` (apply similarly to the others)
`@staticmethod` -def getFov(sd): +def getFov(sd: Union[Stream, model.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 + Return the field of view for a stream or DataArray. + + :param sd: Stream or DataArray to inspect. + :return: Field of view (width, height) in meters. """src/odemis/acq/acqmng.py (1)
623-630:⚠️ Potential issue | 🟠 MajorAdd type hints and convert docstring to reST style for
adjust_metadata.The function lacks type hints on its parameter and return type. Additionally, the docstring embeds type information in a non-reST format, conflicting with the project guidelines requiring type hints and reST-style docstrings (without type information embedded).
🔧 Suggested change
`@staticmethod` -def adjust_metadata(raw_data): +def adjust_metadata(raw_data: Dict[object, 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. + Update/adjust the metadata of the raw data received based on global information. + + :param raw_data: Raw data per stream. Values are lists of DataArrays. """src/odemis/gui/cont/stream_bar.py (1)
485-575:⚠️ Potential issue | 🟠 MajorAdd type hints and convert docstrings to reST format for
_add_streamand_add_stream_cont.Both methods lack type annotations on parameters and return types, and their docstrings use Google/NumPy style instead of the required reStructuredText format. The
sp_optionsparameter is documented but unannotated.Suggested signature updates
-from typing import Optional +from typing import Any, Optional ... - def _add_stream(self, stream, add_to_view=False, visible=True, play=None, stream_cont_cls=StreamController, - sp_options=None): + def _add_stream( + self, + stream: acqstream.Stream, + add_to_view: Any = False, + visible: bool = True, + play: Optional[bool] = None, + stream_cont_cls: type[StreamController] = StreamController, + sp_options: Optional[int] = None, + ) -> acqstream.Stream | StreamController: @@ - def _add_stream_cont(self, stream, show_panel=True, locked=False, static=False, - view=None, cls=StreamController, sp_options=None): + def _add_stream_cont( + self, + stream: acqstream.Stream, + show_panel: bool = True, + locked: bool = False, + static: bool = False, + view: Any = None, + cls: type[StreamController] = StreamController, + sp_options: Optional[int] = None, + ) -> StreamController:Also update docstrings to use reST format (
:param,:return, etc.) instead ofArgs:/Returns:sections.src/odemis/gui/cont/stream.py (1)
69-106:⚠️ Potential issue | 🟠 MajorAdd type hints and convert docstring to reStructuredText format for
StreamController.__init__.The method signature (lines 69-70) is missing type annotations for all parameters and the return type. Additionally, the docstring (lines 71-78) does not follow reStructuredText format with
:param:directives.Suggested implementation
- def __init__(self, stream_bar, stream, tab_data_model, show_panel=True, view=None, - sb_ctrl=None, sp_options=None): + def __init__( + self, + stream_bar, + stream: acqstream.Stream, + tab_data_model, + show_panel: bool = True, + view=None, + sb_ctrl=None, + sp_options: int | None = None, + ) -> None: """ - view (MicroscopeView or None): Link stream to a view. If view is None, the stream + Manage a stream and its accompanying stream panel. + + :param stream_bar: The stream bar container. + :param stream: The stream to manage. + :param tab_data_model: The data model for the tab. + :param show_panel: Whether to show the panel. + :param view: 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) + :param sb_ctrl: 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. + :param sp_options: Combination of OPT_* values for the StreamPanel, or None for default. """Per coding guidelines: Always use type hints for function parameters and return types; Include docstrings in reStructuredText format without embedded type information.
src/odemis/gui/plugin/__init__.py (1)
508-540:⚠️ Potential issue | 🟠 MajorAdd type hints and reST docstring updates for
addStream.
Thestream,index, andsp_optionsparameters lack type annotations, and the docstring does not follow reST format. This violates the coding guidelines requiring type hints for all function parameters/returns and reST-style docstrings.✅ Suggested signature/docstring 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. @@ - 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 (or None to show a viewport only). + :param index: Viewport index or None to add to the hidden view. + :param sp_options: StreamPanel option flags or None for default. """
🤖 Fix all issues with AI agents
In `@plugins/tileacq.py`:
- Around line 84-93: Add reST-style class docstrings for the public enums
TileMode and TileColumnNames to satisfy documentation guidelines: for TileMode
describe the purpose of the enum (controls tiling mode, with values STANDARD and
CUSTOM and brief notes on grid vs TSV/mirror alignment) and for TileColumnNames
describe that it lists expected TSV/CSV column names (Tile Number, Stage
Position.X [m], Stage Position.Y [m]) and their meaning; place the docstrings
immediately under the class definitions, written in reStructuredText style
(short summary line, optional one-line blank, then a short explanatory sentence)
and do not include type annotations.
- Around line 798-826: The _perform_z_survey_point docstring is out-of-sync:
remove the stale parameters (axes, ccd, search_range, max_iter, stop_early,
save_images) and replace with a concise description of the actual input and
output — document the single parameter tile_pos (its expected type Dict and the
keys/values it must contain, e.g., tile coordinates, stage z setpoint, camera
exposure metadata if used) and clearly state the return type
model.ProgressiveFuture and its extra attributes (n_steps, current_step,
task_canceller and exception propagation behavior). Also update the short
summary line to describe that this starts a Z-survey alignment task for the
given tile_pos and mention any side effects (saving images/logging) that callers
must know.
- Around line 940-954: The code spawns detached threads in do_tile_acq via
threading.Thread(...).start() calling save_tile, which can race with
showAcquisition and hide export failures; change this to perform the save
synchronously (call save_tile(tile_path, da) directly) or store the Thread
objects and join them before any code that displays results (e.g., before
showAcquisition). Update do_tile_acq to either call save_tile synchronously
after appending da to da_list, or collect threads in a list (e.g.,
tile_save_threads) and ensure they are joined at the point where showAcquisition
is invoked; reference functions save_tile and do_tile_acq and the
tile_acq_task/da_list flow when making the change.
- Around line 434-462: Custom mode computes area from tile centers so a single
tile yields (0,0); include the smallest FoV extents to reflect actual coverage.
After calculating xmin/xmax and ymin/ymax from self.tile_map (or when there is
only one tile), call self._guess_smallest_fov() (handle ValueError as in
STANDARD mode) and add the FoV width to (xmax-xmin) and FoV height to
(ymax-ymin) before calling self.totalArea._set_value(..., force_write=True);
keep existing use of self.tile_map, self._guess_smallest_fov, and
self.totalArea._set_value to locate where to change.
- Around line 389-418: The duration estimate can go negative when
TileMode.CUSTOM is selected and tile_map is empty because task._number_of_tiles
becomes 0 before calling TiledAcquisitionTask. Add a guard after computing task
(or before setting task._number_of_tiles) that detects TileMode.CUSTOM and an
empty self.tile_map and short‑circuits: either set task._number_of_tiles to a
safe minimum (e.g., 1) or set self.expectedDuration to a safe default and return
early; update the block that references tile_mode, TileMode.CUSTOM,
self.tile_map, task._number_of_tiles and the subsequent call to
task.estimateTime() accordingly.
- Around line 1171-1179: Replace the lambda assignment for z_interpolator with a
proper function definition: instead of "z_interpolator = lambda x, y:
near_interp(x, y)" define "def z_interpolator(x, y): return near_interp(x, y)".
Update the block where NearestNDInterpolator is constructed (references:
NearestNDInterpolator, near_interp, z_interpolator, known_points, known_z) so
the new def has the same (x, y) signature and returns the interpolator result;
no other behavior changes are needed.
- Around line 317-346: The _on_tile_file_change method should early-return when
filepath is falsy to avoid FileNotFoundError noise and include a proper
docstring and return type hint; update the signature to include -> None, replace
the empty docstring with a short one describing parameters and return type
(e.g., "Handle updates when tile file changes: filepath: str -> None"), and add
an initial guard like "if not filepath: return" before clearing or opening the
file so tile_map population and subsequent calls to _update_total_area,
_update_exp_dur, and _memory_check only run for a valid path.
- Around line 294-316: Add explicit type hints and a reST-style docstring to the
_on_tile_mode_change method: annotate the parameter as mode: str and the return
type as -> None, and add a short reST docstring immediately under the def that
describes the purpose, the mode parameter (str, the tile mode name), and that it
returns None. Keep the existing logic unchanged and place the docstring in reST
format (":param mode: ..." and ":return: None") so static type checkers and
documentation tools can pick it up.
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 657-673: The overlay acquisition time is only added once but
`_getTileDAs` invokes the overlay stream for each tile; update the overlay
calculation so that overlay_time reflects per-tile cost by multiplying the
single-tile estimate (from self._overlay_stream.estimateAcquisitionTime()) by
the number of tiles (`remaining`). Modify the block that sets overlay_time
(referencing _overlay_stream and estimateAcquisitionTime) to compute
overlay_time = self._overlay_stream.estimateAcquisitionTime() * remaining (when
_overlay_stream is not None), leaving the rest of the method (move_time,
logging, return) unchanged.
🧹 Nitpick comments (2)
src/odemis/acq/acqmng.py (1)
619-621: Avoid quadratic list concatenation when flatteningraw_images.Line 620 uses
sum(..., []), which is O(n²) for lists. A linear flatten avoids avoidable overhead, especially with many tiles.♻️ Suggested change
+from itertools import chain ... - ret = sum(raw_images.values(), []) + ret = list(chain.from_iterable(raw_images.values()))plugins/tileacq.py (1)
176-191: Shut down the single‑thread executor on plugin termination.
self._executoris created but never closed, which can leave a worker thread alive after the plugin is stopped or reloaded.♻️ Suggested cleanup hook
+ def terminate(self) -> None: + """Release resources owned by the plugin.""" + if self._executor: + self._executor.shutdown(wait=False, cancel_futures=True)
Comment on lines
+84
to
93
| class TileMode(Enum): | ||
| STANDARD = "Standard (X × Y grid)" | ||
| CUSTOM = "Custom (from tsv file with mirror alignment)" | ||
|
|
||
|
|
||
| class TileColumnNames(Enum): | ||
| NUMBER = "Tile Number" | ||
| POSX = "Stage Position.X [m]" | ||
| POSY = "Stage Position.Y [m]" | ||
|
|
There was a problem hiding this comment.
Add reST docstrings for the new enums.
TileMode and TileColumnNames are public and should include class docstrings for generated docs.
As per coding guidelines: Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 85-85: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 84 - 93, Add reST-style class docstrings for
the public enums TileMode and TileColumnNames to satisfy documentation
guidelines: for TileMode describe the purpose of the enum (controls tiling mode,
with values STANDARD and CUSTOM and brief notes on grid vs TSV/mirror alignment)
and for TileColumnNames describe that it lists expected TSV/CSV column names
(Tile Number, Stage Position.X [m], Stage Position.Y [m]) and their meaning;
place the docstrings immediately under the class definitions, written in
reStructuredText style (short summary line, optional one-line blank, then a
short explanatory sentence) and do not include type annotations.
Comment on lines
294
to
316
| def _on_tile_mode_change(self, mode): | ||
| """Show/hide relevant controls based on tile mode""" | ||
| if not self._dlg: | ||
| return | ||
|
|
||
| if mode == TileMode.CUSTOM.name: | ||
| self.overlap.value = 0 | ||
|
|
||
| for entry in self._dlg.setting_controller.entries: | ||
| if hasattr(entry, "vigilattr"): | ||
| # Show grid controls only in standard mode | ||
| if entry.vigilattr in (self.nx, self.ny, self.overlap): | ||
| entry.lbl_ctrl.Show(mode == TileMode.STANDARD.name) | ||
| entry.value_ctrl.Show(mode == TileMode.STANDARD.name) | ||
| # Show file control only in custom mode | ||
| if entry.vigilattr is self.tiles_file: | ||
| entry.lbl_ctrl.Show(mode == TileMode.CUSTOM.name) | ||
| entry.value_ctrl.Show(mode == TileMode.CUSTOM.name) | ||
| self._update_total_area(None) | ||
| self._update_exp_dur(None) | ||
| self._memory_check(None) | ||
| wx.CallAfter(self._dlg.setting_controller.panel.Layout) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n plugins/tileacq.py | sed -n '290,320p'Repository: delmic/odemis
Length of output: 1522
🏁 Script executed:
rg "_on_tile_mode_change" plugins/tileacq.pyRepository: delmic/odemis
Length of output: 158
🏁 Script executed:
rg "tile_mode\s*=" plugins/tileacq.py | head -20Repository: delmic/odemis
Length of output: 151
🏁 Script executed:
rg "class TileMode|enum TileMode" plugins/tileacq.pyRepository: delmic/odemis
Length of output: 78
🏁 Script executed:
rg "class TileMode" plugins/tileacq.py -A 5Repository: delmic/odemis
Length of output: 208
🏁 Script executed:
rg "def _on_" plugins/tileacq.py | head -10Repository: delmic/odemis
Length of output: 232
Add type hints and a reST docstring for _on_tile_mode_change.
This handler is unannotated despite being a subscription callback that receives the tile mode string value. The method requires type hints for the mode parameter (string) and return type (None), plus a properly formatted reST docstring.
✅ Suggested signature/docstring update
- def _on_tile_mode_change(self, mode):
- """Show/hide relevant controls based on tile mode"""
+ def _on_tile_mode_change(self, mode: str) -> None:
+ """Show/hide relevant controls based on tile mode.
+
+ :param mode: Selected tile mode name.
+ """🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 294 - 316, Add explicit type hints and a
reST-style docstring to the _on_tile_mode_change method: annotate the parameter
as mode: str and the return type as -> None, and add a short reST docstring
immediately under the def that describes the purpose, the mode parameter (str,
the tile mode name), and that it returns None. Keep the existing logic unchanged
and place the docstring in reST format (":param mode: ..." and ":return: None")
so static type checkers and documentation tools can pick it up.
Comment on lines
+317
to
+346
| def _on_tile_file_change(self, filepath: str): | ||
| """ | ||
| """ | ||
| self.tile_map.clear() | ||
| try: | ||
| with open(filepath, 'r', encoding='utf-8-sig', newline='') as f: | ||
| reader = csv.DictReader(f, delimiter='\t') | ||
| for row in reader: | ||
| logging.debug(row) | ||
| # Skip empty rows | ||
| if not row: | ||
| continue | ||
|
|
||
| try: | ||
| # Parse the columns | ||
| tile_num = int(row[TileColumnNames.NUMBER.value].strip()) | ||
| x = float(row[TileColumnNames.POSX.value].strip()) | ||
| y = float(row[TileColumnNames.POSY.value].strip()) | ||
|
|
||
| self.main_app.main_data.stage._checkMoveAbs({"x": x, "y" : y}) | ||
| self.tile_map[tile_num] = (x, y) | ||
|
|
||
| except (ValueError, KeyError): | ||
| logging.debug(f"Not able to process row {row}") | ||
| continue | ||
| self._update_total_area(None) | ||
| self._update_exp_dur(None) | ||
| self._memory_check(None) | ||
| except Exception: | ||
| logging.exception("Failed to load tile positions from %s", filepath) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
find . -name "tileacq.py" -type fRepository: delmic/odemis
Length of output: 77
🏁 Script executed:
# Read the file at the specified lines with context
cat -n plugins/tileacq.py | sed -n '310,350p'Repository: delmic/odemis
Length of output: 2052
🏁 Script executed:
# Check the full method and any calling code
cat -n plugins/tileacq.py | sed -n '315,350p'Repository: delmic/odemis
Length of output: 1769
🏁 Script executed:
# Search for any guards around filepath or tiles_file
rg "tiles_file|filepath" plugins/tileacq.py -B 2 -A 2Repository: delmic/odemis
Length of output: 1620
🏁 Script executed:
python3 <<'EOF'
try:
with open("", 'r') as f:
pass
except Exception as e:
print(f"Error type: {type(e).__name__}")
print(f"Error message: {e}")
EOFRepository: delmic/odemis
Length of output: 141
Add early return guard and complete the method docstring with proper type hints.
The method raises FileNotFoundError when filepath is an empty string (initial state), causing unnecessary log noise. Add an early return guard and fix the empty docstring. The method also lacks a return type hint, which is required by the coding guidelines.
Suggested fix
- def _on_tile_file_change(self, filepath: str):
- """
- """
+ def _on_tile_file_change(self, filepath: str) -> None:
+ """Load tile positions from a TSV file.
+
+ :param filepath: Path to the TSV file.
+ """
self.tile_map.clear()
+ if not filepath:
+ return
try:
with open(filepath, 'r', encoding='utf-8-sig', newline='') as f:🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 317 - 346, The _on_tile_file_change method
should early-return when filepath is falsy to avoid FileNotFoundError noise and
include a proper docstring and return type hint; update the signature to include
-> None, replace the empty docstring with a short one describing parameters and
return type (e.g., "Handle updates when tile file changes: filepath: str ->
None"), and add an initial guard like "if not filepath: return" before clearing
or opening the file so tile_map population and subsequent calls to
_update_total_area, _update_exp_dur, and _memory_check only run for a valid
path.
Comment on lines
389
to
418
| try: | ||
| stitch_ss = self._get_stitch_streams() | ||
| if not stitch_ss: | ||
| self.expectedDuration._set_value(1, force_write=True) | ||
| return | ||
|
|
||
| # Calculate bounding box of the acquisition region | ||
| region = self._get_region(self.main_app.main_data.stage.position.value) | ||
| if region is None: | ||
| return | ||
|
|
||
| overlap_frac = self.overlap.value / 100 | ||
| overlay_stream = None | ||
| if self.fineAlign.value and self._can_fine_align(stitch_ss): | ||
| overlay_stream = self._ovrl_stream | ||
|
|
||
| task = TiledAcquisitionTask( | ||
| stitch_ss, | ||
| self.main_app.main_data.stage, | ||
| region, | ||
| overlap=overlap_frac, | ||
| settings_obs=self.main_app.main_data.settings_obs, | ||
| weaver=self.weaver.value if self.stitch.value else None, | ||
| registrar=self.registrar if self.stitch.value else None, | ||
| overlay_stream=overlay_stream, | ||
| sfov=self._guess_smallest_fov(), | ||
| ) | ||
| if self.tile_mode.value == TileMode.CUSTOM.name: | ||
| task._number_of_tiles = len(self.tile_map) | ||
| tat = task.estimateTime() |
There was a problem hiding this comment.
Handle empty custom tile maps in duration estimate.
When tile_map is empty, _number_of_tiles becomes 0 and estimateTime() can go negative (Line 416). Short‑circuit to a safe default.
🧭 Suggested guard
stitch_ss = self._get_stitch_streams()
if not stitch_ss:
self.expectedDuration._set_value(1, force_write=True)
return
+ if self.tile_mode.value == TileMode.CUSTOM.name and not self.tile_map:
+ self.expectedDuration._set_value(1, force_write=True)
+ return🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 389 - 418, The duration estimate can go
negative when TileMode.CUSTOM is selected and tile_map is empty because
task._number_of_tiles becomes 0 before calling TiledAcquisitionTask. Add a guard
after computing task (or before setting task._number_of_tiles) that detects
TileMode.CUSTOM and an empty self.tile_map and short‑circuits: either set
task._number_of_tiles to a safe minimum (e.g., 1) or set self.expectedDuration
to a safe default and return early; update the block that references tile_mode,
TileMode.CUSTOM, self.tile_map, task._number_of_tiles and the subsequent call to
task.estimateTime() accordingly.
Comment on lines
798
to
826
| def _perform_z_survey_point( | ||
| self, | ||
| tile_pos: Dict, | ||
| ) -> model.ProgressiveFuture: | ||
| """ | ||
| return (int): the number of pixels the stream will generate during an | ||
| acquisition | ||
| Starts a ParabolicMirrorAlignmentTask that performs a multi-stage optimization | ||
| (coarse per-axis scans followed by Nelder-Mead refinements) to align the | ||
| mirror and stage based on camera measurements for SPARC. | ||
|
|
||
| :param axes: List of alignment axes to optimize of len 1 or 3. | ||
| :param ccd: Camera component used to measure the spot. | ||
| :param search_range: Initial half-range (in metres) for each | ||
| coarse search around the current position. Defaults to 50e-6. | ||
| :param max_iter: Maximum number of optimizer iterations. | ||
| This value is passed to the underlying scipy | ||
| optimizers to limit work. Defaults to 100. | ||
| :param stop_early: When True the task attempts to stop the Nelder-Mead stages early | ||
| if progress stalls. Set to False to force the optimizers to run | ||
| until their normal convergence criteria or the supplied max_iter | ||
| budget is exhausted. | ||
| :param save_images: When True, save alignment step images. | ||
| :returns: model.ProgressiveFuture | ||
| A future representing the background alignment task. The future's | ||
| task_canceller is set so callers (or GUI) can cancel the running task. | ||
| Any exception raised during alignment will be propagated when calling | ||
| returned_future.result(). It has additional attributes: | ||
| - n_steps: Total number of optimization steps performed. | ||
| - current_step: Current optimization step. | ||
| """ |
There was a problem hiding this comment.
Docstring doesn’t match the actual parameters.
The docstring describes axes/ccd/search_range, but the method only accepts tile_pos. Please update it to reflect the real inputs/outputs to avoid misuse.
🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 798 - 826, The _perform_z_survey_point
docstring is out-of-sync: remove the stale parameters (axes, ccd, search_range,
max_iter, stop_early, save_images) and replace with a concise description of the
actual input and output — document the single parameter tile_pos (its expected
type Dict and the keys/values it must contain, e.g., tile coordinates, stage z
setpoint, camera exposure metadata if used) and clearly state the return type
model.ProgressiveFuture and its extra attributes (n_steps, current_step,
task_canceller and exception propagation behavior). Also update the short
summary line to describe that this starts a Z-survey alignment task for the
given tile_pos and mention any side effects (saving images/logging) that callers
must know.
plugins/tileacq.py
Outdated
Comment on lines
940
to
954
| def save_tile(tile_path, da): | ||
| exporter = dataio.find_fittest_converter(tile_path) | ||
| logging.debug("Will save data of tile %s", tile_path) | ||
| exporter.export(tile_path, da) | ||
|
|
||
| ft.running_subf = acqmng.acquire(ss, self.main_app.main_data.settings_obs) | ||
| das, e = ft.running_subf.result() # blocks until all the acquisitions are finished | ||
| def do_tile_acq(f_tile_pos, tile_acq_task, tile_path, da_list): | ||
| f_tile_pos.result() # Wait for z-align to complete | ||
| try: | ||
| das, e = tile_acq_task.run() | ||
| if e: | ||
| logging.warning("Acquisition for tile %dx%d partially failed: %s", | ||
| ix, iy, e) | ||
| logging.warning(f"Acquisition for tile {tile_path} partially failed: {e}") | ||
| da = das[0] # return first da | ||
| da_list.append(da) | ||
| threading.Thread(target=save_tile, args=(tile_path, da)).start() | ||
| except IndexError: |
There was a problem hiding this comment.
Avoid fire‑and‑forget tile‑saving threads.
The background Thread(...).start() can race with later showAcquisition() calls and can hide export failures. Consider saving synchronously or tracking threads and joining before display.
🧵 Safer (synchronous) save
- threading.Thread(target=save_tile, args=(tile_path, da)).start()
+ save_tile(tile_path, da)🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 940 - 954, The code spawns detached threads
in do_tile_acq via threading.Thread(...).start() calling save_tile, which can
race with showAcquisition and hide export failures; change this to perform the
save synchronously (call save_tile(tile_path, da) directly) or store the Thread
objects and join them before any code that displays results (e.g., before
showAcquisition). Update do_tile_acq to either call save_tile synchronously
after appending da to da_list, or collect threads in a list (e.g.,
tile_save_threads) and ensure they are joined at the point where showAcquisition
is invoked; reference functions save_tile and do_tile_acq and the
tile_acq_task/da_list flow when making the change.
Comment on lines
+657
to
+673
| # 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.
Scale overlay acquisition time per tile in estimateTime.
Line 657-661 adds overlay time only once, but _getTileDAs acquires the overlay stream per tile. This underestimates progress for multi‑tile runs.
🔧 Suggested fix
- overlay_time = 0
- if self._overlay_stream is not None:
- overlay_time = self._overlay_stream.estimateAcquisitionTime()
+ overlay_time = 0
+ if self._overlay_stream is not None:
+ overlay_time = self._overlay_stream.estimateAcquisitionTime() * remaining📝 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.
Suggested change
| # 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 | |
| # Estimate overlay stream acquisition time | |
| overlay_time = 0 | |
| if self._overlay_stream is not None: | |
| overlay_time = self._overlay_stream.estimateAcquisitionTime() * remaining | |
| 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._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 + overlay_time |
🤖 Prompt for AI Agents
In `@src/odemis/acq/stitching/_tiledacq.py` around lines 657 - 673, The overlay
acquisition time is only added once but `_getTileDAs` invokes the overlay stream
for each tile; update the overlay calculation so that overlay_time reflects
per-tile cost by multiplying the single-tile estimate (from
self._overlay_stream.estimateAcquisitionTime()) by the number of tiles
(`remaining`). Modify the block that sets overlay_time (referencing
_overlay_stream and estimateAcquisitionTime) to compute overlay_time =
self._overlay_stream.estimateAcquisitionTime() * remaining (when _overlay_stream
is not None), leaving the rest of the method (move_time, logging, return)
unchanged.
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow-v2
branch
from
21c492a to
18ca94e
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/odemis/acq/stitching/_tiledacq.py (4)
732-780:⚠️ Potential issue | 🟠 Major
adjust_metadataexpects lists; currently passingDataArraybreaks overlay handling.Line 760 stores a single
DataArrayper stream, butAcquisitionTask.adjust_metadata()expectslist[DataArray]and assumes theOverlayStreamcontains two entries (data[0]/data[1]). This can crash or silently use the wrong metadata.Suggested fix
- def _acquireStreamTile(self, i, ix, iy, stream): + def _acquireStreamTile(self, i, ix, iy, stream, return_all: bool = False): @@ - return das[0] # return first da + return das if return_all else das[0] # return all if requested @@ - raw_images = OrderedDict() # stream -> list of raw images + 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 + das = self._acquireStreamTile(i, ix, iy, self._overlay_stream, return_all=True) + raw_images[self._overlay_stream] = list(das) @@ - raw_images[stream] = da + raw_images[stream] = [da] @@ - return list(raw_images.values()) + return [da for das in raw_images.values() for da in das]
288-317:⚠️ Potential issue | 🟠 MajorAdd type hints and reST docstrings for
getFovandguessSmallestFov.These methods (lines 288-317) lack parameter and return type annotations, and their docstrings embed type information. Add type hints and convert docstrings to reST field lists without type details.
Suggested fix
`@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 + :param sd: Stream or acquired data to estimate FoV. + :return: Width and height in meters. """ @@ `@classmethod` - def guessSmallestFov(cls, ss): + def guessSmallestFov(cls, ss: List[Stream]) -> Tuple[float, float]:
1002-1030:⚠️ Potential issue | 🟠 MajorAdd type hints and update docstring for
stitchTilesto follow reST style.The function lacks type hints on parameters and return type, and the docstring embeds type information. Per coding guidelines, add type hints and update the docstring to follow reST format without type information.
Suggested fix
`@staticmethod` - def stitchTiles(da_list, registrar, weaver): + def stitchTiles( + da_list: List[Union[DataArray, Tuple[DataArray, ...]]], + registrar: int, + weaver: int, + ) -> List[DataArray]: """ - Stitch the acquired tiles to create a complete view of the required total area - :return: (list of DataArrays): a stitched data for each stream acquisition + Stitch the acquired tiles to create a complete view of the required total area. + :return: Stitched data for each stream acquisition. """
1100-1121:⚠️ Potential issue | 🟠 MajorAdd type hints and convert docstring to reST field-style format for
acquireTiledArea.The function signature lacks type annotations and the docstring is not in proper reST format. Add type hints to all parameters and the return type, then convert the docstring to use
:param name: descriptionfield-style (without type information in the docstring itself, per the style guide).Suggested signature and docstring structure
-def acquireTiledArea(streams, stage, area, overlap=0.2, settings_obs=None, log_path=None, zlevels=None, - registrar=REGISTER_GLOBAL_SHIFT, weaver=WEAVER_MEAN, focusing_method=FocusingMethod.NONE, - focus_points=None, focus_range=None, centered_acq=True, overlay_stream=None, sfov=None): +def acquireTiledArea( + streams: List[Stream], + stage: object, + area: Union[Tuple[float, float, float, float], List[Tuple[float, float]]], + overlap: float = 0.2, + settings_obs: Optional[object] = None, + log_path: Optional[str] = None, + zlevels: Optional[List[float]] = None, + registrar: Optional[int] = REGISTER_GLOBAL_SHIFT, + weaver: Optional[int] = WEAVER_MEAN, + focusing_method: FocusingMethod = FocusingMethod.NONE, + focus_points: Optional[List[Tuple[float, float, float]]] = None, + focus_range: Optional[Tuple[float, float]] = None, + centered_acq: bool = True, + overlay_stream: Optional[Stream] = None, + sfov: Optional[Tuple[float, float]] = None, +) -> model.ProgressiveFuture: """ - Start a tiled acquisition task for the given streams (SEM or FM) in order to - build a complete view of the TEM grid. Needed tiles are first acquired for - each stream, then the complete view is created by stitching the tiles. + Start a tiled acquisition task for the given streams (SEM or FM) in order to build a complete view of the TEM grid. + + Needed tiles are first acquired for each stream, then the complete view is created by stitching the tiles. - Parameters are the same as for TiledAcquisitionTask + :param streams: the streams to acquire + :param stage: the sample stage to move to the possible tiles locations + :param area: Bounding box as (xmin, ymin, xmax, ymax) or list of (x, y) points defining a polygon + :param overlap: the amount of overlap between each acquisition + :param settings_obs: class that contains a list of all VAs that should be saved as metadata + :param log_path: directory and filename pattern to save acquired images for debugging + :param zlevels: focus z positions required for zstack acquisition + :param registrar: type of registration method; if registrar and weaver are None, do not stitch + :param weaver: type of weaving method; if registrar and weaver are None, do not stitch + :param focusing_method: Defines when will the autofocuser be run + :param focus_points: list of focus points corresponding to the known (x, y, z) at good focus + :param focus_range: tuple defining the focus range for good focus + :param centered_acq: If True, center the acquisition area on the given region; any extra area is added symmetrically + :param overlay_stream: If provided, the overlay stream will be used for metadata adjustment after acquisition + :param sfov: If provided, use this as the smallest field of view :return: an object that represents the task, allow to know how much time before it is over and to cancel it. It also permits to receive the result of the task, which is a list of model.DataArray: the stitched acquired tiles data """src/odemis/acq/acqmng.py (1)
623-631: 🛠️ Refactor suggestion | 🟠 MajorAdd type hints and convert docstring to reST format for
adjust_metadata.The method at lines 624–631 is missing type hints on the parameter and return type, and the docstring uses a non-standard format instead of reStructuredText fields. Per coding guidelines, all functions must include type hints and reST-style docstrings.
Suggested fix
-from typing import Set, Dict +from typing import Dict, List, Set @@ - def adjust_metadata(raw_data): - """ - 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. - """ + def adjust_metadata(raw_data: Dict[object, List[model.DataArray]]) -> None: + """Update/adjust the metadata of the raw data received based on global information. + + :param raw_data: Raw data for each stream (mutated in place). + """src/odemis/gui/cont/stream_bar.py (1)
485-501: 🛠️ Refactor suggestion | 🟠 MajorAdd type hints and convert to reStructuredText docstring format for
_add_stream.The method signature (line 485) lacks required type hints for all parameters and return type. The docstring embeds types inline using Args/Kwargs format instead of following reStructuredText style without type information.
Suggested fix
- def _add_stream(self, stream, add_to_view=False, visible=True, play=None, stream_cont_cls=StreamController, - sp_options=None): - """ Add the given stream to the tab data model and appropriate views + def _add_stream( + self, + stream: acqstream.Stream, + add_to_view: bool | "acqstream.Stream" = False, + visible: bool = True, + play: bool | None = None, + stream_cont_cls: type[StreamController] = StreamController, + sp_options: int | None = None, + ) -> StreamController | acqstream.Stream: + """Add the given stream to the tab data model and appropriate views. - Args: - stream (Stream): the new stream to add + :param stream: the new stream to add + :param add_to_view: if True, add the stream to all the compatible views, + if False add to the current view, otherwise, add to the given view + :param visible: if True, create a stream entry, otherwise add the stream + without creating any entry + :param play: if True immediately start it, if False let it stopped, and if + None, only play if already a stream is playing + :param stream_cont_cls: the stream controller class + :param sp_options: StreamPanel options bitmask or None for default + :return: the stream controller or stream (if visible is False) that was created
🤖 Fix all issues with AI agents
In `@plugins/tileacq.py`:
- Around line 754-796: The docstring for _generate_triangulation_points should
be converted to reStructuredText: replace the current descriptive/bullet
docstring with a reST docstring that documents parameters and return value using
:param max_dist: (float) maximum distance/step for grid generation and :return:
List[Tuple[float, float]] of generated survey points (centroid for Point/Line or
grid points strictly inside Polygon), and include a brief one-line summary; keep
the implementation unchanged and ensure the docstring appears immediately above
the def _generate_triangulation_points(...) declaration.
- Around line 370-374: The handler _on_weaver_change currently lacks a type
annotation and docstring; update its signature to accept an integer (weaver:
int) because WEAVER_COLLAGE and related constants are integers, and add a
one-line docstring describing that it updates self.registrar based on the weaver
value; keep the existing logic that sets self.registrar to REGISTER_IDENTITY
when weaver == WEAVER_COLLAGE and to REGISTER_GLOBAL_SHIFT otherwise, and
reference the symbols _on_weaver_change, WEAVER_COLLAGE, REGISTER_IDENTITY, and
REGISTER_GLOBAL_SHIFT when making the change.
- Around line 989-1020: The _custom_tiled_acquisition docstring incorrectly
describes a ParabolicMirrorAlignmentTask and lists unrelated parameters; also
the parameters streams, tile_path and da_list lack type hints. Update the
docstring of _custom_tiled_acquisition to describe its actual behavior (per-tile
Z alignment during tiled acquisition, what tile_pos and streams represent,
side-effects like saved images or metadata, and the returned
model.ProgressiveFuture details) and remove the irrelevant parameters (axes,
ccd, search_range, max_iter, stop_early, save_images). Add precise type
annotations to the signature: e.g. streams: Iterable[Any] (or a more specific
Stream type if available), tile_path: str | pathlib.Path, da_list:
Sequence[Mapping[str, Any]] (or List[DataAcq] if a concrete type exists), and
import typing/pathlib as needed; keep the return type as model.ProgressiveFuture
and reference the same symbols (_custom_tiled_acquisition, tile_pos, streams,
tile_path, da_list, model.ProgressiveFuture) so reviewers can locate the
changes.
- Around line 890-922: The docstring for _custom_tiled_target_z_acquisition is
copy-pasted and contains wrong parameters (axes, ccd, search_range, etc.) and
the function parameters lack type hints; update the signature to add precise
type hints for tile_pos, streams, tile_path, da_list, and target_z (e.g.,
Dict/Mapping for tile_pos, an iterable/Sequence or specific Stream type for
streams, str/Path for tile_path, List/Sequence for da_list, and float or
Optional[float] for target_z) and keep the return type model.ProgressiveFuture,
then replace the docstring with a concise description that documents the actual
parameters (tile_pos, streams, tile_path, da_list, target_z), return value,
raised exceptions and any future attributes (n_steps, current_step,
task_canceller) while removing all references to ParabolicMirrorAlignmentTask
and the unrelated axes/ccd/search_range parameters so the docstring matches the
implementation.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 565-575: The _add_stream_cont method always forwards sp_options to
the constructor (cls(...)), but FastEMStreamController.__init__ does not accept
sp_options, causing a TypeError when sp_options is not None; update
FastEMStreamController.__init__ to accept an sp_options parameter (or add
**kwargs) so its signature matches StreamController, or alternatively modify
_add_stream_cont to only pass sp_options when it is not None—prefer changing
FastEMStreamController.__init__ to add sp_options or **kwargs to maintain
interface compatibility with StreamController.
In `@src/odemis/gui/cont/stream.py`:
- Around line 90-96: The code assumes stream.raw has at least one element when
checking spectrum shape; guard access before indexing to avoid IndexError. In
the block that sets sp_options (the branch using stream,
acqstream.SpectrumStream, OPT_FIT_RGB, OPT_BTN_TINT and OPT_NO_COLORMAPS) add a
defensive check that stream.raw is present and non-empty (e.g., hasattr/getattr
and len(stream.raw) > 0) before evaluating stream.raw[0].shape[0] > 1; only set
OPT_FIT_RGB when that guarded condition is true.
- Around line 69-78: StreamController.__init__ currently lacks type annotations
and uses a non-reST docstring; add type hints for all parameters and the return
type (-> None) and import Optional from typing if missing, e.g. annotate
stream_bar, stream, tab_data_model, show_panel: bool, view:
Optional[MicroscopeView], sb_ctrl: Optional[StreamBarController], sp_options:
Optional[int]; then replace the existing docstring with reStructuredText
field-list style using :param name: and :return: entries (do not embed types in
the docstring) and keep the explanatory text for view/ sb_ctrl/ sp_options
within those :param: descriptions.
🧹 Nitpick comments (1)
src/odemis/acq/acqmng.py (1)
619-620: Avoid quadratic list concatenation when flatteningraw_images.Line 620 uses
sum(..., []), which is O(n²) for many lists. A list comprehension keeps it linear.Suggested fix
- ret = sum(raw_images.values(), []) + ret = [da for das in raw_images.values() for da in das]
Comment on lines
370
to
374
| def _on_weaver_change(self, weaver): | ||
| if weaver == WEAVER_COLLAGE: | ||
| self.registrar = REGISTER_IDENTITY | ||
| else: | ||
| self.registrar = REGISTER_GLOBAL_SHIFT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n plugins/tileacq.py | sed -n '365,380p'Repository: delmic/odemis
Length of output: 684
🏁 Script executed:
rg "WEAVER_COLLAGE|WEAVER_" plugins/tileacq.py -B 2 -A 2 | head -30Repository: delmic/odemis
Length of output: 1050
🏁 Script executed:
head -50 plugins/tileacq.py | cat -nRepository: delmic/odemis
Length of output: 1911
🏁 Script executed:
rg "_on_weaver_change" plugins/tileacq.py -B 3 -A 3Repository: delmic/odemis
Length of output: 497
🏁 Script executed:
rg "WEAVER_COLLAGE|WEAVER_MEAN|WEAVER_COLLAGE_REVERSE" odemis/acq/stitching.py | head -20Repository: delmic/odemis
Length of output: 120
🏁 Script executed:
find . -name "model.py" -o -name "model" -type d | head -5Repository: delmic/odemis
Length of output: 98
🏁 Script executed:
rg "class VAEnumerated" --type py -A 15Repository: delmic/odemis
Length of output: 1180
🏁 Script executed:
rg "WEAVER_COLLAGE\s*=" --type pyRepository: delmic/odemis
Length of output: 114
🏁 Script executed:
rg "def subscribe" src/odemis/model/_vattributes.py -A 10 | head -30Repository: delmic/odemis
Length of output: 1143
🏁 Script executed:
cat src/odemis/acq/stitching/_constants.pyRepository: delmic/odemis
Length of output: 838
Correct the type hint for _on_weaver_change: the weaver parameter is an integer, not a string.
This handler is unannotated and undocumented. The suggested type hint should be weaver: int since WEAVER_COLLAGE and related constants are integers.
✏️ Corrected signature/docstring
- def _on_weaver_change(self, weaver):
+ def _on_weaver_change(self, weaver: int) -> None:
+ """Update registrar based on the selected weaver.
+
+ :param weaver: Weaver type key.
+ :return: Nothing.
+ """
if weaver == WEAVER_COLLAGE:
self.registrar = REGISTER_IDENTITY🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 370 - 374, The handler _on_weaver_change
currently lacks a type annotation and docstring; update its signature to accept
an integer (weaver: int) because WEAVER_COLLAGE and related constants are
integers, and add a one-line docstring describing that it updates self.registrar
based on the weaver value; keep the existing logic that sets self.registrar to
REGISTER_IDENTITY when weaver == WEAVER_COLLAGE and to REGISTER_GLOBAL_SHIFT
otherwise, and reference the symbols _on_weaver_change, WEAVER_COLLAGE,
REGISTER_IDENTITY, and REGISTER_GLOBAL_SHIFT when making the change.
Comment on lines
754
to
796
| def _generate_triangulation_points(self, max_dist: float) -> List[Tuple[float, float]]: | ||
| """ | ||
| Checks the fov based on the data arrays. | ||
| das: list of DataArryas | ||
| sfov: previous estimate for the fov | ||
| Generates survey points for the Z-Map. | ||
| - If shape is a Point or Line (Area=0): Returns the Centroid. | ||
| - If shape is a Polygon (Area>0): Returns grid points strictly inside the polygon. | ||
| """ | ||
| afovs = [self._get_fov(d) for d in das] | ||
| asfov = (min(f[1] for f in afovs), | ||
| min(f[0] for f in afovs)) | ||
| if not all(util.almost_equal(e, a) for e, a in zip(sfov, asfov)): | ||
| logging.warning("Unexpected min FoV = %s, instead of %s", asfov, sfov) | ||
| sfov = asfov | ||
| return sfov | ||
| if not self.tile_map: | ||
| return [] | ||
|
|
||
| def _estimateStreamPixels(self, s): | ||
| coords = list(self.tile_map.values()) | ||
|
|
||
| # Convex Hull wraps the points. | ||
| # It returns a Point, LineString, or Polygon. | ||
| geom = MultiPoint(coords).convex_hull | ||
|
|
||
| # CASE 1: Point or Line (Area is 0) | ||
| # As requested: just return the midpoint. | ||
| if geom.area == 0: | ||
| return [(geom.centroid.x, geom.centroid.y)] | ||
|
|
||
| # CASE 2: Valid Polygon (Area > 0) | ||
| # Generate the grid strictly inside. | ||
| minx, miny, maxx, maxy = geom.bounds | ||
|
|
||
| # Use arange for fixed steps. | ||
| x_arr = numpy.arange(minx, maxx, max_dist) | ||
| y_arr = numpy.arange(miny, maxy, max_dist) | ||
|
|
||
| valid_points = [] | ||
| for px in x_arr: | ||
| for py in y_arr: | ||
| # .contains is strict (points on the edge are False). | ||
| # Use .intersects or buffer if you want edge points, | ||
| # but usually strict inside is safer for microscope limits. | ||
| if geom.contains(Point(px, py)): | ||
| valid_points.append((px, py)) | ||
|
|
||
| # Fallback: If the grid was too coarse and missed the polygon entirely | ||
| # (e.g., a thin diagonal polygon between grid points), add the centroid. | ||
| if not valid_points: | ||
| valid_points.append((geom.centroid.x, geom.centroid.y)) | ||
|
|
||
| return valid_points |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
find . -name "tileacq.py" -type fRepository: delmic/odemis
Length of output: 77
🏁 Script executed:
# Read the specific lines from the file
sed -n '754,796p' ./plugins/tileacq.pyRepository: delmic/odemis
Length of output: 1755
Use a reST docstring for _generate_triangulation_points.
The current docstring uses descriptive text with bullet points instead of following the reStructuredText style guide. Convert to reST format with :param: and :return: tags as shown:
✏️ Suggested docstring
- """
- Generates survey points for the Z-Map.
- - If shape is a Point or Line (Area=0): Returns the Centroid.
- - If shape is a Polygon (Area>0): Returns grid points strictly inside the polygon.
- """
+ """Generate survey points for the Z-Map.
+
+ :param max_dist: Maximum spacing between grid points.
+ :return: Survey point coordinates inside the convex hull.
+ """🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 754 - 796, The docstring for
_generate_triangulation_points should be converted to reStructuredText: replace
the current descriptive/bullet docstring with a reST docstring that documents
parameters and return value using :param max_dist: (float) maximum distance/step
for grid generation and :return: List[Tuple[float, float]] of generated survey
points (centroid for Point/Line or grid points strictly inside Polygon), and
include a brief one-line summary; keep the implementation unchanged and ensure
the docstring appears immediately above the def
_generate_triangulation_points(...) declaration.
plugins/tileacq.py
Outdated
Comment on lines
890
to
922
| def _custom_tiled_target_z_acquisition( | ||
| self, | ||
| tile_pos: Dict, | ||
| streams, | ||
| tile_path, | ||
| da_list, | ||
| target_z, | ||
| ) -> model.ProgressiveFuture: | ||
| """ | ||
| Makes an estimate for the amount of memory that will be consumed during | ||
| stitching and compares it to the available memory on the computer. | ||
| Displays a warning if memory exceeds available memory. | ||
| Starts a ParabolicMirrorAlignmentTask that performs a multi-stage optimization | ||
| (coarse per-axis scans followed by Nelder-Mead refinements) to align the | ||
| mirror and stage based on camera measurements for SPARC. | ||
|
|
||
| :param axes: List of alignment axes to optimize of len 1 or 3. | ||
| :param ccd: Camera component used to measure the spot. | ||
| :param search_range: Initial half-range (in metres) for each | ||
| coarse search around the current position. Defaults to 50e-6. | ||
| :param max_iter: Maximum number of optimizer iterations. | ||
| This value is passed to the underlying scipy | ||
| optimizers to limit work. Defaults to 100. | ||
| :param stop_early: When True the task attempts to stop the Nelder-Mead stages early | ||
| if progress stalls. Set to False to force the optimizers to run | ||
| until their normal convergence criteria or the supplied max_iter | ||
| budget is exhausted. | ||
| :param save_images: When True, save alignment step images. | ||
| :returns: model.ProgressiveFuture | ||
| A future representing the background alignment task. The future's | ||
| task_canceller is set so callers (or GUI) can cancel the running task. | ||
| Any exception raised during alignment will be propagated when calling | ||
| returned_future.result(). It has additional attributes: | ||
| - n_steps: Total number of optimization steps performed. | ||
| - current_step: Current optimization step. | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "tileacq.py" -type f 2>/dev/null | head -5Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
find . -name "tileacq.py" -type fRepository: delmic/odemis
Length of output: 77
🏁 Script executed:
wc -l ./plugins/tileacq.pyRepository: delmic/odemis
Length of output: 82
🏁 Script executed:
sed -n '880,930p' ./plugins/tileacq.pyRepository: delmic/odemis
Length of output: 2523
🏁 Script executed:
sed -n '890,970p' ./plugins/tileacq.pyRepository: delmic/odemis
Length of output: 3890
🏁 Script executed:
rg "ParabolicMirrorAlignmentTask" ./plugins/tileacq.py -A 20 -B 5Repository: delmic/odemis
Length of output: 7600
🏁 Script executed:
head -50 ./plugins/tileacq.pyRepository: delmic/odemis
Length of output: 1561
🏁 Script executed:
sed -n '1050,1120p' ./plugins/tileacq.pyRepository: delmic/odemis
Length of output: 3144
Add type hints and fix docstring for _custom_tiled_target_z_acquisition.
The method has a copy-pasted docstring about ParabolicMirrorAlignmentTask (with parameters axes, ccd, search_range, etc. that don't exist in the signature) and missing type hints on all parameters.
✏️ Suggested changes
def _custom_tiled_target_z_acquisition(
self,
- tile_pos: Dict,
- streams,
- tile_path,
- da_list,
- target_z,
+ tile_pos: Dict[str, float],
+ streams: List[Any],
+ tile_path: str,
+ da_list: List[Any],
+ target_z: float,
) -> model.ProgressiveFuture:
- """
- Starts a ParabolicMirrorAlignmentTask that performs a multi-stage optimization
- (coarse per-axis scans followed by Nelder-Mead refinements) to align the
- mirror and stage based on camera measurements for SPARC.
-
- :param axes: List of alignment axes to optimize of len 1 or 3.
- :param ccd: Camera component used to measure the spot.
- :param search_range: Initial half-range (in metres) for each
- coarse search around the current position. Defaults to 50e-6.
- :param max_iter: Maximum number of optimizer iterations.
- This value is passed to the underlying scipy
- optimizers to limit work. Defaults to 100.
- :param stop_early: When True the task attempts to stop the Nelder-Mead stages early
- if progress stalls. Set to False to force the optimizers to run
- until their normal convergence criteria or the supplied max_iter
- budget is exhausted.
- :param save_images: When True, save alignment step images.
- :returns: model.ProgressiveFuture
- A future representing the background alignment task. The future's
- task_canceller is set so callers (or GUI) can cancel the running task.
- Any exception raised during alignment will be propagated when calling
- returned_future.result(). It has additional attributes:
- - n_steps: Total number of optimization steps performed.
- - current_step: Current optimization step.
- """
+ """Acquire a single tile at a precomputed Z position.
+
+ :param tile_pos: Tile position with 'x' and 'y' keys.
+ :param streams: Streams to acquire for the tile.
+ :param tile_path: Output filename for the tile.
+ :param da_list: Accumulator for acquired data arrays.
+ :param target_z: Absolute Z position to move to before acquisition.
+ :return: Future tracking the tile move and acquisition.
+ """🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 890 - 922, The docstring for
_custom_tiled_target_z_acquisition is copy-pasted and contains wrong parameters
(axes, ccd, search_range, etc.) and the function parameters lack type hints;
update the signature to add precise type hints for tile_pos, streams, tile_path,
da_list, and target_z (e.g., Dict/Mapping for tile_pos, an iterable/Sequence or
specific Stream type for streams, str/Path for tile_path, List/Sequence for
da_list, and float or Optional[float] for target_z) and keep the return type
model.ProgressiveFuture, then replace the docstring with a concise description
that documents the actual parameters (tile_pos, streams, tile_path, da_list,
target_z), return value, raised exceptions and any future attributes (n_steps,
current_step, task_canceller) while removing all references to
ParabolicMirrorAlignmentTask and the unrelated axes/ccd/search_range parameters
so the docstring matches the implementation.
plugins/tileacq.py
Outdated
Comment on lines
989
to
1020
| def _custom_tiled_acquisition( | ||
| self, | ||
| tile_pos: Dict, | ||
| streams, | ||
| tile_path, | ||
| da_list, | ||
| ) -> model.ProgressiveFuture: | ||
| """ | ||
| Starts a ParabolicMirrorAlignmentTask that performs a multi-stage optimization | ||
| (coarse per-axis scans followed by Nelder-Mead refinements) to align the | ||
| mirror and stage based on camera measurements for SPARC. | ||
|
|
||
| :param axes: List of alignment axes to optimize of len 1 or 3. | ||
| :param ccd: Camera component used to measure the spot. | ||
| :param search_range: Initial half-range (in metres) for each | ||
| coarse search around the current position. Defaults to 50e-6. | ||
| :param max_iter: Maximum number of optimizer iterations. | ||
| This value is passed to the underlying scipy | ||
| optimizers to limit work. Defaults to 100. | ||
| :param stop_early: When True the task attempts to stop the Nelder-Mead stages early | ||
| if progress stalls. Set to False to force the optimizers to run | ||
| until their normal convergence criteria or the supplied max_iter | ||
| budget is exhausted. | ||
| :param save_images: When True, save alignment step images. | ||
| :returns: model.ProgressiveFuture | ||
| A future representing the background alignment task. The future's | ||
| task_canceller is set so callers (or GUI) can cancel the running task. | ||
| Any exception raised during alignment will be propagated when calling | ||
| returned_future.result(). It has additional attributes: | ||
| - n_steps: Total number of optimization steps performed. | ||
| - current_step: Current optimization step. | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n plugins/tileacq.py | sed -n '989,1020p'Repository: delmic/odemis
Length of output: 2050
🏁 Script executed:
# Check imports for type hints
head -50 plugins/tileacq.py | grep -E "^from|^import"Repository: delmic/odemis
Length of output: 591
🏁 Script executed:
# Read the implementation of _custom_tiled_acquisition
sed -n '989,1050p' plugins/tileacq.pyRepository: delmic/odemis
Length of output: 3222
🏁 Script executed:
# Read more of the implementation to understand method purpose
sed -n '989,1100p' plugins/tileacq.pyRepository: delmic/odemis
Length of output: 5468
Fix docstring and add missing type hints for _custom_tiled_acquisition.
The docstring documents a ParabolicMirrorAlignmentTask with unrelated parameters (axes, ccd, search_range, max_iter, stop_early, save_images). The method actually performs tile acquisition with per-tile Z alignment.
Type hints are incomplete: streams, tile_path, and da_list lack type annotations.
✏️ Suggested signature/docstring
def _custom_tiled_acquisition(
self,
- tile_pos: Dict,
- streams,
- tile_path,
- da_list,
+ tile_pos: Dict[str, float],
+ streams: List[Any],
+ tile_path: str,
+ da_list: List[Any],
) -> model.ProgressiveFuture:
- """
- Starts a ParabolicMirrorAlignmentTask that performs a multi-stage optimization
- (coarse per-axis scans followed by Nelder-Mead refinements) to align the
- mirror and stage based on camera measurements for SPARC.
- ...
- """
+ """Acquire a single tile with per-tile Z alignment.
+
+ :param tile_pos: Tile position with 'x' and 'y' keys.
+ :param streams: Streams to acquire for the tile.
+ :param tile_path: Output filename for the tile.
+ :param da_list: Accumulator for acquired data arrays.
+ :return: Future tracking the tile move, alignment, and acquisition.
+ """🤖 Prompt for AI Agents
In `@plugins/tileacq.py` around lines 989 - 1020, The _custom_tiled_acquisition
docstring incorrectly describes a ParabolicMirrorAlignmentTask and lists
unrelated parameters; also the parameters streams, tile_path and da_list lack
type hints. Update the docstring of _custom_tiled_acquisition to describe its
actual behavior (per-tile Z alignment during tiled acquisition, what tile_pos
and streams represent, side-effects like saved images or metadata, and the
returned model.ProgressiveFuture details) and remove the irrelevant parameters
(axes, ccd, search_range, max_iter, stop_early, save_images). Add precise type
annotations to the signature: e.g. streams: Iterable[Any] (or a more specific
Stream type if available), tile_path: str | pathlib.Path, da_list:
Sequence[Mapping[str, Any]] (or List[DataAcq] if a concrete type exists), and
import typing/pathlib as needed; keep the return type as model.ProgressiveFuture
and reference the same symbols (_custom_tiled_acquisition, tile_pos, streams,
tile_path, da_list, model.ProgressiveFuture) so reviewers can locate the
changes.
Comment on lines
565
to
575
| def _add_stream_cont(self, stream, show_panel=True, locked=False, static=False, | ||
| view=None, cls=StreamController): | ||
| view=None, cls=StreamController, sp_options=None): | ||
| """ Create and add a stream controller for the given stream | ||
|
|
||
| :return: (StreamController) | ||
|
|
||
| """ | ||
|
|
||
| stream_cont = cls(self._stream_bar, stream, self._tab_data_model, | ||
| show_panel, view, sb_ctrl=self) | ||
| show_panel, view, sb_ctrl=self, sp_options=sp_options) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find StreamController subclasses
echo "=== Finding StreamController subclasses ==="
find src/odemis/gui -name "*.py" -type f | xargs rg -l "class.*StreamController"
# Get detailed class definitions
echo -e "\n=== StreamController class definitions ==="
find src/odemis/gui -name "*.py" -type f | xargs rg -n "class.*StreamController" --type=pyRepository: delmic/odemis
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Check the method signature and how sp_options is used
echo "=== Checking _add_stream_cont method ==="
sed -n '565,575p' src/odemis/gui/cont/stream_bar.py
# Look for FastEMStreamController mentioned in the review
echo -e "\n=== Looking for FastEMStreamController ==="
find src/odemis/gui -name "*.py" -type f | xargs rg -n "class FastEMStreamController" -A20Repository: delmic/odemis
Length of output: 2489
🏁 Script executed:
#!/bin/bash
# Get StreamController __init__ signature
echo "=== StreamController.__init__ signature ==="
sed -n '66,150p' src/odemis/gui/cont/stream.py | head -100
# Find all places where _add_stream_cont is called with sp_options
echo -e "\n=== Calls to _add_stream_cont with sp_options ==="
rg -n "_add_stream_cont.*sp_options" src/odemis/guiRepository: delmic/odemis
Length of output: 3729
🏁 Script executed:
#!/bin/bash
# More comprehensive search for StreamController subclasses
echo "=== All references to StreamController in class definitions ==="
rg -n "StreamController" src/odemis/gui/cont/stream.py | grep -E "(class|def __init__|extends)"
# Search for calls to _add_stream_cont
echo -e "\n=== Calls to _add_stream_cont ==="
rg -n "_add_stream_cont" src/odemis/gui
# Check if there are other subclasses anywhere in the GUI
echo -e "\n=== All StreamController subclasses in entire gui ==="
find src/odemis/gui -name "*.py" | xargs rg -n "class.*\(.*StreamController\)"Repository: delmic/odemis
Length of output: 1093
🏁 Script executed:
#!/bin/bash
# Check each call to _add_stream_cont with context
echo "=== Call at line 553 ==="
sed -n '550,560p' src/odemis/gui/cont/stream_bar.py
echo -e "\n=== Call at line 2142 ==="
sed -n '2140,2150p' src/odemis/gui/cont/stream_bar.py
echo -e "\n=== Call at line 2155 ==="
sed -n '2153,2163p' src/odemis/gui/cont/stream_bar.py
echo -e "\n=== Call at line 2260 ==="
sed -n '2258,2268p' src/odemis/gui/cont/stream_bar.py
echo -e "\n=== Call at line 2271 ==="
sed -n '2269,2279p' src/odemis/gui/cont/stream_bar.py
# Also check if cls parameter is ever passed as FastEMStreamController
echo -e "\n=== Search for cls=FastEMStreamController ==="
rg -n "cls.*FastEMStreamController" src/odemis/guiRepository: delmic/odemis
Length of output: 2871
🏁 Script executed:
#!/bin/bash
# Check the fastem_setup_tab.py call
echo "=== FastEMStreamController usage in fastem_setup_tab.py ==="
sed -n '110,130p' src/odemis/gui/cont/tabs/fastem_setup_tab.py
# Get more context around line 553 to understand how stream_cont_cls is determined
echo -e "\n=== Context around line 553 in stream_bar.py ==="
sed -n '530,570p' src/odemis/gui/cont/stream_bar.pyRepository: delmic/odemis
Length of output: 3227
Update FastEMStreamController to accept sp_options parameter.
Line 573 unconditionally forwards sp_options to cls(...). The subclass FastEMStreamController lacks this parameter in its __init__ signature (lines 1384–1385 of src/odemis/gui/cont/stream.py), so any non-None sp_options will raise TypeError. Update FastEMStreamController.__init__ to accept sp_options (or use **kwargs for flexibility) to match the base StreamController interface.
Possible mitigation approach
Update the FastEMStreamController.__init__ signature to accept sp_options:
- def __init__(self, stream_bar, stream, tab_data_model, show_panel=True, view=None,
- sb_ctrl=None):
+ def __init__(self, stream_bar, stream, tab_data_model, show_panel=True, view=None,
+ sb_ctrl=None, sp_options=None):
super().__init__(
stream_bar=stream_bar, stream=stream, tab_data_model=tab_data_model,
- show_panel=show_panel, view=view, sb_ctrl=sb_ctrl
+ show_panel=show_panel, view=view, sb_ctrl=sb_ctrl, sp_options=sp_options
)Alternatively, conditionally pass sp_options only when not None in _add_stream_cont.
🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/stream_bar.py` around lines 565 - 575, The
_add_stream_cont method always forwards sp_options to the constructor
(cls(...)), but FastEMStreamController.__init__ does not accept sp_options,
causing a TypeError when sp_options is not None; update
FastEMStreamController.__init__ to accept an sp_options parameter (or add
**kwargs) so its signature matches StreamController, or alternatively modify
_add_stream_cont to only pass sp_options when it is not None—prefer changing
FastEMStreamController.__init__ to add sp_options or **kwargs to maintain
interface compatibility with StreamController.
Comment on lines
69
to
78
| 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.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant lines
wc -l src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 91
🏁 Script executed:
# Read the __init__ method around lines 69-78
sed -n '60,120p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 2800
🏁 Script executed:
# Check imports at the top of the file to understand available types
head -50 src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 1923
🏁 Script executed:
# Search for StreamBar class definition
rg "class StreamBar" --type pyRepository: delmic/odemis
Length of output: 270
🏁 Script executed:
# Search for MicroscopeView definition
rg "class MicroscopeView" --type pyRepository: delmic/odemis
Length of output: 381
🏁 Script executed:
# Search for StreamBarController definition
rg "class StreamBarController" --type pyRepository: delmic/odemis
Length of output: 125
🏁 Script executed:
# Search for MicroscopyGUIData definition
rg "class MicroscopyGUIData" --type pyRepository: delmic/odemis
Length of output: 137
🏁 Script executed:
# Check Python version requirement in the repository
find . -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" | head -5 | xargs grep -l "python_requires" 2>/dev/null || echo "Checking for version requirements..."Repository: delmic/odemis
Length of output: 93
🏁 Script executed:
# Check if these types are imported in stream.py or if they would need forward references
rg "from.*Stream|import.*Stream" src/odemis/gui/cont/stream.py | head -20Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check which module the stream.py file is in and look for type imports
rg "from odemis.gui.comp.stream_bar|from odemis.gui.cont.stream_bar|from odemis.gui.model" src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 89
🏁 Script executed:
# Check Python version support in setup or requirements
find . -name "setup.py" -o -name "requirements.txt" -o -name "pyproject.toml" | head -3 | xargs cat 2>/dev/null | head -100Repository: delmic/odemis
Length of output: 3799
🏁 Script executed:
# Check if __future__ annotations is used for type hints in this file
head -30 src/odemis/gui/cont/stream.py | grep -i "from __future__"Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check if there are existing examples of reST-style docstrings in this file
rg -A 10 "def.*:" src/odemis/gui/cont/stream.py | grep -A 8 ":param" | head -40Repository: delmic/odemis
Length of output: 1708
🏁 Script executed:
# Check what Python version the code base targets more broadly
cat setup.py 2>/dev/null | grep -i "python_requires\|Programming Language" | head -10Repository: delmic/odemis
Length of output: 116
🏁 Script executed:
# Check if any type hints exist in the file already
rg "def \w+\([^)]*:" src/odemis/gui/cont/stream.py | head -5Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check what's imported and available in stream.py for the type hints
sed -n '1,50p' src/odemis/gui/cont/stream.py | grep -E "^import|^from"Repository: delmic/odemis
Length of output: 898
🏁 Script executed:
# Check if the union syntax (int | None) or Optional is used elsewhere in stream.py
rg "int \|" src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check how other methods in this file handle type hints and docstrings
sed -n '100,250p' src/odemis/gui/cont/stream.py | head -80Repository: delmic/odemis
Length of output: 3194
🏁 Script executed:
# Verify the exact line numbers of the __init__ method
rg -n "def __init__" src/odemis/gui/cont/stream.py | head -3Repository: delmic/odemis
Length of output: 238
🏁 Script executed:
# Check the exact docstring style used in the StreamController class
sed -n '69,90p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 1196
🏁 Script executed:
# Check another method's docstring and type hints in stream.py for consistency
sed -n '200,230p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 1398
🏁 Script executed:
# Verify acqstream.Stream exists
rg "class Stream" src/odemis/acq/stream.py | head -3Repository: delmic/odemis
Length of output: 121
🏁 Script executed:
# Find the stream module
find . -path "*/acq/stream*" -type fRepository: delmic/odemis
Length of output: 336
🏁 Script executed:
# Check what Stream classes are available
rg "^class.*Stream\(" src/odemis/acq/stream/__init__.py 2>/dev/null | head -10Repository: delmic/odemis
Length of output: 241
🏁 Script executed:
# Check if there's a stream.py in acq directory
ls -la src/odemis/acq/ | grep streamRepository: delmic/odemis
Length of output: 186
🏁 Script executed:
# Check the actual structure of acq directory
find src/odemis/acq -name "*stream*" -type f | head -10Repository: delmic/odemis
Length of output: 382
🏁 Script executed:
# Check what's exported from the stream module
head -100 src/odemis/acq/stream/__init__.pyRepository: delmic/odemis
Length of output: 2640
🏁 Script executed:
# Check the base Stream class
rg "^class Stream" src/odemis/acq/stream/_base.py | head -5Repository: delmic/odemis
Length of output: 78
🏁 Script executed:
# Verify if the type annotations use Union or pipe syntax in this codebase
rg "Union\[|Optional\[|\|" src/odemis/gui/cont/stream.py | head -10Repository: delmic/odemis
Length of output: 668
🏁 Script executed:
# Verify the reST docstring format used elsewhere in the codebase
rg -A 5 "def.*:\n.*\"\"\"" src/odemis/gui/cont/stream.py | grep -A 5 ":param" | head -30Repository: delmic/odemis
Length of output: 248
🏁 Script executed:
# Check if Python 3.10 union syntax (|) is used anywhere in the codebase
rg -r ":\s*\w+\s*\|\s*None" src/odemis --type py | head -5Repository: delmic/odemis
Length of output: 778
🏁 Script executed:
# Check if Optional or Union is imported and used for type hints
rg "from typing import|Optional\[|Union\[" src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check what Python version is actually supported
grep -r "python_requires" . --include="*.py" --include="*.cfg" --include="*.toml" 2>/dev/null | head -5Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check if __future__ annotations is used in similar files
rg "from __future__ import annotations" src/odemis --type py | head -5Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check the actual imports that are available in stream.py for forward references
sed -n '1,45p' src/odemis/gui/cont/stream.py | grep -E "^from|^import"Repository: delmic/odemis
Length of output: 751
🏁 Script executed:
# Look for examples of type hints with forward references in the codebase
rg "def.*:.*\|.*None" src/odemis --type py -A 2 | head -20Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check if the StreamPanel import is at the top of stream.py
grep "StreamPanel\|StreamBar\|MicroscopyGUIData" src/odemis/gui/cont/stream.py | head -10Repository: delmic/odemis
Length of output: 539
🏁 Script executed:
# Verify StreamBar is imported in stream.py
grep -n "StreamBar" src/odemis/gui/cont/stream.py | head -5Repository: delmic/odemis
Length of output: 234
🏁 Script executed:
# Check if StreamBar is a separate import or needs to be imported
grep -n "from odemis.gui.comp.stream_bar import StreamBar" src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Verify the complete import section around StreamPanel
sed -n '30,45p' src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 860
🏁 Script executed:
# Check the minimum Python version requirement in setup.py or pyproject.toml
find . -name "setup.py" -o -name "pyproject.toml" | xargs grep -i "python\|version" | grep -i "require\|>=\|3\." | head -10Repository: delmic/odemis
Length of output: 85
🏁 Script executed:
# Check how type hints are done in other GUI files
rg "def.*:.*Optional\[|def.*:.*Union\[" src/odemis/gui --type py | head -5Repository: delmic/odemis
Length of output: 696
🏁 Script executed:
# See if there are any existing type hints in stream.py at all
rg "def \w+\([^)]*:" src/odemis/gui/cont/stream.py -A 1 | grep -E "->|:" | head -10Repository: delmic/odemis
Length of output: 167
🏁 Script executed:
# Check for existing type hints in stream.py using a different approach
rg "def \w+\([^)]*:" src/odemis/gui/cont/stream.py | head -10Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check what's imported for typing
rg "from typing import|import typing" src/odemis/gui --type py | head -10Repository: delmic/odemis
Length of output: 778
🏁 Script executed:
# Verify that the types referenced in the suggested fix are actually available
grep -n "class StreamBar\|class StreamBarController\|class MicroscopyGUIData\|class MicroscopeView" src/odemis/gui/comp/stream_bar.py src/odemis/gui/cont/stream_bar.py src/odemis/gui/model/tab_gui_data.py src/odemis/gui/model/stream_view.py 2>/dev/nullRepository: delmic/odemis
Length of output: 350
🏁 Script executed:
# Check if Optional is imported or used in stream.py
grep -n "typing\|Optional\|Union" src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Verify the minimum Python version requirement more thoroughly
cat setup.py | grep -A 20 "version"Repository: delmic/odemis
Length of output: 4734
🏁 Script executed:
# Check how type hints are done with forward references in other stream-related files
rg "def.*Optional\[.*\|.*None" src/odemis/gui/cont --type py | head -10Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check if pipe syntax is actually used anywhere in the codebase
rg ": \w+ \| None" src/odemis --type py | head -5Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Check how Optional is used in stream_bar.py for reference
sed -n '1,100p' src/odemis/gui/cont/stream_bar.py | grep -A 5 "from typing"Repository: delmic/odemis
Length of output: 162
🏁 Script executed:
# Look at an actual example of type hints with Optional in the codebase
rg -A 8 "def.*Optional\[" src/odemis/gui/cont/stream_bar.py | head -20Repository: delmic/odemis
Length of output: 39
Add type hints and convert docstring to reStructuredText format for StreamController.__init__.
The method signature and docstring do not comply with the coding guidelines. All parameters and return type must have type hints, and the docstring must use reStructuredText field list format (:param and :return:) without embedding type information.
Suggested fix
- def __init__(self, stream_bar, stream, tab_data_model, show_panel=True, view=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.
- """
+ def __init__(
+ self,
+ stream_bar: "StreamBar",
+ stream: acqstream.Stream,
+ tab_data_model: "MicroscopyGUIData",
+ show_panel: bool = True,
+ view: Optional["MicroscopeView"] = None,
+ sb_ctrl: Optional["StreamBarController"] = None,
+ sp_options: Optional[int] = None,
+ ) -> None:
+ """Manage a stream and its accompanying stream panel.
+
+ :param stream_bar: Stream bar owning the panel.
+ :param stream: Stream instance.
+ :param tab_data_model: Tab data model.
+ :param show_panel: Whether to show the panel.
+ :param view: Optional view to link to the stream.
+ :param sb_ctrl: Optional StreamBarController instance.
+ :param sp_options: StreamPanel options bitmask.
+ """Note: Add from typing import Optional to the imports if not already present.
🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/stream.py` around lines 69 - 78,
StreamController.__init__ currently lacks type annotations and uses a non-reST
docstring; add type hints for all parameters and the return type (-> None) and
import Optional from typing if missing, e.g. annotate stream_bar, stream,
tab_data_model, show_panel: bool, view: Optional[MicroscopeView], sb_ctrl:
Optional[StreamBarController], sp_options: Optional[int]; then replace the
existing docstring with reStructuredText field-list style using :param name: and
:return: entries (do not embed types in the docstring) and keep the explanatory
text for view/ sb_ctrl/ sp_options within those :param: descriptions.
Comment on lines
90
to
+96
| if hasattr(stream, "tint"): | ||
| options |= OPT_BTN_TINT | ||
| sp_options |= OPT_BTN_TINT | ||
| if isinstance(stream, acqstream.RGBStream): | ||
| options |= OPT_NO_COLORMAPS | ||
| sp_options |= OPT_NO_COLORMAPS | ||
| # (Temporal)SpectrumStreams *with spectrum data* accept the FIT_TO_RGB option | ||
| if isinstance(stream, acqstream.SpectrumStream) and stream.raw[0].shape[0] > 1: | ||
| options |= OPT_FIT_RGB | ||
| sp_options |= OPT_FIT_RGB |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file at the specified lines
cat -n src/odemis/gui/cont/stream.py | sed -n '85,100p'Repository: delmic/odemis
Length of output: 1007
🏁 Script executed:
# Let's also check if the file exists and get more context
ls -la src/odemis/gui/cont/stream.py
wc -l src/odemis/gui/cont/stream.pyRepository: delmic/odemis
Length of output: 171
🏁 Script executed:
# Find the SpectrumStream class definition to understand stream.raw
fd "acqstream" --type f -e ".py" | head -20Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Search for SpectrumStream class definition
rg "class SpectrumStream" --type py -A 10Repository: delmic/odemis
Length of output: 653
🏁 Script executed:
# Search for stream.raw usage patterns in the codebase
rg "stream\.raw" --type py -B 2 -A 2 | head -100Repository: delmic/odemis
Length of output: 6723
🏁 Script executed:
# Search for stream.raw initialization and type hints
rg "raw\s*=" --type py -B 2 -A 2 | grep -E "(raw|\.raw|stream\.raw)" | head -50Repository: delmic/odemis
Length of output: 5004
🏁 Script executed:
# Look for raw attribute definition in stream classes
rg "self\.raw\s*=" --type py -B 3 -A 3 | head -100Repository: delmic/odemis
Length of output: 6414
🏁 Script executed:
# Check the SpectrumStream class and related implementations
cat -n src/odemis/acq/stream/__init__.py | grep -A 50 "class SpectrumSettingsStream"Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Search for more guards before stream.raw[0] access to understand the pattern
rg "stream\.raw\[0\]" --type py -B 3 -A 1 | head -150Repository: delmic/odemis
Length of output: 10459
🏁 Script executed:
# Search for SpectrumStream creation and initialization
rg "class.*SpectrumStream" --type py -A 20 | head -100Repository: delmic/odemis
Length of output: 7155
🏁 Script executed:
# Look at how stream.raw is initialized in different stream types
rg "def __init__" --type py -A 15 | grep -B 10 -A 5 "self.raw" | head -80Repository: delmic/odemis
Length of output: 4373
🏁 Script executed:
# Look for SpectrumSettingsStream and StaticSpectrumStream initialization
rg "class SpectrumSettingsStream" --type py -A 30 | head -60Repository: delmic/odemis
Length of output: 2525
🏁 Script executed:
# Check StaticStream initialization to see how raw is set
rg "class StaticStream" --type py -A 40 | head -80Repository: delmic/odemis
Length of output: 5811
🏁 Script executed:
# Look for SpectrumSettingsStream initialization
fd . --type f -name "*.py" -exec grep -l "SpectrumSettingsStream" {} \; | head -10Repository: delmic/odemis
Length of output: 287
🏁 Script executed:
# Search more specifically for how SpectrumSettingsStream initializes raw
rg "SpectrumSettingsStream" --type py -B 5 -A 10 | head -150Repository: delmic/odemis
Length of output: 10528
🏁 Script executed:
# Find CCDSettingsStream to see how it initializes raw
rg "class CCDSettingsStream" --type py -A 50 | head -100Repository: delmic/odemis
Length of output: 3867
🏁 Script executed:
# Also look at LiveStream initialization to understand the pattern
rg "class LiveStream" --type py -A 30 | head -80Repository: delmic/odemis
Length of output: 2378
🏁 Script executed:
# Find the Stream base class initialization to see raw initialization
rg "class Stream" --type py -A 100 | grep -A 100 "def __init__" | head -150Repository: delmic/odemis
Length of output: 9762
🏁 Script executed:
# Search specifically for raw initialization in Stream class
rg "self\.raw\s*=" --type py -B 2 -A 2 | grep -A 5 "class Stream" | head -50Repository: delmic/odemis
Length of output: 39
🏁 Script executed:
# Look at the stream module to find base Stream class
cat -n src/odemis/acq/stream/_base.py | head -150Repository: delmic/odemis
Length of output: 8949
🏁 Script executed:
# Continue reading the Stream __init__ method to see how raw is initialized
cat -n src/odemis/acq/stream/_base.py | sed -n '150,250p'Repository: delmic/odemis
Length of output: 5876
🏁 Script executed:
# Check if there are cases where stream.raw could be empty when the code at line 95 executes
rg "stream.raw\[0\].shape" --type py -B 5 -A 2Repository: delmic/odemis
Length of output: 2849
🏁 Script executed:
# Check if the code in the review has tint attribute check before accessing raw
cat -n src/odemis/gui/cont/stream.py | sed -n '88,100p'Repository: delmic/odemis
Length of output: 868
Guard stream.raw[0] before access.
Line 95 assumes stream.raw has at least one item; for newly created streams this can be empty and raise IndexError.
Suggested fix
- if isinstance(stream, acqstream.SpectrumStream) and stream.raw[0].shape[0] > 1:
+ if (isinstance(stream, acqstream.SpectrumStream)
+ and stream.raw
+ and stream.raw[0] is not None
+ and stream.raw[0].shape[0] > 1):
sp_options |= OPT_FIT_RGB🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/stream.py` around lines 90 - 96, The code assumes
stream.raw has at least one element when checking spectrum shape; guard access
before indexing to avoid IndexError. In the block that sets sp_options (the
branch using stream, acqstream.SpectrumStream, OPT_FIT_RGB, OPT_BTN_TINT and
OPT_NO_COLORMAPS) add a defensive check that stream.raw is present and non-empty
(e.g., hasattr/getattr and len(stream.raw) > 0) before evaluating
stream.raw[0].shape[0] > 1; only set OPT_FIT_RGB when that guarded condition is
true.
3407bfe to
98862dd
Compare
nandishjpatel
force-pushed
the
CSDM-1162-extend-tiled-acquisition-plugin-to-support-wafer-acquisition-workflow-v2
branch
from
98862dd to
12d598b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/odemis/gui/plugin/__init__.py (1)
507-540:⚠️ Potential issue | 🟡 MinorAdd type hints and reST docstring for
addStream.The updated public API still lacks annotations and uses a non‑reST docstring format. Please align with the repository’s Python guidelines.
🔧 Suggested update
-from typing import Callable +from typing import Callable, Optional +import odemis.acq.stream as acqstream @@ - def addStream(self, stream, index=0, sp_options=None): + def addStream( + self, + stream: Optional["acqstream.Stream"], + 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. @@ - 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 shown without adding a stream. + :param index: Viewport index (0 left, 1 right, 2 spectrum) or None to add to the hidden view. + :param sp_options: StreamPanel options bitmask, or None for default behavior. """As per coding guidelines, “Always use type hints for function parameters and return types in Python code” and “Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/plugin/__init__.py` around lines 507 - 540, The addStream method lacks Python type annotations and uses a non-reST docstring; update the signature of addStream (decorated with call_in_wx_main) to include type hints for parameters (stream: Optional[Stream], index: Optional[int]=0, sp_options: Optional[int]=None) and a return type of None, and replace the current multi-line docstring with a reStructuredText-style docstring (no inline type info) describing parameters and behavior; ensure references to self.hidden_view, self._viewports, and self.streambar_controller.addStream remain correct and keep the existing logic and assertions intact.src/odemis/acq/stitching/_tiledacq.py (2)
291-321:⚠️ Potential issue | 🟡 MinorAdd type hints and reST docstrings for
getFov/guessSmallestFov.These newly converted utility methods still lack annotations and use non‑reST docstring text.
🔧 Suggested update
- def getFov(sd): + def getFov(sd: Union[Stream, model.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 + :param sd: Stream or DataArray used to estimate FoV. + :return: Width and height in meters. """ @@ - def guessSmallestFov(cls, ss): + def guessSmallestFov(cls, ss: List[Stream]) -> 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 + Return the smallest FoV width and height across all streams. + :param ss: Streams to inspect. + :return: Smallest (width, height) FoV. + :raise ValueError: If no stream selected. """As per coding guidelines, “Always use type hints for function parameters and return types in Python code” and “Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).”
🤖 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 291 - 321, Add proper Python type hints and replace the informal docstrings with reStructuredText-style ones for the two methods: annotate getFov to accept sd: Union[model.DataArray, Stream] and return Tuple[float, float] (or use typing overloads if preferred) and annotate guessSmallestFov as cls, ss: Iterable[Union[model.DataArray, Stream]] -> Tuple[float, float]; update the docstrings for both getFov and guessSmallestFov to follow reST (describe parameters and return without inline type annotations) and keep the same runtime behavior and error raising (TypeError/ValueError) and ensure any used typing names are imported from typing at the top of the module.
106-141:⚠️ Potential issue | 🟡 MinorAdd type hints and reST docstring to the updated
__init__.The extended constructor still lacks annotations and uses non‑reST docstring style (with embedded type info). Please annotate the full signature and switch to reST field lists without types.
As per coding guidelines, “Always use type hints for function parameters and return types in Python code” and “Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).”
🤖 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 106 - 141, The __init__ method should be annotated with full type hints and its docstring converted to reST field lists without embedded types: update def __init__(...) on class _TiledAcq to include annotations (e.g., streams: List[Stream], stage: Actuator, region: Union[Tuple[float,float,float,float], List[Tuple[float,float]]], overlap: float, settings_obs: Optional[SettingsObserver]=None, log_path: Optional[str]=None, future: Optional[ProgressiveFuture]=None, zlevels: Optional[List[float]]=None, registrar: Optional[int]=..., weaver: Optional[int]=..., focusing_method: FocusingMethod=FocusingMethod.NONE, focus_points: Optional[List[Tuple[float,float,float]]]=None, focus_range: Optional[Tuple[float,float]]=None, centered_acq: bool=True, overlay_stream: Optional[OverlayStream]=None, sfov: Optional[Tuple[float,float]]=None, batch_acquire_streams: bool=False) -> None and add necessary typing imports (List, Tuple, Optional, Union) at the top; replace the existing inline-type docstring with reST :param: and :return: field entries that describe each parameter and remove type annotations from the docstring body so only descriptive text remains.src/odemis/gui/cont/stream_bar.py (1)
485-506:⚠️ Potential issue | 🟡 MinorAdd type hints and reST docstrings to
_add_stream/_add_stream_cont.Both updated methods still lack annotations and use non‑reST docstrings. Please bring them into guideline compliance.
🔧 Suggested update
-from typing import Optional +from typing import Optional, Type, Union @@ - def _add_stream(self, stream, add_to_view=False, visible=True, play=None, stream_cont_cls=StreamController, - sp_options=None): - """ Add the given stream to the tab data model and appropriate views - - Args: - stream (Stream): the new stream to add - - Kwargs: - add_to_view (boolean or View): if True, add the stream to all the compatible views, - if False add to the current view, otherwise, add to the given view. - visible (boolean): If True, create a stream entry, otherwise adds the stream but do not - create any entry. - play (None or boolean): If True, immediately start it, if False, let it stopped, and if - None, only play if already a stream is playing. - stream_cont_cls: The stream controller class. - sp_options: (int or None) combination of OPT_* values for the StreamPanel or None for default. - - Returns: - (StreamController or Stream): the stream controller or stream (if visible is False) that - was created - - """ + def _add_stream( + self, + stream: acqstream.Stream, + add_to_view: Union[bool, guimodel.StreamView] = False, + visible: bool = True, + play: Optional[bool] = None, + stream_cont_cls: Type[StreamController] = StreamController, + sp_options: Optional[int] = None, + ) -> Union[StreamController, acqstream.Stream]: + """Add the given stream to the tab data model and appropriate views. + + :param stream: Stream to add. + :param add_to_view: True to add to all compatible views, False for focused view, or a specific view. + :param visible: Whether to create a stream panel entry. + :param play: Whether to start acquisition immediately. + :param stream_cont_cls: Stream controller class to instantiate. + :param sp_options: StreamPanel options bitmask, or None for defaults. + :return: Stream controller or stream (if visible is False). + """ @@ - def _add_stream_cont(self, stream, show_panel=True, locked=False, static=False, - view=None, cls=StreamController, sp_options=None): - """ Create and add a stream controller for the given stream - - :return: (StreamController) - - """ + def _add_stream_cont( + self, + stream: acqstream.Stream, + show_panel: bool = True, + locked: bool = False, + static: bool = False, + view: Optional[guimodel.StreamView] = None, + cls: Type[StreamController] = StreamController, + sp_options: Optional[int] = None, + ) -> StreamController: + """Create and add a stream controller for the given stream. + + :param stream: Stream to add. + :param show_panel: Whether to show the panel. + :param locked: Whether to lock the panel. + :param static: Whether the stream is static. + :param view: Optional view to bind to the stream. + :param cls: Stream controller class to instantiate. + :param sp_options: StreamPanel options bitmask, or None for defaults. + :return: Stream controller instance. + """As per coding guidelines, “Always use type hints for function parameters and return types in Python code” and “Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).”
Also applies to: 565-571
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/stream_bar.py` around lines 485 - 506, Update the _add_stream and _add_stream_cont function signatures to include proper Python type hints (use typing.Optional, typing.Union, typing.Type for parameters like stream, add_to_view (bool or View), visible: bool, play: Optional[bool], stream_cont_cls: Type[StreamController], sp_options: Optional[int]) and add explicit return types (e.g., StreamController | Stream or Union[...] as appropriate); refactor their existing docstrings to reStructuredText style (brief summary, Args:, Returns:) but remove any type annotations from the docstrings (leave types only in the function signature) and ensure parameter names match the annotated signature (refer to _add_stream and _add_stream_cont to locate the methods and maintain the same default values and behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 780-801: The overlay DA is being prepended when
_batch_acquire_streams is True (via _overlay_stream) which shifts the DA order
expected by downstream methods like _adjustFocus and _sortDAs; change
_getTileDAs (the block that calls _acquireStreamsTile and builds das) to
separate the overlay DA from the list used for stitching and focus: capture the
overlay DA (from _overlay_stream) separately and do not include it in the array
that maps 1:1 to self._streams, or alternatively return a tuple like
(overlay_da, stream_das) so callers (_adjustFocus, _sortDAs) can explicitly skip
the overlay; ensure that any code expecting MD_ACQ_TYPE is only given the
stream_das and not the overlay_da.
- Around line 755-772: The _acquireStreamsTile function is missing type
annotations and uses a non-reST docstring; add Python type hints for parameters
(e.g., i: int, ix: int, iy: int, streams: Sequence[StreamType] or the concrete
stream type used in your code) and a return annotation (e.g., -> DataArray or
the correct Das collection type), import typing symbols if needed, and convert
the docstring to reStructuredText (short summary, params and returns sections
without inline types). Keep the existing logic and references to self._future,
acqmng.acquire, CANCELLED and CancelledError unchanged, and update any imports
(typing.Tuple/Sequence) required for the new annotations.
---
Outside diff comments:
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 291-321: Add proper Python type hints and replace the informal
docstrings with reStructuredText-style ones for the two methods: annotate getFov
to accept sd: Union[model.DataArray, Stream] and return Tuple[float, float] (or
use typing overloads if preferred) and annotate guessSmallestFov as cls, ss:
Iterable[Union[model.DataArray, Stream]] -> Tuple[float, float]; update the
docstrings for both getFov and guessSmallestFov to follow reST (describe
parameters and return without inline type annotations) and keep the same runtime
behavior and error raising (TypeError/ValueError) and ensure any used typing
names are imported from typing at the top of the module.
- Around line 106-141: The __init__ method should be annotated with full type
hints and its docstring converted to reST field lists without embedded types:
update def __init__(...) on class _TiledAcq to include annotations (e.g.,
streams: List[Stream], stage: Actuator, region:
Union[Tuple[float,float,float,float], List[Tuple[float,float]]], overlap: float,
settings_obs: Optional[SettingsObserver]=None, log_path: Optional[str]=None,
future: Optional[ProgressiveFuture]=None, zlevels: Optional[List[float]]=None,
registrar: Optional[int]=..., weaver: Optional[int]=..., focusing_method:
FocusingMethod=FocusingMethod.NONE, focus_points:
Optional[List[Tuple[float,float,float]]]=None, focus_range:
Optional[Tuple[float,float]]=None, centered_acq: bool=True, overlay_stream:
Optional[OverlayStream]=None, sfov: Optional[Tuple[float,float]]=None,
batch_acquire_streams: bool=False) -> None and add necessary typing imports
(List, Tuple, Optional, Union) at the top; replace the existing inline-type
docstring with reST :param: and :return: field entries that describe each
parameter and remove type annotations from the docstring body so only
descriptive text remains.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 485-506: Update the _add_stream and _add_stream_cont function
signatures to include proper Python type hints (use typing.Optional,
typing.Union, typing.Type for parameters like stream, add_to_view (bool or
View), visible: bool, play: Optional[bool], stream_cont_cls:
Type[StreamController], sp_options: Optional[int]) and add explicit return types
(e.g., StreamController | Stream or Union[...] as appropriate); refactor their
existing docstrings to reStructuredText style (brief summary, Args:, Returns:)
but remove any type annotations from the docstrings (leave types only in the
function signature) and ensure parameter names match the annotated signature
(refer to _add_stream and _add_stream_cont to locate the methods and maintain
the same default values and behavior).
In `@src/odemis/gui/plugin/__init__.py`:
- Around line 507-540: The addStream method lacks Python type annotations and
uses a non-reST docstring; update the signature of addStream (decorated with
call_in_wx_main) to include type hints for parameters (stream: Optional[Stream],
index: Optional[int]=0, sp_options: Optional[int]=None) and a return type of
None, and replace the current multi-line docstring with a reStructuredText-style
docstring (no inline type info) describing parameters and behavior; ensure
references to self.hidden_view, self._viewports, and
self.streambar_controller.addStream remain correct and keep the existing logic
and assertions intact.
---
Duplicate comments:
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 660-676: Overlay acquisition time is currently computed once
(overlay_time = self._overlay_stream.estimateAcquisitionTime()) but overlay is
performed per tile; multiply the per-tile overlay_time by the number of
remaining tiles (variable remaining) before adding to the total. Update the
computation that sets overlay_time and the final return expression (and the
logging message that reports overlay/acquisition times) to use overlay_time *
remaining (or store overlay_time_per_tile and compute overlay_time_total =
overlay_time_per_tile * remaining) and ensure you reference self._overlay_stream
and its estimateAcquisitionTime() method.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 565-575: The bug is that _add_stream_cont forwards sp_options to
controller constructors even when the controller class (e.g.,
FastEMStreamController.__init__) does not accept it; update
FastEMStreamController.__init__ to accept sp_options=None and forward it to
super() (or otherwise ignore it) so the call signature matches StreamController,
or alternatively change _add_stream_cont to only pass sp_options when the cls
signature accepts it; modify FastEMStreamController.__init__ (and any other
subclass missing the param) to include sp_options=None in its parameters and
propagate it to StreamController initialization.
In `@src/odemis/gui/cont/stream.py`:
- Around line 90-96: The code accesses stream.raw[0] without checking that
stream.raw is non-empty, which can raise IndexError for newly created streams;
update the conditional in the block that checks isinstance(stream,
acqstream.SpectrumStream) to first ensure stream.raw is truthy and has at least
one element (e.g., check if stream.raw and len(stream.raw) > 0) before indexing,
then only evaluate stream.raw[0].shape[0] > 1 to decide to set OPT_FIT_RGB so
the guard prevents IndexError when raw data is absent.
- Around line 69-78: The __init__ of StreamController lacks proper Python type
hints and its docstring should follow reStructuredText without embedding type
info; update the signature of StreamController.__init__ to add type annotations
for parameters (stream_bar, stream, tab_data_model, show_panel: bool = True,
view: Optional[MicroscopeView] = None, sb_ctrl: Optional[StreamBarController] =
None, sp_options: Optional[int] = None) and adjust return type to -> None, then
replace the current docstring with an reST-style description (short summary
line, extended description and parameter directives using :param name: and
:returns: without type declarations) referencing the same parameter names so
tooling and linters accept it.
Comment on lines
+755
to
+772
| 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.
Add type hints and reST docstring for _acquireStreamsTile.
This new method lacks annotations and uses a non‑reST docstring.
🔧 Suggested update
- 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
- """
+ def _acquireStreamsTile(
+ self,
+ i: int,
+ ix: int,
+ iy: int,
+ streams: List[Stream],
+ ) -> List[DataArray]:
+ """Call acquire and block until the tile data are returned.
+
+ :param i: Tile index in acquisition order.
+ :param ix: Tile x-index.
+ :param iy: Tile y-index.
+ :param streams: Streams to acquire.
+ :return: Data arrays for the current tile.
+ """As per coding guidelines, “Always use type hints for function parameters and return types in Python code” and “Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).”
🤖 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 755 - 772, The
_acquireStreamsTile function is missing type annotations and uses a non-reST
docstring; add Python type hints for parameters (e.g., i: int, ix: int, iy: int,
streams: Sequence[StreamType] or the concrete stream type used in your code) and
a return annotation (e.g., -> DataArray or the correct Das collection type),
import typing symbols if needed, and convert the docstring to reStructuredText
(short summary, params and returns sections without inline types). Keep the
existing logic and references to self._future, acqmng.acquire, CANCELLED and
CancelledError unchanged, and update any imports (typing.Tuple/Sequence)
required for the new annotations.
Comment on lines
+780
to
+801
| 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) | ||
|
|
There was a problem hiding this comment.
Overlay stream shifts DA ordering and breaks focus/stitch logic.
When batch_acquire_streams is True and overlay_stream is provided, _getTileDAs returns the overlay DA before the regular stream DAs. Downstream code assumes a 1:1 alignment with self._streams (e.g., _adjustFocus and _sortDAs), which can cause wrong focus selection and KeyError for missing MD_ACQ_TYPE.
Please separate the overlay DA from the list used for focus/stitching (or include the overlay stream in the downstream mapping and explicitly skip it in _sortDAs).
🔧 Suggested fix (drop overlay DA from stitching list)
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)
+ das = self._acquireStreamsTile(i, ix, iy, streams)
+ if self._overlay_stream:
+ # Keep overlay DA separate from the stream DAs used for focus/stitching
+ das = das[1:]🤖 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 780 - 801, The overlay DA
is being prepended when _batch_acquire_streams is True (via _overlay_stream)
which shifts the DA order expected by downstream methods like _adjustFocus and
_sortDAs; change _getTileDAs (the block that calls _acquireStreamsTile and
builds das) to separate the overlay DA from the list used for stitching and
focus: capture the overlay DA (from _overlay_stream) separately and do not
include it in the array that maps 1:1 to self._streams, or alternatively return
a tuple like (overlay_da, stream_das) so callers (_adjustFocus, _sortDAs) can
explicitly skip the overlay; ensure that any code expecting MD_ACQ_TYPE is only
given the stream_das and not the overlay_da.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.