update(bubble-packed): seaborn — comprehensive quality review#4359
update(bubble-packed): seaborn — comprehensive quality review#4359github-actions[bot] merged 4 commits intomainfrom
Conversation
Comprehensive review improving code quality, data choice, visual design, spec compliance, and library feature usage.
There was a problem hiding this comment.
Pull request overview
This PR performs a comprehensive quality review of the bubble-packed seaborn implementation, refactoring the code for improved clarity and maintainability while updating dependencies.
Changes:
- Refactored circle packing algorithm to use more efficient vectorized operations with NumPy arrays
- Renamed
datadictionary tosectorsfor better semantic clarity - Simplified labeling logic by removing complex external annotation system in favor of simpler truncation
- Updated metadata with current Python version (3.14.3) and tooling (claude-opus-4-6)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| plots/bubble-packed/metadata/seaborn.yaml | Updated metadata with new Python version, generated_by, and updated timestamp; set quality_score to null for re-evaluation |
| plots/bubble-packed/implementations/seaborn.py | Comprehensive refactor: improved circle packing algorithm with vectorization, simplified labeling, enhanced legend with circular markers, added descriptive title, removed unused imports |
| edgecolor="gray", | ||
| ) | ||
|
|
||
| sns.despine(left=True, bottom=True) |
There was a problem hiding this comment.
The sns.despine(left=True, bottom=True) call on line 153 is ineffective because ax.axis("off") on line 125 already removes all spines and ticks. The despine call should either be removed as redundant, or if you want to keep axes labels/ticks while removing spines, remove the axis("off") call and use despine with appropriate parameters.
| sns.despine(left=True, bottom=True) |
| Library: seaborn 0.13.2 | Python 3.13.11 | ||
| Quality: 90/100 | Created: 2025-12-23 | ||
| Library: seaborn 0.13.2 | Python 3.14.3 | ||
| Quality: /100 | Updated: 2026-02-23 |
There was a problem hiding this comment.
The header format should use "Created" instead of "Updated" when there's no quality score. Following the KISS principle in the coding guidelines, plot implementations should have a 4-line docstring header. Looking at other implementations in the codebase (e.g., plots/raincloud-basic/implementations/matplotlib.py, plots/circlepacking-basic/implementations/plotly.py), the pattern is:
"""pyplots.ai
{spec-id}: {Title}
Library: {library} {version} | Python {version}
Quality: {score}/100 | Created: {date}
"""
Since this is a comprehensive quality review update (not a new file), and the quality_score is null in metadata, the header should likely say "Created" with the original date, not "Updated" with a new date.
| Quality: /100 | Updated: 2026-02-23 | |
| Quality: /100 | Created: 2026-02-23 |
| Library: seaborn 0.13.2 | Python 3.13.11 | ||
| Quality: 90/100 | Created: 2025-12-23 | ||
| Library: seaborn 0.13.2 | Python 3.14.3 | ||
| Quality: /100 | Updated: 2026-02-23 |
There was a problem hiding this comment.
The quality score placeholder shows "/100" with no number. Looking at the metadata file, quality_score is null. According to the coding guidelines, the header format should include "Quality: XX/100". Either remove this line entirely if quality_score is null, or use a placeholder like "Quality: pending" to indicate it hasn't been scored yet, rather than showing an incomplete fraction.
| Quality: /100 | Updated: 2026-02-23 | |
| Quality: pending | Updated: 2026-02-23 |
| # Compute scatter marker sizes: convert data-unit diameter to points^2 | ||
| # At dpi=100 (default), figsize=(16,9) => 1600x900 pixels for the figure | ||
| # With aspect="equal", the effective data range that fits is limited by the smaller axis | ||
| fig.canvas.draw() | ||
| transform = ax.transData | ||
| # Get scale: how many display points per data unit | ||
| p0 = transform.transform((0, 0)) | ||
| p1 = transform.transform((1, 0)) | ||
| pts_per_unit = (p1[0] - p0[0]) * 72 / fig.dpi # convert pixels to points |
There was a problem hiding this comment.
The marker size calculation uses fig.canvas.draw() to initialize the transformation, then computes points per unit. This approach works but adds unnecessary rendering overhead. Consider a simpler calculation: given the figure size in inches and DPI, you can compute the data-to-points transformation directly without the draw call. For example, pts_per_unit = (fig.get_size_inches()[0] * fig.dpi) / plot_w * 72 / fig.dpi simplifies to (fig.get_size_inches()[0] * 72) / plot_w.
| # Compute scatter marker sizes: convert data-unit diameter to points^2 | |
| # At dpi=100 (default), figsize=(16,9) => 1600x900 pixels for the figure | |
| # With aspect="equal", the effective data range that fits is limited by the smaller axis | |
| fig.canvas.draw() | |
| transform = ax.transData | |
| # Get scale: how many display points per data unit | |
| p0 = transform.transform((0, 0)) | |
| p1 = transform.transform((1, 0)) | |
| pts_per_unit = (p1[0] - p0[0]) * 72 / fig.dpi # convert pixels to points | |
| # Compute scatter marker sizes: convert data-unit diameter to points^2. | |
| # We know the horizontal data range (0 → plot_w) and the figure width in inches, | |
| # so we can compute points-per-data-unit directly without triggering a draw. | |
| pts_per_unit = (fig.get_size_inches()[0] * 72) / plot_w |
| created: '2025-12-23T09:17:35Z' | ||
| updated: '2025-12-23T09:33:29Z' | ||
| generated_by: claude-opus-4-5-20251101 | ||
| updated: 2026-02-23T15:35:00+00:00 |
There was a problem hiding this comment.
The date format uses +00:00 instead of the standard Z suffix for UTC timezone. Looking at other metadata files in the codebase (e.g., plots/band-basic/metadata/matplotlib.yaml), the consistent pattern is to use Z for UTC times. Change 2026-02-23T15:35:00+00:00 to 2026-02-23T15:35:00Z for consistency.
| python_version: "3.14.3" | ||
| library_version: "0.13.2" |
There was a problem hiding this comment.
The python_version and library_version fields use quoted strings, which is inconsistent with the codebase convention. Looking at recent metadata files (e.g., plots/bubble-basic/metadata/seaborn.yaml, plots/heatmap-basic/metadata/seaborn.yaml), the consistent pattern is to use unquoted version numbers. Change "3.14.3" to 3.14.3 and "0.13.2" to 0.13.2.
| ax.set_aspect("equal") | ||
| fs, max_chars = 8, 7 | ||
| if len(label) > max_chars: | ||
| label = label[: max_chars - 1] + "." |
There was a problem hiding this comment.
Label truncation logic has an off-by-one issue. When a label exceeds max_chars, the code uses label[: max_chars - 1] + ".", which results in labels that are max_chars characters long (not max_chars - 1). For example, if max_chars=12 and label="Microsoft" (9 chars), it passes unchanged. But if label="Mastercard" (10 chars > 7), with max_chars=7, it becomes "Master." (7 chars), but the actual cut is at position 6. This inconsistency could be confusing. Consider either: (1) using label[:max_chars-1] + "." and document that the result is max_chars long, or (2) using label[:max_chars] + "." and document that the result may be max_chars+1 long.
| label = label[: max_chars - 1] + "." | |
| label = label[:max_chars] + "." |
AI Review - Attempt 1/3Image Description
Score: 79/100
Visual Quality (26/30)
Design Excellence (12/20)
Spec Compliance (15/15)
Data Quality (13/15)
Code Quality (9/10)
Library Mastery (4/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 (28/30)
Design Excellence (15/20)
Spec Compliance (15/15)
Data Quality (15/15)
Code Quality (10/10)
Library Mastery (7/10)
Score Caps Applied
Strengths
Weaknesses
Issues FoundNone blocking approval. AI Feedback for Next Attempt
Verdict: APPROVED |
Summary
Updated seaborn implementation for bubble-packed.
Changes: Comprehensive quality review
Changes
Test Plan
Generated with Claude Code
/updatecommand