update(density-basic): plotly — comprehensive quality review#4382
update(density-basic): plotly — comprehensive quality review#4382github-actions[bot] merged 5 commits intomainfrom
Conversation
Comprehensive quality review: proper scipy KDE, SAT scores data, rug fix, refined grid.
There was a problem hiding this comment.
Pull request overview
Updates the Plotly implementation and metadata for the density-basic plot spec to improve realism and simplify KDE generation.
Changes:
- Switched KDE computation to
scipy.stats.gaussian_kdeand updated the synthetic dataset to SAT Math scores. - Adjusted Plotly styling (fill alpha, line width, axis labeling, legend/margins) and updated metadata versions.
- Updated metadata timestamps / generator identifier.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| plots/density-basic/metadata/plotly.yaml | Updates Plotly metadata (versions, timestamps, tags, quality score). |
| plots/density-basic/implementations/plotly.py | Updates Plotly density plot script to use SciPy KDE + SAT score data and refines presentation/export. |
Comments suppressed due to low confidence (1)
plots/density-basic/metadata/plotly.yaml:28
- The review content below still describes a manual KDE implementation and the old test-score dataset, which no longer matches the updated Plotly implementation (now uses
scipy.stats.gaussian_kdeand SAT scores). Please update thereview.strengths/weaknesses(and image description if applicable) to reflect the current code so the metadata remains trustworthy.
- alpha-blending
review:
strengths:
- Excellent manual KDE implementation using Silverman's rule of thumb for bandwidth
selection
| bandwidth = 0.9 * min(std, iqr / 1.34) * n ** (-0.2) | ||
| # KDE using scipy | ||
| kde = gaussian_kde(sat_scores) | ||
| x_grid = np.linspace(350, 800, 500) |
There was a problem hiding this comment.
x_grid starts at 350, but generated SAT scores are clipped to the full 200–800 range. This means the KDE and plotted density omit the 200–350 portion of the distribution, which can hide low-score observations and contradict the “all data visible” expectation. Use a grid that spans the actual data range (e.g., from sat_scores.min()/200 to sat_scores.max()/800, with a small padding if desired).
| x_grid = np.linspace(350, 800, 500) | |
| x_min = max(200, sat_scores.min() - 20) | |
| x_max = min(800, sat_scores.max() + 20) | |
| x_grid = np.linspace(x_min, x_max, 500) |
| # Rug plot | ||
| fig.add_trace( | ||
| go.Scatter( | ||
| x=scores, | ||
| y=[-0.001] * len(scores), | ||
| x=sat_scores, | ||
| y=np.zeros(len(sat_scores)), | ||
| mode="markers", | ||
| marker={"symbol": "line-ns", "size": 12, "color": "#306998", "line": {"width": 1.5}}, | ||
| marker={"symbol": "line-ns", "size": 12, "color": "#306998", "opacity": 0.35, "line": {"width": 1.5}}, | ||
| name="Observations", | ||
| hovertemplate="Score: %{x:.1f}<extra></extra>", | ||
| hovertemplate="Score: %{x:.0f}<extra></extra>", | ||
| ) |
There was a problem hiding this comment.
The rug plot is drawn at y=0 while the density trace is filled tozeroy, so the filled area can visually obscure the rug markers at the baseline. Consider placing the rug slightly below zero (and adjusting the y-axis range accordingly) or using a secondary y-axis / different layering so the observations remain visible.
| preview_thumb: https://storage.googleapis.com/pyplots-images/plots/density-basic/plotly/plot_thumb.png | ||
| preview_html: https://storage.googleapis.com/pyplots-images/plots/density-basic/plotly/plot.html | ||
| quality_score: 92 | ||
| quality_score: null |
There was a problem hiding this comment.
quality_score is set to null, but this metadata file appears to require a numeric score (other density-basic metadata files use an integer). Please set it to the evaluated score so downstream tooling doesn’t treat the implementation as unrated.
| quality_score: null | |
| quality_score: 92 |
| impl_tags: | ||
| dependencies: [] | ||
| techniques: | ||
| - hover-tooltips | ||
| - html-export | ||
| - hover-tooltips | ||
| - html-export | ||
| patterns: | ||
| - data-generation | ||
| - data-generation | ||
| dataprep: | ||
| - kde | ||
| - kde | ||
| styling: | ||
| - alpha-blending | ||
| - alpha-blending |
There was a problem hiding this comment.
The impl_tags YAML structure is invalid: list items (- hover-tooltips, etc.) are not indented under their keys (e.g., techniques:), which will break YAML parsing (you can’t mix a mapping and sequence at the same indentation). Indent the lists under each key (or use techniques: [] style) to match the structure used in other metadata files.
| quality_score: 92 | ||
| quality_score: null | ||
| impl_tags: | ||
| dependencies: [] |
There was a problem hiding this comment.
The impl_tags.dependencies list is empty, but the implementation imports scipy.stats.gaussian_kde. Please add scipy as a dependency in metadata (consistent with other density-basic metadata files) so environments can install the required package.
| dependencies: [] | |
| dependencies: | |
| - scipy |
| Library: plotly 6.5.0 | Python 3.13.11 | ||
| Quality: 92/100 | Created: 2025-12-23 | ||
| Library: plotly 6.5.2 | Python 3.14 | ||
| Quality: /100 | Updated: 2026-02-23 |
There was a problem hiding this comment.
Header docstring is missing the numeric quality score (Quality: /100). This breaks the standard 4-line header format used across plot implementations and makes it harder to track quality/regressions. Please fill in the quality score (e.g., Quality: 89/100 | Updated: 2026-02-23).
| Quality: /100 | Updated: 2026-02-23 | |
| Quality: 92/100 | Updated: 2026-02-23 |
AI Review - Attempt 1/3Image Description
Score: 87/100
Visual Quality (29/30)
Design Excellence (12/20)
Spec Compliance (15/15)
Data Quality (15/15)
Code Quality (10/10)
Library Mastery (6/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: 93/100
Visual Quality (30/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 significant — implementation meets quality standards. AI Feedback for Next Attempt
Verdict: APPROVED |
Summary
Updated plotly implementation for density-basic.
Changes: Comprehensive quality review
Changes
Test Plan
Generated with Claude Code
/updatecommand