Add colorblind-safe visualization theme and resolve merge conflict#136
Add colorblind-safe visualization theme and resolve merge conflict#136Software566 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds a new colorblind-friendly theme to the visualization styling module. A colorblind palette with semantically distinct colors for severity levels is defined, then packaged as a ChangesColorblind theme support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@trustlens/visualization/style.py`:
- Around line 211-215: COLORBLIND_THEME currently sets
semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS) which omits the default keys from
SEMANTIC_COLORS and can cause KeyError; fix by merging the defaults and the
overrides instead of replacing them — for example, create the theme semantic
mapping by starting from a deepcopy of SEMANTIC_COLORS and updating it with
COLORBLIND_SEMANTIC_COLORS so only "severity" is overridden while "verdict",
"grade", "direction", and "neutral" remain present in COLORBLIND_THEME;
alternatively, add all missing semantic keys to COLORBLIND_SEMANTIC_COLORS so it
fully defines every required semantic mapping.
🪄 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: 17511316-9420-450f-ae76-ef6be3f833af
📒 Files selected for processing (1)
trustlens/visualization/style.py
| COLORBLIND_THEME: Theme = Theme( | ||
| name="colorblind", | ||
| palette=list(COLORBLIND_PALETTE), | ||
| semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS), | ||
| ) |
There was a problem hiding this comment.
Missing semantic color mappings will cause KeyError at runtime.
COLORBLIND_SEMANTIC_COLORS only defines the "severity" key (lines 171-178), but the default SEMANTIC_COLORS provides five semantic mappings: "severity", "verdict", "grade", "direction", and "neutral". By explicitly passing semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS), the colorblind theme will lack the other four mappings. Code that accesses theme.semantic["verdict"] or other missing keys when using COLORBLIND_THEME will raise KeyError.
🛡️ Proposed fixes
Option 1 (recommended): Merge with defaults, override severity only
COLORBLIND_THEME: Theme = Theme(
name="colorblind",
palette=list(COLORBLIND_PALETTE),
- semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS),
+ semantic={
+ **{k: dict(v) for k, v in SEMANTIC_COLORS.items()},
+ **COLORBLIND_SEMANTIC_COLORS,
+ },
)Option 2: Define all semantic mappings in COLORBLIND_SEMANTIC_COLORS
Add the missing keys to COLORBLIND_SEMANTIC_COLORS (lines 171-178):
COLORBLIND_SEMANTIC_COLORS: dict[str, dict[str, str]] = {
"severity": {
"acceptable": "`#0072B2`",
"moderate": "`#E69F00`",
"severe": "`#D55E00`",
"unknown": "`#999999`",
- }
+ },
+ "verdict": {
+ "deploy": "`#0072B2`",
+ "caution": "`#E69F00`",
+ "do_not_deploy": "`#D55E00`",
+ },
+ "grade": {
+ "A": "`#009E73`",
+ "B": "`#0072B2`",
+ "C": "`#E69F00`",
+ "D": "`#D55E00`",
+ },
+ "direction": {
+ "positive": "`#0072B2`",
+ "negative": "`#E69F00`",
+ },
+ "neutral": {
+ "reference": "`#999999`",
+ "edge": "`#FFFFFF`",
+ "annotation_edge": "`#CCCCCC`",
+ "annotation_face": "`#FFFFFF`",
+ },
}Then update line 214:
- semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS),
+ semantic={k: dict(v) for k, v in COLORBLIND_SEMANTIC_COLORS.items()},📝 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.
| COLORBLIND_THEME: Theme = Theme( | |
| name="colorblind", | |
| palette=list(COLORBLIND_PALETTE), | |
| semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS), | |
| ) | |
| COLORBLIND_THEME: Theme = Theme( | |
| name="colorblind", | |
| palette=list(COLORBLIND_PALETTE), | |
| semantic={ | |
| **{k: dict(v) for k, v in SEMANTIC_COLORS.items()}, | |
| **COLORBLIND_SEMANTIC_COLORS, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@trustlens/visualization/style.py` around lines 211 - 215, COLORBLIND_THEME
currently sets semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS) which omits the
default keys from SEMANTIC_COLORS and can cause KeyError; fix by merging the
defaults and the overrides instead of replacing them — for example, create the
theme semantic mapping by starting from a deepcopy of SEMANTIC_COLORS and
updating it with COLORBLIND_SEMANTIC_COLORS so only "severity" is overridden
while "verdict", "grade", "direction", and "neutral" remain present in
COLORBLIND_THEME; alternatively, add all missing semantic keys to
COLORBLIND_SEMANTIC_COLORS so it fully defines every required semantic mapping.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR adds a colorblind-safe visualization theme but introduces a maintainability regression by removing structured docstring sections from core style functions, degrading API discoverability and documentation quality.
🚨 Critical Issues (P0)
None identified.
⚡ Key Risks & Improvements (P1)
- trustlens/visualization/style.py: Removal of
Attributes,Parameters,Returns, andExamplessections from docstrings ofTheme,apply_style,styled_figure, andget_categorical_colorsdegrades developer experience and reduces compatibility with auto-documentation tools like Sphinx.
💡 Suggestions (P2)
- trustlens/visualization/style.py: The new
COLORBLIND_THEMEis defined but never integrated into any plotting function or exposed via a public parameter, making it dead code that may confuse users expecting the theme to be functional.
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: trustlens/visualization/style.py
The PR removes the Attributes, Parameters, Returns, Raises, and Examples sections from the docstrings of Theme, apply_style, styled_figure, and get_categorical_colors. These structured docstrings provided critical usage guidance and autodoc-friendly metadata for downstream consumers (both human developers and tools like Sphinx). Their removal degrades maintainability by reducing discoverability of the API contract and makes the library harder to use programmatically. The change is observed directly in the diff (verbatim snippet above shows the removal of the Attributes block). This is not a breaking change but represents a clear regression in documentation quality.
Suggestion:
Restore the removed docstring sections. For example, for `Theme`:
```python
"""
Immutable bundle of style constants for a single named theme.
...
Attributes
----------
name : str
Identifier (e.g. ``"default"``).
...
"""(Apply analogous restoration to the other three functions.)
**Related Code:**
```python
"""
Immutable bundle of style constants for a single named theme.
A theme groups every visual constant needed to render TrustLens plots so
that future themes (``"dark"``, ``"colorblind"``, ``"publication"``) can be
added by registering a new :class:`Theme` instance without touching
plotting code.
- Attributes
- ----------
- name : str
- Identifier (e.g. ``"default"``).
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| COLORBLIND_THEME: Theme = Theme( | ||
| name="colorblind", | ||
| palette=list(COLORBLIND_PALETTE), | ||
| semantic=deepcopy(COLORBLIND_SEMANTIC_COLORS), | ||
| ) |
There was a problem hiding this comment.
P2 | Confidence: Medium
Speculative: The new COLORBLIND_THEME and its supporting constants (COLORBLIND_PALETTE, COLORBLIND_SEMANTIC_COLORS) are defined and exported in __all__, but no internal plotting function or test references them. The related context shows that all existing plotting modules (e.g., bias_plots.py, fairness.py) call apply_style() with no argument, defaulting to DEFAULT_THEME. The theme is thus dead code at this point. While the PR description states the goal is to “add colorblind-safe visualization theme,” the definition alone does not deliver value to end users; the missing integration creates a false sense of completeness and may lead to confusion if users attempt to use COLORBLIND_THEME without any guidance on how to apply it. Additionally, COLORBLIND_PALETTE and COLORBLIND_SEMANTIC_COLORS are omitted from __all__, making them less discoverable.
Code Suggestion:
def apply_style(theme: Theme | None = None) -> Iterator[Theme]:
if theme is None:
# Optionally allow global setting
theme = _ACTIVE_THEME # or DEFAULT_THEME
...
Khanz9664
left a comment
Khanz9664
left a comment
There was a problem hiding this comment.
@Software566 Thanks for the contribution and for working on accessibility improvements.
I reviewed the implementation and found one blocking issue before this can be merged.
The new COLORBLIND_THEME replaces the entire semantic mapping with COLORBLIND_SEMANTIC_COLORS, but that mapping currently only defines "severity". Existing visualizations expect additional semantic groups such as "verdict", "grade", "direction", and "neutral". As a result, using COLORBLIND_THEME can raise KeyError at runtime when those mappings are accessed.
I recommend constructing the colorblind theme by starting from the existing SEMANTIC_COLORS mapping and overriding only the entries that need colorblind-safe replacements. That preserves compatibility with the rest of the visualization layer while still providing the new palette.
Additionally, it looks like several detailed docstrings added during the recent documentation initiative were reduced to single-line descriptions. Since those expanded docstrings were intentionally added to improve contributor onboarding and API documentation quality, please preserve the existing documentation sections when updating this module.
Once the semantic mapping issue is resolved and the documentation regression is addressed, I'd be happy to take another look.
2 participants
Pull Request Template
Summary
Related Issue
Type of Change
Please check the type of change:
Changes Made
Testing
Pre-submit Checklist
Before submitting, please ensure you have:
pre-commit run --all-filesand all hooks passed.make test).CHANGELOG.mdif applicable.Notes for Reviewers
Summary by CodeRabbit