feat: implement CIAO explainer pipeline and strategy pattern for configurations#10
Open
dhalmazna wants to merge 14 commits intofeat/lookahead-algorithmfrom
Open
feat: implement CIAO explainer pipeline and strategy pattern for configurations#10dhalmazna wants to merge 14 commits intofeat/lookahead-algorithmfrom
dhalmazna wants to merge 14 commits intofeat/lookahead-algorithmfrom
Conversation
…ance CIAOExplainer with new ExplanationResult structure
…ing and image replacement
…lts and explanations
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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 refactors the CIAO explainer to use a strategy pattern with dataclasses for configuration (segmentation, replacement, and search methods) and introduces the CIAOExplainer class. It also converts HyperpixelResult from a dictionary to a dataclass. Feedback includes updating docstrings to reflect these type changes and removing a redundant type conversion in the sorting logic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ciao/explainer/strategies.py (1)
12-15: Add__post_init__validation to make strategy configs fail-fast.Several strategy params can be invalid at construction time (e.g., negative lookahead, invalid blur kernel/sigma, out-of-range RGB, non-positive segment sizes). Validating in dataclasses catches config errors earlier and keeps downstream code simpler.
💡 Proposed refactor
`@dataclass` class LookaheadMethod(ExplanationMethod): """Configuration for the lookahead hyperpixel building method.""" lookahead_distance: int = 2 + + def __post_init__(self) -> None: + if self.lookahead_distance < 1: + raise ValueError( + f"lookahead_distance must be >= 1, got {self.lookahead_distance}" + ) `@dataclass` class BlurReplacement(Replacement): """Configuration for blur replacement strategy.""" sigma: tuple[float, float] = (5.0, 5.0) kernel_size: tuple[int, int] = (15, 15) + + def __post_init__(self) -> None: + if any(k <= 0 or k % 2 == 0 for k in self.kernel_size): + raise ValueError(f"kernel_size must contain positive odd values, got {self.kernel_size}") + if any(s <= 0 for s in self.sigma): + raise ValueError(f"sigma must contain positive values, got {self.sigma}") `@dataclass` class SolidColorReplacement(Replacement): """Configuration for solid color replacement strategy.""" color: tuple[int, int, int] = (0, 0, 0) + + def __post_init__(self) -> None: + if any(c < 0 or c > 255 for c in self.color): + raise ValueError(f"color must be in RGB 0-255 range, got {self.color}") `@dataclass` class HexagonalSegmentation(SegmentationMethod): """Configuration for hexagonal grid segmentation.""" hex_radius: int = 4 + + def __post_init__(self) -> None: + if self.hex_radius <= 0: + raise ValueError(f"hex_radius must be positive, got {self.hex_radius}") `@dataclass` class SquareSegmentation(SegmentationMethod): """Configuration for square grid segmentation.""" square_size: int = 4 neighborhood: int = 8 + + def __post_init__(self) -> None: + if self.square_size <= 0: + raise ValueError(f"square_size must be positive, got {self.square_size}") + if self.neighborhood not in (4, 8): + raise ValueError(f"neighborhood must be 4 or 8, got {self.neighborhood}")Also applies to: 29-33, 42-45, 54-57, 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/explainer/strategies.py` around lines 12 - 15, Add a __post_init__ method to LookaheadMethod (and the other strategy dataclasses in this file) that validates inputs at construction time and raises ValueError on invalid values; for LookaheadMethod ensure lookahead_distance is an int >= 0, and for the other strategy classes add similar checks (e.g., blur kernel size is a positive odd int and sigma > 0, RGB values within 0–255, segment sizes > 0) so invalid configs fail-fast and downstream code can assume valid parameters.ciao/algorithm/builder.py (2)
56-61: State initialization is clear, though default formethodis also set upstream.The default
LookaheadMethod()is also assigned inCIAOExplainer.explain(). This redundancy is defensive but consider documenting whether callers should rely on this default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/builder.py` around lines 56 - 61, Defaulting method to LookaheadMethod() in builder.py duplicates the upstream default set in CIAOExplainer.explain(); remove ambiguity by documenting the intended ownership of the default and either keep the local fallback with a comment or remove it. Update the code around the "if method is None: method = LookaheadMethod()" block in builder.py and add a single-line doc comment to CIAOExplainer.explain (or to the builder function signature) stating whether callers are expected to pass method or rely on the upstream default; if you choose to keep the fallback, mark it as a defensive default with a comment referencing LookaheadMethod and CIAOExplainer.explain for clarity.
101-111: Consider validating empty region before updating state for clarity.The current order works correctly (empty region doesn't change
used_segments), but checking before state modification is more intuitive.♻️ Proposed reordering
# Extract and update state hyperpixel_region = result.region - used_segments = frozenset(used_segments | hyperpixel_region) if not hyperpixel_region: raise RuntimeError( f"Builder failed to generate any segments for seed {seed_idx}." ) + used_segments = frozenset(used_segments | hyperpixel_region) hyperpixels.append(result) processed_segments.update(hyperpixel_region)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/builder.py` around lines 101 - 111, Move the empty-region check to occur before any state updates so we validate and bail out early: after assigning hyperpixel_region = result.region, test if not hyperpixel_region and raise the RuntimeError (including seed_idx) before computing used_segments or updating processed_segments/hyperpixels; adjust the order of assignments involving used_segments, processed_segments.update(...), and hyperpixels.append(result) so state is only mutated after the non-empty check.
🤖 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/builder.py`:
- Around line 53-55: The docstring's Returns section incorrectly says "List of
hyperpixel dicts" while the function actually returns list[HyperpixelResult];
update the Returns description in the function docstring to reflect the real
return type (e.g., "List[HyperpixelResult]: list of HyperpixelResult objects
sorted by absolute score") and ensure any type name used matches the
HyperpixelResult symbol in the code so readers and tools see the correct type.
In `@ciao/explainer/ciao_explainer.py`:
- Around line 72-74: Update the docstring's Returns section to accurately
reflect that the method returns an ExplanationResult dataclass (not a generic
dictionary); change the text to something like "ExplanationResult dataclass
containing explanation artifacts and stats" and ensure any type references or
examples in the docstring reference ExplanationResult (the dataclass) to match
the actual return value in ciao_explainer.py.
In `@ciao/model/predictor.py`:
- Around line 29-32: get_predicted_class currently assumes a single-item batch
but silently uses the first element; add a strict check at the start of
get_predicted_class that verifies input_batch has batch size 1 (e.g., check
input_batch.dim() and input_batch.shape[0] == 1) and raise a clear ValueError if
not, then call get_predictions and return int(torch.argmax(probs[0]).item()) as
before; reference get_predicted_class and get_predictions so the check and error
live inside get_predicted_class.
---
Nitpick comments:
In `@ciao/algorithm/builder.py`:
- Around line 56-61: Defaulting method to LookaheadMethod() in builder.py
duplicates the upstream default set in CIAOExplainer.explain(); remove ambiguity
by documenting the intended ownership of the default and either keep the local
fallback with a comment or remove it. Update the code around the "if method is
None: method = LookaheadMethod()" block in builder.py and add a single-line doc
comment to CIAOExplainer.explain (or to the builder function signature) stating
whether callers are expected to pass method or rely on the upstream default; if
you choose to keep the fallback, mark it as a defensive default with a comment
referencing LookaheadMethod and CIAOExplainer.explain for clarity.
- Around line 101-111: Move the empty-region check to occur before any state
updates so we validate and bail out early: after assigning hyperpixel_region =
result.region, test if not hyperpixel_region and raise the RuntimeError
(including seed_idx) before computing used_segments or updating
processed_segments/hyperpixels; adjust the order of assignments involving
used_segments, processed_segments.update(...), and hyperpixels.append(result) so
state is only mutated after the non-empty check.
In `@ciao/explainer/strategies.py`:
- Around line 12-15: Add a __post_init__ method to LookaheadMethod (and the
other strategy dataclasses in this file) that validates inputs at construction
time and raises ValueError on invalid values; for LookaheadMethod ensure
lookahead_distance is an int >= 0, and for the other strategy classes add
similar checks (e.g., blur kernel size is a positive odd int and sigma > 0, RGB
values within 0–255, segment sizes > 0) so invalid configs fail-fast and
downstream code can assume valid parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5102548b-104a-4771-9b8f-a6778e869cf1
📒 Files selected for processing (10)
README.mdciao/algorithm/builder.pyciao/algorithm/lookahead.pyciao/data/replacement.pyciao/data/segmentation.pyciao/explainer/__init__.pyciao/explainer/ciao_explainer.pyciao/explainer/strategies.pyciao/model/predictor.pyciao/scoring/hyperpixel.py
There was a problem hiding this comment.
Pull request overview
This PR introduces a new CIAOExplainer orchestration class and replaces string/dict-based configuration and result passing with a strategy-pattern API using dataclasses, improving type safety and ergonomics across the explanation pipeline.
Changes:
- Added strategy dataclasses for explanation method, segmentation, and replacement configuration.
- Implemented
CIAOExplainer.explain()to run preprocessing → segmentation → surrogate scoring → hyperpixel building end-to-end. - Added a unified hyperpixel builder loop and migrated hyperpixel/explanation results to dataclasses (plus a predictor helper for auto-target selection).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ciao/scoring/hyperpixel.py | Converts HyperpixelResult to a dataclass and updates selection sorting to use attribute access. |
| ciao/model/predictor.py | Adds get_predicted_class() helper for automatic target selection. |
| ciao/explainer/strategies.py | Introduces strategy/config dataclasses for methods, segmentation, and replacement. |
| ciao/explainer/ciao_explainer.py | Adds CIAOExplainer and ExplanationResult to orchestrate the full pipeline. |
| ciao/explainer/init.py | Exposes CIAOExplainer as the explainer package’s public API. |
| ciao/data/segmentation.py | Refactors segmentation to accept strategy objects instead of string/literal parameters. |
| ciao/data/replacement.py | Refactors replacement/masking to accept strategy objects instead of string/literal parameters. |
| ciao/algorithm/lookahead.py | Updates lookahead builder to return the new HyperpixelResult dataclass. |
| ciao/algorithm/builder.py | Adds a unified outer-loop hyperpixel builder with registry-based method dispatch. |
| README.md | Updates the project directory tree to reflect new files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ciao/algorithm/builder.py (1)
56-58: Consider consolidatingprocessed_segmentsandused_segments.Both
processed_segmentsandused_segmentstrack the same data (segments included in built hyperpixels) and are updated identically at line 101-103. The only difference is the type—setvsfrozenset.You could simplify by using just
used_segmentsfor both purposes, converting tosetonly when checking membership for seed selection:available_segments = [ seg_id for seg_id in scores if seg_id not in used_segments ]This would eliminate the duplication while maintaining the
frozensetsemantics for the builder call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ciao/algorithm/builder.py` around lines 56 - 58, processed_segments and used_segments track the same segment IDs and are updated identically; remove processed_segments and use used_segments (frozenset) everywhere, converting to a mutable set only when necessary for membership or updates: replace membership checks like "seg_id not in processed_segments" with "seg_id not in used_segments", and when adding new IDs update used_segments immutably (e.g. used_segments = used_segments.union(new_ids) or used_segments = frozenset(set(used_segments) | set(new_ids))) so the builder call that expects a frozenset still receives one; update references to processed_segments to used_segments and delete the processed_segments declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ciao/algorithm/builder.py`:
- Around line 56-58: processed_segments and used_segments track the same segment
IDs and are updated identically; remove processed_segments and use used_segments
(frozenset) everywhere, converting to a mutable set only when necessary for
membership or updates: replace membership checks like "seg_id not in
processed_segments" with "seg_id not in used_segments", and when adding new IDs
update used_segments immutably (e.g. used_segments =
used_segments.union(new_ids) or used_segments = frozenset(set(used_segments) |
set(new_ids))) so the builder call that expects a frozenset still receives one;
update references to processed_segments to used_segments and delete the
processed_segments declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69886fe9-ce37-415e-ba11-c4c6d5440da5
📒 Files selected for processing (4)
ciao/algorithm/builder.pyciao/explainer/ciao_explainer.pyciao/explainer/strategies.pyciao/model/predictor.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ciao/model/predictor.py
- ciao/explainer/ciao_explainer.py
… in hyperpixel construction
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context:
This PR introduces the main
CIAOExplainerclass, which orchestrates the entire explanation pipeline. To keep the API clean and scalable as we add more algorithms, we migrated from dictionary/string-based parameters to a strongly-typed Strategy Pattern using Python dataclasses. We also refactored the output data structures to dataclasses.What's Changed / Added:
ciao/explainer/strategies.py: Added base classes and dataclasses for all pipeline configurations (ExplanationMethod,Replacement,SegmentationMethod).ciao/explainer/ciao_explainer.py: Implemented the statelessCIAOExplainerclass. Theexplain()method seamlessly ties preprocessing, segmentation, surrogate scoring, and hyperpixel building together.ciao/algorithm/builder.py: Added thebuild_all_hyperpixelsfunction. It handles the outer loop for finding seeds, dynamically unpacks algorithm parameters usingasdict(), and sorts the final dataclass objects.ciao/data/segmentation.py&replacement.py: Refactored to accept the new strategy dataclasses instead of literal strings, improving type safety and removing manualkwargsparsing.hyperpixel.py&lookahead.py): ConvertedHyperpixelResultandExplanationResultfromTypedDictto@dataclass. This drops the clunky dict-syntax in favor of clean dot-notation access (e.g.,x.scoreinstead ofx["score"]) and significantly improves IDE autocomplete.ciao/model/predictor.py: Added a handyget_predicted_classhelper method for auto-selecting the target class if the user doesn't provide one.README.md: Updated the project directory tree.How to review this PR (Suggested order from a chatbot):
strategies.py& Data Structures: Start here to understand the new OOP configs and result formats.ciao_explainer.py: Look at the whole explanation algorithm.builder.py: Check the dynamic unpacking of algorithm parameters.segmentation,replacementand others: Review the minor updates that integrate the strategies.Related Task:
XAI-29
Summary by CodeRabbit
New Features
Documentation