update(arc-basic): bokeh — comprehensive quality review#4370
update(arc-basic): bokeh — comprehensive quality review#4370github-actions[bot] merged 5 commits intomainfrom
Conversation
Comprehensive review and update of bokeh implementation for arc-basic.
There was a problem hiding this comment.
Pull request overview
This PR updates the bokeh implementation for the arc-basic specification, representing a comprehensive refactoring that improves both code quality and visual design. The update transitions from manually computed quadratic bezier curves to Bokeh's native cubic bezier glyph, adds interactive hover tooltips (addressing a previously noted weakness), and shifts from distance-based to weight-based arc coloring for better semantic clarity.
Changes:
- Refactored arc rendering to use Bokeh's native
bezierglyph with cubic bezier curves instead of manually computed quadratic curves - Added interactive HoverTool for both arcs (showing connection details) and nodes (showing character names)
- Changed color scheme from distance-based (yellow/blue) to weight-based (light/medium/dark blue gradient)
- Updated metadata with new versions (Python 3.14.3, bokeh 3.8.2) and set quality_score to null pending automated review
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| plots/arc-basic/metadata/bokeh.yaml | Updated metadata with new Python/library versions, timestamp, and null quality score for pending review |
| plots/arc-basic/implementations/bokeh.py | Comprehensive refactor using native bezier glyphs, added hover interactivity, weight-based styling, and improved visual design |
| y_axis_label="", | ||
| x_range=(-0.5, 10.5), | ||
| y_range=(-1.5, 4.5), | ||
| title="arc-basic \u00b7 bokeh \u00b7 pyplots.ai", |
There was a problem hiding this comment.
Using Unicode character '\u00b7' (middle dot) directly in the title string. While this works, it's inconsistent with the approach in the old version and some other files that use the actual character '·' directly. For better readability and consistency with other implementations, consider using the actual character directly in the string.
| title="arc-basic \u00b7 bokeh \u00b7 pyplots.ai", | |
| title="arc-basic · bokeh · pyplots.ai", |
|
|
||
| # HoverTool for edge details (Bokeh-distinctive interactivity) | ||
| hover = HoverTool( | ||
| tooltips=[("Connection", "@source_name \u2194 @target_name"), ("Frequency", "@weight_label")], line_policy="interp" |
There was a problem hiding this comment.
The HoverTool uses the bidirectional arrow character '\u2194' (↔) in the tooltip. While functional, consider using the actual character '↔' directly in the string for better code readability, similar to the recommendation for the title's middle dot.
| tooltips=[("Connection", "@source_name \u2194 @target_name"), ("Frequency", "@weight_label")], line_policy="interp" | |
| tooltips=[("Connection", "@source_name ↔ @target_name"), ("Frequency", "@weight_label")], line_policy="interp" |
|
|
||
| # HoverTool for edge details (Bokeh-distinctive interactivity) | ||
| hover = HoverTool( | ||
| tooltips=[("Connection", "@source_name \u2194 @target_name"), ("Frequency", "@weight_label")], line_policy="interp" |
There was a problem hiding this comment.
This line appears to exceed the project's 120 character line length limit. Consider splitting the tooltips list across multiple lines for better readability and to comply with the project's formatting standards.
| tooltips=[("Connection", "@source_name \u2194 @target_name"), ("Frequency", "@weight_label")], line_policy="interp" | |
| tooltips=[ | |
| ("Connection", "@source_name \u2194 @target_name"), | |
| ("Frequency", "@weight_label"), | |
| ], | |
| line_policy="interp", |
| p.scatter(x="x", y="y", source=node_source, size=30, fill_color="#306998", line_color="white", line_width=3) | ||
|
|
||
| # Node hover | ||
| node_hover = HoverTool(tooltips=[("Character", "@name")], renderers=[p.renderers[-1]]) |
There was a problem hiding this comment.
The node_hover HoverTool on line 119 references p.renderers[-1], which assumes the last renderer added is the scatter plot for nodes. This is fragile and could break if additional renderers are added later. Consider storing the scatter renderer in a variable (e.g., node_renderer = p.scatter(...)) and then referencing it explicitly in the HoverTool for better maintainability.
| p.scatter(x="x", y="y", source=node_source, size=30, fill_color="#306998", line_color="white", line_width=3) | |
| # Node hover | |
| node_hover = HoverTool(tooltips=[("Character", "@name")], renderers=[p.renderers[-1]]) | |
| node_renderer = p.scatter( | |
| x="x", | |
| y="y", | |
| source=node_source, | |
| size=30, | |
| fill_color="#306998", | |
| line_color="white", | |
| line_width=3, | |
| ) | |
| # Node hover | |
| node_hover = HoverTool(tooltips=[("Character", "@name")], renderers=[node_renderer]) |
| @@ -1,18 +1,17 @@ | |||
| """ pyplots.ai | |||
| """pyplots.ai | |||
There was a problem hiding this comment.
The docstring header has a space missing after the opening triple quotes. The format should be """ pyplots.ai (with a space) not """pyplots.ai (without space), based on the convention seen in other implementations like matplotlib and seaborn for this same plot.
| """pyplots.ai | |
| """ pyplots.ai |
AI Review - Attempt 1/3Image Description
Score: 85/100
Visual Quality (27/30)
Design Excellence (12/20)
Spec Compliance (15/15)
Data Quality (13/15)
Code Quality (10/10)
Library Mastery (8/10)
Score Caps Applied
Strengths
Weaknesses
Issues Found
AI Feedback for Next Attempt
Verdict: REJECTED |
Attempt 1/3 - fixes based on AI review
🔧 Repair Attempt 1/3Applied fixes based on AI review feedback. Status: Repair completed, re-triggering review... |
AI Review - Attempt 2/3Image Description
Score: 90/100
Visual Quality (27/30)
Design Excellence (15/20)
Spec Compliance (15/15)
Data Quality (15/15)
Code Quality (10/10)
Library Mastery (8/10)
Score Caps Applied
Strengths
Weaknesses
Issues FoundNone critical. Minor visual polish items noted in weaknesses. AI Feedback for Next Attempt
Verdict: APPROVED |
Summary
Updated bokeh implementation for arc-basic.
Changes: Comprehensive quality review and update
Changes
Test Plan
Generated with Claude Code
/updatecommand