[feat] added save_images for offline drift correction #3480
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds optional persistence of drift correction images to the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/acq/drift/__init__.py (1)
53-65: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd type hints and document
save_imagesin reStructuredText.Line 53 introduces a new constructor arg but the method is still untyped and the docstring is missing
save_images, which breaks the Python guideline contract for touched code.Proposed update
-from odemis import model +from typing import Tuple +from odemis import model @@ - def __init__(self, scanner, detector, region, dwell_time, max_pixels=MAX_PIXELS, follow_drift=True, save_images=False): + def __init__( + self, + scanner: model.Emitter, + detector, + region: Tuple[float, float, float, float], + dwell_time: float, + max_pixels: int = MAX_PIXELS, + follow_drift: bool = True, + save_images: bool = False, + ) -> None: """ - scanner (Emitter) - detector (Detector) - region (4 floats) - dwell_time (float) - max_pixels (int): the maximum number of pixels that the anchor region will use. If the region - acquired is large, then the scanner scale is adjusted so that the anchor region contains at most - max_pixels pixels. - follow_drift (bool): If True, the anchor region position is adjusted based on the drift measured. It is useful - when drift compensation is done by adjusting the scanner settings. If False, the anchor region is fixed. - It is useful when drift compensation is based on beam shift or stage movement. + :param scanner: Scanner used to acquire the anchor region. + :param detector: Detector used for anchor acquisition. + :param region: Normalized ROI as (left, top, right, bottom). + :param dwell_time: Dwell time per pixel in seconds. + :param max_pixels: Maximum number of pixels for the anchor region acquisition. + :param follow_drift: Whether to update anchor position based on measured drift. + :param save_images: Whether to persist acquired drift anchor images to disk. """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".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/drift/__init__.py` around lines 53 - 65, The __init__ method lacks type hints and its docstring omits the new save_images parameter; update def __init__ to add Python type annotations for all parameters and the return type (None) — e.g. annotate scanner, detector, region, dwell_time, max_pixels, follow_drift, save_images — and extend the reStructuredText docstring to include a :param save_images: line describing its type (bool) and behavior, keeping the existing param descriptions in reST format and preserving MAX_PIXELS default handling and semantics.
🧹 Nitpick comments (1)
src/odemis/acq/drift/__init__.py (1)
39-39: ⚡ Quick winUse
pathlib.Pathfor the new drift image path handling.The newly added path construction and directory creation use
os.path/os.makedirs; this should usepathlib.Pathper repository rule.Proposed update
-import os +from pathlib import Path @@ -DRIFT_IMAGES_DIR = os.path.join(get_picture_folder(), "Drift Correction images") +DRIFT_IMAGES_DIR = Path(get_picture_folder()) / "Drift Correction images" @@ - os.makedirs(DRIFT_IMAGES_DIR, exist_ok=True) + DRIFT_IMAGES_DIR.mkdir(parents=True, exist_ok=True) @@ - filename = os.path.join(DRIFT_IMAGES_DIR, f"drift_{self._session_id}_{self._image_counter:05d}.tif") + filename = DRIFT_IMAGES_DIR / f"drift_{self._session_id}_{self._image_counter:05d}.tif" @@ - success = cv2.imwrite(filename, data) + success = cv2.imwrite(str(filename), data)As per coding guidelines: "For file paths, use
pathlib.Path, unless interfacing with existing code that usesos.pathstrings".Also applies to: 74-74, 158-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/drift/__init__.py` at line 39, Replace os.path usage for DRIFT_IMAGES_DIR and any other path constructions in this module (e.g., the places around DRIFT_IMAGES_DIR, and the creation logic referenced near lines with mkdir calls) with pathlib.Path: make DRIFT_IMAGES_DIR a Path built from get_picture_folder() / "Drift Correction images", use Path methods (exists(), mkdir(parents=True, exist_ok=True), joinpath) instead of os.path.join and os.makedirs, and ensure any code that expects strings converts Paths via str() only where necessary; update references to use the DRIFT_IMAGES_DIR Path methods consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/drift/__init__.py`:
- Around line 157-159: The session ID is being regenerated on every acquire
call; initialize self._session_id once in the class __init__ (e.g., assign
datetime.now().strftime("%Y%m%d_%H%M%S") there) and remove the assignment from
acquire so acquire only builds the filename using the existing self._session_id
and increments self._image_counter; optionally guard acquire to set
self._session_id only if it doesn't exist (for backwards compatibility).
---
Outside diff comments:
In `@src/odemis/acq/drift/__init__.py`:
- Around line 53-65: The __init__ method lacks type hints and its docstring
omits the new save_images parameter; update def __init__ to add Python type
annotations for all parameters and the return type (None) — e.g. annotate
scanner, detector, region, dwell_time, max_pixels, follow_drift, save_images —
and extend the reStructuredText docstring to include a :param save_images: line
describing its type (bool) and behavior, keeping the existing param descriptions
in reST format and preserving MAX_PIXELS default handling and semantics.
---
Nitpick comments:
In `@src/odemis/acq/drift/__init__.py`:
- Line 39: Replace os.path usage for DRIFT_IMAGES_DIR and any other path
constructions in this module (e.g., the places around DRIFT_IMAGES_DIR, and the
creation logic referenced near lines with mkdir calls) with pathlib.Path: make
DRIFT_IMAGES_DIR a Path built from get_picture_folder() / "Drift Correction
images", use Path methods (exists(), mkdir(parents=True, exist_ok=True),
joinpath) instead of os.path.join and os.makedirs, and ensure any code that
expects strings converts Paths via str() only where necessary; update references
to use the DRIFT_IMAGES_DIR Path methods consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e741060-b77b-46e3-aeea-da34a33b1907
📒 Files selected for processing (1)
src/odemis/acq/drift/__init__.py
yxkdejong
force-pushed
the
drift-alignment-correction
branch
from
df930b6 to
aa076cb
Compare
|
@pieleric , @tepals I have hijacked @yxkdejong PR and co-committed this feature in a new PR: #3487 hence closing this |
3 participants
No description provided.