update(scatter-basic): seaborn — comprehensive quality review#4200
update(scatter-basic): seaborn — comprehensive quality review#4200github-actions[bot] merged 5 commits intomainfrom
Conversation
Comprehensive quality review: improved data context, white marker edges, subtler grid, explicit font sizing, spec tag additions.
AI Review - Attempt 1/3Image Description
Quality Score: 82/100Criteria ChecklistVisual Quality (26/30)
Design Excellence (10/20)
Spec Compliance (15/15)
Data Quality (14/15)
Code Quality (10/10)
Library Mastery (7/10)
Strengths
Weaknesses
Verdict: REJECTED |
There was a problem hiding this comment.
Pull request overview
This pull request updates the seaborn implementation for the scatter-basic plot with quality improvements including descriptive variable names, DataFrame-based data structure, seaborn theme configuration, white marker edges, and adjusted styling parameters. However, there are several issues that need to be addressed before merging.
Changes:
- Added pandas import and DataFrame-based data structure with descriptive column names
- Introduced sns.set_theme() for global styling configuration
- Changed marker size from 200 to 120 and adjusted grid styling
- Added sns.despine() call for cleaner appearance
- Updated metadata with new timestamps and model version
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| plots/scatter-basic/metadata/seaborn.yaml | Updated metadata with new timestamps, Python 3.14, claude-opus-4-6 model, and null quality_score |
| plots/scatter-basic/implementations/seaborn.py | Refactored implementation with DataFrame structure, theme configuration, and styling adjustments |
| @@ -1,31 +1,43 @@ | |||
| """ pyplots.ai | |||
There was a problem hiding this comment.
The header docstring should start with a triple quote followed by a space (""" pyplots.ai), not without a space ("""pyplots.ai). All other seaborn implementations in the codebase follow the convention of including a space after the opening triple quote, as seen in plots/bar-basic/implementations/seaborn.py:1, plots/line-basic/implementations/seaborn.py:1, and plots/histogram-basic/implementations/seaborn.py:1.
| import pandas as pd | ||
| import seaborn as sns | ||
|
|
||
|
|
||
| # Data - study hours vs exam scores with realistic correlation | ||
| np.random.seed(42) | ||
| study_hours = np.random.uniform(1, 10, 150) | ||
| exam_scores = study_hours * 8 + np.random.randn(150) * 8 + 25 | ||
| df = pd.DataFrame({"Study Hours (per week)": study_hours, "Exam Score (points)": exam_scores}) |
There was a problem hiding this comment.
Adding a new pandas import without using DataFrame-based plotting is not idiomatic for seaborn. While the code creates a DataFrame with descriptive column names, this pattern diverges from simpler seaborn implementations in the codebase. Compare with plots/bar-basic/implementations/seaborn.py which uses DataFrame-based plotting with seaborn's data parameter, or plots/line-basic/implementations/seaborn.py. For a basic scatter plot, using sns.scatterplot with x and y arrays directly (without pandas) would be simpler and more consistent with the KISS principle stated in the coding guidelines.
| y="Exam Score (points)", | ||
| ax=ax, | ||
| alpha=0.7, | ||
| s=120, |
There was a problem hiding this comment.
The marker size changed from s=200 to s=120. For a 4800x2700px canvas (16x9 at 300dpi), this may make the points too small to see clearly, especially with the white edge (linewidth=0.5). The original size of 200 was likely chosen to ensure visibility at high resolution. Consider whether 120 is sufficiently visible for the intended canvas size, or if this reduction was intentional to accommodate more data points.
| s=120, | |
| s=200, |
| ax.set_title("scatter-basic · seaborn · pyplots.ai", fontsize=24, fontweight="medium") | ||
| ax.set_xlabel("Study Hours (per week)", fontsize=20) | ||
| ax.set_ylabel("Exam Score (points)", fontsize=20) |
There was a problem hiding this comment.
The title is set before the axis labels, which is inconsistent with the codebase convention. Most seaborn implementations in the repository (e.g., plots/bar-basic/implementations/seaborn.py:28-31, plots/line-basic/implementations/seaborn.py:27-29) set the title after the axis labels. While functionally equivalent, following the established pattern improves code consistency and readability.
| ax.set_title("scatter-basic · seaborn · pyplots.ai", fontsize=24, fontweight="medium") | |
| ax.set_xlabel("Study Hours (per week)", fontsize=20) | |
| ax.set_ylabel("Exam Score (points)", fontsize=20) | |
| ax.set_xlabel("Study Hours (per week)", fontsize=20) | |
| ax.set_ylabel("Exam Score (points)", fontsize=20) | |
| ax.set_title("scatter-basic · seaborn · pyplots.ai", fontsize=24, fontweight="medium") |
| Library: seaborn 0.13.2 | Python 3.13.11 | ||
| Quality: 92/100 | Created: 2025-12-22 | ||
| Library: seaborn 0.13.2 | Python 3.14 | ||
| Quality: 82/100 | Created: 2025-12-22 |
There was a problem hiding this comment.
The Quality field in the header shows "/100" without a numeric value. Based on the codebase convention (e.g., plots/histogram-basic/implementations/seaborn.py:4 shows "Quality: 95/100"), this should either include the actual quality score (e.g., "Quality: 92/100") or use "Created" instead of "Updated" if this is meant to represent a new implementation. Additionally, the metadata file sets quality_score to null, which is inconsistent with the header claim of having a quality score.
|
|
||
| # Create plot | ||
| # Plot | ||
| sns.set_theme(style="ticks", context="talk", font_scale=1.1) |
There was a problem hiding this comment.
The sns.set_theme() call with context="talk" and font_scale=1.1 is being used, but then explicit fontsize parameters are set for all text elements (title=24, xlabel=20, ylabel=20, tick_params=16). This creates redundancy and makes the theme settings ineffective. Either rely on the theme's font scaling or remove the theme configuration and use explicit sizes throughout. Compare with plots/histogram-basic/implementations/seaborn.py which uses sns.set_theme() with a comprehensive rc dict but still sets explicit fontsizes, suggesting explicit sizes are preferred for the 4800x2700 canvas as mentioned in the coding guidelines.
| sns.set_theme(style="ticks", context="talk", font_scale=1.1) | |
| sns.set_theme(style="ticks") |
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
Quality Score: 96/100Criteria ChecklistVisual Quality (30/30)
Design Excellence (16/20)
Spec Compliance (15/15)
Data Quality (15/15)
Code Quality (10/10)
Library Mastery (10/10)
Strengths
Weaknesses
Verdict: APPROVED |
Summary
Updated LIB_PLACEHOLDER implementation for scatter-basic.
Changes: Comprehensive quality review — improved variable naming, realistic data context, white marker edges for point definition, subtler grid styling, and explicit font sizing for large canvas.
Changes
Test Plan
Generated with Claude Code
/updatecommand