feature: add core data structures and search algorithms#3
feature: add core data structures and search algorithms#3
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CIAO: an XAI library for image classification. Introduces four hyperpixel search algorithms (lookahead, MCTS, MCGS, potential), segmentation and bitmask utilities, node structures, ModelPredictor and batching utilities, a 1000-entry ImageNet labels resource, and README + project metadata updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant Segmentation as "Segmentation\n(create_segmentation)"
participant Bitmask as "BitmaskGraph\n(get_frontier / sample)"
participant Search as "SearchAlgo\n(MCTS/MCGS/lookahead/potential)"
participant Predictor as "ModelPredictor"
participant Results
User->>Segmentation: create_segmentation(image)
Segmentation-->>User: segments, adjacency (adj_masks)
User->>Search: build_all_hyperpixels(segments, model, target_class)
loop per seed
Search->>Bitmask: compute frontier / propose candidate masks
Bitmask-->>Search: candidate masks
Search->>Predictor: evaluate_masks(batch of masks)
Predictor-->>Search: delta scores / logits
Search->>Search: select/commit best prefix (update used_mask)
end
Search-->>Results: hyperpixels list (masks, scores, stats)
Results-->>User: explanations / visualizations
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 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 @dhalmazna, 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 establishes the fundamental mathematical and algorithmic infrastructure for the CIAO explainable AI package. It introduces robust data structures for efficient graph manipulation and node management, alongside a suite of advanced search algorithms designed to identify influential image regions. These core components are integrated with comprehensive utility functions for model interaction, image processing, and segmentation, laying the groundwork for the user-facing Highlights
Changelog
Activity
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 significant amount of core functionality, including various search algorithms (Greedy Lookahead, MCTS, MCGS, Potential-based) and the underlying data structures and utilities to support them. The code is well-structured, leveraging bitmasks for efficiency. My review focuses on improving robustness, maintainability, and consistency. Key suggestions include replacing print statements with a proper logging framework, handling potential None returns more gracefully to prevent crashes, removing unused parameters, and ensuring all documentation is in English for consistency.
ciao/algorithm/mcgs.py
Outdated
| if mode == "rave": | ||
| uct_result = select_uct_child_rave( | ||
| node, exploration_c, virtual_loss, rave_k | ||
| ) | ||
| assert uct_result is not None | ||
| action, child = uct_result | ||
|
|
||
| # Apply virtual loss to edge | ||
| if action not in node.pending_edges: | ||
| node.pending_edges[action] = 0 | ||
| node.pending_edges[action] += 1 | ||
|
|
||
| actions_taken.append(action) | ||
| else: | ||
| uct_result = select_uct_child(node, exploration_c, virtual_loss) | ||
| assert uct_result is not None | ||
| action, child = uct_result | ||
|
|
||
| # Apply virtual loss to edge | ||
| if action not in node.pending_edges: | ||
| node.pending_edges[action] = 0 | ||
| node.pending_edges[action] += 1 | ||
|
|
||
| actions_taken.append(action) |
There was a problem hiding this comment.
Using assert uct_result is not None could lead to a crash with an AssertionError if select_uct_child* returns None (e.g., if a node has no children). While the surrounding logic might prevent this, it's safer to handle this case gracefully by checking for None and breaking the loop. You can also refactor the common logic out of the if/else block.
if mode == "rave":
uct_result = select_uct_child_rave(
node, exploration_c, virtual_loss, rave_k
)
else:
uct_result = select_uct_child(node, exploration_c, virtual_loss)
if uct_result is None:
break # No valid child to select
action, child = uct_result
# Apply virtual loss to edge
if action not in node.pending_edges:
node.pending_edges[action] = 0
node.pending_edges[action] += 1
actions_taken.append(action)| if mode == "rave": | ||
| child = select_uct_child_rave( | ||
| node, exploration_c, virtual_loss, rave_k | ||
| ) | ||
| else: | ||
| child = select_uct_child(node, exploration_c, virtual_loss) | ||
|
|
||
| assert child is not None | ||
| child.pending += 1 | ||
| node = child | ||
| path.append(node) |
There was a problem hiding this comment.
Using assert child is not None can lead to a crash if select_uct_child returns None. It's safer to handle this case gracefully, for example by checking for None and breaking the loop.
| if mode == "rave": | |
| child = select_uct_child_rave( | |
| node, exploration_c, virtual_loss, rave_k | |
| ) | |
| else: | |
| child = select_uct_child(node, exploration_c, virtual_loss) | |
| assert child is not None | |
| child.pending += 1 | |
| node = child | |
| path.append(node) | |
| if mode == "rave": | |
| child = select_uct_child_rave( | |
| node, exploration_c, virtual_loss, rave_k | |
| ) | |
| else: | |
| child = select_uct_child(node, exploration_c, virtual_loss) | |
| if child is None: | |
| break # No valid child to select | |
| child.pending += 1 | |
| node = child | |
| path.append(node) |
| total_gpu_evaluations = 0 | ||
|
|
||
| # --- MAIN MCTS LOOP --- | ||
| for _ in tqdm(range(num_iterations), desc=f"MCTS-{mode.upper()}", unit="iter"): |
There was a problem hiding this comment.
Using tqdm directly prints to the console, which might not be desirable when this code is used as a library. Consider making the progress bar optional, for example by adding a show_progress: bool = True parameter and wrapping tqdm in an if block.
ciao/algorithm/mcgs.py
Outdated
|
|
||
| # --- MAIN MCGS LOOP --- | ||
| mode_label = f"MCGS-{mode.upper()}" | ||
| for iteration in tqdm(range(num_iterations), desc=f" {mode_label}", ncols=80): |
There was a problem hiding this comment.
Using tqdm directly prints to the console, which might not be desirable when this code is used as a library. Consider making the progress bar optional, for example by adding a show_progress: bool = True parameter and wrapping tqdm in an if block.
ciao/algorithm/potential.py
Outdated
| total_evaluations = 0 | ||
| total_samples = 0 | ||
|
|
||
| print("\n=== Sequential Monte Carlo Set Extension ===") |
There was a problem hiding this comment.
Using print() for logging throughout this file is not ideal for a library. It's better to use the logging module. This allows consumers of your library to control log levels and output destinations.
ciao/utils/calculations.py
Outdated
| ].item() | ||
|
|
||
| # Get replacement image using the specified strategy | ||
| assert predictor.replacement_image is not None |
There was a problem hiding this comment.
The function relies on predictor.replacement_image being set before this function is called, which is enforced by an assert. This creates a temporal dependency that can make the code harder to reason about and test in isolation. Consider if this dependency can be made more explicit, for example by passing the replacement image as a parameter.
ciao/utils/calculations.py
Outdated
| if torch.cuda.is_available(): | ||
| torch.cuda.empty_cache() |
There was a problem hiding this comment.
Calling torch.cuda.empty_cache() can be expensive and may not be necessary. Deleting tensors (del batch_inputs, masked_logits) should be sufficient for PyTorch's garbage collector to free up memory. You might want to profile this to see if empty_cache() is providing a real benefit or just adding overhead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (18)
ciao/utils/search_utils.py (1)
14-18:adj_maskstyped as baretuple; should betuple[int, ...]to matchget_frontier's signature.✏️ Proposed fix
-def is_terminal(mask: int, adj_masks: tuple, used_mask: int, max_depth: int) -> bool: +def is_terminal(mask: int, adj_masks: tuple[int, ...], used_mask: int, max_depth: int) -> bool:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/search_utils.py` around lines 14 - 18, The parameter adj_masks in is_terminal is typed as a bare tuple but get_frontier expects tuple[int, ...]; update is_terminal's signature to use adj_masks: tuple[int, ...] and ensure any callers pass the same typed structure so the types align (refer to functions is_terminal and get_frontier to locate and update the annotation).ciao/__init__.py (1)
7-7: Missing__all__—CIAOExplainerwon't be part of the package's public surface contract.✏️ Proposed fix
from ciao.explainer.ciao_explainer import CIAOExplainer + +__all__ = ["CIAOExplainer"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/__init__.py` at line 7, The package __init__ currently imports CIAOExplainer but does not declare a public API, so add an explicit __all__ that includes "CIAOExplainer" to make it part of the package's public surface; update ciao/__init__.py to declare __all__ = ["CIAOExplainer"] (keeping the existing import from ciao.explainer.ciao_explainer import CIAOExplainer) so consumers using from ciao import * or introspection will expose CIAOExplainer.README.md (1)
84-116: Project structure lists files that don't exist in this PR or PR#2.Modules such as
ciao/data/,ciao/explainer/,ciao/visualization/,ciao/__main__.py, andconfigs/are shown in the directory tree but are not introduced until future PRs. Contributors cloning this branch will find a mismatched structure. Consider either trimming the tree to only what's present on this branch, or adding a note that the tree shows the planned final layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 84 - 116, The README's project tree currently lists modules that are not present in this branch (e.g., ciao/data/, ciao/explainer/, ciao/visualization/, ciao/__main__.py, configs/); update README.md so the directory tree either (a) only shows files and directories actually in this PR, or (b) keeps the full planned tree but clearly marks missing items as "planned" or "coming in future PRs" (add a short note and/or annotate entries like ciao/data/ and configs/ as planned). Ensure references to files such as ciao/explainer/ciao_explainer.py, ciao/visualization/visualization.py, and configs/ciao.yaml are consistent with the branch contents.ciao/algorithm/mcts.py (2)
436-442: Unusedindicesvariable from unpacking.
indicesfrom thezip(*masks_to_evaluate)unpacking is never used. Prefix with_to signal intent.Proposed fix
- indices, masks = zip(*masks_to_evaluate, strict=True) + _indices, masks = zip(*masks_to_evaluate, strict=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/mcts.py` around lines 436 - 442, The unpacking in the masks evaluation branch creates an unused variable `indices` (in the line that currently does `indices, masks = zip(*masks_to_evaluate, strict=True)`); update this to `_indices, masks = zip(*masks_to_evaluate, strict=True)` (or simply `_` instead of `_indices`) so the intent of an unused value is clear in the `evaluate_masks` logic inside the gpu evaluation block of `mcts.py`.
559-593: Simplify segment extraction and dict iteration.Two minor cleanups:
- Line 561: Use
seg_id in scoresinstead ofseg_id in scores.keys().- Lines 591-593: Use the existing
mask_to_idsutility instead of manually iteratingrange(next_id).Proposed fix
available_segments = [ - seg_id for seg_id in scores.keys() if seg_id not in processed_segments + seg_id for seg_id in scores if seg_id not in processed_segments ]- hyperpixel_segments = [ - seg_id for seg_id in range(next_id) if hyperpixel_mask & (1 << seg_id) - ] + hyperpixel_segments = mask_to_ids(hyperpixel_mask)Note:
mask_to_idsis already imported fromciao.structures.bitmask_graphat line 28.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/mcts.py` around lines 559 - 593, Replace the two manual iterations with the simpler constructs: for available_segments, iterate directly over scores (use "seg_id for seg_id in scores if seg_id not in processed_segments" instead of "scores.keys()"); for hyperpixel_segments, call the existing utility mask_to_ids(hyperpixel_mask, next_id) (or mask_to_ids(hyperpixel_mask) if that is the function signature) instead of the manual range/bit-check loop. These changes touch the block that calls build_hyperpixel_mcts and the variables scores, processed_segments, hyperpixel_mask and next_id.ciao/algorithm/potential.py (3)
500-503: Useseg_id in scoresinstead ofseg_id in scores.keys().Per Ruff SIM118, iterating over
.keys()is redundant.Proposed fix
available_segments = [ - seg_id for seg_id in scores.keys() if seg_id not in processed_segments + seg_id for seg_id in scores if seg_id not in processed_segments ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/potential.py` around lines 500 - 503, The list comprehension building available_segments uses the redundant .keys() call; update it to check membership directly against the mapping by changing the comprehension to use "if seg_id not in processed_segments and seg_id in scores" (or simply iterate over scores and filter out processed_segments) so that scores is used directly instead of scores.keys(); this touches the variable available_segments and the variables scores and processed_segments in the potential computation code.
379-411:scoreslist may containNoneif a prefix mask is somehow both in cache and inmissing_indices.
scoresis initialized as[None] * len(prefix_masks)and filled conditionally. While the current logic correctly partitions indices between cache hits andmissing_indices, the type ofscoresislist[None]which later gets compared with> max_score(line 409). If any entry remainedNonedue to a future code change, this would raise aTypeErrorat runtime.Consider initializing with
-float("inf")or adding a type annotation to make the intent explicit:Proposed fix
- scores = [None] * len(prefix_masks) + scores: list[float] = [-float("inf")] * len(prefix_masks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/potential.py` around lines 379 - 411, The scores list is initialized with None which can cause a TypeError when compared to max_score; change initialization of scores in the selection logic to a numeric sentinel (e.g., scores = [-float("inf")] * len(prefix_masks)) and keep setting cache hits via scores[i] = cache[mask] and computed values from calculate_hyperpixel_deltas (used with missing_indices) as you already do; ensure every branch that assigns to scores (the cache hit loop and the for i, score in zip(missing_indices, computed_scores) block) writes numeric values so no None remains before the final max search using best_idx and max_score.
112-113: Replaceprint()calls withloggingmodule.Same pattern as
lookahead_bitset.py— extensive debugprint()statements throughoutbuild_hyperpixel_using_potential,sampling_phase, andselect_best_prefix. Uselogging.debug()/logging.info()for controllable verbosity.Also applies to: 121-123, 128-130, 133-133, 155-155, 167-170, 186-188, 190-190, 315-315, 415-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/potential.py` around lines 112 - 113, Replace the numerous print() debug/info statements in build_hyperpixel_using_potential, sampling_phase, and select_best_prefix with logging calls following the same pattern used in lookahead_bitset.py: import the logging module (if not already), replace user-facing prints with logging.info(...) and verbose/debug prints with logging.debug(...), and keep the message text identical or more descriptive to preserve context (e.g., the seed/target/sim counts and progress messages referenced around Seed: {seed_idx}, Target: {desired_length}, Sims: {num_simulations}). Make these replacements for all occurrences noted (around lines with prints at 112-113, 121-123, 128-130, 133, 155, 167-170, 186-188, 190, 315, 415-421) so verbosity can be controlled via logger configuration. Ensure imports and any logger = logging.getLogger(__name__) usage match lookahead_bitset.py.ciao/structures/nodes.py (1)
32-64: Consider typed structures for edge/RAVE stats instead ofdict[str, float].The string-keyed dicts for
edge_statsandrave_stats(e.g.,{"N": 0, "W": 0.0, "Q": 0.0, "max_reward": -inf}) are easy to misuse — typos in keys silently create new entries and"N"(an integer count) is typed asfloat. ATypedDictordataclasswould catch key errors statically and clarify types.This is a low-risk pattern for now since
init_edgecentralizes creation, but worth considering as the codebase grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/nodes.py` around lines 32 - 64, Replace the loose dicts used for per-edge stats with a typed structure to prevent key typos and clarify types: define a TypedDict or dataclass (e.g., EdgeStats or EdgeStatsDict) containing fields N: int, W: float, Q: float, max_reward: float and update the MCGSNode.edge_stats and MCGSNode.rave_stats annotations to use that type; then refactor MCGSNode.init_edge to create and assign that typed object instead of a string-keyed dict and adjust any code accessing keys (e.g., places that read/write edge_stats[action]["N"] etc.) to use the typed fields or keys consistently.ciao/algorithm/mcgs.py (2)
322-331: RAVE backup recomputes frontier for every node in every path — potentially expensive.
backup_paths_ravecallsget_frontier(node.mask, adj_masks, used_mask)for every node in every path during backpropagation. Withbatch_sizepaths of depth up todesired_length, this is O(batch_size × depth × frontier_cost) per iteration.For moderate parameters this is likely acceptable, but consider caching the frontier on the node (e.g.,
node.frontier_cache) if profiling reveals this as a bottleneck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/mcgs.py` around lines 322 - 331, backup_paths_rave currently recomputes the frontier for every node via get_frontier(node.mask, adj_masks, used_mask) during backpropagation, causing repeated expensive work; to fix, add a cached frontier on the node (e.g., node.frontier_cache) and change backup_paths_rave to check and reuse node.frontier_cache before calling get_frontier, updating node.frontier_cache when absent or invalid, then use that cached bitmask for the membership check and call update_rave_stats(node, seg_id, reward) as before; locate get_frontier, backup_paths_rave, and update_rave_stats to implement the cache and ensure any cache invalidation logic is applied where node.mask or adj_masks/used_mask change.
636-670: Same minor cleanup opportunities as inmcts.py.
- Line 638:
seg_id in scores.keys()→seg_id in scores- Lines 668-670: Replace manual bit-checking with
mask_to_ids(hyperpixel_mask)(already imported viaiter_bits;mask_to_idswould need to be added to the import).Proposed fix
available_segments = [ - seg_id for seg_id in scores.keys() if seg_id not in processed_segments + seg_id for seg_id in scores if seg_id not in processed_segments ]Add
mask_to_idsto the import fromciao.structures.bitmask_graph:from ciao.structures.bitmask_graph import ( add_node, get_frontier, iter_bits, + mask_to_ids, sample_connected_superset, )Then:
- hyperpixel_segments = [ - seg_id for seg_id in range(next_id) if hyperpixel_mask & (1 << seg_id) - ] + hyperpixel_segments = mask_to_ids(hyperpixel_mask)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/mcgs.py` around lines 636 - 670, Replace the small inefficiencies in the loop: change the comprehension that builds available_segments to use "seg_id in scores" instead of "seg_id in scores.keys()", and replace the manual bit-checking that builds hyperpixel_segments with a call to mask_to_ids(hyperpixel_mask); also add mask_to_ids to the existing import from ciao.structures.bitmask_graph so mask_to_ids is available for use in build_hyperpixel_mcgs result handling.ciao/structures/bitmask_graph.py (2)
97-148:base_frontierparameter is unused — document deprecation or remove.The
base_frontierparameter is accepted but never used (the docstring says "unused, kept for compatibility"). If this is a deliberate compatibility shim, consider marking it as deprecated or removing it to avoid confusion for new callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/bitmask_graph.py` around lines 97 - 148, The parameter base_frontier in sample_connected_superset is unused; either remove it from the function signature and update all call sites (and tests) to stop passing it, or retain it but mark it explicitly deprecated: add a short note to the docstring and emit a runtime warnings.warn("base_frontier is deprecated and ignored", DeprecationWarning) at the top of sample_connected_superset so callers are informed, and keep behavior unchanged; update any internal references to call _pick_weighted_frontier_segment / pick_random_set_bit using the computed frontier only and remove mentions of base_frontier from comments and docs.
151-207: Mixed RNG sources may complicate reproducibility.
_pick_weighted_frontier_segmentusesnp.random.choice(line 206) while other sampling in this module uses Python'srandommodule (e.g.,pick_random_set_bitat line 58). If reproducible runs are needed (e.g., for debugging or benchmarking), both RNGs need to be seeded independently. Consider standardizing on one RNG, or at least documenting the dual-source requirement for seed management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/bitmask_graph.py` around lines 151 - 207, The function _pick_weighted_frontier_segment currently uses np.random.choice while other samplers like pick_random_set_bit use Python's random module, which breaks reproducibility unless both RNGs are seeded separately; fix by standardizing RNG usage—either switch _pick_weighted_frontier_segment to accept and use a single RNG (e.g., pass in a random.Random or a numpy.random.Generator) or replace np.random.choice with the same RNG used by pick_random_set_bit, and update callers to pass the chosen RNG; ensure you reference and use the same RNG instance across pick_random_set_bit and _pick_weighted_frontier_segment (or document dual-RNG seeding requirements if you intentionally keep both).ciao/utils/segmentation.py (1)
88-120:build_adjacency_graphiterates pixel-by-pixel — consider using NumPy vectorized adjacency detection.For large images, the triple nested Python loop (horizontal, vertical, diagonal) over every pixel is slow. NumPy operations like comparing shifted arrays (
segments[:-1, :] != segments[1:, :]) would be significantly faster for building the adjacency set.This is fine if segmentation is a one-time init cost, but worth noting for scalability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/segmentation.py` around lines 88 - 120, build_adjacency_graph currently loops pixel-by-pixel; replace the triple nested loops with vectorized NumPy adjacency detection by comparing shifted arrays (e.g., segments[:-1,:] vs segments[1:,:], segments[:,:-1] vs segments[:,1:], and the two diagonals when neighborhood==8) to produce pairs of differing segment ids, use np.where and flatten/zip or np.stack+unique to get edge pairs, then call adj_graph.add_edges_from on those pairs instead of adding edges one-at-a-time; keep function signature build_adjacency_graph(segments, neighborhood=8) and ensure you still add all segment_ids to adj_graph before adding edges.ciao/algorithm/lookahead_bitset.py (1)
56-56: Replaceprint()calls withloggingfor production readiness.There are numerous
print()statements throughout this module for debug tracing. These will produce noisy output in production and cannot be easily toggled. Use Python'sloggingmodule atDEBUGorINFOlevel instead, so callers can control verbosity.This applies across all functions in this file (
build_hyperpixel_greedy_lookahead,build_all_hyperpixels_greedy_lookahead).Also applies to: 73-76, 78-80, 104-106, 113-113, 128-130, 287-288, 318-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/lookahead_bitset.py` at line 56, Replace all raw print(...) calls in this module with Python logging calls: import logging at the top, create logger = logging.getLogger(__name__), and use logger.debug(...) or logger.info(...) instead of print for the debug/tracing messages found in build_hyperpixel_greedy_lookahead and build_all_hyperpixels_greedy_lookahead (e.g., the "Starting greedy lookahead from seed {seed_idx}" message and the other prints referenced around lines 73-76, 78-80, 104-106, 113, 128-130, 287-288, 318-320). Preserve the original message text and formatting but pass it to logger.debug or logger.info so callers can control verbosity via the logging configuration.ciao/utils/calculations.py (3)
7-14:replacement_imageis initialized asNoneand relied upon viaassert— consider a clearer lifecycle.
ModelPredictor.replacement_imageis set toNonein__init__(line 14) and must be set externally beforecalculate_hyperpixel_deltasis called (which asserts it's notNoneat line 331). This implicit contract is fragile — callers have no compile-time signal that setup is incomplete.Consider either requiring it in the constructor, providing a setter method that validates, or raising a descriptive error instead of using
assert(assertions are stripped in optimized mode with-O).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 7 - 14, ModelPredictor currently sets replacement_image = None in __init__ but calculate_hyperpixel_deltas asserts it's set, which is fragile; add an explicit setter and replace the assert with a clear runtime check. Implement a method set_replacement_image(self, image) on ModelPredictor that validates the image (type/shape/device) and assigns self.replacement_image, update __init__ docstring to mention the lifecycle, and change the assertion in calculate_hyperpixel_deltas to raise a descriptive ValueError (e.g., "replacement_image must be set via set_replacement_image before calling calculate_hyperpixel_deltas").
160-167: Remove commented-out experiment code.Lines 163-165 contain a commented-out alternative implementation. This is dead code that adds confusion. Remove it or document the intent in a proper comment/ADR if it's planned work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 160 - 167, Remove the dead experimental code inside the with torch.no_grad() block that calls get_predictions and computes log-odds (the commented lines referencing "probabilities = self.get_predictions(input_batch)" and "result = torch.log(probabilities) - torch.log(1 - probabilities)"); simply return the logits slice as currently done (outputs[:, target_class_idx]) or, if you intend to preserve the experiment, move that logic and explanation into a dedicated ADR or a clearly named helper (e.g., a new method compute_log_odds) instead of leaving commented-out code in the body of the method.
203-211: Replaceprint()calls withloggingin utility functions.Same pattern as the algorithm modules —
create_surrogate_datasetandcalculate_scores_from_surrogateuseprint()for diagnostics. Useloggingfor consistent, controllable output.Also applies to: 253-254, 283-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 203 - 211, The utility functions in ciao/utils/calculations.py currently use print() for diagnostics (e.g., the prints showing original_logit and class probability around the predictor.get_class_logit_batch and predictor.get_predictions calls) — replace these print() calls with a module logger: add "import logging" and "logger = logging.getLogger(__name__)" at top if missing, then change prints to logger.debug() or logger.info() as appropriate (use logger.debug for verbose diagnostics); make the same replacements in the other occurrences mentioned (around the blocks at the create_surrogate_dataset and calculate_scores_from_surrogate areas, i.e., the lines you noted at 253-254 and 283-284) so all diagnostic output uses the logger consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ciao/__init__.py`:
- Around line 1-5: The module docstring's first line in ciao/__init__.py lacks a
trailing period; update the top-level docstring so its first line ends with a
period (e.g., change the first line of the triple-quoted string to "CIAO:
Contextual Importance Assessment via Obfuscation.") to satisfy Ruff D415 and
keep the rest of the docstring unchanged.
- Line 7: The top-level import of CIAOExplainer from
ciao.explainer.ciao_explainer is causing ImportError when that module isn't
present; remove or guard the direct import in __init__ and instead either
perform a lazy import where CIAOExplainer is actually used or wrap the import in
a try/except that sets CIAOExplainer = None (or a safe fallback) and optionally
logs the absence; update any code that expects CIAOExplainer to handle the
None/fallback case so package import no longer fails if
ciao.explainer.ciao_explainer is missing.
In `@ciao/algorithm/lookahead_bitset.py`:
- Around line 186-198: The fallback in lookahead_bitset.py where mask not in
candidates and first_step = _find_first_step(current_mask, new_mask) can
silently pick an arbitrary segment; update the BFS to log a warning when this
branch is hit so unexpected reachability is visible: inside the block that
computes first_step for depth > 1, call your logger (or warnings.warn) to emit a
warning that includes the current_mask, new_mask, and computed first_step and
note that this is a fallback path; keep behavior unchanged (still set
candidates[new_mask]=first_step) but ensure the warning message gives enough
context to debug (_find_first_step, current_mask, new_mask, first_step).
In `@ciao/imagenet_classes.txt`:
- Line 419: Update the two inconsistent labels by replacing the exact string
"Band Aid" with the hyphenated trademark form "Band-Aid" and replace "web site"
with the single-word modern form "website" (locate these entries by their
current label text "Band Aid" and "web site" in the imagenet_classes list and
edit them in place).
In `@ciao/utils/calculations.py`:
- Around line 78-96: The get_replacement_image function uses hardcoded 224 for
spatial dims causing shape mismatches for non-ImageNet inputs; instead read
height and width from the input tensor (e.g., h = input_tensor.size(1), w =
input_tensor.size(2)) and use those when expanding mean_color in
calculate_image_mean_color/replacement_image, when building interlacing indices
(torch.arange(0, h, 2) and torch.arange(0, w, 2) for even row/column indices
used with replacement_image and even_indices), and when creating solid_color
replacements so all branches (mean_color, interlacing, solid_color) produce
replacement_image with matching [3, h, w] shape.
- Around line 338-345: The gpu_segments tensor is being re-created on the GPU
each iteration; move the conversion
torch.from_numpy(segments).to(predictor.device) out of the for batch_start in
range(...) loop so it is computed once and reused. Specifically, create
gpu_segments = torch.from_numpy(segments).to(predictor.device) before the loop
that iterates over batch_start (the loop where batch_inputs =
input_batch.repeat(...) is used) and remove the in-loop conversion so subsequent
iterations reuse the same gpu_segments tensor.
In `@ciao/utils/search_utils.py`:
- Around line 21-38: In evaluate_masks, detect any zero bitmask in the masks
list before calling iter_bits/calculate_hyperpixel_deltas: iterate over masks
and if any mask == 0 raise a ValueError (or convert to a non-empty default) with
a clear message identifying the offending index and that a zero bitmask produced
an empty segment list; reference the masks parameter and the all_segment_ids
construction that uses iter_bits so the error points to the real root cause
rather than the downstream calculate_hyperpixel_deltas "Empty segment list"
error.
In `@ciao/utils/segmentation.py`:
- Around line 123-154: Translate all Czech docstrings and inline comments in
build_fast_adjacency_list (and similarly in build_adjacency_bitmasks) into clear
English: update the function docstring to describe args and return value in
English, convert inline comments such as the initialization comment, the
neighbor offsets comment, the "if neighbor exists" check, and the final
conversion/sorting comment to English, and keep the original variable names
(e.g., temp_adj, hex_neighbors, final_adj) and logic unchanged so only textual
content is modified for readability by an international team.
In `@pyproject.toml`:
- Line 29: The numpy dependency in pyproject.toml is too permissive
("numpy>=1.21.0"); update that requirement to a tighter range to avoid pulling
in incompatible NumPy 2.x versions—for example replace "numpy>=1.21.0" with
"numpy>=1.24.0,<3" (or at minimum "numpy>=1.24.0") in pyproject.toml, then
regenerate/lock dependencies and run the test matrix / dependency resolver to
verify compatibility with torch and scikit-image.
In `@README.md`:
- Line 83: The fenced code block containing the snippet "ciao/" lacks a language
specifier; update the opening triple-backticks to include a language (e.g.,
change ``` to ```text) so the block is annotated (identify the block by its
content "ciao/") and commit the README.md change.
---
Nitpick comments:
In `@ciao/__init__.py`:
- Line 7: The package __init__ currently imports CIAOExplainer but does not
declare a public API, so add an explicit __all__ that includes "CIAOExplainer"
to make it part of the package's public surface; update ciao/__init__.py to
declare __all__ = ["CIAOExplainer"] (keeping the existing import from
ciao.explainer.ciao_explainer import CIAOExplainer) so consumers using from ciao
import * or introspection will expose CIAOExplainer.
In `@ciao/algorithm/lookahead_bitset.py`:
- Line 56: Replace all raw print(...) calls in this module with Python logging
calls: import logging at the top, create logger = logging.getLogger(__name__),
and use logger.debug(...) or logger.info(...) instead of print for the
debug/tracing messages found in build_hyperpixel_greedy_lookahead and
build_all_hyperpixels_greedy_lookahead (e.g., the "Starting greedy lookahead
from seed {seed_idx}" message and the other prints referenced around lines
73-76, 78-80, 104-106, 113, 128-130, 287-288, 318-320). Preserve the original
message text and formatting but pass it to logger.debug or logger.info so
callers can control verbosity via the logging configuration.
In `@ciao/algorithm/mcgs.py`:
- Around line 322-331: backup_paths_rave currently recomputes the frontier for
every node via get_frontier(node.mask, adj_masks, used_mask) during
backpropagation, causing repeated expensive work; to fix, add a cached frontier
on the node (e.g., node.frontier_cache) and change backup_paths_rave to check
and reuse node.frontier_cache before calling get_frontier, updating
node.frontier_cache when absent or invalid, then use that cached bitmask for the
membership check and call update_rave_stats(node, seg_id, reward) as before;
locate get_frontier, backup_paths_rave, and update_rave_stats to implement the
cache and ensure any cache invalidation logic is applied where node.mask or
adj_masks/used_mask change.
- Around line 636-670: Replace the small inefficiencies in the loop: change the
comprehension that builds available_segments to use "seg_id in scores" instead
of "seg_id in scores.keys()", and replace the manual bit-checking that builds
hyperpixel_segments with a call to mask_to_ids(hyperpixel_mask); also add
mask_to_ids to the existing import from ciao.structures.bitmask_graph so
mask_to_ids is available for use in build_hyperpixel_mcgs result handling.
In `@ciao/algorithm/mcts.py`:
- Around line 436-442: The unpacking in the masks evaluation branch creates an
unused variable `indices` (in the line that currently does `indices, masks =
zip(*masks_to_evaluate, strict=True)`); update this to `_indices, masks =
zip(*masks_to_evaluate, strict=True)` (or simply `_` instead of `_indices`) so
the intent of an unused value is clear in the `evaluate_masks` logic inside the
gpu evaluation block of `mcts.py`.
- Around line 559-593: Replace the two manual iterations with the simpler
constructs: for available_segments, iterate directly over scores (use "seg_id
for seg_id in scores if seg_id not in processed_segments" instead of
"scores.keys()"); for hyperpixel_segments, call the existing utility
mask_to_ids(hyperpixel_mask, next_id) (or mask_to_ids(hyperpixel_mask) if that
is the function signature) instead of the manual range/bit-check loop. These
changes touch the block that calls build_hyperpixel_mcts and the variables
scores, processed_segments, hyperpixel_mask and next_id.
In `@ciao/algorithm/potential.py`:
- Around line 500-503: The list comprehension building available_segments uses
the redundant .keys() call; update it to check membership directly against the
mapping by changing the comprehension to use "if seg_id not in
processed_segments and seg_id in scores" (or simply iterate over scores and
filter out processed_segments) so that scores is used directly instead of
scores.keys(); this touches the variable available_segments and the variables
scores and processed_segments in the potential computation code.
- Around line 379-411: The scores list is initialized with None which can cause
a TypeError when compared to max_score; change initialization of scores in the
selection logic to a numeric sentinel (e.g., scores = [-float("inf")] *
len(prefix_masks)) and keep setting cache hits via scores[i] = cache[mask] and
computed values from calculate_hyperpixel_deltas (used with missing_indices) as
you already do; ensure every branch that assigns to scores (the cache hit loop
and the for i, score in zip(missing_indices, computed_scores) block) writes
numeric values so no None remains before the final max search using best_idx and
max_score.
- Around line 112-113: Replace the numerous print() debug/info statements in
build_hyperpixel_using_potential, sampling_phase, and select_best_prefix with
logging calls following the same pattern used in lookahead_bitset.py: import the
logging module (if not already), replace user-facing prints with
logging.info(...) and verbose/debug prints with logging.debug(...), and keep the
message text identical or more descriptive to preserve context (e.g., the
seed/target/sim counts and progress messages referenced around Seed: {seed_idx},
Target: {desired_length}, Sims: {num_simulations}). Make these replacements for
all occurrences noted (around lines with prints at 112-113, 121-123, 128-130,
133, 155, 167-170, 186-188, 190, 315, 415-421) so verbosity can be controlled
via logger configuration. Ensure imports and any logger =
logging.getLogger(__name__) usage match lookahead_bitset.py.
In `@ciao/structures/bitmask_graph.py`:
- Around line 97-148: The parameter base_frontier in sample_connected_superset
is unused; either remove it from the function signature and update all call
sites (and tests) to stop passing it, or retain it but mark it explicitly
deprecated: add a short note to the docstring and emit a runtime
warnings.warn("base_frontier is deprecated and ignored", DeprecationWarning) at
the top of sample_connected_superset so callers are informed, and keep behavior
unchanged; update any internal references to call
_pick_weighted_frontier_segment / pick_random_set_bit using the computed
frontier only and remove mentions of base_frontier from comments and docs.
- Around line 151-207: The function _pick_weighted_frontier_segment currently
uses np.random.choice while other samplers like pick_random_set_bit use Python's
random module, which breaks reproducibility unless both RNGs are seeded
separately; fix by standardizing RNG usage—either switch
_pick_weighted_frontier_segment to accept and use a single RNG (e.g., pass in a
random.Random or a numpy.random.Generator) or replace np.random.choice with the
same RNG used by pick_random_set_bit, and update callers to pass the chosen RNG;
ensure you reference and use the same RNG instance across pick_random_set_bit
and _pick_weighted_frontier_segment (or document dual-RNG seeding requirements
if you intentionally keep both).
In `@ciao/structures/nodes.py`:
- Around line 32-64: Replace the loose dicts used for per-edge stats with a
typed structure to prevent key typos and clarify types: define a TypedDict or
dataclass (e.g., EdgeStats or EdgeStatsDict) containing fields N: int, W: float,
Q: float, max_reward: float and update the MCGSNode.edge_stats and
MCGSNode.rave_stats annotations to use that type; then refactor
MCGSNode.init_edge to create and assign that typed object instead of a
string-keyed dict and adjust any code accessing keys (e.g., places that
read/write edge_stats[action]["N"] etc.) to use the typed fields or keys
consistently.
In `@ciao/utils/calculations.py`:
- Around line 7-14: ModelPredictor currently sets replacement_image = None in
__init__ but calculate_hyperpixel_deltas asserts it's set, which is fragile; add
an explicit setter and replace the assert with a clear runtime check. Implement
a method set_replacement_image(self, image) on ModelPredictor that validates the
image (type/shape/device) and assigns self.replacement_image, update __init__
docstring to mention the lifecycle, and change the assertion in
calculate_hyperpixel_deltas to raise a descriptive ValueError (e.g.,
"replacement_image must be set via set_replacement_image before calling
calculate_hyperpixel_deltas").
- Around line 160-167: Remove the dead experimental code inside the with
torch.no_grad() block that calls get_predictions and computes log-odds (the
commented lines referencing "probabilities = self.get_predictions(input_batch)"
and "result = torch.log(probabilities) - torch.log(1 - probabilities)"); simply
return the logits slice as currently done (outputs[:, target_class_idx]) or, if
you intend to preserve the experiment, move that logic and explanation into a
dedicated ADR or a clearly named helper (e.g., a new method compute_log_odds)
instead of leaving commented-out code in the body of the method.
- Around line 203-211: The utility functions in ciao/utils/calculations.py
currently use print() for diagnostics (e.g., the prints showing original_logit
and class probability around the predictor.get_class_logit_batch and
predictor.get_predictions calls) — replace these print() calls with a module
logger: add "import logging" and "logger = logging.getLogger(__name__)" at top
if missing, then change prints to logger.debug() or logger.info() as appropriate
(use logger.debug for verbose diagnostics); make the same replacements in the
other occurrences mentioned (around the blocks at the create_surrogate_dataset
and calculate_scores_from_surrogate areas, i.e., the lines you noted at 253-254
and 283-284) so all diagnostic output uses the logger consistently.
In `@ciao/utils/search_utils.py`:
- Around line 14-18: The parameter adj_masks in is_terminal is typed as a bare
tuple but get_frontier expects tuple[int, ...]; update is_terminal's signature
to use adj_masks: tuple[int, ...] and ensure any callers pass the same typed
structure so the types align (refer to functions is_terminal and get_frontier to
locate and update the annotation).
In `@ciao/utils/segmentation.py`:
- Around line 88-120: build_adjacency_graph currently loops pixel-by-pixel;
replace the triple nested loops with vectorized NumPy adjacency detection by
comparing shifted arrays (e.g., segments[:-1,:] vs segments[1:,:],
segments[:,:-1] vs segments[:,1:], and the two diagonals when neighborhood==8)
to produce pairs of differing segment ids, use np.where and flatten/zip or
np.stack+unique to get edge pairs, then call adj_graph.add_edges_from on those
pairs instead of adding edges one-at-a-time; keep function signature
build_adjacency_graph(segments, neighborhood=8) and ensure you still add all
segment_ids to adj_graph before adding edges.
In `@README.md`:
- Around line 84-116: The README's project tree currently lists modules that are
not present in this branch (e.g., ciao/data/, ciao/explainer/,
ciao/visualization/, ciao/__main__.py, configs/); update README.md so the
directory tree either (a) only shows files and directories actually in this PR,
or (b) keeps the full planned tree but clearly marks missing items as "planned"
or "coming in future PRs" (add a short note and/or annotate entries like
ciao/data/ and configs/ as planned). Ensure references to files such as
ciao/explainer/ciao_explainer.py, ciao/visualization/visualization.py, and
configs/ciao.yaml are consistent with the branch contents.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
README.mdciao/__init__.pyciao/algorithm/__init__.pyciao/algorithm/lookahead_bitset.pyciao/algorithm/mcgs.pyciao/algorithm/mcts.pyciao/algorithm/potential.pyciao/imagenet_classes.txtciao/structures/__init__.pyciao/structures/bitmask_graph.pyciao/structures/nodes.pyciao/utils/__init__.pyciao/utils/calculations.pyciao/utils/search_utils.pyciao/utils/segmentation.pypyproject.toml
| bakery | ||
| balance beam | ||
| balloon | ||
| ballpoint |
There was a problem hiding this comment.
Minor label spelling inconsistencies.
Two labels deviate from their canonical spellings:
- Line 419:
Band Aid→Band-Aid(registered trademark, always hyphenated). - Line 917:
web site→website(single-word form is now standard).
✏️ Proposed fix
-Band Aid
+Band-Aid-web site
+websiteAlso applies to: 917-917
🧰 Tools
🪛 LanguageTool
[uncategorized] ~419-~419: The sticky tape used to cover wounds is normally capitalized (trademark) but also spelled with a hyphen.
Context: ...k bakery balance beam balloon ballpoint Band Aid banjo bannister barbell barber chair ba...
(BAND_AID)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/imagenet_classes.txt` at line 419, Update the two inconsistent labels by
replacing the exact string "Band Aid" with the hyphenated trademark form
"Band-Aid" and replace "web site" with the single-word modern form "website"
(locate these entries by their current label text "Band Aid" and "web site" in
the imagenet_classes list and edit them in place).
| if replacement == "mean_color": | ||
| # Fill entire image with mean color | ||
| mean_color = self.calculate_image_mean_color(input_tensor) # [3, 1, 1] | ||
| replacement_image = mean_color.expand(-1, 224, 224) # [3, 224, 224] | ||
|
|
||
| elif replacement == "interlacing": | ||
| # Create interlaced pattern: even columns flipped vertically, then even indices flipped horizontally | ||
| replacement_image = input_tensor.clone() | ||
| even_indices = torch.arange(0, 224, 2) # [0, 2, 4, ..., 222] | ||
|
|
||
| # Step 1: Flip even columns vertically (upside down) | ||
| replacement_image[:, :, even_indices] = torch.flip( | ||
| replacement_image[:, :, even_indices], dims=[1] | ||
| ) | ||
|
|
||
| # Step 2: Flip even indices horizontally (left-right) | ||
| replacement_image[:, even_indices, :] = torch.flip( | ||
| replacement_image[:, even_indices, :], dims=[2] | ||
| ) |
There was a problem hiding this comment.
Hardcoded 224 in replacement image generation will break for non-ImageNet input sizes.
get_replacement_image hardcodes 224 in the mean_color (line 81), interlacing (line 86), and solid_color (line 144) branches. If the input tensor has a different spatial resolution, the replacement image dimensions won't match, causing shape mismatches downstream.
Derive dimensions from the input tensor:
Proposed fix (showing mean_color branch as example)
+ _, h, w = input_tensor.shape
+
if replacement == "mean_color":
mean_color = self.calculate_image_mean_color(input_tensor) # [3, 1, 1]
- replacement_image = mean_color.expand(-1, 224, 224) # [3, 224, 224]
+ replacement_image = mean_color.expand(-1, h, w) # [3, H, W]Apply the same pattern for the interlacing (torch.arange(0, h, 2) / torch.arange(0, w, 2)) and solid_color branches.
Also applies to: 128-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/utils/calculations.py` around lines 78 - 96, The get_replacement_image
function uses hardcoded 224 for spatial dims causing shape mismatches for
non-ImageNet inputs; instead read height and width from the input tensor (e.g.,
h = input_tensor.size(1), w = input_tensor.size(2)) and use those when expanding
mean_color in calculate_image_mean_color/replacement_image, when building
interlacing indices (torch.arange(0, h, 2) and torch.arange(0, w, 2) for even
row/column indices used with replacement_image and even_indices), and when
creating solid_color replacements so all branches (mean_color, interlacing,
solid_color) produce replacement_image with matching [3, h, w] shape.
| "pillow>=9.0.0", | ||
|
|
||
| # Scientific computing | ||
| "numpy>=1.21.0", |
There was a problem hiding this comment.
numpy>=1.21.0 lower bound is very permissive — NumPy 2.x breaking changes may surface.
NumPy 2.0 (June 2024) removed many deprecated APIs and introduced C-extension ABI changes. Depending on which transitive packages are resolved, this open floor could pull in a numpy version that conflicts with pre-2.x-compatible builds of torch>=2.0.0 and scikit-image>=0.19.0. Consider tightening to >=1.24.0,<3 or at least >=1.24.0 to avoid the earliest affected builds, or explicitly verifying that the full dependency graph resolves cleanly against numpy 2.x.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` at line 29, The numpy dependency in pyproject.toml is too
permissive ("numpy>=1.21.0"); update that requirement to a tighter range to
avoid pulling in incompatible NumPy 2.x versions—for example replace
"numpy>=1.21.0" with "numpy>=1.24.0,<3" (or at minimum "numpy>=1.24.0") in
pyproject.toml, then regenerate/lock dependencies and run the test matrix /
dependency resolver to verify compatibility with torch and scikit-image.
|
|
||
| ## Project Structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block (flagged by markdownlint MD040).
✏️ Proposed fix
-```
+```text
ciao/📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 83, The fenced code block containing the snippet "ciao/"
lacks a language specifier; update the opening triple-backticks to include a
language (e.g., change ``` to ```text) so the block is annotated (identify the
block by its content "ciao/") and commit the README.md change.
There was a problem hiding this comment.
Pull request overview
This PR introduces the foundational building blocks for CIAO’s segmentation and hyperpixel search, adding core data structures (bitmask graph primitives + search nodes) and multiple search algorithm implementations (MCTS/MCGS/lookahead/potential), along with shared evaluation utilities.
Changes:
- Added segmentation utilities for square/hex grids, including adjacency graph construction.
- Added bitmask-based graph primitives and node data structures for MCTS/MCGS.
- Implemented core search algorithms (MCTS, MCGS, greedy lookahead, potential-based search) plus shared search utilities and surrogate/score calculations.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
ciao/utils/segmentation.py |
Grid segmentation + adjacency graph/list utilities. |
ciao/utils/search_utils.py |
Shared helpers for terminal detection and batched evaluation of masks. |
ciao/utils/calculations.py |
Model predictor utilities, masking delta computation, surrogate dataset helpers. |
ciao/utils/__init__.py |
Exposes common utils (ModelPredictor, create_segmentation). |
ciao/structures/nodes.py |
Node structures for MCTS and MCGS. |
ciao/structures/bitmask_graph.py |
Bitmask graph primitives: frontier computation, sampling, weighted selection. |
ciao/structures/__init__.py |
Exposes node structures. |
ciao/algorithm/mcts.py |
Unified MCTS implementation (standard + RAVE). |
ciao/algorithm/mcgs.py |
Unified MCGS implementation (standard + RAVE) with transpositions. |
ciao/algorithm/lookahead_bitset.py |
Greedy rolling-horizon lookahead search using bitmasks. |
ciao/algorithm/potential.py |
Potential-based sequential Monte Carlo region growing. |
ciao/algorithm/__init__.py |
Algorithm package init. |
ciao/__init__.py |
Package init (currently imports CIAOExplainer). |
ciao/imagenet_classes.txt |
ImageNet class label list resource. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ciao/utils/calculations.py
Outdated
| for batch_start in range(0, num_masks, batch_size): | ||
| batch_end = min(batch_start + batch_size, num_masks) | ||
| current_batch_size = batch_end - batch_start | ||
|
|
||
| batch_inputs = input_batch.repeat(current_batch_size, 1, 1, 1) | ||
|
|
||
| # Convert segments numpy array to GPU tensor once | ||
| gpu_segments = torch.from_numpy(segments).to(predictor.device) | ||
|
|
There was a problem hiding this comment.
segments is converted to a GPU tensor inside the batching loop, so it is re-created every batch. This adds avoidable overhead; move gpu_segments = torch.from_numpy(segments).to(predictor.device) outside the for batch_start ... loop and reuse it for all batches.
| for batch_start in range(0, num_masks, batch_size): | |
| batch_end = min(batch_start + batch_size, num_masks) | |
| current_batch_size = batch_end - batch_start | |
| batch_inputs = input_batch.repeat(current_batch_size, 1, 1, 1) | |
| # Convert segments numpy array to GPU tensor once | |
| gpu_segments = torch.from_numpy(segments).to(predictor.device) | |
| # Convert segments numpy array to GPU tensor once outside the batch loop | |
| gpu_segments = torch.from_numpy(segments).to(predictor.device) | |
| for batch_start in range(0, num_masks, batch_size): | |
| batch_end = min(batch_start + batch_size, num_masks) | |
| current_batch_size = batch_end - batch_start | |
| batch_inputs = input_batch.repeat(current_batch_size, 1, 1, 1) |
| """ | ||
| for path, reward in zip(batch_paths, rewards, strict=True): | ||
| for node in path: | ||
| node.pending -= 1 # Release virtual loss |
There was a problem hiding this comment.
Virtual loss bookkeeping is inconsistent: selection/expansion increments child.pending, but backup decrements pending for every node in path (including the root, whose pending is never incremented). This will drive root.pending negative over time. Decrement only for nodes whose pending was incremented (e.g., path[1:]), or increment pending for the root at the start of each simulation.
| node.pending -= 1 # Release virtual loss | |
| if node.pending > 0: | |
| node.pending -= 1 # Release virtual loss |
ciao/utils/segmentation.py
Outdated
| def create_square_grid(input_tensor, square_size=14, neighborhood=8): | ||
| """Create a grid of squares with graph structure representing spatial relationships""" | ||
| channels, height, width = input_tensor.shape | ||
| segments = np.zeros((height, width), dtype=np.int32) |
There was a problem hiding this comment.
Unused local channels will be flagged by Ruff (F841). If the channel count isn't needed, unpack as _, height, width = ... (or name it _channels) to satisfy linting.
ciao/utils/segmentation.py
Outdated
| channels, height, width = input_tensor.shape | ||
| segments = np.zeros((height, width), dtype=np.int32) | ||
|
|
There was a problem hiding this comment.
Unused local channels will be flagged by Ruff (F841). If the channel count isn't needed, unpack as _, height, width = ... (or name it _channels) to satisfy linting.
ciao/structures/bitmask_graph.py
Outdated
|
|
||
|
|
||
| def mask_to_ids(mask: int) -> list[int]: | ||
| """Convert integer bitmask to list of segment indices.""" | ||
| return [i for i in range(mask.bit_length()) if (mask >> i) & 1] | ||
|
|
||
|
|
||
| def iter_bits(mask: int): |
There was a problem hiding this comment.
iter_bits is missing a return type annotation. Under mypy strict mode (strict = True in .mypy.ini), untyped function defs are errors. Annotate it as returning an iterator/generator of int to keep mypy passing.
| def mask_to_ids(mask: int) -> list[int]: | |
| """Convert integer bitmask to list of segment indices.""" | |
| return [i for i in range(mask.bit_length()) if (mask >> i) & 1] | |
| def iter_bits(mask: int): | |
| from collections.abc import Iterator | |
| def mask_to_ids(mask: int) -> list[int]: | |
| """Convert integer bitmask to list of segment indices.""" | |
| return [i for i in range(mask.bit_length()) if (mask >> i) & 1] | |
| def iter_bits(mask: int) -> Iterator[int]: |
ciao/utils/calculations.py
Outdated
|
|
||
| return replacement_image | ||
|
|
||
| def plot_image_mean_color(self, input_tensor): |
There was a problem hiding this comment.
plot_image_mean_color is missing type annotations for its parameter/return value. With mypy strict = True, this will fail uv run mypy .. Please annotate input_tensor (and the return type, likely None) or mark the method to be ignored by mypy.
| def plot_image_mean_color(self, input_tensor): | |
| def plot_image_mean_color(self, input_tensor: torch.Tensor) -> None: |
ciao/algorithm/potential.py
Outdated
| def build_hyperpixel_using_potential( | ||
| predictor, | ||
| input_batch, | ||
| segments, | ||
| adj_list: tuple[tuple[int, ...], ...], |
There was a problem hiding this comment.
This function has multiple untyped parameters (predictor, input_batch, segments, etc.). With .mypy.ini set to strict = True, this will be a mypy error when running uv run mypy .. Add type annotations (or configure mypy to ignore this module) to keep strict type checking passing.
ciao/structures/nodes.py
Outdated
| def mean_value(self): | ||
| return self.value_sum / self.visits if self.visits > 0 else 0.0 | ||
|
|
||
| def rave_mean(self): | ||
| return self.rave_value_sum / self.rave_visits if self.rave_visits > 0 else 0.0 |
There was a problem hiding this comment.
mean_value / rave_mean are missing return type annotations. Under mypy strict = True, unannotated defs are errors. Add explicit -> float return types (and consider typing children with forward references via from __future__ import annotations for consistency).
| # Calculate deltas for all local groups | ||
| deltas = calculate_hyperpixel_deltas( | ||
| predictor, | ||
| input_batch, | ||
| segments, |
There was a problem hiding this comment.
create_surrogate_dataset() calls calculate_hyperpixel_deltas(...), but calculate_hyperpixel_deltas requires predictor.replacement_image to be pre-initialized (it asserts non-None). Since create_surrogate_dataset doesn't set it, this will raise at runtime unless every caller remembers to set it beforehand. Consider initializing predictor.replacement_image here (or accept a replacement strategy param and build it internally) to make the function self-contained.
| for node in path: | ||
| # --- STANDARD BACKUP --- | ||
| node.pending -= 1 # Release virtual loss |
There was a problem hiding this comment.
Same virtual-loss issue as backup_paths(): backup_paths_rave() decrements pending for every node in path even though pending is only incremented on selected/expanded children. This will make the root's pending negative and makes virtual loss unreliable. Decrement only for nodes whose pending was incremented (typically path[1:]) or increment root pending at rollout start.
| for node in path: | |
| # --- STANDARD BACKUP --- | |
| node.pending -= 1 # Release virtual loss | |
| for depth, node in enumerate(path): | |
| # --- STANDARD BACKUP --- | |
| if depth > 0: | |
| # Release virtual loss only for nodes whose pending was incremented | |
| node.pending -= 1 |
Co-authored-by: Adam Kukučka <adam.kukucka4@gmail.com>
Add bitmask graph operations, MCTS/MCGS node structures, segmentation utilities, model predictor, and supporting ImageNet class names.
Add MCTS, MCGS, lookahead, and potential search algorithms for hyperpixel construction.
Improve error handling and remove legacy parameters in algorithm functions.
dhalmazna
force-pushed
the
feature/ciao-core-algorithm
branch
from
a29b524 to
96eb26c
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
README.md (1)
83-83:⚠️ Potential issue | 🟡 MinorAdd a language specifier to the fenced code block.
Line 83 still violates markdownlint MD040 (
```without language). Please change it to something like```text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 83, Update the fenced code block that currently uses only ``` to include a language specifier (e.g., change ``` to ```text or another appropriate language) so it satisfies markdownlint MD040; locate the plain triple-backtick fenced block in README.md (the one without a language tag) and add the language identifier to the opening fence.ciao/imagenet_classes.txt (1)
420-420: Minor label spelling inconsistencies remain unfixed.
Band Aid(line 420) →Band-Aidandweb site(line 917) →website— as flagged in a prior review.Also applies to: 917-917
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/imagenet_classes.txt` at line 420, Update the two inconsistent labels in imagenet_classes.txt: change the entry "Band Aid" to "Band-Aid" and change the entry "web site" to "website" so spelling/hyphenation matches the prior review; locate and edit the literal strings "Band Aid" and "web site" in the file (entries around the previously flagged lines) to apply the corrections.pyproject.toml (1)
29-29:numpy>=1.21.0lower bound is still too permissive — previously flagged.NumPy 2.x introduced breaking changes. Consider tightening to
>=1.24.0,<3as suggested in the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 29, Update the NumPy version specifier in pyproject.toml: replace the current requirement string "numpy>=1.21.0" with a stricter range "numpy>=1.24.0,<3" to avoid NumPy 2.x breaking changes; edit the dependency entry where "numpy>=1.21.0" appears to use the new specifier.ciao/utils/search_utils.py (1)
32-36:⚠️ Potential issue | 🟡 MinorReport the offending zero-mask index in the validation error.
Line 32 still raises a generic batch-level error. Include the failing index so batched debugging is actionable.
Proposed fix
- if any(mask == 0 for mask in masks): - raise ValueError( - "Cannot evaluate zero mask: A mask with value 0 contains no segments. " - "Ensure all masks have at least one bit set." - ) + for idx, mask in enumerate(masks): + if mask == 0: + raise ValueError( + f"Cannot evaluate zero mask at index {idx}: mask value 0 contains no segments. " + "Ensure all masks have at least one bit set." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/search_utils.py` around lines 32 - 36, Change the generic batch-level ValueError raised when a zero mask is found to include the index of the offending mask: instead of using the any(mask == 0 for mask in masks) check, iterate with enumerate over masks (e.g., for i, mask in enumerate(masks)) and when mask == 0 raise a ValueError that mentions the index i (and optionally the mask value) so callers of the function in search_utils.py can identify which item in the batch failed validation.ciao/algorithm/lookahead_bitset.py (1)
194-198:⚠️ Potential issue | 🟡 MinorEmit a warning when the BFS fallback path is hit.
Line 194 silently falls back to
_find_first_step(...). If this invariant breaks, debugging becomes difficult without visibility.Proposed fix
else: # This shouldn't happen in proper BFS, but handle edge case # This mask came from current_mask, find the connection first_step = _find_first_step(current_mask, new_mask) + import warnings + warnings.warn( + "Fallback in _generate_lookahead_candidates: " + f"mask={mask}, new_mask={new_mask}, first_step={first_step}", + RuntimeWarning, + stacklevel=2, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/lookahead_bitset.py` around lines 194 - 198, The else branch that silently falls back to calling _find_first_step(current_mask, new_mask) needs to emit a warning so this invariant break is visible; update the else block to log/warn (using the module logger or warnings.warn consistent with the file) with a clear message that includes the function/context, the values of current_mask and new_mask, and that the BFS invariant was violated before calling _find_first_step; keep the existing call to _find_first_step but ensure the warning is emitted immediately prior to that call.
🧹 Nitpick comments (5)
ciao/algorithm/__init__.py (1)
1-3: Consider populating__all__with public symbols from sibling modules.The empty
__all__ = []is valid scaffolding, but sincelookahead_bitset.py,mcgs.py,mcts.py, andpotential.pyare all introduced in this same PR, consumers currently have no ergonomic top-level import path from theciao.algorithmpackage (e.g.,from ciao.algorithm import MCTSwon't work). Deferring this until integration intoCIAOExplaineris reasonable, but it's worth a deliberate follow-up once the public API of each module stabilises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/__init__.py` around lines 1 - 3, Populate the package export list so top-level imports work: update __all__ in ciao.algorithm to include the public symbols exported by the sibling modules (e.g., add the module names 'lookahead_bitset', 'mcgs', 'mcts', 'potential' or the concrete public classes/functions such as 'MCTS', 'MCGS', 'LookaheadBitset', 'Potential' after importing them). Modify the file to import the selected public symbols from lookahead_bitset, mcgs, mcts, and potential and list those symbol names in __all__ so consumers can do from ciao.algorithm import MCTS (or other public names).ciao/utils/calculations.py (1)
375-378: Callingtorch.cuda.empty_cache()on every batch iteration hurts throughput.
empty_cache()forces CUDA to return freed memory to the allocator, which is expensive. It's generally only useful before a known peak-memory operation or in OOM recovery. Inside a tight batch loop it can degrade performance significantly. Consider removing it or calling it only when an OOM is caught.♻️ Proposed change
# Memory cleanup del batch_inputs, masked_logits - if torch.cuda.is_available(): - torch.cuda.empty_cache()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 375 - 378, Remove the unconditional torch.cuda.empty_cache() call inside the batch loop (where batch_inputs and masked_logits are deleted) because it hurts throughput; either delete the empty_cache() invocation entirely or guard it to only run in OOM handling code paths (e.g., inside an except RuntimeError catching "out of memory"), keeping the memory cleanup of del batch_inputs, masked_logits as-is and referencing the existing torch.cuda.empty_cache() call to locate and modify it.ciao/utils/segmentation.py (1)
165-190: Duplicated pixel-to-hex mapping loop acrosscreate_hexagonal_grid_with_listandcreate_hexagonal_grid.Lines 176-185 and 258-267 contain identical logic for mapping pixels to hex IDs. Consider extracting a shared helper (e.g.,
_assign_hex_segments) that returns(segments, hex_to_id, num_segments), and have both functions call it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/segmentation.py` around lines 165 - 190, The pixel-to-hex mapping loop in create_hexagonal_grid_with_list and create_hexagonal_grid is duplicated; extract it into a shared helper (e.g., _assign_hex_segments(input_tensor: torch.Tensor, hex_radius: int) -> tuple[np.ndarray, dict, int]) that performs the nested x/y loop, calls pixel_to_hex, populates segments and hex_to_id and returns (segments, hex_to_id, next_id); then replace the duplicated loops in create_hexagonal_grid_with_list and create_hexagonal_grid to call _assign_hex_segments(input_tensor, hex_radius) and use its returned segments, hex_to_id and num_segments (pass num_segments to build_fast_adjacency_list and keep variable names consistent like adjacency_list = build_fast_adjacency_list(hex_to_id, num_segments)).ciao/structures/nodes.py (2)
1-23: InconsistentOptionalvs|union syntax.Line 6 uses
Optional["MCTSNode"]while line 23 usesint | None. Since the project targets Python ≥3.11, consider standardizing onX | Nonethroughout and dropping thefrom typing import Optionalimport. For the forward reference, addingfrom __future__ import annotationsat the top would allowMCTSNode | Nonewithout quotes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/nodes.py` around lines 1 - 23, The file mixes typing styles: change the forward-reference and optional types to consistent PEP 604 unions by adding "from __future__ import annotations" at the top, replace the __init__ parent parameter type Optional["MCTSNode"] with "MCTSNode | None" (and any other Optional[...] usages), and remove the now-unused "from typing import Optional" import; confirm the annotations for attributes like frontier_cache and children remain valid (e.g., frontier_cache: int | None, children: dict[int, MCTSNode]) and update the MCTSNode.__init__ signature and related attribute annotations accordingly.
37-62: String-keyed edge stats dicts are fragile.
edge_statsandrave_statsuse magic string keys ("N","W","Q","max_reward"). A typo in any consumer will silently produce aKeyErrorat runtime. Consider defining aTypedDictor a small dataclass for these stat bundles to get static type checking.♻️ Example with TypedDict
from typing import TypedDict class EdgeStat(TypedDict): N: float W: float Q: float max_reward: floatThen type the dicts as
dict[int, EdgeStat].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/nodes.py` around lines 37 - 62, Replace fragile string-keyed stat dicts with a typed structure: define a TypedDict (e.g., EdgeStat with keys N, W, Q, max_reward) or a small dataclass, change the types of edge_stats and rave_stats to dict[int, EdgeStat] (or dict[int, EdgeStatClass]) and update init_edge to initialize instances of that typed structure instead of raw dicts; then update all consumers that access edge_stats and rave_stats (references to edge_stats, rave_stats, init_edge, and any code reading "N"/"W"/"Q"/"max_reward") to use the typed fields (or attributes) so you get static checking and avoid silent KeyError at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ciao/algorithm/mcts.py`:
- Around line 220-223: The backup loop that iterates over for path, reward in
zip(batch_paths, rewards, strict=True) decrements node.pending for every node
(including the root), causing underflow because the root never receives a
virtual-loss increment; change the decrement to only release virtual loss when
appropriate by guarding the decrement with either a check that node.pending > 0
(e.g., if node.pending: node.pending -= 1) or by skipping the root (e.g., if
node.parent is not None: node.pending -= 1), and apply the same guard in the
corresponding backup at lines ~261-264 so pending cannot go negative.
In `@ciao/structures/bitmask_graph.py`:
- Around line 194-197: The softmax computation using effective_scores divides by
temperature without validation (in code around effective_scores -> exp_scores ->
softmax_probs); add a guard that ensures temperature is > 0 before using it
(either raise a clear ValueError mentioning temperature and function/class name,
or clamp temperature to a small positive epsilon like 1e-8), then proceed to
compute exp_scores = np.exp((effective_scores - max_score) / temperature) and
softmax_probs as before so np.random.choice receives valid probabilities.
In `@ciao/utils/calculations.py`:
- Around line 206-214: Replace the direct print(...) diagnostic calls in this
module with the logging module: create or use a module logger (e.g., logger =
logging.getLogger(__name__)) and change the prints that report original_logit
and prediction probability (calls around predictor.get_class_logit_batch and
predictor.get_predictions) to logger.debug or logger.info as appropriate; also
find and replace the other print usages mentioned (the prints near the other
predictor-related diagnostics) so all diagnostic output is emitted via the
logger allowing configurable verbosity.
- Around line 154-157: plot_image_mean_color currently sends ImageNet-normalized
values from calculate_image_mean_color straight to plt.imshow, which shows
non-human-readable colors; update plot_image_mean_color to unnormalize the
tensor before display by undoing ImageNet normalization (multiply by std
[0.229,0.224,0.225], add mean [0.485,0.456,0.406]), clamp to [0,1], move to
CPU/detach, permute to HWC and convert to numpy for plt.imshow; alternatively,
if you prefer to keep normalized display, add a clear note in
plot_image_mean_color's docstring explaining the image is normalized. Ensure you
reference the methods calculate_image_mean_color and plot_image_mean_color when
making the change.
- Around line 335-336: Replace the runtime assert with an explicit check and
raise a clear exception when the replacement image is missing: in the block that
accesses predictor.replacement_image (the lines currently doing "assert
predictor.replacement_image is not None" and "replacement_image =
predictor.replacement_image"), change to an if-statement that tests "if
predictor.replacement_image is None" and raise a ValueError or RuntimeError with
a descriptive message (e.g., "replacement_image is required but missing on
predictor"); then assign replacement_image = predictor.replacement_image after
the check.
- Around line 279-284: When computing per-segment means in the loop (variables
X, y, scores, segment_id, segment_scores), guard against empty segment_scores:
check if segment_scores.size == 0 (or len(segment_scores) == 0) and assign a
safe default (e.g., 0.0 or another chosen fallback) to scores[segment_id];
otherwise assign float(segment_scores.mean()). This prevents np.ndarray.mean()
from returning NaN when no samples mask a given segment.
In `@ciao/utils/segmentation.py`:
- Around line 112-122: The 8-neighborhood diagonal adjacency misses
anti-diagonals at the rightmost column because the loop uses for x in
range(width - 1) so center x==width-1 is never processed; update the logic in
the neighborhood==8 block (where segments, adj_graph, width, height are used) to
either iterate x over range(width) and guard diagonals by bounds checks or keep
range(width - 1) for the main diagonal and add a separate check/loop that
examines the anti-diagonal neighbor (segments[y + 1, x - 1]) for x == width - 1
(or generally x in 1..width-1) so the edge between (y, width-1) and (y+1,
width-2) is added to adj_graph.
---
Duplicate comments:
In `@ciao/algorithm/lookahead_bitset.py`:
- Around line 194-198: The else branch that silently falls back to calling
_find_first_step(current_mask, new_mask) needs to emit a warning so this
invariant break is visible; update the else block to log/warn (using the module
logger or warnings.warn consistent with the file) with a clear message that
includes the function/context, the values of current_mask and new_mask, and that
the BFS invariant was violated before calling _find_first_step; keep the
existing call to _find_first_step but ensure the warning is emitted immediately
prior to that call.
In `@ciao/imagenet_classes.txt`:
- Line 420: Update the two inconsistent labels in imagenet_classes.txt: change
the entry "Band Aid" to "Band-Aid" and change the entry "web site" to "website"
so spelling/hyphenation matches the prior review; locate and edit the literal
strings "Band Aid" and "web site" in the file (entries around the previously
flagged lines) to apply the corrections.
In `@ciao/utils/search_utils.py`:
- Around line 32-36: Change the generic batch-level ValueError raised when a
zero mask is found to include the index of the offending mask: instead of using
the any(mask == 0 for mask in masks) check, iterate with enumerate over masks
(e.g., for i, mask in enumerate(masks)) and when mask == 0 raise a ValueError
that mentions the index i (and optionally the mask value) so callers of the
function in search_utils.py can identify which item in the batch failed
validation.
In `@pyproject.toml`:
- Line 29: Update the NumPy version specifier in pyproject.toml: replace the
current requirement string "numpy>=1.21.0" with a stricter range
"numpy>=1.24.0,<3" to avoid NumPy 2.x breaking changes; edit the dependency
entry where "numpy>=1.21.0" appears to use the new specifier.
In `@README.md`:
- Line 83: Update the fenced code block that currently uses only ``` to include
a language specifier (e.g., change ``` to ```text or another appropriate
language) so it satisfies markdownlint MD040; locate the plain triple-backtick
fenced block in README.md (the one without a language tag) and add the language
identifier to the opening fence.
---
Nitpick comments:
In `@ciao/algorithm/__init__.py`:
- Around line 1-3: Populate the package export list so top-level imports work:
update __all__ in ciao.algorithm to include the public symbols exported by the
sibling modules (e.g., add the module names 'lookahead_bitset', 'mcgs', 'mcts',
'potential' or the concrete public classes/functions such as 'MCTS', 'MCGS',
'LookaheadBitset', 'Potential' after importing them). Modify the file to import
the selected public symbols from lookahead_bitset, mcgs, mcts, and potential and
list those symbol names in __all__ so consumers can do from ciao.algorithm
import MCTS (or other public names).
In `@ciao/structures/nodes.py`:
- Around line 1-23: The file mixes typing styles: change the forward-reference
and optional types to consistent PEP 604 unions by adding "from __future__
import annotations" at the top, replace the __init__ parent parameter type
Optional["MCTSNode"] with "MCTSNode | None" (and any other Optional[...]
usages), and remove the now-unused "from typing import Optional" import; confirm
the annotations for attributes like frontier_cache and children remain valid
(e.g., frontier_cache: int | None, children: dict[int, MCTSNode]) and update the
MCTSNode.__init__ signature and related attribute annotations accordingly.
- Around line 37-62: Replace fragile string-keyed stat dicts with a typed
structure: define a TypedDict (e.g., EdgeStat with keys N, W, Q, max_reward) or
a small dataclass, change the types of edge_stats and rave_stats to dict[int,
EdgeStat] (or dict[int, EdgeStatClass]) and update init_edge to initialize
instances of that typed structure instead of raw dicts; then update all
consumers that access edge_stats and rave_stats (references to edge_stats,
rave_stats, init_edge, and any code reading "N"/"W"/"Q"/"max_reward") to use the
typed fields (or attributes) so you get static checking and avoid silent
KeyError at runtime.
In `@ciao/utils/calculations.py`:
- Around line 375-378: Remove the unconditional torch.cuda.empty_cache() call
inside the batch loop (where batch_inputs and masked_logits are deleted) because
it hurts throughput; either delete the empty_cache() invocation entirely or
guard it to only run in OOM handling code paths (e.g., inside an except
RuntimeError catching "out of memory"), keeping the memory cleanup of del
batch_inputs, masked_logits as-is and referencing the existing
torch.cuda.empty_cache() call to locate and modify it.
In `@ciao/utils/segmentation.py`:
- Around line 165-190: The pixel-to-hex mapping loop in
create_hexagonal_grid_with_list and create_hexagonal_grid is duplicated; extract
it into a shared helper (e.g., _assign_hex_segments(input_tensor: torch.Tensor,
hex_radius: int) -> tuple[np.ndarray, dict, int]) that performs the nested x/y
loop, calls pixel_to_hex, populates segments and hex_to_id and returns
(segments, hex_to_id, next_id); then replace the duplicated loops in
create_hexagonal_grid_with_list and create_hexagonal_grid to call
_assign_hex_segments(input_tensor, hex_radius) and use its returned segments,
hex_to_id and num_segments (pass num_segments to build_fast_adjacency_list and
keep variable names consistent like adjacency_list =
build_fast_adjacency_list(hex_to_id, num_segments)).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
README.mdciao/__init__.pyciao/algorithm/__init__.pyciao/algorithm/lookahead_bitset.pyciao/algorithm/mcgs.pyciao/algorithm/mcts.pyciao/algorithm/potential.pyciao/imagenet_classes.txtciao/structures/__init__.pyciao/structures/bitmask_graph.pyciao/structures/nodes.pyciao/utils/__init__.pyciao/utils/calculations.pyciao/utils/search_utils.pyciao/utils/segmentation.pypyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
pyproject.toml (1)
29-29: Line 29 NumPy floor likely needs tightening for the declared Python baseline.
requires-python = ">=3.11"plusnumpy>=1.21.0is a weak compatibility signal; consider bumping NumPy minimum to a Python-3.11-compatible floor.What is the earliest NumPy release that supports Python 3.11, and what minimum NumPy version is recommended for Python 3.11+ projects in 2026?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 29, The dependency floor "numpy>=1.21.0" in pyproject.toml is too low for the declared requires-python=">=3.11"; bump the NumPy minimum to a Python-3.11-compatible release (earliest official NumPy release that added Python 3.11 support is 1.24.0), and for 2026 projects I recommend setting a safer minimum like "numpy>=1.26.0" (or whichever current stable minor you standardize on); update the "numpy>=1.21.0" line in pyproject.toml accordingly and run the test matrix/CI to verify compatibility.ciao/algorithm/lookahead_bitset.py (1)
195-197: Add visibility on unexpected BFS fallback path.This branch is marked as “shouldn’t happen”; emitting a warning here would make debugging easier when invariants break.
🔎 Proposed tweak
+ import logging + logging.getLogger(__name__).warning( + "Unexpected lookahead fallback: current_mask=%s new_mask=%s", + current_mask, + new_mask, + ) first_step = _find_first_step(current_mask, new_mask)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/lookahead_bitset.py` around lines 195 - 197, This branch in lookahead_bitset.py where we handle the “shouldn't happen” BFS fallback (calling _find_first_step(current_mask, new_mask)) needs visibility: add a warning log that includes contextual details (e.g., current_mask, new_mask, maybe node or step identifiers available in scope) so when the invariant is violated you can trace the inputs that triggered the fallback; use the module/logger used elsewhere in this file (or the standard logging module) and keep the message concise but descriptive.ciao/utils/calculations.py (3)
335-336:⚠️ Potential issue | 🟠 MajorUse an explicit exception instead of
assertfor required runtime state.This precondition is part of normal runtime correctness;
assertcan be disabled and should not guard required state.🛡️ Proposed fix
- assert predictor.replacement_image is not None - replacement_image = predictor.replacement_image + if predictor.replacement_image is None: + raise RuntimeError( + "predictor.replacement_image must be set before " + "calculate_hyperpixel_deltas is called." + ) + replacement_image = predictor.replacement_image🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 335 - 336, Replace the runtime-guarding assert with an explicit exception: instead of "assert predictor.replacement_image is not None", check "if predictor.replacement_image is None: raise RuntimeError('predictor.replacement_image is required')" (or ValueError) and then set replacement_image = predictor.replacement_image; update the block around predictor.replacement_image to perform this explicit None check and error raise so the required state cannot be skipped when Python assertions are disabled.
283-285:⚠️ Potential issue | 🟠 MajorGuard empty segment buckets before
mean().When a segment never appears in
X,segment_scoresis empty andmean()returnsNaN, which silently contaminates ranking.🛡️ Proposed fix
segment_scores = y[mask] - scores[segment_id] = float(segment_scores.mean()) + scores[segment_id] = ( + float(segment_scores.mean()) if segment_scores.size > 0 else 0.0 + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 283 - 285, When computing per-segment means, guard against empty buckets so mean() doesn't return NaN: check if segment_scores (derived from y[mask]) has any elements (e.g., segment_scores.size or len(segment_scores)) and if empty assign a default numeric score (e.g., 0.0 or another sentinel) to scores[segment_id] instead of calling segment_scores.mean(); otherwise compute float(segment_scores.mean()). Update the block that sets segment_scores = y[mask] and scores[segment_id] to include this conditional.
92-101:⚠️ Potential issue | 🟠 MajorFix interlacing indices for non-square images.
The same index vector is used for both axes. On non-square inputs this can index the width axis with height-based indices and fail at runtime.
🐛 Proposed fix
- even_indices = torch.arange(0, height, 2) # Even row indices + even_rows = torch.arange(0, height, 2, device=self.device) + even_cols = torch.arange(0, width, 2, device=self.device) # Step 1: Flip even columns vertically (upside down) - replacement_image[:, :, even_indices] = torch.flip( - replacement_image[:, :, even_indices], dims=[1] + replacement_image[:, :, even_cols] = torch.flip( + replacement_image[:, :, even_cols], dims=[1] ) # Step 2: Flip even indices horizontally (left-right) - replacement_image[:, even_indices, :] = torch.flip( - replacement_image[:, even_indices, :], dims=[2] + replacement_image[:, even_rows, :] = torch.flip( + replacement_image[:, even_rows, :], dims=[2] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 92 - 101, The code incorrectly reuses even_indices (built from height) for both axes, causing out-of-bounds on non-square images; create two index vectors—even_row_indices = torch.arange(0, height, 2) and even_col_indices = torch.arange(0, width, 2)—then apply the vertical flip to replacement_image[:, even_row_indices, :] with dims=[1] and the horizontal flip to replacement_image[:, :, even_col_indices] with dims=[2], updating uses of replacement_image and the flip calls accordingly.ciao/algorithm/mcts.py (1)
220-223:⚠️ Potential issue | 🟡 MinorPrevent
pendingunderflow during backup.
pendingis decremented on every node in the path, including root, which is never incremented. This drives rootpendingnegative.🛡️ Proposed fix
- node.pending -= 1 # Release virtual loss + if node.pending > 0: + node.pending -= 1 # Release virtual loss @@ - node.pending -= 1 # Release virtual loss + if node.pending > 0: + node.pending -= 1 # Release virtual lossAlso applies to: 261-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/mcts.py` around lines 220 - 223, The backup loop over batch_paths (the for path, reward in zip(batch_paths, rewards, strict=True) loop) decrements node.pending for every node including the root which was never incremented; update the logic to avoid underflow by only decrementing pending when it is positive or by skipping the root (e.g., check node.pending > 0 or node.parent is not None before doing node.pending -= 1) and leave node.visits increment as-is; apply the same guard to the other identical backup section that updates node.pending/node.visits.ciao/utils/segmentation.py (1)
112-122:⚠️ Potential issue | 🟠 Major8-neighborhood diagonal pass misses right-edge anti-diagonal adjacency.
The current
xloop excludes the last column as a center pixel, so anti-diagonal edges from that column are not evaluated.🐛 Proposed fix
if neighborhood == 8: # Add diagonal adjacency for 8-neighborhood for y in range(height - 1): - for x in range(width - 1): + for x in range(width): center_seg = segments[y, x] # Check diagonal neighbors - if segments[y + 1, x + 1] != center_seg: + if x < width - 1 and segments[y + 1, x + 1] != center_seg: adj_graph.add_edge(center_seg, segments[y + 1, x + 1]) if x > 0 and segments[y + 1, x - 1] != center_seg: adj_graph.add_edge(center_seg, segments[y + 1, x - 1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/segmentation.py` around lines 112 - 122, The diagonal adjacency pass currently loops x in range(width - 1) so the last column is never a center pixel and its anti-diagonal (down-left) adjacency is missed; modify the loop to iterate x in range(width) and guard neighbor access with bounds checks: inside the for y in range(height - 1): for x in range(width): get center_seg = segments[y, x], then if x < width - 1 and segments[y + 1, x + 1] != center_seg: adj_graph.add_edge(...), and if x > 0 and segments[y + 1, x - 1] != center_seg: adj_graph.add_edge(...), keeping the same variables (neighborhood, segments, adj_graph) and y range to avoid index errors.ciao/imagenet_classes.txt (1)
420-420:⚠️ Potential issue | 🟡 MinorNormalize two label spellings for consistency.
Band Aidandweb siteare inconsistent with the preferred forms used elsewhere in label sets.✏️ Proposed fix
-Band Aid +Band-Aid ... -web site +websiteAlso applies to: 917-917
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/imagenet_classes.txt` at line 420, Replace inconsistent label spellings by normalizing the strings "Band Aid" and "web site" to the preferred forms used across the label set: change "Band Aid" to "Band-Aid" and "web site" to "website" wherever they occur (the diff shows "Band Aid" and also an occurrence of "web site"); update both occurrences so all labels match the canonical spelling.
🧹 Nitpick comments (1)
pyproject.toml (1)
9-34: Move visualization and experiment-tracking packages to optional extras.
mlflow,plotly, andipywidgetsare unused in the codebase.matplotlibis only used in theplot_image_mean_color()visualization method, which is not called by any core algorithm code. These packages can be safely moved to optional dependencies to reduce bloat for algorithm-only users.Proposed refactor
[project] dependencies = [ # Core ML/DL frameworks "torch>=2.0.0", "torchvision>=0.15.0", # Configuration and experiment tracking "hydra-core>=1.3.0", - "mlflow>=3.0", "omegaconf>=2.3.0", - # XAI and visualization - "matplotlib>=3.5.0", - "plotly>=5.0.0", - "ipywidgets>=7.0.0", - # Image processing and segmentation "scikit-image>=0.19.0", "pillow>=9.0.0", @@ "tqdm>=4.0.0", ] + +[project.optional-dependencies] +tracking = ["mlflow>=3.0"] +viz = [ + "matplotlib>=3.5.0", + "plotly>=5.0.0", + "ipywidgets>=7.0.0", +]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 9 - 34, Update pyproject.toml to move non-core visualization and experiment-tracking packages into optional extras: remove "mlflow", "plotly", "ipywidgets", and "matplotlib" from the main dependencies list and add them under an [project.optional-dependencies] (or extras) group such as extras = {viz = ["matplotlib>=3.5.0","plotly>=5.0.0","ipywidgets>=7.0.0"], tracking = ["mlflow>=3.0"] } so core installs remain lightweight; ensure package names and version specifiers match the originals and update any CI/dev docs that reference installing these extras.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ciao/utils/search_utils.py`:
- Around line 31-36: The validation currently only rejects zero masks but allows
negative integers which break iter_bits() (Python's negative ints have infinite
set bits); update the guard in search_utils.py to reject any mask <= 0 (or
explicitly mask == 0 or mask < 0) and raise a clear ValueError mentioning
negative masks, and optionally validate masks are ints before calling
iter_bits() (reference: masks and iter_bits()) so invalid negative bitmasks fail
fast with an informative message.
In `@ciao/utils/segmentation.py`:
- Around line 275-304: Validate segment_size at the start of create_segmentation
to prevent zero/negative values from reaching create_square_grid or
create_hexagonal_grid: check that segment_size is an integer (or castable to
int) and > 0, and if not raise a clear ValueError (e.g., "segment_size must be a
positive integer"), before dispatching to create_square_grid or
create_hexagonal_grid so you avoid step=0 or division-by-zero errors in those
functions.
---
Duplicate comments:
In `@ciao/algorithm/lookahead_bitset.py`:
- Around line 195-197: This branch in lookahead_bitset.py where we handle the
“shouldn't happen” BFS fallback (calling _find_first_step(current_mask,
new_mask)) needs visibility: add a warning log that includes contextual details
(e.g., current_mask, new_mask, maybe node or step identifiers available in
scope) so when the invariant is violated you can trace the inputs that triggered
the fallback; use the module/logger used elsewhere in this file (or the standard
logging module) and keep the message concise but descriptive.
In `@ciao/algorithm/mcts.py`:
- Around line 220-223: The backup loop over batch_paths (the for path, reward in
zip(batch_paths, rewards, strict=True) loop) decrements node.pending for every
node including the root which was never incremented; update the logic to avoid
underflow by only decrementing pending when it is positive or by skipping the
root (e.g., check node.pending > 0 or node.parent is not None before doing
node.pending -= 1) and leave node.visits increment as-is; apply the same guard
to the other identical backup section that updates node.pending/node.visits.
In `@ciao/imagenet_classes.txt`:
- Line 420: Replace inconsistent label spellings by normalizing the strings
"Band Aid" and "web site" to the preferred forms used across the label set:
change "Band Aid" to "Band-Aid" and "web site" to "website" wherever they occur
(the diff shows "Band Aid" and also an occurrence of "web site"); update both
occurrences so all labels match the canonical spelling.
In `@ciao/utils/calculations.py`:
- Around line 335-336: Replace the runtime-guarding assert with an explicit
exception: instead of "assert predictor.replacement_image is not None", check
"if predictor.replacement_image is None: raise
RuntimeError('predictor.replacement_image is required')" (or ValueError) and
then set replacement_image = predictor.replacement_image; update the block
around predictor.replacement_image to perform this explicit None check and error
raise so the required state cannot be skipped when Python assertions are
disabled.
- Around line 283-285: When computing per-segment means, guard against empty
buckets so mean() doesn't return NaN: check if segment_scores (derived from
y[mask]) has any elements (e.g., segment_scores.size or len(segment_scores)) and
if empty assign a default numeric score (e.g., 0.0 or another sentinel) to
scores[segment_id] instead of calling segment_scores.mean(); otherwise compute
float(segment_scores.mean()). Update the block that sets segment_scores =
y[mask] and scores[segment_id] to include this conditional.
- Around line 92-101: The code incorrectly reuses even_indices (built from
height) for both axes, causing out-of-bounds on non-square images; create two
index vectors—even_row_indices = torch.arange(0, height, 2) and even_col_indices
= torch.arange(0, width, 2)—then apply the vertical flip to replacement_image[:,
even_row_indices, :] with dims=[1] and the horizontal flip to
replacement_image[:, :, even_col_indices] with dims=[2], updating uses of
replacement_image and the flip calls accordingly.
In `@ciao/utils/segmentation.py`:
- Around line 112-122: The diagonal adjacency pass currently loops x in
range(width - 1) so the last column is never a center pixel and its
anti-diagonal (down-left) adjacency is missed; modify the loop to iterate x in
range(width) and guard neighbor access with bounds checks: inside the for y in
range(height - 1): for x in range(width): get center_seg = segments[y, x], then
if x < width - 1 and segments[y + 1, x + 1] != center_seg:
adj_graph.add_edge(...), and if x > 0 and segments[y + 1, x - 1] != center_seg:
adj_graph.add_edge(...), keeping the same variables (neighborhood, segments,
adj_graph) and y range to avoid index errors.
In `@pyproject.toml`:
- Line 29: The dependency floor "numpy>=1.21.0" in pyproject.toml is too low for
the declared requires-python=">=3.11"; bump the NumPy minimum to a
Python-3.11-compatible release (earliest official NumPy release that added
Python 3.11 support is 1.24.0), and for 2026 projects I recommend setting a
safer minimum like "numpy>=1.26.0" (or whichever current stable minor you
standardize on); update the "numpy>=1.21.0" line in pyproject.toml accordingly
and run the test matrix/CI to verify compatibility.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 9-34: Update pyproject.toml to move non-core visualization and
experiment-tracking packages into optional extras: remove "mlflow", "plotly",
"ipywidgets", and "matplotlib" from the main dependencies list and add them
under an [project.optional-dependencies] (or extras) group such as extras = {viz
= ["matplotlib>=3.5.0","plotly>=5.0.0","ipywidgets>=7.0.0"], tracking =
["mlflow>=3.0"] } so core installs remain lightweight; ensure package names and
version specifiers match the originals and update any CI/dev docs that reference
installing these extras.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ciao/__init__.pyciao/algorithm/__init__.pyciao/algorithm/lookahead_bitset.pyciao/algorithm/mcgs.pyciao/algorithm/mcts.pyciao/algorithm/potential.pyciao/imagenet_classes.txtciao/structures/__init__.pyciao/structures/bitmask_graph.pyciao/structures/nodes.pyciao/utils/__init__.pyciao/utils/calculations.pyciao/utils/search_utils.pyciao/utils/segmentation.pypyproject.toml
✅ Files skipped from review due to trivial changes (1)
- ciao/init.py
🚧 Files skipped from review as they are similar to previous changes (6)
- ciao/structures/init.py
- ciao/utils/init.py
- ciao/algorithm/potential.py
- ciao/structures/bitmask_graph.py
- ciao/algorithm/init.py
- ciao/algorithm/mcgs.py
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
ciao/structures/bitmask_graph.py (1)
153-215: Temperature validation is in place — good. Past concern resolved.The
temperature <= 0guard at Line 177 properly addresses the prior review feedback. The softmax + epsilon-greedy mixing logic is numerically sound (max-subtraction for stability, renormalization for floating-point drift).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/bitmask_graph.py` around lines 153 - 215, The _pick_weighted_frontier_segment function already validates temperature (>0), handles -inf weights, applies optimization_sign, uses numerically-stable softmax (subtracting max_score) and renormalizes after epsilon-greedy mixing, so no code change is required; leave _pick_weighted_frontier_segment as implemented.ciao/algorithm/lookahead_bitset.py (1)
156-214: BFS candidate generation looks correct.The depth-tracking logic properly inherits
first_stepfrom parent masks and theRuntimeErrorat Line 200 replaces the old silent fallback. The "first path wins" deduplication is sound BFS behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/lookahead_bitset.py` around lines 156 - 214, No change required: the BFS in _generate_lookahead_candidates correctly tracks depth, assigns first_step (using seg_id at depth==1 and inheriting from candidates for deeper depths), enforces "first path wins" deduplication, and replaces the old silent fallback with a RuntimeError for inconsistency; leave _generate_lookahead_candidates as-is.ciao/utils/calculations.py (1)
335-396: Previous review concerns addressed — LGTM.The
replacement_imageis now guarded with a properRuntimeError(Line 349-353), andgpu_segmentsis correctly moved outside the batch loop (Line 357). The batched delta computation logic is sound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 335 - 396, The previous issues have been resolved: replacement_image is now validated with a RuntimeError (see replacement_image and create_replacement_image()) and gpu_segments is moved outside the batch loop in calculate_hyperpixel_deltas; no further changes are required in this section—approve the PR as-is.
🧹 Nitpick comments (5)
ciao/utils/calculations.py (1)
174-179: Minor: commented-out experimental code.Lines 175-177 contain a commented-out alternative logit computation. Consider removing it or converting to a documented configuration option to keep the codebase clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 174 - 179, Remove or formalize the commented-out experimental logit computation in calculations.py: eliminate the commented lines referencing get_predictions, probabilities and the torch.log expression, or move them behind a clear, documented configuration flag (e.g., an optional mode in the method that currently returns outputs[:, target_class_idx]) so the alternative computation can be toggled explicitly; update the function/method docstring to mention the available modes and ensure the code only contains the active implementation using outputs and target_class_idx.ciao/structures/bitmask_graph.py (1)
99-150: Dead parameterbase_frontierinsample_connected_superset.The docstring at Line 124 says
base_frontieris "unused, kept for compatibility", and indeed the function body never references it — frontier is recomputed viaget_frontieron every loop iteration. Carrying a dead parameter in a new module creates unnecessary API surface and confusion for callers.Consider removing it now while the API is fresh, or at minimum mark it as deprecated so callers don't construct it needlessly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/structures/bitmask_graph.py` around lines 99 - 150, sample_connected_superset currently accepts a dead parameter base_frontier that is never used; remove this parameter from sample_connected_superset's signature and all its callers (and update the docstring and any tests) to eliminate the unnecessary API surface, or if you must preserve compatibility, mark base_frontier as deprecated by keeping it in the signature but adding a warning (e.g., via warnings.warn) in sample_connected_superset and documenting that it is ignored; update references to the function name sample_connected_superset and the docstring accordingly so callers no longer construct or pass base_frontier.ciao/algorithm/potential.py (2)
349-437:select_best_prefixalways receives an empty cache — the caching mechanism is unused.
build_hyperpixel_using_potentialcallsselect_best_prefixwithcache={}(Line 206), so the cache lookup at Lines 394-398 will never hit, and the writes at Line 415 are discarded. Either wire through a persistent cache across calls, or simplify the function by removing the caching logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/potential.py` around lines 349 - 437, select_best_prefix currently contains dead caching because build_hyperpixel_using_potential calls it with cache={} so lookups never hit and stored values are thrown away; either remove the cache logic from select_best_prefix or (preferred) thread a persistent cache through the caller: create a single dict cache in build_hyperpixel_using_potential and pass that same cache into each call to select_best_prefix (keep the mask keys as used, i.e., prefix_masks and cache[prefix_masks[i]] assignments), or if removing caching, delete the cache parameter and all cache lookups/assignments (lines around scores population and cache[...] = signed_score) and simplify score handling accordingly.
289-310:base_frontieris computed but unused bysample_connected_superset.Line 298-300 computes
valid_n_neighborsandbase_frontier, butsample_connected_supersetnever uses itsbase_frontierparameter (it recomputes the frontier internally on each step). This work is wasted on every simulation.This is the downstream effect of the dead
base_frontierparameter flagged inbitmask_graph.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/potential.py` around lines 289 - 310, The loop builds a precomputed exploration boundary (valid_n_neighbors and base_frontier) but sample_connected_superset currently ignores its base_frontier argument, wasting work; update sample_connected_superset to accept and initialize its internal frontier from the passed base_frontier (instead of recomputing from base_mask/current state), and ensure its signature and callers (here in potential.py) use that frontier; alternatively, if you prefer to keep sample_connected_superset unchanged, remove the base_frontier computation and argument here (symbols to inspect: sample_connected_superset, base_frontier, valid_n_neighbors, current_frontier_mask, add_node, remove_node, adj_masks) and either pass only base_mask/target_length/used_mask or refactor so frontier logic is centralized in one place.ciao/algorithm/lookahead_bitset.py (1)
217-223: Remove the unused_find_first_stepfunction.After replacing the BFS fallback with a
RuntimeError(lines 200–203), this function at lines 217–223 is no longer called anywhere in the module and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/lookahead_bitset.py` around lines 217 - 223, Remove the now-unused helper function _find_first_step from the module: delete the entire def _find_first_step(base_mask: int, target_mask: int) block (the docstring, computation of diff, loop over iter_bits, and the final ValueError) since the BFS fallback was replaced by raising a RuntimeError and nothing references _find_first_step any more.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 173-174: Remove the ".pre-commit-config.yaml" entry from
.gitignore and commit the actual ".pre-commit-config.yaml" file into the
repository so the shared pre-commit configuration is tracked; if you need
local-only ignores, move them to .git/info/exclude or a separate local file, but
do not keep ".pre-commit-config.yaml" listed in .gitignore.
In `@ciao/algorithm/lookahead_bitset.py`:
- Around line 23-36: The function build_hyperpixel_greedy_lookahead has an
unused parameter scores; remove scores from the signature and any type hints,
update the docstring to stop mentioning it, and adjust all call sites to stop
passing scores (they should continue using optimization_sign and the internally
computed scores_list from calculate_hyperpixel_deltas()); ensure any tests or
usages that relied on the old parameter are updated to the new signature and run
type checking to catch mismatches.
In `@ciao/algorithm/mcts.py`:
- Around line 220-225: The backup loop over batch_paths currently skips the root
(path[1:]) causing root.visits to remain zero and collapsing exploration in
select_uct_child; change the loop to iterate over the entire path and handle the
root specially: for the root node (path[0]) increment visits, value_sum, and
max_value but do not decrement pending (avoid releasing virtual loss), while for
non-root nodes continue to decrement pending, increment visits, and update
value_sum and max_value; reference the loop over batch_paths/rewards and the
attributes pending, visits, value_sum, max_value and the select_uct_child
behavior to locate where to apply this conditional logic.
- Around line 261-266: The RAVE backup loop currently skips the root via for
node in path[1:], causing root visit-starvation; apply the same fix used in
backup_paths by including the root in the RAVE backup (or explicitly updating
the root before the loop): ensure path[0].visits (and its value_sum/max_value as
appropriate) is incremented and that node.pending is only decremented for
non-root nodes (or only when pending>0) so you can change the loop to iterate
over path (for node in path:) and guard pending decrement, or add an explicit
increment/update for path[0] before the existing loop; reference the loop
variables node, path, pending, visits, value_sum, and max_value when making the
change.
- Around line 520-536: The function signature for build_all_hyperpixels_mcts
declares an unused parameter next_id; remove next_id from the parameter list of
build_all_hyperpixels_mcts and update any call sites that pass next_id to it
(search for build_all_hyperpixels_mcts(...) to find callers) so they no longer
provide that argument, and run tests/linting to ensure no other references
remain; leave all other parameters and internal logic unchanged.
In `@ciao/utils/calculations.py`:
- Around line 16-28: The __init__ uses next(model.parameters()).device which
will raise StopIteration for parameter-free models; update the __init__ in
calculations.Calculations (or the class containing __init__) to defensively
obtain device: try to get device from next(model.parameters()), and if
StopIteration occurs fall back to a safe default (e.g., torch.device('cpu')) or
by checking for a model-level attribute (like model.device) before defaulting;
ensure self.device is always set and include a clear error or warning message
when falling back so debugging is easier.
- Around line 94-107: The code uses even_indices = torch.arange(0, height, 2)
for both row and column operations, which can index out of bounds on non-square
images; change to compute separate index ranges (e.g., even_row_indices =
torch.arange(0, height, 2) and even_col_indices = torch.arange(0, width, 2)) and
use even_col_indices when indexing the width/channel slice replacement_image[:,
:, ...] (flip vertically with torch.flip dims=[1]) and use even_row_indices when
indexing the height slice replacement_image[:, even_row_indices, :] (flip
horizontally with torch.flip dims=[2]); keep the same variables
replacement_image and input_tensor and the existing flip dims.
- Around line 255-263: The code assumes node IDs are 0..num_segments-1 which can
cause IndexError; instead build an explicit mapping from graph node IDs to
column indices (e.g., id_to_col = {node: idx for idx,node in
enumerate(segment_ids)}) and use that mapping when filling the surrogate matrix
X (replace uses of segment_id as a column index with id_to_col[segment_id]);
also update calculate_scores_from_surrogate (and any loop using
range(num_segments)) to iterate using the same mapping (or map columns back to
original node IDs) so all indexing is consistent for arbitrary graph node IDs
and add a validation/assertion that all masked segment IDs exist in id_to_col to
surface mismatches early.
---
Duplicate comments:
In `@ciao/algorithm/lookahead_bitset.py`:
- Around line 156-214: No change required: the BFS in
_generate_lookahead_candidates correctly tracks depth, assigns first_step (using
seg_id at depth==1 and inheriting from candidates for deeper depths), enforces
"first path wins" deduplication, and replaces the old silent fallback with a
RuntimeError for inconsistency; leave _generate_lookahead_candidates as-is.
In `@ciao/structures/bitmask_graph.py`:
- Around line 153-215: The _pick_weighted_frontier_segment function already
validates temperature (>0), handles -inf weights, applies optimization_sign,
uses numerically-stable softmax (subtracting max_score) and renormalizes after
epsilon-greedy mixing, so no code change is required; leave
_pick_weighted_frontier_segment as implemented.
In `@ciao/utils/calculations.py`:
- Around line 335-396: The previous issues have been resolved: replacement_image
is now validated with a RuntimeError (see replacement_image and
create_replacement_image()) and gpu_segments is moved outside the batch loop in
calculate_hyperpixel_deltas; no further changes are required in this
section—approve the PR as-is.
---
Nitpick comments:
In `@ciao/algorithm/lookahead_bitset.py`:
- Around line 217-223: Remove the now-unused helper function _find_first_step
from the module: delete the entire def _find_first_step(base_mask: int,
target_mask: int) block (the docstring, computation of diff, loop over
iter_bits, and the final ValueError) since the BFS fallback was replaced by
raising a RuntimeError and nothing references _find_first_step any more.
In `@ciao/algorithm/potential.py`:
- Around line 349-437: select_best_prefix currently contains dead caching
because build_hyperpixel_using_potential calls it with cache={} so lookups never
hit and stored values are thrown away; either remove the cache logic from
select_best_prefix or (preferred) thread a persistent cache through the caller:
create a single dict cache in build_hyperpixel_using_potential and pass that
same cache into each call to select_best_prefix (keep the mask keys as used,
i.e., prefix_masks and cache[prefix_masks[i]] assignments), or if removing
caching, delete the cache parameter and all cache lookups/assignments (lines
around scores population and cache[...] = signed_score) and simplify score
handling accordingly.
- Around line 289-310: The loop builds a precomputed exploration boundary
(valid_n_neighbors and base_frontier) but sample_connected_superset currently
ignores its base_frontier argument, wasting work; update
sample_connected_superset to accept and initialize its internal frontier from
the passed base_frontier (instead of recomputing from base_mask/current state),
and ensure its signature and callers (here in potential.py) use that frontier;
alternatively, if you prefer to keep sample_connected_superset unchanged, remove
the base_frontier computation and argument here (symbols to inspect:
sample_connected_superset, base_frontier, valid_n_neighbors,
current_frontier_mask, add_node, remove_node, adj_masks) and either pass only
base_mask/target_length/used_mask or refactor so frontier logic is centralized
in one place.
In `@ciao/structures/bitmask_graph.py`:
- Around line 99-150: sample_connected_superset currently accepts a dead
parameter base_frontier that is never used; remove this parameter from
sample_connected_superset's signature and all its callers (and update the
docstring and any tests) to eliminate the unnecessary API surface, or if you
must preserve compatibility, mark base_frontier as deprecated by keeping it in
the signature but adding a warning (e.g., via warnings.warn) in
sample_connected_superset and documenting that it is ignored; update references
to the function name sample_connected_superset and the docstring accordingly so
callers no longer construct or pass base_frontier.
In `@ciao/utils/calculations.py`:
- Around line 174-179: Remove or formalize the commented-out experimental logit
computation in calculations.py: eliminate the commented lines referencing
get_predictions, probabilities and the torch.log expression, or move them behind
a clear, documented configuration flag (e.g., an optional mode in the method
that currently returns outputs[:, target_class_idx]) so the alternative
computation can be toggled explicitly; update the function/method docstring to
mention the available modes and ensure the code only contains the active
implementation using outputs and target_class_idx.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignoreciao/algorithm/__init__.pyciao/algorithm/lookahead_bitset.pyciao/algorithm/mcts.pyciao/algorithm/potential.pyciao/structures/bitmask_graph.pyciao/utils/calculations.pyciao/utils/search_utils.pyciao/utils/segmentation.py
🚧 Files skipped from review as they are similar to previous changes (3)
- ciao/algorithm/init.py
- ciao/utils/search_utils.py
- ciao/utils/segmentation.py
| # pre-commit | ||
| .pre-commit-config.yaml No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pre-commit .pre-commit-config.yaml should be committed to version control
💡 Result:
Yes — .pre-commit-config.yaml should be committed to version control so everyone on the team (and CI) runs the same hooks and versions. The config file is meant to “live in version control,” while each developer still needs to run pre-commit install locally because Git hooks under .git/hooks/ are not versioned. [1], [2]
This is also how services like pre-commit.ci work: they rely on the repository’s committed .pre-commit-config.yaml to know what to run. [3]
Sources:
[1] Stefanie Molin pre-commit workshop (config lives in version control / commit it) (stefaniemolin.com)
[2] DevSecOps guide (config can be versioned; hooks install is local) (devsecops.puziol.com.br)
[3] pre-commit.ci (configured via .pre-commit-config.yaml) (pre-commit.ci)
Citations:
- 1: https://stefaniemolin.com/pre-commit-workshop/?utm_source=openai
- 2: https://devsecops.puziol.com.br/en/development/pre-commit/?utm_source=openai
- 3: https://pre-commit.ci/?utm_source=openai
Remove .pre-commit-config.yaml from .gitignore.
.pre-commit-config.yaml is a shared project configuration file that must be committed to version control. It ensures all team members and CI/CD services run the same hooks and versions. Ignoring it breaks team consistency and prevents services like pre-commit.ci from functioning correctly—each developer would need to independently set up their own configuration, defeating the purpose of a shared tool.
If the intent is to keep a personal/local variation out of VCS, use .git/info/exclude (local-only) instead and commit the shared .pre-commit-config.yaml to the repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 173 - 174, Remove the ".pre-commit-config.yaml"
entry from .gitignore and commit the actual ".pre-commit-config.yaml" file into
the repository so the shared pre-commit configuration is tracked; if you need
local-only ignores, move them to .git/info/exclude or a separate local file, but
do not keep ".pre-commit-config.yaml" listed in .gitignore.
| def build_hyperpixel_greedy_lookahead( | ||
| predictor: ModelPredictor, | ||
| input_batch: torch.Tensor, | ||
| segments: np.ndarray, | ||
| adj_masks: tuple[int, ...], | ||
| target_class_idx: int, | ||
| scores: dict[int, float], | ||
| seed_idx: int, | ||
| desired_length: int, | ||
| lookahead_distance: int, | ||
| optimization_sign: int, | ||
| used_mask: int, | ||
| batch_size: int = 64, | ||
| ) -> dict[str, object]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the function to understand the context
cat -n ciao/algorithm/lookahead_bitset.py | head -60Repository: RationAI/ciao
Length of output: 2277
🏁 Script executed:
# Now read the function body from lines 59-153 to verify scores usage
sed -n '59,153p' ciao/algorithm/lookahead_bitset.py | cat -nRepository: RationAI/ciao
Length of output: 4000
🏁 Script executed:
# Search for any reference to 'scores' in the entire function
rg -n 'scores' ciao/algorithm/lookahead_bitset.py -A 2 -B 2Repository: RationAI/ciao
Length of output: 2294
Remove unused scores parameter from function signature.
The scores parameter (line 29) is never referenced in the function body. The docstring incorrectly suggests it's used "for determining sign," but the function actually receives optimization_sign as a separate parameter and computes scores_list internally via calculate_hyperpixel_deltas(). Remove this parameter to avoid misleading callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/algorithm/lookahead_bitset.py` around lines 23 - 36, The function
build_hyperpixel_greedy_lookahead has an unused parameter scores; remove scores
from the signature and any type hints, update the docstring to stop mentioning
it, and adjust all call sites to stop passing scores (they should continue using
optimization_sign and the internally computed scores_list from
calculate_hyperpixel_deltas()); ensure any tests or usages that relied on the
old parameter are updated to the new signature and run type checking to catch
mismatches.
| for path, reward in zip(batch_paths, rewards, strict=True): | ||
| for node in path[1:]: # Skip root (never incremented, so shouldn't decrement) | ||
| node.pending -= 1 # Release virtual loss | ||
| node.visits += 1 | ||
| node.value_sum += reward # Mean tracking | ||
| node.max_value = max(node.max_value, reward) # MAX backup |
There was a problem hiding this comment.
Skipping root in backup fixes pending underflow but starves root of visit counts, killing UCT exploration at depth 1.
path[1:] means root.visits stays 0 forever. In select_uct_child (Line 87), parent_visits = node.visits + 1 = 1, so log(1) = 0, and the exploration term for all of root's children is zero. Selection at depth 1 degrades to pure exploitation — once one child receives a positive max_value, it is always selected over unvisited siblings.
The fix should increment root visits without decrementing root's pending:
Proposed fix
def backup_paths(batch_paths: list[list[MCTSNode]], rewards: list[float]) -> None:
for path, reward in zip(batch_paths, rewards, strict=True):
- for node in path[1:]: # Skip root (never incremented, so shouldn't decrement)
+ # Update root visits (but not pending, since root is never given virtual loss)
+ path[0].visits += 1
+ path[0].value_sum += reward
+ path[0].max_value = max(path[0].max_value, reward)
+
+ for node in path[1:]:
node.pending -= 1 # Release virtual loss
node.visits += 1
node.value_sum += reward # Mean tracking
node.max_value = max(node.max_value, reward) # MAX backup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/algorithm/mcts.py` around lines 220 - 225, The backup loop over
batch_paths currently skips the root (path[1:]) causing root.visits to remain
zero and collapsing exploration in select_uct_child; change the loop to iterate
over the entire path and handle the root specially: for the root node (path[0])
increment visits, value_sum, and max_value but do not decrement pending (avoid
releasing virtual loss), while for non-root nodes continue to decrement pending,
increment visits, and update value_sum and max_value; reference the loop over
batch_paths/rewards and the attributes pending, visits, value_sum, max_value and
the select_uct_child behavior to locate where to apply this conditional logic.
| def __init__(self, model: torch.nn.Module, class_names: list[str]) -> None: | ||
| self.model = model | ||
| self.class_names = class_names | ||
| self.device = next(model.parameters()).device | ||
| self.replacement_image = None | ||
|
|
||
| # Pre-compute normalization constants | ||
| self.imagenet_mean = ( | ||
| torch.tensor([0.485, 0.456, 0.406]).view(1, 3, 1, 1).to(self.device) | ||
| ) | ||
| self.imagenet_std = ( | ||
| torch.tensor([0.229, 0.224, 0.225]).view(1, 3, 1, 1).to(self.device) | ||
| ) |
There was a problem hiding this comment.
next(model.parameters()).device will raise StopIteration on a parameter-free model.
If a model with no parameters is ever passed (e.g., a wrapper or identity model during testing), this will crash without a clear message. A defensive guard would improve debuggability.
Proposed fix
def __init__(self, model: torch.nn.Module, class_names: list[str]) -> None:
self.model = model
self.class_names = class_names
- self.device = next(model.parameters()).device
+ try:
+ self.device = next(model.parameters()).device
+ except StopIteration:
+ self.device = torch.device("cpu")
self.replacement_image = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/utils/calculations.py` around lines 16 - 28, The __init__ uses
next(model.parameters()).device which will raise StopIteration for
parameter-free models; update the __init__ in calculations.Calculations (or the
class containing __init__) to defensively obtain device: try to get device from
next(model.parameters()), and if StopIteration occurs fall back to a safe
default (e.g., torch.device('cpu')) or by checking for a model-level attribute
(like model.device) before defaulting; ensure self.device is always set and
include a clear error or warning message when falling back so debugging is
easier.
| # Create surrogate dataset | ||
| num_samples = len(local_groups) | ||
| X = np.zeros((num_samples, num_segments), dtype=np.float32) | ||
| y = np.array(deltas, dtype=np.float32) | ||
|
|
||
| # Fill indicator matrix | ||
| for i, masked_segments in enumerate(local_groups): | ||
| for segment_id in masked_segments: | ||
| X[i, segment_id] = 1.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the file and check its size
find . -name "calculations.py" -path "*/ciao/utils/*" | head -5Repository: RationAI/ciao
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Read the specific lines and surrounding context to understand the code
if [ -f "ciao/utils/calculations.py" ]; then
wc -l ciao/utils/calculations.py
fiRepository: RationAI/ciao
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Get the function context around lines 255-263
sed -n '240,280p' ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 1405
🏁 Script executed:
#!/bin/bash
# Search for where num_segments is defined in the function
rg -n "num_segments\s*=" ciao/utils/calculations.py -B5 -A5Repository: RationAI/ciao
Length of output: 850
🏁 Script executed:
#!/bin/bash
# Search for segment_ids definition and how local_groups is constructed
rg -n "segment_ids|local_groups" ciao/utils/calculations.py -B3 -A3Repository: RationAI/ciao
Length of output: 3188
🏁 Script executed:
#!/bin/bash
# Find the function signature and understand graph parameter
sed -n '200,230p' ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 1133
🏁 Script executed:
#!/bin/bash
# Search for where this function is called to see the graph source
rg -n "def.*graph.*:" ciao/utils/calculations.py -B2 -A10Repository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for graph imports and creation
rg -n "graph\s*=" ciao/utils/calculations.py -B5 -A5Repository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for where the segments are created/obtained
rg -n "segments\s*=" ciao/utils/calculations.py -B3 -A3Repository: RationAI/ciao
Length of output: 760
🏁 Script executed:
#!/bin/bash
# Look for imports to understand the graph library being used
head -30 ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 916
🏁 Script executed:
#!/bin/bash
# Find the function definition that uses graph parameter
rg -n "def.*graph.*:" ciao/utils/calculations.py -B2 -A15Repository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for where this function is called
rg -n "create_surrogate_dataset" --type=py -B2 -A2Repository: RationAI/ciao
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Look for segmentation library usage and graph creation
rg -n "Graph|nx\.Graph|segment" ciao/utils/calculations.py | head -20Repository: RationAI/ciao
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Check if there's segment ID validation anywhere
rg -n "validate|check|assert.*segment" ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for calls to create_surrogate_dataset in all Python files
rg "create_surrogate_dataset" --type=py -B3 -A3Repository: RationAI/ciao
Length of output: 413
🏁 Script executed:
#!/bin/bash
# Search for graph creation patterns - NetworkX graph instantiation
rg "nx\.Graph|from_nx|DiGraph" --type=py -B2 -A2Repository: RationAI/ciao
Length of output: 2318
🏁 Script executed:
#!/bin/bash
# Look for segmentation imports and segment ID generation
rg "skimage|watershed|felzenszwalb|slic|segment" --type=py -B2 -A2 | head -40Repository: RationAI/ciao
Length of output: 2315
🏁 Script executed:
#!/bin/bash
# Check main files and entry points
find . -name "main.py" -o -name "__main__.py" -o -name "demo.py" | head -5Repository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check create_square_grid implementation
sed -n '/def create_square_grid/,/^def /p' ciao/utils/segmentation.py | head -50Repository: RationAI/ciao
Length of output: 1002
🏁 Script executed:
#!/bin/bash
# Check create_hexagonal_grid implementation
sed -n '/def create_hexagonal_grid/,/^def /p' ciao/utils/segmentation.py | head -60Repository: RationAI/ciao
Length of output: 2132
🏁 Script executed:
#!/bin/bash
# Check the complete segmentation.py to understand segment ID generation
wc -l ciao/utils/segmentation.pyRepository: RationAI/ciao
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Get the complete create_hexagonal_grid function
sed -n '/^def create_hexagonal_grid\(/,/^def /p' ciao/utils/segmentation.py | head -70Repository: RationAI/ciao
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Verify segment ID generation is always contiguous by checking build_adjacency_graph
sed -n '/^def build_adjacency_graph/,/^def /p' ciao/utils/segmentation.pyRepository: RationAI/ciao
Length of output: 1375
🏁 Script executed:
#!/bin/bash
# Escape the parenthesis properly and get the complete create_hexagonal_grid function
sed -n '/^def create_hexagonal_grid/,/^def /p' ciao/utils/segmentation.py | head -80Repository: RationAI/ciao
Length of output: 2413
🏁 Script executed:
#!/bin/bash
# Let's verify: are segment_ids always contiguous in both grid creation functions?
# Check if there are any other segmentation methods that might not use contiguous IDs
rg "def create_" ciao/utils/segmentation.py | grep -v "^Binary"Repository: RationAI/ciao
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check if there's any external segmentation library used that might have non-contiguous IDs
rg "felzenszwalb|watershed|slic|mark_boundaries" --type=pyRepository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Verify the complete flow - trace from segment creation to graph creation
cat ciao/utils/segmentation.py | grep -A5 "segment_id = \|next_id = "Repository: RationAI/ciao
Length of output: 621
🏁 Script executed:
#!/bin/bash
# Check if there's any validation of segment IDs in the function
sed -n '180,270p' ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 2999
🏁 Script executed:
#!/bin/bash
# Search for any tests that verify the behavior with different graph structures
find . -name "*test*.py" -o -name "test_*.py" | head -10Repository: RationAI/ciao
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are any docstrings or comments about segment ID requirements
rg "segment.*id|contiguous|range" ciao/utils/calculations.py -B2 -A2Repository: RationAI/ciao
Length of output: 4415
X[i, segment_id] = 1.0 assumes segment IDs are contiguous 0..N-1.
The function accepts a generic nx.Graph parameter but doesn't validate that graph.nodes() returns IDs in the range [0, num_segments). When segment_ids = list(graph.nodes()) is unpacked, if any segment ID exceeds the column count, X[i, segment_id] = 1.0 will raise an IndexError. This assumption is further embedded in calculate_scores_from_surrogate() (line 288), which iterates with range(num_segments), expecting indices 0..N-1.
Add validation to ensure segment IDs are contiguous, or build an ID-to-column mapping to handle arbitrary node IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/utils/calculations.py` around lines 255 - 263, The code assumes node IDs
are 0..num_segments-1 which can cause IndexError; instead build an explicit
mapping from graph node IDs to column indices (e.g., id_to_col = {node: idx for
idx,node in enumerate(segment_ids)}) and use that mapping when filling the
surrogate matrix X (replace uses of segment_id as a column index with
id_to_col[segment_id]); also update calculate_scores_from_surrogate (and any
loop using range(num_segments)) to iterate using the same mapping (or map
columns back to original node IDs) so all indexing is consistent for arbitrary
graph node IDs and add a validation/assertion that all masked segment IDs exist
in id_to_col to surface mismatches early.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ciao/utils/calculations.py (2)
16-20:⚠️ Potential issue | 🟡 MinorGuard device inference for parameter-free models.
Line 19 can raise
StopIterationfor modules without parameters. Please keep a safe fallback device.In Python, what exception does next(iterator) raise when the iterator is empty?Suggested fix
def __init__(self, model: torch.nn.Module, class_names: list[str]) -> None: self.model = model self.class_names = class_names - self.device = next(model.parameters()).device + try: + self.device = next(model.parameters()).device + except StopIteration: + self.device = torch.device("cpu") self.replacement_image = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 16 - 20, The constructor currently uses next(model.parameters()).device which raises StopIteration for parameter-free models; change to use a safe lookup like p = next(model.parameters(), None) and set self.device = p.device if p is not None else torch.device("cpu") (or another desired fallback), leaving other attributes (self.model, self.class_names, self.replacement_image) unchanged; update the __init__ in the same class/method to perform this guarded device inference.
226-228:⚠️ Potential issue | 🟠 MajorValidate or remap graph node IDs before filling
X.Line 264 uses
segment_idas a direct column index, which breaks when graph node IDs are not contiguous0..N-1(and then also misaligns Line 289’s scoring loop). This is still the same unresolved issue from earlier review.Suggested fix
segment_ids = list(graph.nodes()) num_segments = len(segment_ids) + expected_ids = list(range(num_segments)) + if sorted(segment_ids) != expected_ids: + raise ValueError( + "Graph node IDs must be contiguous integers in [0, num_segments). " + "Provide remapped IDs or add an explicit id->column mapping." + ) @@ for i, masked_segments in enumerate(local_groups): for segment_id in masked_segments: X[i, segment_id] = 1.0Also applies to: 262-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/utils/calculations.py` around lines 226 - 228, The code assumes graph node IDs are contiguous and uses segment_id as a direct column index when filling X and later when scoring; instead create an explicit mapping from graph node IDs to contiguous column indices (e.g., segment_id_to_idx) derived from list(graph.nodes()) (or a sorted list if stable order is required), use that mapping whenever you index into X or iterate to compute scores, and replace any direct uses of segment_id as a column index with segment_id_to_idx[segment_id] so X and the scoring loop remain aligned for non-contiguous or arbitrary node IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ciao/utils/calculations.py`:
- Around line 351-354: The RuntimeError message raised when replacement_image is
uninitialized references the wrong helper name; update the message in the raise
inside calculate_hyperpixel_deltas (the block that checks replacement_image) to
instruct callers to call get_replacement_image() instead of
create_replacement_image(), keeping the same explanatory text otherwise so the
error points to the actual public API function.
- Around line 160-167: plot_image_mean_color currently passes a potentially CUDA
tensor to plt.imshow, which will fail on GPU; modify plot_image_mean_color (and
use calculate_image_mean_color output) to .detach(), .cpu(), convert to a NumPy
array, reorder to HWC (permute as needed) and then pass that NumPy array to
plt.imshow (optionally clip or squeeze if shape differs) before plt.show().
---
Duplicate comments:
In `@ciao/utils/calculations.py`:
- Around line 16-20: The constructor currently uses
next(model.parameters()).device which raises StopIteration for parameter-free
models; change to use a safe lookup like p = next(model.parameters(), None) and
set self.device = p.device if p is not None else torch.device("cpu") (or another
desired fallback), leaving other attributes (self.model, self.class_names,
self.replacement_image) unchanged; update the __init__ in the same class/method
to perform this guarded device inference.
- Around line 226-228: The code assumes graph node IDs are contiguous and uses
segment_id as a direct column index when filling X and later when scoring;
instead create an explicit mapping from graph node IDs to contiguous column
indices (e.g., segment_id_to_idx) derived from list(graph.nodes()) (or a sorted
list if stable order is required), use that mapping whenever you index into X or
iterate to compute scores, and replace any direct uses of segment_id as a column
index with segment_id_to_idx[segment_id] so X and the scoring loop remain
aligned for non-contiguous or arbitrary node IDs.
| def plot_image_mean_color(self, input_tensor: torch.Tensor) -> None: | ||
| """Display the mean color of the image. | ||
|
|
||
| Note: The visualization shows the normalized tensor (ImageNet normalization). | ||
| """ | ||
| normalized_mean = self.calculate_image_mean_color(input_tensor).unsqueeze(0) | ||
| plt.imshow(normalized_mean[0].permute(1, 2, 0)) | ||
| plt.show() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "calculations.py" -path "*/utils/*" | head -5Repository: RationAI/ciao
Length of output: 85
🌐 Web query:
matplotlib.pyplot.imshow PyTorch tensor CUDA GPU support direct input
💡 Result:
matplotlib.pyplot.imshow does not accept a CUDA torch.Tensor “directly”. Matplotlib’s image APIs expect CPU array-like inputs (effectively NumPy/PIL), and a CUDA tensor can’t be converted to NumPy without copying it back to host memory. [1], [2], [3]
Use:
plt.imshow(t.detach().cpu().numpy())
# (optional for CHW tensors)
# plt.imshow(t.detach().permute(1,2,0).cpu().numpy())If you need true “display from GPU without a CPU copy”, Matplotlib isn’t the right tool—you’d need a dedicated GPU visualization path (e.g., OpenGL/Vulkan interop tooling), not imshow. [2]
🏁 Script executed:
cat -n ./ciao/utils/calculations.py | sed -n '155,170p'Repository: RationAI/ciao
Length of output: 769
🏁 Script executed:
# Check broader context and class definition
head -20 ./ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 551
🏁 Script executed:
# Look for calculate_image_mean_color method
rg -n "def calculate_image_mean_color" ./ciao/utils/calculations.py -A 5Repository: RationAI/ciao
Length of output: 361
🏁 Script executed:
# Check the full calculate_image_mean_color method
rg -n "def calculate_image_mean_color" ./ciao/utils/calculations.py -A 15Repository: RationAI/ciao
Length of output: 855
🏁 Script executed:
# Look for other plotting methods in the file to see if they handle tensor conversion
rg -n "plt\." ./ciao/utils/calculations.pyRepository: RationAI/ciao
Length of output: 139
🏁 Script executed:
# Check if there are other similar tensor visualization patterns in the codebase
rg -n "\.detach\(\)\.cpu\(\)\.numpy\(\)" . --type py | head -10Repository: RationAI/ciao
Length of output: 39
🏁 Script executed:
# Check if the class stores/tracks device for the model
rg -n "self.device" ./ciao/utils/calculations.py -B 2 -A 2Repository: RationAI/ciao
Length of output: 1293
🏁 Script executed:
# Verify if input_tensor device can vary (GPU or CPU)
rg -n "plot_image_mean_color" ./ciao/utils/calculations.py -B 5 -A 10Repository: RationAI/ciao
Length of output: 721
Convert tensor to CPU/NumPy before imshow.
plot_image_mean_color passes a tensor directly to matplotlib. When the model is on GPU, normalized_mean will be a CUDA tensor, and matplotlib.pyplot.imshow() cannot accept CUDA tensors directly—it requires CPU-resident NumPy arrays. This will fail in any GPU workflow.
Suggested fix
def plot_image_mean_color(self, input_tensor: torch.Tensor) -> None:
"""Display the mean color of the image.
Note: The visualization shows the normalized tensor (ImageNet normalization).
"""
normalized_mean = self.calculate_image_mean_color(input_tensor).unsqueeze(0)
- plt.imshow(normalized_mean[0].permute(1, 2, 0))
+ img = (
+ normalized_mean[0]
+ .detach()
+ .cpu()
+ .permute(1, 2, 0)
+ .clamp(0, 1)
+ .numpy()
+ )
+ plt.imshow(img)
plt.show()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/utils/calculations.py` around lines 160 - 167, plot_image_mean_color
currently passes a potentially CUDA tensor to plt.imshow, which will fail on
GPU; modify plot_image_mean_color (and use calculate_image_mean_color output) to
.detach(), .cpu(), convert to a NumPy array, reorder to HWC (permute as needed)
and then pass that NumPy array to plt.imshow (optionally clip or squeeze if
shape differs) before plt.show().
| raise RuntimeError( | ||
| "replacement_image is not initialized. " | ||
| "Call create_replacement_image() before using calculate_hyperpixel_deltas." | ||
| ) |
There was a problem hiding this comment.
Fix stale method name in error message.
The message says create_replacement_image(), but this module exposes get_replacement_image(). This makes troubleshooting harder.
Suggested fix
raise RuntimeError(
"replacement_image is not initialized. "
- "Call create_replacement_image() before using calculate_hyperpixel_deltas."
+ "Call get_replacement_image() before using calculate_hyperpixel_deltas."
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ciao/utils/calculations.py` around lines 351 - 354, The RuntimeError message
raised when replacement_image is uninitialized references the wrong helper name;
update the message in the raise inside calculate_hyperpixel_deltas (the block
that checks replacement_image) to instruct callers to call
get_replacement_image() instead of create_replacement_image(), keeping the same
explanatory text otherwise so the error points to the actual public API
function.
**DEPENDS ON PR #2 **
Please review and merge PR #2 first. This PR is stacked on top of it, so it temporarily shows commits from PR #2. Once PR #2 is merged into master, those commits will automatically disappear from this diff.
What was changed:
structures/- nodes, bitmask graphs).utils/- segmentation, surrogate calculations).algorithm/- MCTS, MCGS, lookahead). The logic is described here XAI-A-21.ruffandmypylinting issues for the newly added modules.Why:
To establish the underlying mathematical and algorithmic backbone of the CIAO package. Moving these isolated modules first allows us to keep the codebase clean before wrapping them into the final user-facing
CIAOExplainerinterface in the next PR.Related Task:
XAI-29
Summary by CodeRabbit
New Features
Documentation
Chores