[fix] minor fixes#3486
Conversation
Slider.__del__() does some important cleanups, which need to also be done by NumberSlider. Found by Github CodeQL.
There was a problem hiding this comment.
Pull request overview
This PR applies a small set of “minor fixes” across GUI and acquisition modules, primarily tightening type annotations and improving cleanup behavior.
Changes:
- Adjusts typing usage in the multipoint correlation controller (
stream_groups/previous_groupannotations). - Ensures
NumberSliderteardown also invokes base slider cleanup. - Adds a type hint + docstring tweak to the tiled acquisition DA-sorting helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/odemis/gui/cont/multi_point_correlation.py |
Updates typing imports and annotations for stream grouping state. |
src/odemis/gui/comp/slider.py |
Updates NumberSlider.__del__ to call superclass destructor. |
src/odemis/acq/stitching/_tiledacq.py |
Adds type hint to leader_quality and simplifies branching logic. |
src/odemis/acq/align/goffset.py |
Switches a local variable annotation to use Dict[...]. |
| self.stream_groups: Optional[ | ||
| dict[tuple[tuple[int], tuple[float]], list[int]]] = None # Dictionary to hold the stream groups | ||
| Dict[Tuple[Tuple[int], Tuple[float]], List[int]]] = None # Dictionary to hold the stream groups | ||
| # Key of the selected group chosen previously | ||
| self.previous_group: Optional[tuple[tuple[int], tuple[float]]] = None | ||
| self.previous_group: Optional[Tuple[Tuple[int], Tuple[float]]] = None |
pieleric
force-pushed
the
fix-minor-fixes
branch
from
92d736e to
bbf2787
Compare
|
Warning Review limit reached
More reviews will be available in 22 minutes and 9 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR improves type safety and cleanup robustness across the acquisition and GUI modules. It adds explicit type annotations to a results dictionary and function parameter in alignment and stitching code, ensures proper destructor chaining in NumberSlider to invoke base class cleanup, and standardizes type annotations in the multi-point correlation GUI component to use typing module generics instead of built-in forms. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/odemis/gui/cont/multi_point_correlation.py (1)
31-31: 💤 Low valueConsider using built-in generic types for Python 3.10+ compatibility.
Since the coding guidelines specify Python 3.10+, you can use lowercase built-in generics (
dict,list,tuple) directly instead of importing fromtyping, following PEP 585. While the current change brings consistency with existingListandOptionalimports, the modern approach would be:self.stream_groups: dict[tuple[tuple[int], tuple[float]], list[int]] | None = None self.previous_group: tuple[tuple[int], tuple[float]] | None = NoneThis would also allow removing
Dictfrom thetypingimports and eventually migrating other annotations (List→list,Optional[X]→X | None).Also applies to: 223-225
🤖 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/gui/cont/multi_point_correlation.py` at line 31, Update annotations to use PEP 585/604 built-in generics and native union syntax: remove or stop using Dict/List/Optional/Union from typing and change the annotations for self.stream_groups and self.previous_group (and the similar annotations at lines ~223-225) to use dict, list and X | None and tuple[...] forms (e.g., dict[tuple[tuple[int], tuple[float]], list[int]] | None and tuple[tuple[int], tuple[float]] | None) and adjust the import line (from typing import Tuple) accordingly so typing imports no longer include Dict/List/Optional/Union.Source: Coding guidelines
src/odemis/acq/align/goffset.py (1)
609-609: ⚡ Quick winUse
Tuplefrom typing for consistency and specify element types.The annotation mixes
Dictfrom thetypingmodule with the built-intuple. For consistency, useTuplefromtyping(already imported on line 16). Additionally, specify the tuple element types for better type safety—based on the test evidence, keys are(grating_index, detector_name)pairs.🔧 Suggested fix
- results: Dict[tuple, bool] = {} + results: Dict[Tuple[int, str], bool] = {}🤖 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/align/goffset.py` at line 609, Replace the loose annotation "results: Dict[tuple, bool] = {}" with a typed tuple using the typing.Tuple already imported; update it to "results: Dict[Tuple[int, str], bool] = {}" (keys are (grating_index: int, detector_name: str)) so the dict uses consistent typing and explicit element types.Source: Coding guidelines
src/odemis/acq/stitching/_tiledacq.py (1)
556-558: ⚡ Quick winRemove type information from the docstring.
The docstring includes
:return float:, but the coding guidelines require docstrings "without type information". The return type is already declared in the function signature (-> float), so the docstring should not duplicate it.📝 Suggested fix
def leader_quality(da: model.DataArray) -> float: """ - :return float: (> 0) The bigger, the more leadership + :return: (> 0) The bigger, the more leadership """🤖 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/stitching/_tiledacq.py` around lines 556 - 558, The docstring for leader_quality(da: model.DataArray) duplicates the return type; remove the type information so it only describes the return value (e.g., "Return the leadership score; the bigger, the more leadership") and leave the function signature's -> float to convey the type; update the triple-quoted string inside leader_quality accordingly so no ":return float:" or other type annotations remain.Source: Coding guidelines
🤖 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/gui/comp/slider.py`:
- Around line 575-578: The destructor __del__ is missing a return type
annotation and a docstring; update the method signature to include -> None and
add a concise docstring describing that it unbinds the linked_field
EVT_COMMAND_ENTER handler (self._update_slider) during cleanup and then calls
the superclass destructor; reference the symbols __del__, linked_field, Unbind,
_update_slider and super().__del__ to locate where to add the annotation and
docstring.
---
Nitpick comments:
In `@src/odemis/acq/align/goffset.py`:
- Line 609: Replace the loose annotation "results: Dict[tuple, bool] = {}" with
a typed tuple using the typing.Tuple already imported; update it to "results:
Dict[Tuple[int, str], bool] = {}" (keys are (grating_index: int, detector_name:
str)) so the dict uses consistent typing and explicit element types.
In `@src/odemis/acq/stitching/_tiledacq.py`:
- Around line 556-558: The docstring for leader_quality(da: model.DataArray)
duplicates the return type; remove the type information so it only describes the
return value (e.g., "Return the leadership score; the bigger, the more
leadership") and leave the function signature's -> float to convey the type;
update the triple-quoted string inside leader_quality accordingly so no ":return
float:" or other type annotations remain.
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Line 31: Update annotations to use PEP 585/604 built-in generics and native
union syntax: remove or stop using Dict/List/Optional/Union from typing and
change the annotations for self.stream_groups and self.previous_group (and the
similar annotations at lines ~223-225) to use dict, list and X | None and
tuple[...] forms (e.g., dict[tuple[tuple[int], tuple[float]], list[int]] | None
and tuple[tuple[int], tuple[float]] | None) and adjust the import line (from
typing import Tuple) accordingly so typing imports no longer include
Dict/List/Optional/Union.
🪄 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: 16a71d28-3cd1-46a7-9b6a-2ac679214e92
📒 Files selected for processing (4)
src/odemis/acq/align/goffset.pysrc/odemis/acq/stitching/_tiledacq.pysrc/odemis/gui/comp/slider.pysrc/odemis/gui/cont/multi_point_correlation.py
It should always return a positive float, but could return None if the metadata is missing. Found by Github CodeQL.
pieleric
force-pushed
the
fix-minor-fixes
branch
from
bbf2787 to
405bfb8
Compare
Small fixes to issues found automatically, via Github CodeQL, or via
grep " (dict|tuple|list|set)\[" -IER .