Finish Matplotlib gallery acceptance repairs [mpl compatibility] - #281
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 |
3905133 to
896c655
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
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
Main's Matplotlib-compatibility stack (#280, #281) landed independent work on the same seam, so four files conflicted for real rather than textually. `_svg.py` and `_raster.py` had both grown per-slot chrome styling. Main read `spec["dom"]["styles"]["title"]` inline for the title and axis title, moved chrome text to Matplotlib's weight-400 default, reworked legend geometry around a layout-measured `font_size`, and added legend hatching, per-legend clipping for anchored legends, and `_native_font_emphasis`. This branch had routed a wider set of slots through shared helpers. Neither side wins line-by-line, so each hunk keeps main's geometry and defaults and routes the lookup through the helpers; `_raster.py` was reset to main's copy and the slot work re-applied on top, because hand-merging would have silently damaged the new hatch and clip paths. Three consequences worth naming: - The atlas turns out to carry a bold and an italic face, so `SLOT_RASTER_PROPS` gains `font-weight` and `font-style`. The vector-only set is now `font-family`, `letter-spacing` and `opacity` — no family axis, no per-glyph advance, no alpha on the blit. - Main reads the raw `dom.styles` block, which keeps whatever spelling the caller used, so `styles={"title": {"font_size": 22}}` missed its `font-size` lookup. Routing through `slot_styles` canonicalizes both spellings. - `slot_text_attrs` escaped with `escape`, which leaves `"` intact — a quoted font-family stack closed the attribute and broke the document. It uses `_escape_attr` now, which is the helper main added for exactly this. `legend(loc=)` needed a design decision rather than a merge. Main resolves `top`/`bottom` inside `_legend_layout` and pins the core to pass the caller's spelling through, which fixes the same `loc="top left"` bug this branch fixed by normalizing at validation. Main's design wins: validation now only lowercases and collapses whitespace, checks the vocabulary with aliases folded in for the check alone, and returns the caller's string. The part main does not cover — refusing `northeast`, `middle left`, `zzz` and `""` instead of landing the legend somewhere arbitrary — is kept. `spec/api/styling.md` took both new sections. The before/after sheet is re-rendered against current main: the `loc="top left"` panel is gone, because main now places it correctly on its own, replaced by an unrecognized `loc` that this branch refuses rather than centering. 3078 tests pass. `ty` reports 36 diagnostics, all of them in files byte-identical to origin/main — the mpl-compat stack raised the baseline from 13, and this branch adds none.
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