Finish Matplotlib gallery acceptance repairs [mpl compatibility]#281
Finish Matplotlib gallery acceptance repairs [mpl compatibility]#281sselvakumaran wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds Matplotlib-compatible ChangesFlag colormap support
Plotting compatibility contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9297980 to
1df83ef
Compare
3905133 to
896c655
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
1df83ef to
ba93136
Compare
7ca7b58 to
1d8cad0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1d8cad0 to
cadf5cf
Compare
![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
🧹 Nitpick comments (5)
tests/test_svg_export.py (2)
764-771: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate generated LUT behavior, not only source fragments.
The
flagbranch checks the helper name and two expressions, but not the green expression, 256-entry length, clipping, or numerical output. A JavaScript/Python LUT divergence could therefore pass this test.Suggested additions
+ assert "Math.sin(x * 31.5 * Math.PI)" in js + assert "Array.from({ length: 256 })" in jsPreferably compare the generated 256-byte LUT against the Python values.
🤖 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 `@tests/test_svg_export.py` around lines 764 - 771, Extend the flag-specific assertions in the colormap export test around COLORMAP_STOPS to validate generated LUT behavior: verify the green expression, ensure the LUT has 256 entries and applies clipping, and numerically compare the JavaScript-generated 256-byte LUT with the corresponding Python values. Retain the existing helper-name and source-expression checks.
856-871: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the new resolver contract and more LUT positions.
This test validates
channels.is_colormap()and raw stop reversal, but never exercisesresolve_cmap("flag")orresolve_cmap("flag_r"); a typo in the mapping at Line 95 would go unnoticed. Five sampled entries also will not catch quantization drift elsewhere.Suggested additions
+ assert resolve_cmap("flag") == "flag" + assert resolve_cmap("flag_r") == "flag_r"Also compare the complete 256-entry LUT, or include samples known to distinguish truncation from rounding.
🤖 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 `@tests/test_svg_export.py` around lines 856 - 871, Extend test_flag_colormap_matches_matplotlib_lut_and_gray_aliases to call resolve_cmap for both "flag" and "flag_r", asserting the resolver outputs match the expected 256-entry LUT and its reversal. Replace or supplement the sparse five-index checks with a complete entry-by-entry comparison, or add samples that detect truncation versus rounding, while preserving the existing colormap and alias assertions.tests/pyplot/test_categorical_gallery_regressions.py (1)
80-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining iterable inputs.
This regression only exercises dictionary views for
xandheight; the changed helper also materializes one-shotwidthandbottominputs. Add generator cases and assertions for those fields so those paths cannot regress silently.🤖 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 `@tests/pyplot/test_categorical_gallery_regressions.py` around lines 80 - 90, Extend test_bar_materializes_dictionary_views_before_shape_and_autoscale to pass one-shot generators for bar width and bottom, then assert the recorded entry fields preserve their materialized values and remain correctly aligned with the bars. Cover both changed helper inputs without altering the existing dictionary-view assertions.tests/pyplot/test_best_legend_placement.py (1)
350-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the legend-scoring assertion distinguish the new paths.
The added artists exercise datetime conversion, but the existing expected location may still pass if text/annotation entries are ignored. Place one artist in a candidate corner that changes the result, or compare against a line-only control.
🤖 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 `@tests/pyplot/test_best_legend_placement.py` around lines 350 - 351, Update the test setup around the added ax.text and ax.annotate artists so the legend-scoring assertion produces a different expected placement when those datetime-based artists are considered. Place an artist in a candidate corner that affects scoring, or add a line-only control comparison, ensuring the assertion cannot pass when text and annotation entries are ignored.tests/pyplot/test_p3_option_contracts.py (1)
464-473: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the quantile values, not only their count.
The current assertion would still pass if normalization emitted three incorrect or duplicated coordinates. Verify the expected values as well.
Proposed test adjustment
- assert len(result["cquantiles"]._entry["args"][0]) == 3 + assert list(result["cquantiles"]._entry["args"][1]) == [1.75, 2.5, 3.25]🤖 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 `@tests/pyplot/test_p3_option_contracts.py` around lines 464 - 473, Update test_violinplot_flat_quantiles_are_one_single_violin_group to assert that result["cquantiles"]._entry["args"][0] contains the expected quantile values [0.25, 0.5, 0.75], while retaining the existing count assertion if useful.
🤖 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 `@js/src/10_colormaps.ts`:
- Around line 15-17: Use truncation rather than rounding for 8-bit channel
conversion in both colormap implementations: in js/src/10_colormaps.ts lines
15-17, replace Math.round around the clipped channel values; in
python/xy/_svg.py lines 57-58, remove np.rint before astype(np.uint8). Preserve
clipping and ensure both implementations match Matplotlib’s truncating
bytes=True behavior.
---
Nitpick comments:
In `@tests/pyplot/test_best_legend_placement.py`:
- Around line 350-351: Update the test setup around the added ax.text and
ax.annotate artists so the legend-scoring assertion produces a different
expected placement when those datetime-based artists are considered. Place an
artist in a candidate corner that affects scoring, or add a line-only control
comparison, ensuring the assertion cannot pass when text and annotation entries
are ignored.
In `@tests/pyplot/test_categorical_gallery_regressions.py`:
- Around line 80-90: Extend
test_bar_materializes_dictionary_views_before_shape_and_autoscale to pass
one-shot generators for bar width and bottom, then assert the recorded entry
fields preserve their materialized values and remain correctly aligned with the
bars. Cover both changed helper inputs without altering the existing
dictionary-view assertions.
In `@tests/pyplot/test_p3_option_contracts.py`:
- Around line 464-473: Update
test_violinplot_flat_quantiles_are_one_single_violin_group to assert that
result["cquantiles"]._entry["args"][0] contains the expected quantile values
[0.25, 0.5, 0.75], while retaining the existing count assertion if useful.
In `@tests/test_svg_export.py`:
- Around line 764-771: Extend the flag-specific assertions in the colormap
export test around COLORMAP_STOPS to validate generated LUT behavior: verify the
green expression, ensure the LUT has 256 entries and applies clipping, and
numerically compare the JavaScript-generated 256-byte LUT with the corresponding
Python values. Retain the existing helper-name and source-expression checks.
- Around line 856-871: Extend
test_flag_colormap_matches_matplotlib_lut_and_gray_aliases to call resolve_cmap
for both "flag" and "flag_r", asserting the resolver outputs match the expected
256-entry LUT and its reversal. Replace or supplement the sparse five-index
checks with a complete entry-by-entry comparison, or add samples that detect
truncation versus rounding, while preserving the existing colormap and alias
assertions.
🪄 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 Plus
Run ID: 42f17b8e-80d0-4ff5-b497-0afba42e85bc
⛔ Files ignored due to path filters (2)
pr-assets/matplotlib-gallery-acceptance/pdsh-text-annotation.pngis excluded by!**/*.pngpr-assets/matplotlib-gallery-acceptance/violinplot.pngis excluded by!**/*.png
📒 Files selected for processing (13)
js/src/10_colormaps.tspr-assets/matplotlib-gallery-acceptance/provenance.jsonpython/xy/_svg.pypython/xy/channels.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_colors.pypython/xy/pyplot/_plot_types.pytests/pyplot/test_best_legend_placement.pytests/pyplot/test_categorical_gallery_regressions.pytests/pyplot/test_p3_option_contracts.pytests/test_svg_export.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
96cc716 to
10ade55
Compare
10ade55 to
446b854
Compare
1 participant
Summary
This is the second and final draft in the consolidation stack. It targets #280 and adds the post-integration acceptance repairs for:
flagcolormap across Python and the browser clientloc="best"legend scoringWedge.theta1/theta2metadataExact visual evidence
The commit labels, source identities, dimensions, and SHA-256 hashes are recorded in
pr-assets/matplotlib-gallery-acceptance/provenance.json.Violin gallery execution
Stack A fails this exact gallery script while normalizing a flat single-violin quantile. Stack B executes all 12 panels.
PDSH annotation integration
This is the user-reviewed PDSH text/annotation cell on the final integrated stack. The labels, boxes, axes, legend, and arrows remain readable and within the frame; small placement and stroke differences from Matplotlib remain visible rather than hidden.
Verification
97bfeb6: 81/88 paired examples, up from 77/88 on Stack A and 52/88 on main, with 143 paired figures.9b080851…: 153/154 applicable non-3D cells passed, with 289 figure captures and zero capture errors.FacetGrid.map(xy_plt.axhline)forwardingmarker=".".git diff --checkalso pass.Performance
The exact-head CodSpeed comparison of
446b854against Stack Ac196c12is green: 102 benchmarks unchanged, 2 skipped, and 0 regressions.Known deferred compatibility gaps
Gallery execution
Seven of the 88 examples still fail:
image_antialiasing— display-size-dependentinterpolation="auto".line_demo_dash_control—legend(handlelength=...).fill_spiral— non-simple polygon tessellation.pie_and_donut_labels— annotationzorderafter the Wedge metadata repair.log_demo— minor log grid lines.time_series_histogram— string logarithmic normalization.subplots_adjust— explicitcolorbar(cax=...)placement.Gallery visual acceptance
Execution pairing is not the same as compatibility. Manual review of all 143 pairs flagged 25 figures as technically wrong or unreadable. The recurring areas are:
These are intentionally deferred from this shipping stack rather than hidden by the 81/88 execution number.
PDSH visual acceptance
The 153/154 result proves that the selected xy cells execute and save images. It does not prove notebook parity. The visual audit found:
04_01,04_05,04_06,04_07, and04_10involving axes reuse, image autoscale/data ownership, scatter-size legend samples, and missing plots04_14, where many native Seaborn calls leave stale xy figures that are captured againThose PDSH gaps remain compatibility backlog; the 3D notebook remains explicitly out of scope.
Stack