[refactor] util: split __init__.py into geometry and concurrent modules#3474
Open
tepals wants to merge 1 commit into
Open
[refactor] util: split __init__.py into geometry and concurrent modules#3474tepals wants to merge 1 commit into
tepals wants to merge 1 commit into
Conversation
tepals
added
the
llm-delegated
📝 WalkthroughWalkthroughThis PR extracts concurrency utilities into src/odemis/util/concurrent.py and 2D geometry utilities into src/odemis/util/geometry.py, and refactors src/odemis/util/init.py to re-export those helpers for backward compatibility. concurrent.py adds rate-limiting and timeout decorators, RepeatingTimer, BackgroundWorker, executeAsyncTask/bindFuture, and inspect_getmembers. geometry.py adds rectangle/line/polygon helpers (intersection, clipping, rotation, decomposing rotated rectangles, bbox). init.py retains a small set of helpers and now re-exports the new modules. sequenceDiagram
participant Caller
participant limit_invocation_wrapper
participant instance_queue
participant worker_thread
participant target_function
Caller->>limit_invocation_wrapper: call(...)
limit_invocation_wrapper->>instance_queue: enqueue(call,args)
instance_queue->>worker_thread: dequeue(next_call)
worker_thread->>target_function: invoke(args)
worker_thread->>Caller: set_result / propagate exception
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🧹 Nitpick comments (1)
src/odemis/util/concurrent.py (1)
149-149: 💤 Low valueConsider using spread syntax for tuple construction.
The spread syntax is clearer and avoids explicit tuple concatenation.
✨ Suggested change
- q.put((now, f, (self,) + args, kwargs)) + q.put((now, f, (self, *args), kwargs))🤖 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/util/concurrent.py` at line 149, Replace the explicit tuple concatenation (self,) + args with tuple unpacking to construct the arguments tuple more readably; in the q.put call in concurrent.py (the expression currently building (now, f, (self,) + args, kwargs)), change the third element to use spread/unpacking like (self, *args) so the queue entry becomes (now, f, (self, *args), kwargs).
🤖 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/util/concurrent.py`:
- Line 313: The function signature for executeAsyncTask uses an implicit
Optional for kwargs; update the type annotation to explicitly allow None (e.g.,
change kwargs: dict to kwargs: dict | None = None) so the default None is
represented with a union type; keep the function name executeAsyncTask and
existing defaults intact when making this change.
- Line 334: The bindFuture signature uses a bare "dict = None" for kwargs;
update the type annotation to explicitly allow None (match executeAsyncTask) by
changing the parameter to kwargs: dict | None = None so the type is an explicit
union with None, and ensure any references in the function body handle kwargs
possibly being None.
- Around line 368-430: Add static typing to both inspect_getmembers
implementations: annotate the first parameter (rename from object to obj to
avoid shadowing) as Any or types.ModuleType/Type[Any] as appropriate, annotate
predicate as Optional[Callable[[Any], bool]], and set the return type to
List[Tuple[str, Any]] (import typing names). Update both function signatures for
the Python >=3.11 and <=3.10 branches and add necessary imports (from typing
import Any, Callable, List, Optional, Tuple) at the top of the module.
In `@src/odemis/util/geom.py`:
- Around line 110-124: The nested helper _get_pos(xa, ya) lacks type hints and a
docstring; add parameter and return type annotations (e.g., xa: float, ya: float
-> int or appropriate enum/bitmask type) and insert a concise reStructuredText
docstring above the body describing parameters, return value, and the bitflags
(INSIDE, LEFT, RIGHT, LOWER, UPPER) behavior; keep the function name _get_pos
and references to xmin, xmax, ymin, ymax unchanged and ensure the docstring
explains these globals are used to determine the bitmask returned.
- Around line 274-307: The function separate_rect_rotation assumes exactly 4
corner points but indexes corners[3] without validation; add an explicit check
at the start of separate_rect_rotation to validate that len(corners) == 4 (or at
least >=4 if you prefer) and raise a clear ValueError (e.g.,
"separate_rect_rotation requires 4 corner points, got N") before any indexing,
so callers get an actionable error instead of an IndexError when corners is
malformed.
- Around line 189-205: Add explicit type hints to normalize_rect: import TypeVar
and Tuple from typing, declare a type variable like R = TypeVar("R",
bound=Tuple[float, float, float, float]) and annotate the signature as def
normalize_rect(rect: R) -> R:, keeping the existing implementation that returns
type(rect)((x1, y1, x2, y2)). This ensures the parameter is a 4-float tuple type
and the return type matches the input while satisfying the repo's typing
guideline; update imports accordingly.
---
Nitpick comments:
In `@src/odemis/util/concurrent.py`:
- Line 149: Replace the explicit tuple concatenation (self,) + args with tuple
unpacking to construct the arguments tuple more readably; in the q.put call in
concurrent.py (the expression currently building (now, f, (self,) + args,
kwargs)), change the third element to use spread/unpacking like (self, *args) so
the queue entry becomes (now, f, (self, *args), kwargs).
🪄 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: 08edf0a9-df5b-4d61-98ee-dc1cdb07b12d
📒 Files selected for processing (3)
src/odemis/util/__init__.pysrc/odemis/util/concurrent.pysrc/odemis/util/geom.py
pieleric
requested changes
May 28, 2026
Member
pieleric
left a comment
pieleric
left a comment
There was a problem hiding this comment.
Looks to me like a good separation. I only have one small suggestion.
tepals
force-pushed
the
SWM-221-split-util-init
branch
from
b49a5f9 to
6f24fbc
Compare
There was a problem hiding this comment.
Pull request overview
Refactors odemis.util by moving geometry- and concurrency-related helpers out of the package __init__.py into dedicated modules, while keeping backwards compatibility via re-exports from odemis.util.
Changes:
- Added
src/odemis/util/geometry.pycontaining 2D rectangle/line/polygon helpers and related constants. - Added
src/odemis/util/concurrent.pycontaining rate-limiting, timeout, timer/worker utilities, Future helpers, and aninspect.getmemberscompatibility wrapper. - Updated
src/odemis/util/__init__.pyto re-export moved symbols and removed the inlined implementations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/odemis/util/geometry.py | New module for 2D geometry utilities previously in odemis.util. |
| src/odemis/util/concurrent.py | New module for concurrency/timing/Future helpers previously in odemis.util. |
| src/odemis/util/init.py | Re-exports moved symbols for backward compatibility and keeps general helpers local. |
Extract 2D geometry functions into a new `geometry.py` module: - Rectangle operations: rect_intersect, intersect, normalize_rect, expand_rect, is_point_in_rect, rotate_rect, separate_rect_rotation - Line/polygon operations: perpendicular_distance, clip_line, get_polygon_bbox, slope_of_line, intercept_of_line, project_point_on_line - Constants: INSIDE, LEFT, RIGHT, LOWER, UPPER Extract concurrency utilities into a new `concurrent.py` module: - Rate limiting: limit_invocation - Threading: RepeatingTimer, BackgroundWorker - Future helpers: executeAsyncTask, bindFuture - Decorators: timeout - Introspection: inspect_getmembers General-purpose helpers remain in __init__.py All moved symbols are re-exported from `odemis.util` for backward compatibility. Co-authored-by: Copilot
tepals
force-pushed
the
SWM-221-split-util-init
branch
from
6f24fbc to
58ede4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/util/concurrent.py`:
- Line 87: Add explicit return type hints to the decorator factories
`limit_invocation` and `timeout`: declare a TypeVar (e.g., F bound to
Callable[..., Any]) and annotate each function to return a decorator of type
Callable[[F], F] (or the equivalent Callable[[Callable[..., Any]], Callable[...,
Any]]), and add any needed typing imports (TypeVar, Callable, Any) at the top;
update the signatures for `limit_invocation` and `timeout` accordingly so static
checkers know they return a decorator that accepts and returns the original
callable type.
🪄 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: f9a77d7f-6d3b-4553-9b3b-5c772f3bf35a
📒 Files selected for processing (3)
src/odemis/util/__init__.pysrc/odemis/util/concurrent.pysrc/odemis/util/geometry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/util/init.py
| logging.debug("Ending li thread") | ||
|
|
||
|
|
||
| def limit_invocation(delay_s: float): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add return type hints to the decorator factories.
limit_invocation (Line 87) and timeout (Line 167) both return a decorator but omit the return annotation. As per coding guidelines: "Always use type hints for function parameters and return types in Python code".
♻️ Proposed annotations
-def limit_invocation(delay_s: float):
+def limit_invocation(delay_s: float) -> Callable:-def timeout(seconds: float):
+def timeout(seconds: float) -> Callable:🤖 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/util/concurrent.py` at line 87, Add explicit return type hints to
the decorator factories `limit_invocation` and `timeout`: declare a TypeVar
(e.g., F bound to Callable[..., Any]) and annotate each function to return a
decorator of type Callable[[F], F] (or the equivalent Callable[[Callable[...,
Any]], Callable[..., Any]]), and add any needed typing imports (TypeVar,
Callable, Any) at the top; update the signatures for `limit_invocation` and
`timeout` accordingly so static checkers know they return a decorator that
accepts and returns the original callable type.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/util/geometry.py (1)
96-108: ⚡ Quick winDocument
clip_line's parameters, especially the unusual argument order.The signature takes
xmin, ymax, xmax, ymin(not the conventionalxmin, ymin, xmax, ymax), and the docstring documents neither the parameters nor the return value. This is an easy footgun for callers.Proposed fix
This implements the Cohen-Sutherland line clipping algorithm. Although it's not the most efficient clipping algorithm, it was chosen because it's best at cheaply determining the trivial cases (line being completely inside or outside the bounding box). + Note the unusual parameter order: xmin, ymax, xmax, ymin. + + :param xmin: minimum x of the clipping rectangle + :param ymax: maximum y of the clipping rectangle + :param xmax: maximum x of the clipping rectangle + :param ymin: minimum y of the clipping rectangle + :param x1: x of the first endpoint of the line + :param y1: y of the first endpoint of the line + :param x2: x of the second endpoint of the line + :param y2: y of the second endpoint of the line + :returns: clipped line as (x1, y1, x2, y2) rounded to integers, or + (None, None, None, None) if the line lies entirely outside the rectangle + Code based on https://github.com/scienceopen/cv-utils/blob/master/lineClipping.py Copyright (c) 2014 Michael Hirsch """As per coding guidelines: "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/util/geometry.py` around lines 96 - 108, Add a full reStructuredText docstring to clip_line describing each parameter (explicitly call out the unusual order xmin, ymax, xmax, ymin), the point coordinates (x1, y1, x2, y2), expected types, and the return tuple meaning (clipped x1, y1, x2, y2 with None when fully outside) plus any exceptions/edge cases; update the top of the clip_line docstring to include a short summary, param: entries for xmin, ymax, xmax, ymin, x1, y1, x2, y2, and a returns: section describing the Tuple[Optional[int], Optional[int], Optional[int], Optional[int]] semantics, ensuring wording follows the reStructuredText style guide.
🤖 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.
Nitpick comments:
In `@src/odemis/util/geometry.py`:
- Around line 96-108: Add a full reStructuredText docstring to clip_line
describing each parameter (explicitly call out the unusual order xmin, ymax,
xmax, ymin), the point coordinates (x1, y1, x2, y2), expected types, and the
return tuple meaning (clipped x1, y1, x2, y2 with None when fully outside) plus
any exceptions/edge cases; update the top of the clip_line docstring to include
a short summary, param: entries for xmin, ymax, xmax, ymin, x1, y1, x2, y2, and
a returns: section describing the Tuple[Optional[int], Optional[int],
Optional[int], Optional[int]] semantics, ensuring wording follows the
reStructuredText style guide.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f85445ed-3bfe-4fab-8454-ebb316796b6f
📒 Files selected for processing (3)
src/odemis/util/__init__.pysrc/odemis/util/concurrent.pysrc/odemis/util/geometry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/util/init.py
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
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.
Extract 2D geometry functions into a new
geometry.pymodule:Extract concurrency utilities into a new
concurrent.pymodule:General-purpose helpers remain in init.py
All moved symbols are re-exported from
odemis.utilfor backward compatibility.Co-authored-by: Copilot