refactor: add search helpers and rename parameters#8
refactor: add search helpers and rename parameters#8dhalmazna wants to merge 6 commits intofeat/segment-and-hyperpixel-scoringfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for upcoming search algorithms by centralizing common logic and low-level graph operations. It aims to keep the main algorithm implementations clean and optimized by providing dedicated utility modules for tasks such as efficient bitmask manipulation and batched evaluation of segment masks, ensuring a robust and scalable foundation. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a solid set of foundational utilities for search algorithms and bitmask operations. The new functions are well-documented and the separation into bitmask_graph.py and search_helpers.py is logical. My review focuses on improving consistency, efficiency, documentation accuracy, and robustness. I've suggested a more efficient implementation for mask_to_ids, a correction to a docstring, a robustness improvement for sample_connected_superset, and a change to ensure the new mask_to_ids utility is used consistently.
There was a problem hiding this comment.
Pull request overview
Adds foundational utilities to support upcoming Monte Carlo search algorithms by centralizing common terminal-state checks, batched mask evaluation, and extending bitmask graph primitives.
Changes:
- Added
ciao/algorithm/search_helpers.pywithis_terminalandevaluate_masksfor shared MCTS/MCGS logic. - Extended
ciao/algorithm/bitmask_graph.pywith node mutation helpers, random set-bit selection, and a connected-superset sampler. - Re-exported the new utilities from
ciao/algorithm/__init__.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ciao/algorithm/search_helpers.py | New shared helpers for terminal checks and batched reward evaluation. |
| ciao/algorithm/bitmask_graph.py | New bitmask primitives and sampling utilities for graph-based expansions. |
| ciao/algorithm/init.py | Exposes new utilities at the package level via imports and __all__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
ciao/algorithm/bitmask_graph.py
Outdated
| def sample_connected_superset( | ||
| base_mask: int, | ||
| target_length: int, | ||
| adj_masks: tuple[int, ...], | ||
| used_mask: int, | ||
| ) -> int: | ||
| """Sample a connected superset via random walk expansion. | ||
|
|
||
| IMPORTANT: This is NOT a uniform sampler over all connected supersets. | ||
| The distribution is biased towards segments discovered early and | ||
| depends on graph topology. This bias is acceptable for Monte Carlo | ||
| estimation in the parent algorithm. | ||
|
|
||
| Args: | ||
| base_mask: Starting set (must be non-empty and connected) | ||
| target_length: Desired size of the superset | ||
| adj_masks: Adjacency bitmasks for neighbor lookups | ||
| used_mask: Global exclusion mask (segments that must not be added) | ||
|
|
||
| Returns: | ||
| Bitmask of connected superset containing base_mask | ||
| """ | ||
| mask = base_mask | ||
|
|
||
| while mask.bit_count() < target_length: | ||
| frontier = get_frontier(mask, adj_masks, used_mask) | ||
| if frontier == 0: | ||
| break |
There was a problem hiding this comment.
This checking would be too computationally expensive. The mask will be always connected because of the way it is created - we always add a neighbor from the frontier.
dhalmazna
force-pushed
the
feat/algorithm-utils
branch
2 times, most recently
from
dd0ad92 to
b2ab7f8
Compare
dhalmazna
force-pushed
the
feat/segment-and-hyperpixel-scoring
branch
from
f30a530 to
2c6a6ab
Compare
dhalmazna
force-pushed
the
feat/algorithm-utils
branch
from
b2ab7f8 to
4282be2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ciao/algorithm/search_helpers.py (1)
14-23: Consider enforcing a positivemax_depthcontract.At Line 21,
max_depth <= 0makes states terminal immediately. A fast-fail guard can prevent silent misuse.Proposed guard
def is_terminal( current_region: frozenset[int], image_graph: ImageGraph, used_segments: frozenset[int], max_depth: int, ) -> bool: """Check if state is terminal (max depth or no frontier).""" + if max_depth <= 0: + raise ValueError(f"max_depth must be positive, got {max_depth}.") return len(current_region) >= max_depth or not image_graph.get_frontier( current_region, used_segments )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/search_helpers.py` around lines 14 - 23, The is_terminal function should enforce a positive max_depth to avoid silently treating all states as terminal when max_depth <= 0; update is_terminal to validate the max_depth parameter (e.g., raise ValueError or assert) at the start of the function and include a clear message referencing max_depth, leaving the existing logic that checks len(current_region) >= max_depth and image_graph.get_frontier unchanged; locate the validation in the is_terminal definition so callers receive immediate feedback if they pass non-positive depths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ciao/algorithm/search_helpers.py`:
- Around line 14-23: The is_terminal function should enforce a positive
max_depth to avoid silently treating all states as terminal when max_depth <= 0;
update is_terminal to validate the max_depth parameter (e.g., raise ValueError
or assert) at the start of the function and include a clear message referencing
max_depth, leaving the existing logic that checks len(current_region) >=
max_depth and image_graph.get_frontier unchanged; locate the validation in the
is_terminal definition so callers receive immediate feedback if they pass
non-positive depths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f53bc35-b679-4ef9-a881-88ca421e97d5
📒 Files selected for processing (4)
ciao/algorithm/__init__.pyciao/algorithm/graph.pyciao/algorithm/search_helpers.pyciao/data/segmentation.py
dhalmazna
force-pushed
the
feat/algorithm-utils
branch
from
4282be2 to
a53863c
Compare
dhalmazna
changed the title
Adames4
left a comment
Adames4
left a comment
There was a problem hiding this comment.
Please re-request review when PR is ready, thanks
dhalmazna
changed the title
dhalmazna
changed the title
Context:
This PR refactors the hyperpixel scoring utilities to directly accept abstract sets instead of lists, improving type safety and removing redundant conversions. It also adds the
is_terminalfunction.What was added / changed:
algorithm/search_helpers.py: Introduced shared utilities for search algorithms:is_terminal: A state-check helper to determine if a search path has reached its maximum length or exhausted its frontier.calculate_hyperpixel_deltasto acceptSequence[Set[int]].segmentstoregionacrossImageGraphand scoring utilities.create_surrogate_datasetby removing unnecessary sorting and leveragingSetabstractions.Related Task:
XAI-29