Consolidate Matplotlib gallery rendering repairs [mpl compatibility] - #280
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR expands Matplotlib compatibility across axis margins, autoscaling, legends, colorbars, contouring, vector fields, text, layout, SVG/PNG/WebGL rendering, native kernels, colormaps, and regression coverage. ChangesCompatibility and rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9297980 to
1df83ef
Compare
Merging this PR will improve performance by 65.07%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_png_export_line_pyplot |
76.1 ms | 33.1 ms | ×2.3 |
| ⚡ | test_first_payload_errorbar_large |
612.9 ms | 516.2 ms | +18.74% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing agent/gallery-compat-stack-a (cc4bb19) with main (7604775)
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
1df83ef to
ba93136
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/xy/components.py (1)
237-248: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
anchorinserted mid-order breaksLegend's documented positional contract.Lines 245-246 state new fields must append after
renderbecause positional construction over the released field order must keep binding. Insertinganchorat position 3 silently rebinds existingLegend(True, "upper right", 2, "Title")calls (2now lands onanchorand trips the new validation or, worse,ncolsfalls to the default).🔧 Move the new field to the documented append point
class Legend(Component): """Legend chrome; ``render`` remains opaque for Reflex adapters.""" show: bool = True loc: Optional[str] = None - anchor: Optional[tuple[float, ...]] = None ncols: int = 1 title: Optional[str] = None class_name: Optional[str] = None style: dict[str, StyleValue] = field(default_factory=dict) render: Any = None # New fields append after ``render``: Legend is public and positional # construction over the released field order must keep binding. highlight: bool = True toggle: bool = True + anchor: Optional[tuple[float, ...]] = NoneNote the same ordering question applies to
Axis.marginat Line 218, which is inserted betweendomainandbounds.🤖 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 `@python/xy/components.py` around lines 237 - 248, Preserve released positional field ordering in the dataclass definitions: move the new Legend.anchor field to append after render, and likewise move Axis.margin after all existing released fields. Keep existing fields such as ncols, title, domain, and bounds in their original positions so positional construction continues binding unchanged.spec/api/styling.md (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMismatched section name between the two files.
spec/api/styling.mdintroduces the heading "Measured left gutter and the rotated y-axis title", butspec/matplotlib/compat.mdpoints to it by a different name, "Chrome gutters and the rotated y-axis title". Pick one name and use it in both places.
spec/api/styling.md#L206-206: keep this heading as the canonical name (or rename it to match compat.md's phrasing, whichever is intended going forward).spec/matplotlib/compat.md#L67-70: update the cross-reference text to say "Measured left gutter and the rotated y-axis title" so it actually resolves to the section it points to.🤖 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 `@spec/api/styling.md` at line 1, Align the cross-reference in the matplotlib compatibility documentation with the canonical heading in the styling specification. Keep “Measured left gutter and the rotated y-axis title” as the section name in spec/api/styling.md and update the corresponding reference text in spec/matplotlib/compat.md to use that exact wording.Source: Coding guidelines
🧹 Nitpick comments (15)
python/xy/marks.py (2)
2316-2327: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
np.isscalarmisses 0-d arrays and NumPy-scalar-like inputs.
np.isscalar(np.array(1.1))isFalse, so a 0-d width array falls into_as_1d_floatand fails with "contour width must be 1-D".np.ndim(width) == 0covers both cases.♻️ Proposed tweak
- if np.isscalar(width): + if np.ndim(width) == 0: width = self._positive_scalar(width, "contour width") width_values = None🤖 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 `@python/xy/marks.py` around lines 2316 - 2327, Update the width branching logic around the scalar validation to use np.ndim(width) == 0 instead of np.isscalar(width), ensuring Python scalars, NumPy scalars, and 0-D arrays use _positive_scalar while preserving the existing 1-D array validation path.
645-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNon-numeric
widthescapes with a raw NumPy message.
np.asarray(width, dtype=np.float64)on e.g.width="wide"raises NumPy's ownValueErrorwith no{kind} widthlabel, unlike every other validator in this function.♻️ Label the conversion failure
- width_array = np.asarray(width, dtype=np.float64) + try: + width_array = np.asarray(width, dtype=np.float64) + except (TypeError, ValueError) as exc: + raise ValueError(f"{kind} width must be scalar or contain numeric values") from exc🤖 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 `@python/xy/marks.py` around lines 645 - 661, Update the width conversion logic in the validation block to catch failures from np.asarray(width, dtype=np.float64) and re-raise a ValueError labeled with “{kind} width” and the existing scalar-or-numeric-values message, suppressing the raw NumPy exception.python/xy/_figure.py (1)
1047-1069: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse of
marginfor two meanings makes the log branch easy to break.
marginis overwritten with the0.03default at Line 1060, so Lines 1061 and 1075 must re-readopts.get("margin")to recover "was it configured?". A separate name makes the intent explicit and prevents a future edit from collapsing the two checks.♻️ Suggested clarification
- margin = opts.get("margin") - if lo == hi and margin is None: + configured_margin = opts.get("margin") + if lo == hi and configured_margin is None: @@ - if margin is None: - margin = 0.03 - if scale == "log" and opts.get("margin") is not None: + margin = 0.03 if configured_margin is None else configured_margin + if scale == "log" and configured_margin is not None: @@ - if scale == "log" and opts.get("margin") is None: + if scale == "log" and configured_margin is None:🤖 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 `@python/xy/_figure.py` around lines 1047 - 1069, In the extent calculation around the margin handling, preserve whether the caller explicitly configured a margin in a separate variable before assigning the default value. Update the logarithmic branch and any corresponding configured-margin checks to use that explicitness variable, while continuing to use the resolved numeric margin for padding calculations.js/src/50_chartview.ts (1)
5130-5139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fontRoomhere is absolute px; the Python twin takes a font-size multiple.
_svg._axis_tick_label_offset(axis, unstyled, font_room)returnspad + font_size * font_room(called with0.2/0.8), while this returnspad + fontRoom(called withsize * 1.2). The values are correct for their respective anchors (DOM top edge vs baseline), but the comment claims the helpers mirror each other, so a future edit is likely to copy one signature into the other. Naming the argument (e.g.fontRoomPx) would pin the difference down.🤖 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 `@js/src/50_chartview.ts` around lines 5130 - 5139, Rename the absolute-pixel fontRoom parameter in tickLabelOffset to fontRoomPx, preserving the existing pad + fontRoomPx calculation and call behavior. Keep this JavaScript helper’s pixel-based contract distinct from the Python font-size-multiple API.python/xy/styles.py (1)
160-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_signed_pxis_pxminus the sign check — consider one parser.The parse/error-message path is duplicated verbatim; a
signed=Trueflag on_pxkeeps the two in step if the px grammar ever changes.♻️ Fold into `_px`
-def _px(value: StyleValue, label: str, *, positive: bool = False) -> float: +def _px(value: StyleValue, label: str, *, positive: bool = False, signed: bool = False) -> float: if isinstance(value, (int, float)) and not isinstance(value, bool): number = float(value) elif isinstance(value, str): match = _PX_RE.match(value) if match is None: raise ValueError(f"{label} must be a finite CSS px length") number = float(match.group(1)) else: # pragma: no cover - normalize_css_style rejects this first raise ValueError(f"{label} must be a finite CSS px length") + if signed: + if not np.isfinite(number): + raise ValueError(f"{label} must be a finite CSS px length") + return number if not np.isfinite(number) or number < 0 or (positive and number <= 0):🤖 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 `@python/xy/styles.py` around lines 160 - 172, Refactor _signed_px to reuse the existing _px parser by adding a signed=True option to _px and routing signed values through it. Preserve _signed_px’s acceptance of negative lengths while keeping the shared px grammar, finite-number validation, and error-message behavior in one implementation.python/xy/_svg.py (3)
2410-2436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMeasure the bbox with
_fontmetrics.advanceinstead of the em table.The docstring's premise ("neither Python emitter has a browser text measurer") no longer holds —
_legend_text_widthin this same module measures real DejaVu advances from_fontmetrics, which is also the face the rasterizer blits. Reusing it here would maketext(bbox=)boxes fit exactly instead of approximately, and drop the second width model.🤖 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 `@python/xy/_svg.py` around lines 2410 - 2436, Update _estimated_text_width to measure each line using the existing _fontmetrics.advance path, matching _legend_text_width and the rasterizer’s DejaVu face. Remove the hand-written glyph advance table and revise the docstring to reflect the real font-metrics measurement, while preserving the maximum-line-width and empty-input behavior.
2320-2340: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSort and clamp the parsed math ranges before slicing.
rangesis consumed in wire order; an unsorted or overlapping pair makescursorrun past the nextstart, andline[start:end]then re-emits characters already written — duplicated glyphs in the exported label. Producers are internal today, but the guard is two lines.🛡️ Normalize the ranges
if not ranges: return escape(line) + ranges.sort() out: list[str] = [] cursor = 0 for start, end in ranges: + start = max(start, cursor) + if start >= end: + continue if cursor < start:🤖 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 `@python/xy/_svg.py` around lines 2320 - 2340, Normalize the parsed ranges in _svg_mathtext_spans before rendering: sort them by start position and clamp each range against the current cursor so overlapping or out-of-order ranges cannot re-emit already written characters. Preserve the existing escaping and italic tspan output for non-overlapping ranges.
1272-1302: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_y_tick_label_roomruns twice per axis here.
_y_title_baseline(Line 1241) measures the tick labels internally, and Line 1291 measures them again — every label goes through_fontmetrics.advancetwice on the sharedlayout()path used by both SVG and PNG export. A small "has an outside title" predicate would avoid the duplicate pass.🤖 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 `@python/xy/_svg.py` around lines 1272 - 1302, The _y_tick_label_room calculation is duplicated in _y_axis_left_room because _y_title_baseline already measures tick labels. Add a lightweight outside-title predicate or equivalent result reuse, then only call _y_tick_label_room once per axis while preserving the existing gutter calculations for titled and untitled left y axes.python/xy/pyplot/_axes.py (2)
1823-1852: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
n_barsis computed twice.Line 1844 recomputes the value already bound at line 1823;
valsis not rebound in between.♻️ Drop the duplicate
check_unsupported(kwargs, "bar()/barh()") - n_bars = int(np.asarray(vals).size) patch_labels: Optional[list[str]] = None🤖 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 `@python/xy/pyplot/_axes.py` around lines 1823 - 1852, Remove the redundant second assignment to n_bars in the bar-label handling flow; reuse the value computed before thickness validation, leaving the existing label-length validation unchanged.
165-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant flag assignment in the vertical-boundary branch.
hitis alreadyTruewhen that block runs; the innerif hit:after theadyloop only needs to break.♻️ Simplify the break chain
if ady: for boundary in (y_lo, y_hi): ratio = (boundary - ay0) / ady if 0.0 <= ratio <= 1.0 and x_lo <= ax0 + ratio * adx <= x_hi: hit = True break - if hit: - hit = True - break + if hit: + break🤖 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 `@python/xy/pyplot/_axes.py` around lines 165 - 176, In the vertical-boundary handling within the surrounding intersection loop, remove the redundant `hit = True` assignment inside the `if hit:` block after iterating over `(y_lo, y_hi)`. Keep the conditional break so the loop exits once the boundary intersection has set `hit`.tests/test_ui_issue_regressions.py (1)
361-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the
0.4 × label_sizegutter factor. The same magic ratio is asserted in two tests without stating where it comes from (the renderer's title-to-tick pad rule); a module constant plus a one-line comment makes a future drift diagnosable.Also applies to: 402-402
🤖 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_ui_issue_regressions.py` at line 361, Name the shared 0.4 × label_size gutter factor used by the assertions near result["gap"] in both tests. Define a module-level constant representing the renderer’s title-to-tick pad rule, add a concise comment identifying its source, and replace both inline 0.4 multipliers with that constant.python/xy/pyplot/__init__.py (1)
303-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new
layoutkeyword in the docstring.
subplots()now consumeslayout, but the parameter list still routes readers to "remaining keywords are forwarded tofigure", which is no longer true forlayout. Same forsubplot_mosaic()'s docstring at Line 342.📝 Suggested docstring addition
subplot_kw : dict, optional Properties applied to every created axes via ``Axes.set``. + layout : {"none", "tight", "constrained", "compressed"}, optional + Layout engine applied after the grid exists; the rounded/ + constrained engines map onto the shim's tight-layout pass. **kwargs🤖 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 `@python/xy/pyplot/__init__.py` around lines 303 - 328, Update the docstrings for both subplots() and subplot_mosaic() to document the consumed layout keyword explicitly, including its accepted behavior and effect on figure layout. Ensure the remaining-keywords description no longer implies that layout is forwarded to figure().tests/test_text_weight_defaults.py (1)
78-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_raster_text_boldonly decodes styled records with zero ranges. A styled record carrying mathtext ranges (ranges > 0) falls through to the plain-opcode assertion and fails with a misleading "neither a plain nor a styled text record". Deriving the flags offset from the range count keeps the helper honest as soon as a styled label is added.♻️ Suggested offset derivation
- styled = body - _STYLED_HEADER - if styled >= 0 and stream[styled] == _raster._STYLED_TEXT: - (ranges,) = struct.unpack("<I", stream[body - 12 : body - 8]) - if ranges == 0: - return bool(stream[body - 13] & _raster._TEXT_BOLD) + for ranges in range(0, 8): + styled = body - _STYLED_HEADER - 8 * ranges + if styled < 0 or stream[styled] != _raster._STYLED_TEXT: + continue + (declared,) = struct.unpack("<I", stream[body - 12 - 8 * ranges : body - 8 - 8 * ranges]) + if declared == ranges: + return bool(stream[body - 13 - 8 * ranges] & _raster._TEXT_BOLD)🤖 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_text_weight_defaults.py` around lines 78 - 93, Update _raster_text_bold to recognize styled records regardless of their range count. Derive the emphasis-flags offset from the encoded ranges value and inspect _TEXT_BOLD there, while retaining the plain-opcode fallback only for genuinely plain records.tests/pyplot/test_layout_text_parity_fixes.py (1)
248-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment describes a title that neither figure has. Both figures here differ only by
set_ylabel; rewording ("the same axes without the y-label") would keep the intent readable.🤖 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_layout_text_parity_fixes.py` around lines 248 - 259, Update the comments in the test around the left_band and top_band assertions to describe the figures as differing by set_ylabel, not by a title; refer to the comparison as the same axes without the y-label while preserving the existing assertion logic.tests/test_svg_export.py (1)
898-903: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_tick_label_positionshelper across two new test files. Both files independently define the same SVG tick-label-parsing regex/helper; the shared root cause is the lack of a common test utility for this.
tests/test_svg_export.py#L898-L903: keep this as the canonical implementation (or move it into a shared test helper module such astests/conftest.py).tests/pyplot/test_axes_layout.py#L204-L210: drop the local duplicate and import the shared helper instead (adapting thechart.to_svg()call at the call site if the shared version takes a raw svg string).🤖 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 898 - 903, Centralize the duplicate _tick_label_positions helper by keeping tests/test_svg_export.py:898-903 as the canonical implementation, or moving it to a shared test utility. In tests/pyplot/test_axes_layout.py:204-210, remove the local definition and import the shared helper, adapting the chart.to_svg() call if the helper accepts raw SVG text.
🤖 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/50_chartview.ts`:
- Around line 5242-5281: Update the y-axis label flow around _axisLabelCss and
attachYTitleToTicks so tick attachment runs only for fallback-CSS placements,
not object label_position placements that provide raw positioning. Preserve the
author’s structured placement unchanged. In attachYTitleToTicks, eliminate the
second post-write getBoundingClientRect layout read by calculating the canvas
correction from the already captured title geometry and intended position delta.
In `@python/xy/_figure.py`:
- Around line 1087-1092: Update the trace-kind allowlist in the loop over
self.traces within the sticky-zero handling method to include "column" alongside
the existing rectangle kinds. Verify the exact kind values emitted by
marks.column via _bar_like and by histogram creation, preserving the exclusion
of segment-based marks such as stem and errorbar.
In `@python/xy/_paint.py`:
- Around line 27-51: Reject triangles containing non-finite coordinates before
projection or serialization in both SVG and raster joined-fill callers. Update
the boundary-reconstruction helper containing vertex_key to return None when any
triangle coordinate is non-finite, and ensure callers skip those triangles so
NaN or infinity never reaches vertex buffers.
In `@python/xy/components.py`:
- Line 1265: Update the width annotations and documentation for the column and
violin components to match their validation behavior: describe column width as
accepting array-like values via _bar_like, while restrict violin width from Any
to the scalar type accepted by self._positive_scalar(...).
In `@python/xy/pyplot/_artists.py`:
- Around line 1142-1149: Update the extended-band label branches in the
formatter loop around lowers, uppers, and n_bands to remove the literal trailing
“s” after lower_text and upper_text, while preserving the existing interior-band
formatting and boundary conditions.
- Around line 520-534: Update the branching in PathCollection.legend_elements so
num=None follows the existing all-unique-values path alongside num=="auto" when
the unique-value condition applies, assigning chosen=unique and
label_values=transformed without converting None through np.asarray.
In `@python/xy/pyplot/_axes.py`:
- Around line 2602-2613: Bound non-nearest resampling targets in the call site
near the image-grid preparation and the corresponding path around the later
image handling block, keeping dimensions between display resolution and a fixed
upper limit without exceeding the source size. Update _resample_grid and
_interpolation_weights to gather only each kernel’s in-support taps (or use the
existing separable interpolation path) instead of constructing dense
target-by-source weight matrices, while preserving interpolation results and
non-finite handling.
- Line 2054: Update the label handling in the per-dataset plotting flow to pass
sequence labels through the existing dataset_values() validation, while
preserving scalar labels as repeated values for len(datasets). Ensure mismatched
label sequences raise the same named length error as other per-dataset options
instead of reaching the loop and causing IndexError.
In `@python/xy/pyplot/_grid.py`:
- Around line 437-441: Update the suptitle compositing block around
kernels.rasterize to blend and write the canvas alpha channel in addition to
RGB. Preserve the existing alpha-weighted overlay compositing, and ensure the
change applies to both absolute and grid rendering paths so transparent figures
retain visible suptitles.
In `@python/xy/pyplot/_mathtext.py`:
- Around line 216-225: The italic-range scan in _convert_math must use
source-span metadata rather than classifying alphabetic tokens from the
flattened rendered text. Preserve each source token’s provenance through
replacements, ensuring commands such as \sum, \prod, \Sigma, and \Gamma are
excluded from italic ranges while ordinary source words retain the existing
_UPRIGHT_WORDS behavior.
In `@python/xy/pyplot/_ticker.py`:
- Around line 180-192: Update tick_values to apply the axes.autolimit_mode
round-number behavior when calling _raw_ticks, ensuring displayed ticks use the
same widened step as view_limits. In view_limits, mirror the upstream
nonsingular validation for the returned tick pair rather than relying only on
len(ticks) < 2, while preserving the existing non-round-number path.
In `@spec/api/styling.md`:
- Line 206: Update the section heading in spec/api/styling.md to exactly match
the cross-reference name in compat.md: “Chrome gutters and the rotated y-axis
title.”
- Around line 211-216: Add the `text` language tag to the fenced code block
containing the left-inset layout formula in the styling documentation,
preserving its existing content and formatting.
In `@spec/matplotlib/compat.md`:
- Around line 67-70: Update the cross-reference in the
xlabel/ylabel/title/suptitle compatibility entry to use the actual “Measured
left gutter and the rotated y-axis title” heading from spec/api/styling.md,
preserving the existing link target and surrounding documentation.
In `@spec/matplotlib/shim-todo.md`:
- Around line 361-370: Remove the obsolete “Barbs glyph” and “Streamplot uses a
fixed-step integrator” bullets from the “Accepted approximations (documented,
still divergent)” section, leaving the updated checklist entries as the
authoritative status. Do not alter unrelated approximation entries.
In `@src/raster.rs`:
- Around line 824-832: Update symbol_extent to apply the r * SQRT_2 extent to
all marker symbols whose geometry reaches diagonal corners: square (1), snapped
pixel (13), diamond (2, 14), and triangle variants (3, 8, 9, 10). Preserve the
existing r extent for other symbols so every caller receives sufficient bounds
without changing unrelated marker sizing.
- Around line 2170-2196: Bound the wire-driven allocation for italic_ranges in
the OP_STYLED_TEXT reader using the existing Reader::bounded_capacity safeguard
with the appropriate element size before reading ranges. Preserve the existing
parsing behavior while ensuring malformed short commands cannot trigger an
unbounded allocation; add a regression case to
malformed_buffer_is_rejected_not_panicked for a huge range_count with
insufficient range data.
In `@tests/pyplot/test_p3_option_contracts.py`:
- Around line 492-512: Update the contour artist’s set_linewidth path to apply
the same internal pixel-scale conversion used during contour construction, while
preserving array cycling and subsequent artist updates. Ensure get_linewidth and
generated contour trace width values use consistent units regardless of whether
linewidths were supplied at construction or through set_linewidth.
In `@tests/pyplot/test_pdsh_gap_features.py`:
- Around line 326-337: Update the record_text spy around _raster._Cmd.text to
accept arbitrary positional and keyword arguments, record the value without
rejecting styled calls, and forward all arguments to original_text. Replace the
manual assignment and try/finally restoration with monkeypatch.setattr for
automatic cleanup.
---
Outside diff comments:
In `@python/xy/components.py`:
- Around line 237-248: Preserve released positional field ordering in the
dataclass definitions: move the new Legend.anchor field to append after render,
and likewise move Axis.margin after all existing released fields. Keep existing
fields such as ncols, title, domain, and bounds in their original positions so
positional construction continues binding unchanged.
In `@spec/api/styling.md`:
- Line 1: Align the cross-reference in the matplotlib compatibility
documentation with the canonical heading in the styling specification. Keep
“Measured left gutter and the rotated y-axis title” as the section name in
spec/api/styling.md and update the corresponding reference text in
spec/matplotlib/compat.md to use that exact wording.
---
Nitpick comments:
In `@js/src/50_chartview.ts`:
- Around line 5130-5139: Rename the absolute-pixel fontRoom parameter in
tickLabelOffset to fontRoomPx, preserving the existing pad + fontRoomPx
calculation and call behavior. Keep this JavaScript helper’s pixel-based
contract distinct from the Python font-size-multiple API.
In `@python/xy/_figure.py`:
- Around line 1047-1069: In the extent calculation around the margin handling,
preserve whether the caller explicitly configured a margin in a separate
variable before assigning the default value. Update the logarithmic branch and
any corresponding configured-margin checks to use that explicitness variable,
while continuing to use the resolved numeric margin for padding calculations.
In `@python/xy/_svg.py`:
- Around line 2410-2436: Update _estimated_text_width to measure each line using
the existing _fontmetrics.advance path, matching _legend_text_width and the
rasterizer’s DejaVu face. Remove the hand-written glyph advance table and revise
the docstring to reflect the real font-metrics measurement, while preserving the
maximum-line-width and empty-input behavior.
- Around line 2320-2340: Normalize the parsed ranges in _svg_mathtext_spans
before rendering: sort them by start position and clamp each range against the
current cursor so overlapping or out-of-order ranges cannot re-emit already
written characters. Preserve the existing escaping and italic tspan output for
non-overlapping ranges.
- Around line 1272-1302: The _y_tick_label_room calculation is duplicated in
_y_axis_left_room because _y_title_baseline already measures tick labels. Add a
lightweight outside-title predicate or equivalent result reuse, then only call
_y_tick_label_room once per axis while preserving the existing gutter
calculations for titled and untitled left y axes.
In `@python/xy/marks.py`:
- Around line 2316-2327: Update the width branching logic around the scalar
validation to use np.ndim(width) == 0 instead of np.isscalar(width), ensuring
Python scalars, NumPy scalars, and 0-D arrays use _positive_scalar while
preserving the existing 1-D array validation path.
- Around line 645-661: Update the width conversion logic in the validation block
to catch failures from np.asarray(width, dtype=np.float64) and re-raise a
ValueError labeled with “{kind} width” and the existing scalar-or-numeric-values
message, suppressing the raw NumPy exception.
In `@python/xy/pyplot/__init__.py`:
- Around line 303-328: Update the docstrings for both subplots() and
subplot_mosaic() to document the consumed layout keyword explicitly, including
its accepted behavior and effect on figure layout. Ensure the remaining-keywords
description no longer implies that layout is forwarded to figure().
In `@python/xy/pyplot/_axes.py`:
- Around line 1823-1852: Remove the redundant second assignment to n_bars in the
bar-label handling flow; reuse the value computed before thickness validation,
leaving the existing label-length validation unchanged.
- Around line 165-176: In the vertical-boundary handling within the surrounding
intersection loop, remove the redundant `hit = True` assignment inside the `if
hit:` block after iterating over `(y_lo, y_hi)`. Keep the conditional break so
the loop exits once the boundary intersection has set `hit`.
In `@python/xy/styles.py`:
- Around line 160-172: Refactor _signed_px to reuse the existing _px parser by
adding a signed=True option to _px and routing signed values through it.
Preserve _signed_px’s acceptance of negative lengths while keeping the shared px
grammar, finite-number validation, and error-message behavior in one
implementation.
In `@tests/pyplot/test_layout_text_parity_fixes.py`:
- Around line 248-259: Update the comments in the test around the left_band and
top_band assertions to describe the figures as differing by set_ylabel, not by a
title; refer to the comparison as the same axes without the y-label while
preserving the existing assertion logic.
In `@tests/test_svg_export.py`:
- Around line 898-903: Centralize the duplicate _tick_label_positions helper by
keeping tests/test_svg_export.py:898-903 as the canonical implementation, or
moving it to a shared test utility. In tests/pyplot/test_axes_layout.py:204-210,
remove the local definition and import the shared helper, adapting the
chart.to_svg() call if the helper accepts raw SVG text.
In `@tests/test_text_weight_defaults.py`:
- Around line 78-93: Update _raster_text_bold to recognize styled records
regardless of their range count. Derive the emphasis-flags offset from the
encoded ranges value and inspect _TEXT_BOLD there, while retaining the
plain-opcode fallback only for genuinely plain records.
In `@tests/test_ui_issue_regressions.py`:
- Line 361: Name the shared 0.4 × label_size gutter factor used by the
assertions near result["gap"] in both tests. Define a module-level constant
representing the renderer’s title-to-tick pad rule, add a concise comment
identifying its source, and replace both inline 0.4 multipliers with that
constant.
🪄 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: 44a6f24d-11b7-4416-913c-f69a298bc613
⛔ Files ignored due to path filters (3)
pr-assets/matplotlib-gallery-consolidation/annotation-basic.pngis excluded by!**/*.pngpr-assets/matplotlib-gallery-consolidation/bar-label-demo.pngis excluded by!**/*.pngpr-assets/matplotlib-gallery-consolidation/horizontal-barchart-legend.pngis excluded by!**/*.png
📒 Files selected for processing (84)
benchmarks/test_codspeed_pyplot.pyjs/src/10_colormaps.tsjs/src/20_theme.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/51_annotations.tspr-assets/matplotlib-gallery-consolidation/provenance.jsonpython/xy/_figure.pypython/xy/_fontmetrics.pypython/xy/_native.pypython/xy/_paint.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/channels.pypython/xy/components.pypython/xy/marks.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_colors.pypython/xy/pyplot/_grid.pypython/xy/pyplot/_mathtext.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pypython/xy/pyplot/_rc.pypython/xy/pyplot/_ticker.pypython/xy/pyplot/_translate.pypython/xy/styles.pyscripts/gen_font.pyspec/api/styling.mdspec/design/pan-and-zoom-configuration.mdspec/design/rust-engine.mdspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdspec/matplotlib/shim-todo.mdsrc/font.rssrc/kernels.rssrc/lib.rssrc/raster.rstests/pyplot/test_advanced_compatibility.pytests/pyplot/test_annotation_box_text_fidelity.pytests/pyplot/test_autoscale_margin_bar_regressions.pytests/pyplot/test_axes_charts.pytests/pyplot/test_axes_layout.pytests/pyplot/test_axis_tick_gallery_compat.pytests/pyplot/test_best_legend_placement.pytests/pyplot/test_categorical_gallery_regressions.pytests/pyplot/test_color_pipeline_fixes.pytests/pyplot/test_contour_gallery_colors.pytests/pyplot/test_frame_geometry.pytests/pyplot/test_gallery_auto_ticks_compat.pytests/pyplot/test_gallery_canvas_gutters.pytests/pyplot/test_gallery_colorbar_options.pytests/pyplot/test_gallery_hist_errorbar_compat.pytests/pyplot/test_gallery_layout_api_regressions.pytests/pyplot/test_gallery_statistics_semantics.pytests/pyplot/test_gallery_stem_regressions.pytests/pyplot/test_gallery_text_pie_compat.pytests/pyplot/test_grid_legend_contracts.pytests/pyplot/test_launch_compat.pytests/pyplot/test_layout_text_parity_fixes.pytests/pyplot/test_line_gallery_semantics.pytests/pyplot/test_line_legend_gallery_compat.pytests/pyplot/test_marker_fidelity.pytests/pyplot/test_matplotlib_mismatch_regressions.pytests/pyplot/test_mesh_autoscale_regressions.pytests/pyplot/test_p3_option_contracts.pytests/pyplot/test_pdsh_gap_features.pytests/pyplot/test_rc_chrome_contracts.pytests/pyplot/test_rc_color_export_contracts.pytests/pyplot/test_reference_semantics.pytests/pyplot/test_silent_drop_regressions.pytests/pyplot/test_state_and_figs.pytests/pyplot/test_visible_style_contracts.pytests/test_axis_title_gutter.pytests/test_components.pytests/test_css_mark_styles.pytests/test_kernels.pytests/test_legend_resize_regression.pytests/test_png_export.pytests/test_static_client_security.pytests/test_svg_export.pytests/test_text_weight_defaults.pytests/test_ui_issue_regressions.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/xy/_raster.py (2)
2247-2259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender line-only scatter symbols with a visible stroke.
plus_lineandx_linedefault tostroke_width=0, so this emits a transparent stroke. The SVG exporter forces a 1px stroke and uses the resolved marker color for these symbols; mirror that behavior here.Proposed fix
if kind == "scatter": sym = _SYMBOLS.get(style.get("symbol", "circle"), 0) sw = float(style.get("stroke_width", 0.0)) - stroke = _rgba(style.get("stroke"), color_str) if sw > 0 else (0, 0, 0, 0) + line_symbol = style.get("symbol") in {"plus_line", "x_line"} + if line_symbol and sw <= 0: + sw = 1.0 + stroke = _rgba(style.get("stroke"), color_str) if sw > 0 else (0, 0, 0, 0)🤖 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 `@python/xy/_raster.py` around lines 2247 - 2259, Update the scatter-symbol rendering in the kind == "scatter" branch to treat plus_line and x_line as line-only markers: resolve a 1px stroke using the marker color when stroke_width is unset or zero, matching the SVG exporter, while preserving explicit nonzero stroke settings and existing behavior for other symbols.
2222-2226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRespect rounded legend frames in raster export.
When
borderRadiusis enabled, SVG emits a rounded legend<rect>, but raster always fills/strokes_rect_pts. PNG output therefore remains square. Use_round_rect_pts(...)for the frame path (and shadow, if present).🤖 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 `@python/xy/_raster.py` around lines 2222 - 2226, Update the legend frame drawing around _rect_pts so borderRadius uses _round_rect_pts(...) for the frame path, matching SVG’s rounded rectangle output. Apply the same rounded path to the shadow drawing when present, while preserving the existing square path behavior when borderRadius is disabled.
🤖 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.
Outside diff comments:
In `@python/xy/_raster.py`:
- Around line 2247-2259: Update the scatter-symbol rendering in the kind ==
"scatter" branch to treat plus_line and x_line as line-only markers: resolve a
1px stroke using the marker color when stroke_width is unset or zero, matching
the SVG exporter, while preserving explicit nonzero stroke settings and existing
behavior for other symbols.
- Around line 2222-2226: Update the legend frame drawing around _rect_pts so
borderRadius uses _round_rect_pts(...) for the frame path, matching SVG’s
rounded rectangle output. Apply the same rounded path to the shadow drawing when
present, while preserving the existing square path behavior when borderRadius is
disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af93e3bf-4a88-4fd2-b747-7f14507d666c
📒 Files selected for processing (14)
js/src/10_colormaps.tsjs/src/50_chartview.tspython/xy/_figure.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/channels.pypython/xy/components.pypython/xy/marks.pyspec/api/styling.mdspec/design/rust-engine.mdtests/test_axis_title_gutter.pytests/test_png_export.pytests/test_static_client_security.pytests/test_svg_export.py
🚧 Files skipped from review as they are similar to previous changes (12)
- js/src/10_colormaps.ts
- python/xy/channels.py
- spec/design/rust-engine.md
- tests/test_static_client_security.py
- python/xy/_figure.py
- tests/test_axis_title_gutter.py
- tests/test_png_export.py
- tests/test_svg_export.py
- python/xy/components.py
- spec/api/styling.md
- python/xy/marks.py
- python/xy/_svg.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/marks.py (1)
2376-2378: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude values exactly equal to the upper contour level.
With
side="right",dense == edges[-1]producesband == len(edges) - 1, soinsideis false and those cells remain transparent/unpainted. This affects both the scalar and truecolorcontourfpaths.Normalize exact upper-bound samples into the final band before computing
inside, and add a regression test for values equal to the highest contour level.Proposed fix
band = np.searchsorted(edges, dense, side="right") - 1 +at_upper = np.isfinite(dense) & (dense == edges[-1]) +band[at_upper] = len(edges) - 2 -mids = (edges[:-1] + edges[1:]) * 0.5 -inside = np.isfinite(dense) & (band >= 0) & (band < len(edges) - 1) +mids = (edges[:-1] + edges[1:]) * 0.5 +inside = np.isfinite(dense) & (band >= 0) & (band < len(edges) - 1)🤖 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 `@python/xy/marks.py` around lines 2376 - 2378, Update the contour band assignment in the scalar and truecolor contourf paths around `np.searchsorted` so samples exactly equal to `edges[-1]` map to the final valid band before computing `inside`; preserve exclusion of non-finite and out-of-range values. Add a regression test covering values equal to the highest contour level and verify they are painted.
🤖 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.
Outside diff comments:
In `@python/xy/marks.py`:
- Around line 2376-2378: Update the contour band assignment in the scalar and
truecolor contourf paths around `np.searchsorted` so samples exactly equal to
`edges[-1]` map to the final valid band before computing `inside`; preserve
exclusion of non-finite and out-of-range values. Add a regression test covering
values equal to the highest contour level and verify they are painted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f84ddc15-adbf-4573-9cbf-146fd535cd01
📒 Files selected for processing (20)
js/src/20_theme.tsjs/src/40_gl.tsjs/src/50_chartview.tspython/xy/_figure.pypython/xy/_native.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/channels.pypython/xy/components.pypython/xy/marks.pypython/xy/styles.pyspec/api/styling.mdsrc/lib.rssrc/raster.rstests/pyplot/test_line_legend_gallery_compat.pytests/test_components.pytests/test_css_mark_styles.pytests/test_figure.pytests/test_static_client_security.pytests/test_ui_issue_regressions.py
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/test_css_mark_styles.py
- js/src/20_theme.ts
- python/xy/channels.py
- tests/test_static_client_security.py
- python/xy/_native.py
- python/xy/_figure.py
- tests/test_figure.py
- js/src/40_gl.ts
- tests/test_ui_issue_regressions.py
- tests/test_components.py
- src/lib.rs
- src/raster.rs
- python/xy/styles.py
- spec/api/styling.md
- python/xy/components.py
- python/xy/_raster.py
- python/xy/_svg.py
- js/src/50_chartview.ts
The consolidation comparison sheets and their provenance manifest were committed only as review evidence. The PR description links them by pinned raw URL at c196c12, so the images stay reachable without carrying 430 KB of PNGs into the merge.
Review — 33 findings, 7 I'd fix before mergeReviewed at head Method: 11 scoped finder agents over the diff → every candidate finding judged by 3 Blockers1. C ABI signature changed without an
|
| path | time |
|---|---|
ax.streamplot(...) as shipped |
206.7 ms |
_native.streamlines(...) (still present, now unreachable) |
0.110 ms |
The Rust kernel still builds, still works, and is still wired through
python/xy/_native.py:1783 — it is now orphaned dead code on the default path.
(The 206.7 ms figure is the whole streamplot call, so it includes chart build; the
integration-only ratio is smaller but the order of magnitude holds.)
CodSpeed reported no regression because no benchmark covers streamplot.
Fix: restore the start_points is None and integration_direction == "both"
fast path, or state explicitly why the kernel was abandoned and delete it.
3. Quoted font-family produces non-well-formed SVG
python/xy/_svg.py:2028
The newly emitted font attributes pass user strings through escape(), which
replaces only &, <, > — not ". Reproduced:
ax.set_title('T', fontfamily='"Foo Bar", serif')emits
font-family=""Foo Bar", serif" fill="black"
and ET.fromstring(svg) fails: not well-formed (invalid token): line 1, column 3882.
A quoted family name is normal CSS and matplotlib accepts it. The output file is not
merely ugly — it does not parse. Note the built-in default ('Segoe UI', single
quotes) is safe, which is why no test catches it.
Fix: escape(value, {'"': """}) (or quoteattr) on every attribute-context
emit, not just these three.
4. Negative axis margins are accepted, then crash at render
python/xy/pyplot/_axes.py:6415
This PR relaxes _validate_margin to matplotlib's > -0.5 and starts forwarding
the raw margin to xy.x_axis(margin=...), whose validator still rejects negatives.
Reproduced:
ax.margins(-0.1) # accepted
fig.savefig("m.svg") # ValueError: x_axis margin must be non-negativeThe two halves of the change were not reconciled: the error surfaces far from the
call that caused it.
Fix: either clamp/translate the negative margin when building the axis spec, or
keep rejecting it at margins() where the user can see it.
5. Axes.legend() returns the Axes — silent corruption, not a loud failure
python/xy/pyplot/_axes.py:5196
Reproduced:
leg = ax.legend()
leg is ax # True
leg.set_title("LEGEND TITLE")
ax.get_title() # 'LEGEND TITLE' <-- retitled the axesmatplotlib returns a Legend. Correcting the panel here: base declared
def legend(...) -> None, so this is not a fresh API break — but it is a change from
a loud AttributeError: 'NoneType' to silently doing the wrong thing. leg.remove()
now deletes the whole subplot.
Fix: return None as before, or a real Legend handle. Returning self is the
worst of the three.
6. tight_layout() becomes a permanent no-op after the first layout call
python/xy/pyplot/_mplfig.py:461
tight_layout() guards its entire reservation pass on no axes having _figure_rect
set without a _subplot_spec. But subplots_adjust() — which tight_layout itself
calls at line 573 — now materializes _figure_rect on every grid axes. So the first
call works and every subsequent call silently does nothing. Calling tight_layout()
after adding a colorbar or changing labels is routine.
7. Diamond markers clipped at row-band boundaries in PNG
src/raster.rs:2014
symbol_extent() was threaded through point_u8_at, paint_affine_points and
paint_styled_points, but paint_points's band-bucketing closure still uses the bare
radius (let ext = rr + batch.sw + 1.0;). With the enlarged diamond SDF (r*√2) the
marks bucket into too few row bands and their top/bottom tips are cut off at every
band boundary.
Significant
Three-renderer parity — the systemic defect class in this repo. Each of these
lands in fewer than all three renderers:
font-familyis dropped entirely by PNG export —python/xy/_raster.pycontains
zero occurrences offont_family/label_font_family, while_svg.pyand the
client both honor it. This is structural:src/font.rsbakes a single face, so the
native atlas cannot represent an alternate family at all.
spec/api/styling.md:173nonetheless says these are "passed through to the browser,
SVG, and native PNG paths."- Browser and exporters compute different plot rectangles (two distinct causes):
js/src/50_chartview.ts:583sizes tick labels with a 0.62-em-per-character estimate
while_svg.layout()(shared by SVG and PNG) uses the real DejaVu advance table;
and_yAxisLeftRoom(50_chartview.ts:563) has no counterpart to the exporters'
_axis_text_paint_visibleguard, so an axis hidden viashow=Falsestill inflates
the browser's left margin. The documentedshow=False+padding=0sparkline is
edge-to-edge in SVG/PNG and inset ~35 px in the browser. - Scatter artist alpha multiplies instead of replaces in the raster fast paths
(_raster.py:1545,fill_op * paint[3]at 1573/1602) — PNG paints these markers
far more transparent than SVG or the browser. - Rotated multi-line annotations stack down the screen, not along the rotated
baseline (_raster.py:1397). - Annotation text bbox ignores rotation in SVG (
_svg.py:2410) — the box is
missing at 90/270° and axis-aligned at other angles while the glyphs rotate away. - Explicit legend items without
style.colorrender grey in the browser and
palette-colored in both exporters (50_chartview.ts:1817). contour(extend=...)is a validated no-op on the default colormapped path
(python/xy/marks.py:2389) — in all three renderers.
Wire contract
- New colormap names, legend
anchor/border_pad/handleheight/items, colorbar
shrink/anchor/minor_ticks, andmatch_fillstroke producers all ship with
PROTOCOL_VERSIONleft at 7 (python/xy/channels.py:50). A cached older v7 bundle
passes the handshake and then silently misrenders. This repo has already shipped a
protocol-mismatch incident; the version exists to prevent exactly this.
Correctness
bar()categorical autoscale uses positional ordinals instead of the core's
first-seen category map (_axes.py:3333), so the shim's x extent disagrees with what
the core actually renders whenever labels repeat or a second categorical mark
contributes different labels.fill_betweenbaselines anderrorbarerror bars are excluded from the autoscale
extent walk (_axes.py:3372) — the walk matchesfactory == "area"but
fill_betweenrecordskind == "area", and"errorbar"is absent from the map
entirely.get_xlim()/get_ylim()disagree with matplotlib and with xy's own
rendered axis.bar(bottom=)/barh(left=)lose sticky baselines (_axes.py:6415): the core only
pins a baseline of exactly 0, so bars with a non-zero base float above the spine
whileget_ylim()reports them flush.subplots_adjust()drags a rect-less axes onto theadd_axes1×N grid
(_mplfig.py:559).stem(basefmt="k--")crashes at chart build (_plot_types.py:1830) — fractional
(start, stop)span tuples are fed todash=, which requires flat pixel lengths.barbs()raises an opaque reduction error on fully masked/NaN wind data
(_plot_types.py:5125) instead of drawing nothing.
Performance
loc="best"rebuilds the category map by looping in Python over every coordinate
of every entry on the axis (_axes.py:5982), including large purely numeric series
that can never contribute a category. One small string-labelled mark makes legend
placement O(total points) in interpreted code.- The bar branch of
_best_legend_loc(_axes.py:6043) bypasses the
_BEST_LOC_SAMPLEbudget entirely and re-copies four boolean masks inside the
nine-candidate loop — 36 full-length mask copies per_build_chart.
Test quality
tests/pyplot/test_gallery_colorbar_options.py:45—
test_colorbar_location_anchor_shrink_and_minor_ticks_reach_exportsverifies none of
shrink/anchorin either exporter and nominor_ticksin raster. Its only
export-side assertions are one SVG substring and a PNG magic-byte check. The test
cannot fail for the thing it is named after.
Minor
spec/api/styling.md:705and:684say the native atlas "carries one regular and
one bold face" —src/raster.rs:1610synthesizes faux bold by re-blitting each glyph
with a sub-pixel horizontal shift (bold_shift). There is one face.spec/api/styling.md:751documentsloc="best"decimation at 4,096 vertices;
_BEST_LOC_SAMPLE = 512for every family except scatter — 8× smaller.spec/api/styling.md:171saystick_padding"Defaults to0"; all three renderers
default it to 4 px.spec/api/styling.md:309documents a y-title placement (10 px canvas inset,
half-line-box gutter, line-box clamp) that does not exist in the shipped
_y_title_baseline/_y_axis_left_room.PathCollection.get_sizes()ignoresset_sizes()(_artists.py:497) — it
short-circuits to the immutableentry["source_sizes"], so mutated collections
report pre-mutation sizes and mislabellegend_elements(prop="sizes").rcParamsrejects negativextick/ytick.major.pad(_rc.py:167) although
matplotlib accepts it,tick_params(pad=-4)accepts it, and styling.md documents the
value as "negative allowed".CHANGELOG.mdis untouched despite user-visible default changes to xy's own
public API: chart title weight 600→400, axis-label weight 500→400, annotation/legend/
colorbar title weights →400, andpyplot.errorbardefault cap size auto→0.- Measured y-gutter runs off-canvas below ~90 px chart width (
_svg.py:1458).
Correcting the panel: three verifiers called this a general narrow-canvas bug; it
is not. I measured widths 900/320/200/140 — all fine, the f9f1328 cap works. It only
breaks at 80/60/40 px, where the plot rect clamps to a 40 px minimum whilexstays
at 49.8. Degenerate sizes, low practical impact.
Rejected by verification (recorded so they are not re-raised)
- "
_auto_domain()and the rendered view are two disagreeing models" — 0/2. - "
get_dash_capstylereports 'round'; matplotlib's default is rejected" — 1/3. - "
pie_labelalignment ignores the pie center" — 0/3. - "
attachYTitleToTicksforces synchronous layout every chrome frame" — 1/3. - "Legend-title regression test asserts a 4-char prefix" — 1/3.
Verdict
Do not merge as-is. #1 (ABI) and #2 (streamplot) should be fixed before merge —
both are one-to-few-line changes, both are invisible to the entire test suite and to
CodSpeed, and both fail in ways that are hard to diagnose later (a segfault and a
1,900× slowdown). #3–#7 are ordinary pre-merge fixes.
The parity and protocol items are the same institutional gap this repo has hit twice
before, and they will keep recurring until a test asserts option-by-option that the
three renderers agree.
On the PR description's claims
- "122 Rust tests passed" — 125 now; harmless drift from the merge commits.
- "All current GitHub checks pass" — true, verified on
71c5f33(28/28). - "2 improved, 0 regressions" on CodSpeed — accurate as far as CodSpeed's coverage
goes, but no benchmark coversstreamplot, which this PR slowed by roughly three
orders of magnitude. The green CodSpeed result should not be read as "no performance
regressions". - The description's framing that commits 710ba10 and 643e75a "restore" performance is
worth scrutiny: they restore the benchmarked paths.streamplotis the
unbenchmarked one, and it did not get restored.
Addendum — two more, from the sweep that finished after the review aboveA final pass for things the earlier dimensions didn't cover turned these up. Both reproduce on 1.
|
| vertices | SVG output |
|---|---|
| 98 | 1 <polygon>, 5,730 bytes |
| 998 | 1 <polygon>, 17,532 bytes |
| 2,398 | 2,396 polygons, 244,817 bytes |
| 9,998 | 9,996 polygons, 1,007,625 bytes |
It is not a size threshold — it is data-dependent. At a fixed 2,398 vertices, shifting x by 1e-4 flips the output back to a single polygon:
| x shift | polygons |
|---|---|
| 0 | 2,396 |
| 1e-4 | 1 |
| 7e-4 | 1 |
| 3e-3 | 1 |
Each of those triangles is independently antialiased and alpha-blended, so every internal diagonal shows the hairline seam and doubled alpha that triangle_mesh_boundary's own docstring says it exists to prevent. The WebGL client draws both cases seamlessly, so this is browser-vs-export divergence on top of a 60× file-size blowup. savefig(format="png") takes the identical fallback at python/xy/_raster.py:1815.
Fix: merge by snapping to a canonical representative (e.g. sort-and-union adjacent keys, or hash on the rounded midpoint of a ±1 bucket window) rather than trusting a single round() bucket to be stable.
2. Capability registry omits the three new axis font keys
python/xy/styling/capabilities.py:358
This PR adds label_font_family / label_font_style / label_font_weight to the axis style= vocabulary at python/xy/styles.py:54 and includes them in the allowed set at styles.py:384, but axis_style_keys() still unions only the color/length/size/compat sets:
def axis_style_keys() -> tuple[str, ...]:
"""The axis `style=` vocabulary, read from `styles.py` rather than listed.
A fresh-agent evaluation of this repo caught the comparison document
quoting 15 of these after a 16th shipped. Prose cannot hold a count; the
registry derives it and `summary()` publishes it.
"""Verified on this branch: axis_style_keys() returns 18 keys with all three font keys absent, so summary()["axis_style_keys"] reports 18 where the vocabulary is 21. README.md:142 and docs/api-reference/limitations-and-alpha-status.md:57 point readers and doc generators at this module as the generated source of truth, so it now under-reports what is styleable — which is the exact failure the docstring was written about.
tests/test_capability_registry.py pins only MARK_STYLE_PROPERTIES and CHART_SLOTS, never the axis vocabulary, so the suite stays green.
Fix: add styles._AXIS_FONT_PROPERTIES to the union, and extend the registry test to assert axis_style_keys() covers everything _compile_axis_style accepts.
# Conflicts: # python/xy/_native.py # python/xy/_raster.py # python/xy/_svg.py # python/xy/marks.py # spec/api/styling.md # spec/design/rust-engine.md # src/font.rs # src/lib.rs # src/raster.rs # tests/test_png_export.py # tests/test_svg_export.py
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.
2 participants
Summary
This is the base of a two-PR shipping stack for the existing Matplotlib compatibility work. It replaces the review surface of the many small gallery drafts with one integrated snapshot on current
main; #281 adds the final acceptance repairs on top.The stack combines:
loc="best"legend layoutIf this stack is accepted, it supersedes #206, #211, #240–#248, #252–#256, and #268–#274. Several older implementations were intentionally replaced by corrected integrated versions; they should not be replayed mechanically. The component drafts remain open for provenance for now. This PR does not merge or close them.
Exact visual evidence
The commit labels and SHA-256 hashes are recorded in
pr-assets/matplotlib-gallery-consolidation/provenance.json.Bar-label bounds
On main, labels from the penguin stacked bars spill outside the axes and the third category is missing. Stack A keeps labels centered within their segments and restores the complete chart.
Annotation geometry
Stack A brings the arrow position, stroke, and text placement back toward the Matplotlib reference without clipping the frame.
Full survey legend labels
The integrated audit found that the old #256 result still ellipsized all five legend labels. The current stack preserves the complete labels while retaining the measured left gutter and bounded export.
Verification
10b22b9: 77/88 paired examples, up from 52/88 on main, with 133 paired figures.legend(loc="best")choice while retaining the 512-vertex budget for connected paths; its exact regression and focused legend suite pass 33/33.git diff --checkalso pass.10b22b9only by the evidence/provenance commit; it contains no later render-code change.Performance
The exact-head CodSpeed comparison at
c196c12is green:The two measured improvements are pyplot PNG export (2.3×) and first-payload large error bars (18.71%). The sparse-scatter repair adds at most 3,584 vectorized offset checks only when
loc="best"scores a scatter and does not register a CodSpeed regression.Known deferred work
The stack deliberately freezes scope rather than claiming complete Matplotlib parity. #281 documents the seven scoped gallery examples left for later.
Four older #211 review findings are also still deferred: automatic RGBA-stage selection, hidden-RGB bleed through masked RGBA interpolation, thin-feature collisions in triangle-boundary bucketing, and mask/NaN spreading in regular Gouraud interpolation. A log-singleton public/render-domain mismatch from #240 remains deferred as well. They are not regressions introduced by consolidation, but they are real remaining compatibility work.
Stack
Summary by CodeRabbit
New Features
margincontrol and alegendanchoroption for more predictable layout.shrink,anchor, and optionalminor_ticks.corner_masksupport forcontour/contourf, plus improved styled/math-italic text rendering.Bug Fixes
stroke.mode="match_fill"handling for triangle/rect family marks and refined marker/geometry fidelity.Tests