Fix Matplotlib gallery, styling, axes, and label compatibility [mpl compatibility] - #340
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:
📝 WalkthroughWalkthroughThe PR adds shared multiline text measurement, side-aware tick rendering, measured layout, figure decorations, pyplot compatibility behavior, protocol version 10, and extensive SVG, raster, browser, and API regression coverage. ChangesRendering and pyplot compatibility
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (10)
python/xy/pyplot/_axes.py (3)
5756-5766: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMeasure the space advance rather than approximating it at
0.32 em.
_textblockis already imported andmeasure(" ", font_px).widthgives the exact advance the renderers will use, so the padding that centers the cell background stops depending on a magic constant that will drift if the font metrics change.♻️ Suggested change
- space_width_px = font_px * 0.32 + space_width_px = _textblock.measure(" ", font_px).width🤖 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 5756 - 5766, Update the cell padding calculation in the surrounding plotting layout code to derive the space advance from the imported _textblock measurement API, using measure(" ", font_px).width instead of the hard-coded font_px * 0.32 approximation. Keep the existing vertical and horizontal centering behavior and padding formatting unchanged.
5726-5728: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInitialize
_materialize_plot_pxinstead of guarding withgetattr.The attribute is only ever assigned in
_build_chart(Line 6651) and on the twin (Line 6657); thegetattrdefault hides the fact that it is missing on a freshly constructedAxes. Declaring it alongside the other geometry fields in__init__(and resetting it inclear()) makes the contract explicit and lets any future direct_chart_children()caller fail loudly rather than silently sizing cells against_DEFAULT_BEST_PLOT_SIZE.♻️ Suggested change
- plot_width, plot_height = getattr( - self, "_materialize_plot_px", _DEFAULT_BEST_PLOT_SIZE - ) + plot_width, plot_height = self._materialize_plot_pxplus, in
Axes.__init__andAxes.clear():self._materialize_plot_px: tuple[float, float] = _DEFAULT_BEST_PLOT_SIZE🤖 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 5726 - 5728, Initialize _materialize_plot_px in Axes.__init__ and reset it in Axes.clear() to _DEFAULT_BEST_PLOT_SIZE, alongside the existing geometry fields. In _chart_children, replace the getattr fallback with direct access to self._materialize_plot_px so missing initialization fails loudly while preserving the configured tuple value.
5954-5967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated title-gutter measurement into one helper. Both sites build the same merged
title_style, parsefont-sizewith the sameremovesuffix("px")+try/except, and apply the samemax(26/30 compact floor, measured height + 8)rule. Keeping two copies means a future change to the title band silently desynchronizes_plot_rect_px(tick-budget estimation) from_outside_padding(the gutter the renderers actually reserve) — and_svg.layoutalready encodes the same rule a third time, so the shared helper should be the single mirror of it.
python/xy/pyplot/_axes.py#L5954-L5967: replace the inline block with a call to a newself._title_gutter(compact)helper.python/xy/pyplot/_axes.py#L6359-L6372: replace the identical inline block with the sameself._title_gutter(compact)call.♻️ Suggested helper
def _title_gutter(self, compact: bool) -> float: """The title band the renderers reserve, mirroring `_svg.layout()`.""" if not self._title: return 0.0 title_style = {**self._chrome_styles.get("title", {}), **self._title_style} raw_size = title_style.get("font-size", 14.0) try: title_size = float(str(raw_size).removesuffix("px")) except ValueError: title_size = 14.0 return max( 26.0 if compact else 30.0, _textblock.measure(self._title, title_size).height + 8.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/pyplot/_axes.py` around lines 5954 - 5967, Extract the duplicated title-gutter calculation into a new self._title_gutter(compact) helper that handles missing titles, merged title styles, font-size parsing, text measurement, and compact/non-compact minimums. In python/xy/pyplot/_axes.py lines 5954-5967 and 6359-6372, replace each inline block with self._title_gutter(compact); keep the helper as the single shared implementation.python/xy/pyplot/_plot_types.py (2)
4499-4501: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
float(fontsize)rejects Matplotlib's named font sizes.
Axes.textresolves sizes through_font_size_points(fontsize, rcParams["font.size"]), sofontsize="large"works there but raises a bareValueError: could not convert string to floathere. Reuse the shared resolver for consistency.♻️ Proposed fix
- fontsize = float(kwargs.pop("fontsize", rcParams["font.size"])) + fontsize = _font_size_points( + kwargs.pop("fontsize", rcParams["font.size"]), rcParams["font.size"] + ) if fontsize <= 0: raise ValueError("table fontsize must be positive")🤖 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/_plot_types.py` around lines 4499 - 4501, Update the fontsize handling in the table-creation path to use Matplotlib’s shared _font_size_points resolver with rcParams["font.size"] instead of converting fontsize directly with float. Preserve the positive-size validation and ensure named sizes such as "large" resolve consistently with Axes.text.
4519-4521: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRow-label gutter uses a character-count heuristic instead of the shared text metrics.
len(label) * fontsize * 0.6assumes monospace, so wide labels ("MMMM") under-reserve and narrow ones ("iiii") over-reserve the row-label column. This stack already introduced_textblock.measure/_fontmetrics.advancefor renderer-independent measurement; using it here makes the gutter exact.♻️ Sketch
+ from .. import _textblock + row_label_width_points = ( - max((len(label) for label in (row_labels or ())), default=0) * fontsize * 0.6 + 6.0 + max( + (_textblock.measure(label, fontsize).width for label in (row_labels or ())), + default=0.0, + ) + + 6.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/pyplot/_plot_types.py` around lines 4519 - 4521, Replace the character-count calculation assigned to row_label_width_points with the existing renderer-independent text measurement APIs, using _textblock.measure or _fontmetrics.advance for each row label and preserving the empty-label default and 6.0-point padding. Remove the len(label) * fontsize * 0.6 heuristic so gutter width reflects actual label advances.tests/pyplot/test_table_gallery_compat.py (1)
42-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a static-output assertion for the
loc="bottom"claim.Every assertion here stops at the payload spec, but the changelog states natural tables "render outside the axes without clipping in static PNG/SVG". A
fig.savefig(BytesIO(), format="svg")smoke check (cell text present, tableybelow the plot rect) would lock in the behavior this PR actually fixes.As per path instructions, "Enforce pyplot compatibility and boundaries using the corpus and boundary tests, including
tests/pyplot/corpus/andtests/pyplot/test_boundaries.py" — consider adding the bottom-table gallery case to the corpus as well.🤖 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_table_gallery_compat.py` around lines 42 - 54, Extend the bottom-table compatibility test around the existing _build_chart payload assertions with a static SVG save smoke check using BytesIO, verifying cell text is present and the table’s rendered y position is below the plot rectangle. Also add the loc="bottom" gallery case to the pyplot corpus where applicable, preserving the existing payload and boundary assertions.Source: Path instructions
tests/pyplot/test_multiline_chrome_layout.py (2)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixture return type should be an iterator.
_clean_pyplotis a generator fixture, so-> Noneis inaccurate and type checkers will flag it.🧹 Annotation fix
+from collections.abc import Iterator + `@pytest.fixture`(autouse=True) -def _clean_pyplot() -> None: +def _clean_pyplot() -> Iterator[None]: plt.close("all") yield plt.close("all")🤖 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_multiline_chrome_layout.py` around lines 17 - 21, Update the _clean_pyplot fixture’s return annotation from None to the appropriate iterator/generator type, while preserving its existing setup, yield, and teardown behavior.
129-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueString-matching the TypeScript source is brittle.
These assertions break on purely cosmetic TS changes (a formatter splitting the
replace(...).split(...)chain, a rename of_xAxisRoom/hasMultilineTicks, or dropping a space in the CSS literal) while catching nothing about actual browser behavior.tests/test_ui_issue_regressions.pyalready exercises the same contract through a real Chromium probe; consider asserting the rendered gutters/labels there and keeping only a narrow marker here.🤖 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_multiline_chrome_layout.py` around lines 129 - 138, Replace the brittle source-string assertions in test_browser_client_uses_the_same_preline_block_contract with a narrow stable marker, and move behavioral coverage for multiline normalization, text sizing, whitespace, and tick gutters/labels into the existing Chromium-based tests in tests/test_ui_issue_regressions.py. Preserve verification of the actual browser-rendered contract rather than TypeScript formatting or implementation names.python/xy/pyplot/_grid.py (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate of
_svg._text_block_content.Same tspan contract, two implementations that must stay in step (this one escapes with
_html.escapeand formats with:g, the other with_svg.escapeand_num). Reusing the_svghelper keeps one definition of the multiline block emission.🤖 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/_grid.py` around lines 33 - 38, Replace the duplicate implementation in _svg_text_lines with reuse of the existing _svg._text_block_content helper. Preserve the current multiline SVG tspan output contract while removing the separate escaping and numeric-formatting logic from _grid.py.tests/test_ui_issue_regressions.py (1)
662-689: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
everypasses vacuously if no tick labels are collected.If the selector or the
dataset.xyAxisfilter ever stops matching,topInside/bottomInsideare bothtrueand the test silently stops asserting anything. Ship the counts too.✅ Assert the labels were actually found
document.body.setAttribute("data-xy-issue-probe", JSON.stringify({ topRoom: view.plot.y, bottomRoom: view.size.h - view.plot.y - view.plot.h, + topCount: labels.filter((item) => item.axis === "x2").length, + bottomCount: labels.filter((item) => item.axis === "x").length, topInside: labels.filter((item) => item.axis === "x2") .every((item) => item.top >= root.top - 1), bottomInside: labels.filter((item) => item.axis === "x") .every((item) => item.bottom <= root.bottom + 1), }));assert result["topRoom"] > 100, result assert result["bottomRoom"] > 100, result + assert result["topCount"] == 3, result + assert result["bottomCount"] == 3, result assert result["topInside"] is True, result assert result["bottomInside"] is True, result🤖 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` around lines 662 - 689, Update the probe in the rotated x tick gutters test to include counts for the collected x and x2 tick labels in the data stored by data-xy-issue-probe. Add assertions that both counts are greater than zero before retaining the existing topInside and bottomInside checks, ensuring the every predicates cannot pass without matching labels.
🤖 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 `@python/xy/_raster.py`:
- Around line 1079-1088: Update the x-axis tick-label call to _emit_text_block
so its anchor defaults based on the rotation angle sign, matching SVG: use
“start” for positive angles and “end” for negative angles when tick_label_anchor
is unset, while preserving an explicitly configured tick_label_anchor.
In `@python/xy/_svg.py`:
- Around line 1531-1543: Update the styling specification’s description of the
y-axis left-gutter calculation to match _y_axis_left_room(): document reserving
the full multiline title box using ascent, descent, and additional line steps,
rather than half the title line box. Leave the implementation unchanged.
In `@python/xy/pyplot/_axes.py`:
- Around line 6389-6410: Optimize _x_multiline_extra by pre-scanning tick_labels
and skipping the measurement loop when every label is single-line, since such
labels contribute no extra padding. Only measure labels when a multiline label
is present, preserving the existing rotated-extent and axis-label calculations.
In `@python/xy/pyplot/_grid.py`:
- Around line 285-297: The grid suptitle baseline calculation in the PNG
rendering path must match the SVG positioning and remain inside the reserved
title band. Update the relevant grid composition logic to derive the baseline
from the configured title y-position using (1 - y) * height, clamp it to the
title-band boundary, and preserve the existing ascent fallback for missing text
measurements.
In `@python/xy/pyplot/_mplfig.py`:
- Around line 113-123: The probe build around ax._build_chart mutates persistent
axis domains for equal-aspect axes and leaves them based on probe geometry.
Snapshot the relevant ax._axis x/y domain state before the probe and restore it
in the existing finally block, while preserving the current ax._chart
restoration and retaining the generated spec for layout.
- Around line 76-86: The repeated _panel_chrome calls rebuild identical
measurement payloads, causing excessive layout/export work. Add pass-scoped
caching keyed by axes identity and the measured width/height, and reuse cached
_measured_axis_chrome results across _charts, _panel_positions, and
_apply_tight_layout; alternatively, short-circuit only when the axes content
cannot exceed defaults while preserving measurement correctness.
- Around line 963-978: Update multiline mosaic parsing in the mosaic handling
branch to strip whitespace from each parsed row after splitting it into
characters, ensuring trailing spaces or uneven indentation do not become labels.
Preserve the existing row and column validation while applying the normalization
consistently to every multiline row.
In `@python/xy/pyplot/_plot_types.py`:
- Line 4522: Update the edges handling near border construction to explicitly
validate supported values instead of treating every value other than "" or
"open" as a closed border. Either call the existing not_implemented mechanism
for unsupported Matplotlib edge specifications, including values such as
"horizontal" and typos, or document the intentional reduction in the Matplotlib
compatibility specification; do not leave the behavior silent.
In `@spec/matplotlib/compat.md`:
- Line 81: Update the subplot_mosaic compatibility entry to document the
accepted and validated subplot_kw, per_subplot_kw, and gridspec_kw keyword
arguments, including errors for per_subplot_kw labels absent from the mosaic and
duplicate ratio definitions between direct arguments and gridspec_kw. Keep the
existing geometry, sharing, and invalid-region behavior documented.
---
Nitpick comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 5756-5766: Update the cell padding calculation in the surrounding
plotting layout code to derive the space advance from the imported _textblock
measurement API, using measure(" ", font_px).width instead of the hard-coded
font_px * 0.32 approximation. Keep the existing vertical and horizontal
centering behavior and padding formatting unchanged.
- Around line 5726-5728: Initialize _materialize_plot_px in Axes.__init__ and
reset it in Axes.clear() to _DEFAULT_BEST_PLOT_SIZE, alongside the existing
geometry fields. In _chart_children, replace the getattr fallback with direct
access to self._materialize_plot_px so missing initialization fails loudly while
preserving the configured tuple value.
- Around line 5954-5967: Extract the duplicated title-gutter calculation into a
new self._title_gutter(compact) helper that handles missing titles, merged title
styles, font-size parsing, text measurement, and compact/non-compact minimums.
In python/xy/pyplot/_axes.py lines 5954-5967 and 6359-6372, replace each inline
block with self._title_gutter(compact); keep the helper as the single shared
implementation.
In `@python/xy/pyplot/_grid.py`:
- Around line 33-38: Replace the duplicate implementation in _svg_text_lines
with reuse of the existing _svg._text_block_content helper. Preserve the current
multiline SVG tspan output contract while removing the separate escaping and
numeric-formatting logic from _grid.py.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 4499-4501: Update the fontsize handling in the table-creation path
to use Matplotlib’s shared _font_size_points resolver with rcParams["font.size"]
instead of converting fontsize directly with float. Preserve the positive-size
validation and ensure named sizes such as "large" resolve consistently with
Axes.text.
- Around line 4519-4521: Replace the character-count calculation assigned to
row_label_width_points with the existing renderer-independent text measurement
APIs, using _textblock.measure or _fontmetrics.advance for each row label and
preserving the empty-label default and 6.0-point padding. Remove the len(label)
* fontsize * 0.6 heuristic so gutter width reflects actual label advances.
In `@tests/pyplot/test_multiline_chrome_layout.py`:
- Around line 17-21: Update the _clean_pyplot fixture’s return annotation from
None to the appropriate iterator/generator type, while preserving its existing
setup, yield, and teardown behavior.
- Around line 129-138: Replace the brittle source-string assertions in
test_browser_client_uses_the_same_preline_block_contract with a narrow stable
marker, and move behavioral coverage for multiline normalization, text sizing,
whitespace, and tick gutters/labels into the existing Chromium-based tests in
tests/test_ui_issue_regressions.py. Preserve verification of the actual
browser-rendered contract rather than TypeScript formatting or implementation
names.
In `@tests/pyplot/test_table_gallery_compat.py`:
- Around line 42-54: Extend the bottom-table compatibility test around the
existing _build_chart payload assertions with a static SVG save smoke check
using BytesIO, verifying cell text is present and the table’s rendered y
position is below the plot rectangle. Also add the loc="bottom" gallery case to
the pyplot corpus where applicable, preserving the existing payload and boundary
assertions.
In `@tests/test_ui_issue_regressions.py`:
- Around line 662-689: Update the probe in the rotated x tick gutters test to
include counts for the collected x and x2 tick labels in the data stored by
data-xy-issue-probe. Add assertions that both counts are greater than zero
before retaining the existing topInside and bottomInside checks, ensuring the
every predicates cannot pass without matching labels.
🪄 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: 327f7d65-657c-47fb-b6af-2aca4dac7ca5
📒 Files selected for processing (26)
js/src/50_chartview.tspython/xy/_raster.pypython/xy/_svg.pypython/xy/_textblock.pypython/xy/_validate.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_grid.pypython/xy/pyplot/_mathtext.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pyspec/api/styling.mdspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdtests/pyplot/test_annotation_box_text_fidelity.pytests/pyplot/test_autoscale_margin_bar_regressions.pytests/pyplot/test_axes_charts.pytests/pyplot/test_categorical_gallery_regressions.pytests/pyplot/test_gallery_layout_api_regressions.pytests/pyplot/test_layout_text_parity_fixes.pytests/pyplot/test_multiline_chrome_layout.pytests/pyplot/test_p3_option_contracts.pytests/pyplot/test_table_gallery_compat.pytests/test_png_export.pytests/test_svg_export.pytests/test_ui_issue_regressions.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/xy/pyplot/_axes.py (2)
3199-3266: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
set_xlim/set_ylimmixhost's domain withself's own data for the untouched side.Both setters resolve the shared ticker
hostfor the domain write, but fall back toself._entry_extent(...)(nothost._entry_extent(...)) when no domain is set yet. For a shared, non-source axes (self != host) callingset_xlim(left=...)/set_ylim(bottom=...)with only one side given, the untouched side is derived fromself's own plotted data instead of the ticker host's, producing an inconsistent domain relative to whatget_xlim()/other shared panels would report.🐛 Suggested fix
host = (self._y2_of or self)._shared_ticker_source("x") current = host._axis["x"].get("domain") - lo, hi = current if current is not None else self._entry_extent("x") + lo, hi = current if current is not None else host._entry_extent("x")Apply the same change in
set_ylim(host._entry_extent("y")in place ofself._entry_extent("y")).🤖 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 3199 - 3266, Update the fallback extent selection in both set_xlim and set_ylim to use the resolved shared ticker host, calling host._entry_extent("x") and host._entry_extent("y") respectively instead of self._entry_extent(...), while leaving explicit-domain handling unchanged.
6425-6493: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftShared-x multiline gutter still needs the shared ticker source.
_outside_padding()is called from_panel_chrome()and_build_chart_uncached()before_inherit_shared_axis_state()runs, so non-source panels can miss multiline/rotatedtick_labelsand reserve too little gutter. Resolve_x_multiline_extra()from_shared_ticker_source("x")or mirror the shared tick state earlier.🤖 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 6425 - 6493, Update _x_multiline_extra() to resolve x-axis tick labels, angle, and related styling from _shared_ticker_source("x") when a shared ticker source exists, so _outside_padding() reserves the same multiline/rotated gutter for every panel before _inherit_shared_axis_state() runs. Preserve the current local-axis fallback when no shared source is configured.
🧹 Nitpick comments (1)
python/xy/components.py (1)
5102-5113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate
tick_sidesvalidation into_validate.py. Both sites independently implement the identical "allowed sides for x vs y, filter, error" logic; the codebase's established convention (tick_label_strategy,tick_label_anchor,label_position) shares this kind of validator once from_validate.py.
python/xy/components.py#L5102-L5113: replace the local_axis_tick_sidesbody with a call into a new_validate.axis_tick_sides(value, which).python/xy/_figure.py#L288-L297: replace the inline block with a call to the same_validate.axis_tick_sides(tick_sides, axis_dim)(mirroring how_axis_tick_label_strategy/_axis_tick_label_anchorare aliased elsewhere in this class).🤖 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 5102 - 5113, Deduplicate tick_sides validation by adding the shared axis_tick_sides validator in _validate.py, preserving the existing allowed-side checks, filtering, and errors. In python/xy/components.py lines 5102-5113, make _axis_tick_sides delegate to _validate.axis_tick_sides(value, which); in python/xy/_figure.py lines 288-297, replace the inline validation with _validate.axis_tick_sides(tick_sides, axis_dim).
🤖 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 `@python/xy/pyplot/_axes.py`:
- Around line 763-785: Update _SharedAxesGroup.join() so legacy join keeps the
shared ticker source via _shared_axis_sources but no longer aliases the entire
_axis_props dictionary through shared. Give each joined axis independent axis
properties, preserving separate tick_sides and tick_label_strategy state while
retaining the existing sharing and invalidation behavior.
---
Outside diff comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 3199-3266: Update the fallback extent selection in both set_xlim
and set_ylim to use the resolved shared ticker host, calling
host._entry_extent("x") and host._entry_extent("y") respectively instead of
self._entry_extent(...), while leaving explicit-domain handling unchanged.
- Around line 6425-6493: Update _x_multiline_extra() to resolve x-axis tick
labels, angle, and related styling from _shared_ticker_source("x") when a shared
ticker source exists, so _outside_padding() reserves the same multiline/rotated
gutter for every panel before _inherit_shared_axis_state() runs. Preserve the
current local-axis fallback when no shared source is configured.
---
Nitpick comments:
In `@python/xy/components.py`:
- Around line 5102-5113: Deduplicate tick_sides validation by adding the shared
axis_tick_sides validator in _validate.py, preserving the existing allowed-side
checks, filtering, and errors. In python/xy/components.py lines 5102-5113, make
_axis_tick_sides delegate to _validate.axis_tick_sides(value, which); in
python/xy/_figure.py lines 288-297, replace the inline validation with
_validate.axis_tick_sides(tick_sides, axis_dim).
🪄 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: 2ea96046-1b24-4456-a901-c81ea4a2361f
📒 Files selected for processing (25)
js/src/50_chartview.tspython/xy/_figure.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_textblock.pypython/xy/components.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_grid.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pyspec/api/styling.mdspec/matplotlib/compat.mdspec/matplotlib/shim-todo.mdtests/pyplot/test_annotation_box_text_fidelity.pytests/pyplot/test_anscombe_gallery_regressions.pytests/pyplot/test_axis_tick_gallery_compat.pytests/pyplot/test_gallery_layout_api_regressions.pytests/pyplot/test_line_gallery_semantics.pytests/pyplot/test_multiline_chrome_layout.pytests/pyplot/test_table_gallery_compat.pytests/pyplot/test_tick_side_rendering.pytests/test_png_export.pytests/test_static_client_security.pytests/test_svg_export.pytests/test_svg_text_metrics.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/pyplot/test_table_gallery_compat.py
- python/xy/pyplot/_plot_types.py
- python/xy/pyplot/_grid.py
- spec/api/styling.md
- tests/pyplot/test_gallery_layout_api_regressions.py
- js/src/50_chartview.ts
- python/xy/_svg.py
- python/xy/pyplot/_mplfig.py
…o agent/quiver-display-invariants
…llery-layout-text-round2 # Conflicts: # js/src/00_header.ts # python/xy/_raster.py # python/xy/_svg.py # python/xy/config.py # python/xy/pyplot/_axes.py # python/xy/pyplot/_mplfig.py # python/xy/pyplot/_rc.py # spec/design/wire-protocol.md # tests/pyplot/test_axes_charts.py # tests/pyplot/test_frame_geometry.py
…llery-layout-text-round2
sselvakumaran
changed the title
2a7d6eb to
bc73d15
Compare
|
The stack has been refreshed onto current @coderabbitai review |
|
The Comment images show side-by-side Matplotlib, pre-change XY, and updated XY gallery comparisons. They cover pie labels with patch legend handles; quiver and streamplot rendering; multiline categorical tick labels; error-bar endpoint labels and MathText fallbacks; subplot-mosaic spans, ratios, and holes; axes-relative bottom tables; and multiline top-aligned text boxes. The updated XY panels visibly correct several prior layout and rendering discrepancies, including table placement, mosaic geometry, and multiline label handling.
✅ Action performedReview finished.
|
sselvakumaran
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
python/xy/_raster.py (1)
996-1010: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMinor ticks stayed single-sided while major ticks became side-aware.
Lines 1011-1025 and 1042-1056 now iterate
_axis_tick_sides(...), but the minor-tick loops above them still derive a singlesidefromaxis.get("side"). An axis configured with ticks on both sides will draw majors on both edges and minors on only one.♻️ Suggested alignment
- inward, outward = tick_span(xmstyle) - side = xa.get("side", "bottom") - edge = py0 if side == "top" else py1 - for value in xmt: - ... + inward, outward = tick_span(xmstyle) + for side in _axis_tick_sides(xa, is_x=True): + edge = py0 if side == "top" else py1 + for value in xmt: + ...Also applies to: 1026-1041
🤖 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 996 - 1010, Update the minor-tick rendering loops around the visible x-axis loop and the corresponding y-axis loop to iterate over _axis_tick_sides(...) rather than deriving one side from axis.get("side"). Compute each tick’s edge and inward/outward span per returned side, preserving the existing major-tick side-aware behavior so axes configured on both sides render minor ticks on both edges.python/xy/_figure.py (1)
1320-1321: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep minor-axis numerics out of JSON.
minor_tick_valuesand numericminor_stylefields are emitted inline, and the new tests codify that wire shape. Ship numeric values through raw f32 buffer references while retaining f64 values CPU-side; update renderers, tests, and the wire contract accordingly.
python/xy/_figure.py#L1320-L1321: replace the numeric list with a raw f32 buffer reference.python/xy/_figure.py#L1350-L1351: encode numeric minor-style values through the binary transport.tests/pyplot/test_axis_tick_gallery_compat.py#L167-L172: decode buffer-backed minor tick/style values before asserting.tests/pyplot/test_axis_tick_gallery_compat.py#L180-L186: assert decoded or absent minor-tick buffers.tests/pyplot/test_axis_tick_gallery_compat.py#L198-L203: assert decoded promoted tick values.tests/pyplot/test_axis_tick_gallery_compat.py#L273-L277: assert decoded date tick values.As per coding guidelines, “Never place JSON numbers on the wire; transport data as raw f32 buffers.”
🤖 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 1320 - 1321, Keep minor-axis numeric data out of JSON by encoding minor_tick_values and numeric minor_style fields as raw f32 buffer references in python/xy/_figure.py at lines 1320-1321 and 1350-1351, while retaining f64 values for CPU-side use; update the corresponding wire-contract tests in tests/pyplot/test_axis_tick_gallery_compat.py at lines 167-172, 180-186, 198-203, and 273-277 to decode buffer-backed values and assert decoded or absent buffers, including promoted and date tick values.Source: Coding guidelines
python/xy/components.py (1)
2462-2466: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject string minor tick inputs.
"12"is iterated as two values and silently becomes[1.0, 2.0]. Rejectstrandbytesbefore convertingminor_tick_values, consistently in both public factories andFigure.set_axis().
python/xy/components.py#L2462-L2466: reject strings inx_axis().python/xy/components.py#L2591-L2595: reject strings iny_axis().python/xy/_figure.py#L323-L330: reject strings in directFigure.set_axis()calls.🤖 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 2462 - 2466, Reject str and bytes minor_tick_values before numeric conversion in x_axis() (python/xy/components.py:2462-2466), y_axis() (python/xy/components.py:2591-2595), and direct Figure.set_axis() calls (python/xy/_figure.py:323-330); preserve iterable numeric inputs and raise the appropriate validation error for string-like values.js/src/51_annotations.ts (1)
236-263: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle symbols 17/18 in the canvas renderer
_pointMarkStylemapshorizontal_lineandvertical_lineto 17/18, butxyCanvasScatterSymbolfalls through toctx.arc(...)here, so canvas renders them as filled circles while SVG/raster draw line markers.🤖 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/51_annotations.ts` around lines 236 - 263, Update xyCanvasScatterSymbol to handle symbols 17 and 18 explicitly as horizontal and vertical line markers, matching the existing line-rendering behavior for symbols 15 and 16. Ensure these cases draw the corresponding centered line and return before the fallback ctx.arc path.
🧹 Nitpick comments (1)
python/xy/_raster.py (1)
697-715: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the angle normalization out of the per-line loop.
normalized/quarter_flagdepend only onangle, andargsis built even on the quarter-turn branch where it is unused.♻️ Proposed refactor
block = _textblock.measure(text, size) radians = math.radians(float(angle)) + normalized = float(angle) % 360.0 + quarter_flag = ( + _TEXT_ROT_CW + if abs(normalized - 90.0) < 1e-9 + else _TEXT_ROT_CCW + if abs(normalized - 270.0) < 1e-9 + else 0 + ) for index, line in enumerate(block.lines): local_y = index * block.line_step line_x = x - local_y * math.sin(radians) line_y = first_baseline + local_y * math.cos(radians) - args = (line_x, line_y, anchor, size, color, line) - normalized = float(angle) % 360.0 - quarter_flag = (...) if quarter_flag and not italic and not bold: cmd.text(line_x, line_y, anchor | quarter_flag, size, color, line) elif angle or italic or bold: - cmd.text(*args, angle=angle, italic=italic, bold=bold) + cmd.text(line_x, line_y, anchor, size, color, line, angle=angle, italic=italic, bold=bold) else: - cmd.text(*args) + cmd.text(line_x, line_y, anchor, size, color, line)🤖 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 697 - 715, Move angle normalization and quarter_flag calculation before the block.lines iteration so they are computed once per text block, while preserving the existing quarter-turn selection behavior. Construct the text arguments only in branches that use them, avoiding args creation for the direct quarter-turn cmd.text call; update the loop around block.lines without changing rendering behavior.
🤖 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/51_annotations.ts`:
- Around line 567-586: Ensure every iteration of the annotation loop in the
rendering method pairs ctx.save() with ctx.restore(), including band and rule
branches that currently continue early for invalid ranges or off-plot positions.
Replace those early continues with guarded processing or restore the context
immediately before continuing, while preserving the existing behavior for valid
annotations.
In `@python/xy/pyplot/_artists.py`:
- Around line 1476-1486: Update the `_table_bottom_points` recomputation in
`Table.remove()` to scan the host axes’ `_entries`, using the same host-axes
selection logic as table entry storage (`self._axes._y2_of` when applicable,
otherwise `self._axes`). Preserve the existing filtering and maximum calculation
while replacing the direct `self._axes._entries` access.
In `@python/xy/pyplot/_mplfig.py`:
- Around line 715-730: Update the default vertical alignment in supylabel to
"center", while preserving support for explicit va and verticalalignment keyword
values and leaving the other label defaults unchanged.
- Around line 830-846: Preserve the distinct “outside right upper” placement
through the exporter path instead of reducing it to “upper right” in the figure
legend metadata. Update the figure legend handling around Legend construction
and _figure_legend assignment so layout/rendering can identify the outside-right
placement while retaining the existing tight-layout gutter reservation.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 5750-5762: Prevent the colWidths normalization in the table layout
logic from mutating the caller’s array. Update the widths conversion in the
colWidths branch to create an independent writable copy before the in-place
scaling performed when natural_layout is false, while preserving validation and
normalized values.
In `@tests/test_ui_issue_regressions.py`:
- Around line 715-729: The regression probe around the label collection and
containment assertions must require exactly three tick labels for both x and x2
axes, preventing vacuous passes when labels are missing. Extend the recorded
label bounds and assertions to verify every label’s left, top, right, and bottom
edges remain within the root rectangle, covering both horizontal and vertical
overflow.
---
Outside diff comments:
In `@js/src/51_annotations.ts`:
- Around line 236-263: Update xyCanvasScatterSymbol to handle symbols 17 and 18
explicitly as horizontal and vertical line markers, matching the existing
line-rendering behavior for symbols 15 and 16. Ensure these cases draw the
corresponding centered line and return before the fallback ctx.arc path.
In `@python/xy/_figure.py`:
- Around line 1320-1321: Keep minor-axis numeric data out of JSON by encoding
minor_tick_values and numeric minor_style fields as raw f32 buffer references in
python/xy/_figure.py at lines 1320-1321 and 1350-1351, while retaining f64
values for CPU-side use; update the corresponding wire-contract tests in
tests/pyplot/test_axis_tick_gallery_compat.py at lines 167-172, 180-186,
198-203, and 273-277 to decode buffer-backed values and assert decoded or absent
buffers, including promoted and date tick values.
In `@python/xy/_raster.py`:
- Around line 996-1010: Update the minor-tick rendering loops around the visible
x-axis loop and the corresponding y-axis loop to iterate over
_axis_tick_sides(...) rather than deriving one side from axis.get("side").
Compute each tick’s edge and inward/outward span per returned side, preserving
the existing major-tick side-aware behavior so axes configured on both sides
render minor ticks on both edges.
In `@python/xy/components.py`:
- Around line 2462-2466: Reject str and bytes minor_tick_values before numeric
conversion in x_axis() (python/xy/components.py:2462-2466), y_axis()
(python/xy/components.py:2591-2595), and direct Figure.set_axis() calls
(python/xy/_figure.py:323-330); preserve iterable numeric inputs and raise the
appropriate validation error for string-like values.
---
Nitpick comments:
In `@python/xy/_raster.py`:
- Around line 697-715: Move angle normalization and quarter_flag calculation
before the block.lines iteration so they are computed once per text block, while
preserving the existing quarter-turn selection behavior. Construct the text
arguments only in branches that use them, avoiding args creation for the direct
quarter-turn cmd.text call; update the loop around block.lines without changing
rendering behavior.
🪄 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: feb9e872-2a9b-4751-9344-96fe21545251
📒 Files selected for processing (57)
benchmarks/test_codspeed_pyplot.pyjs/src/00_header.tsjs/src/50_chartview.tsjs/src/51_annotations.tspython/xy/_arrowgeom.pypython/xy/_figure.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_textblock.pypython/xy/_validate.pypython/xy/components.pypython/xy/config.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_grid.pypython/xy/pyplot/_mathtext.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pypython/xy/pyplot/_rc.pypython/xy/pyplot/_state.pypython/xy/pyplot/_ticker.pyspec/api/styling.mdspec/design/wire-protocol.mdtests/pyplot/test_annotation_box_text_fidelity.pytests/pyplot/test_anscombe_gallery_regressions.pytests/pyplot/test_autoscale_margin_bar_regressions.pytests/pyplot/test_axes_charts.pytests/pyplot/test_axes_helpers.pytests/pyplot/test_axes_layout.pytests/pyplot/test_axis_tick_gallery_compat.pytests/pyplot/test_categorical_gallery_regressions.pytests/pyplot/test_contour_label_placement.pytests/pyplot/test_figure_decoration_compat.pytests/pyplot/test_figure_state.pytests/pyplot/test_frame_geometry.pytests/pyplot/test_gallery_layout_api_regressions.pytests/pyplot/test_gallery_text_pie_compat.pytests/pyplot/test_launch_compat.pytests/pyplot/test_layout_text_parity_fixes.pytests/pyplot/test_line_gallery_semantics.pytests/pyplot/test_multiline_chrome_layout.pytests/pyplot/test_nonlinear_scale_gallery_compat.pytests/pyplot/test_p3_option_contracts.pytests/pyplot/test_pie_annotation_grouped_repair.pytests/pyplot/test_pyplot_state_management.pytests/pyplot/test_quiver_display_invariants.pytests/pyplot/test_rc_chrome_contracts.pytests/pyplot/test_state_and_figs.pytests/pyplot/test_table_gallery_compat.pytests/pyplot/test_tick_side_rendering.pytests/test_png_export.pytests/test_static_client_security.pytests/test_svg_export.pytests/test_svg_text_metrics.pytests/test_ui_issue_regressions.py
🚧 Files skipped from review as they are similar to previous changes (29)
- python/xy/config.py
- tests/pyplot/test_axes_layout.py
- tests/pyplot/test_pyplot_state_management.py
- spec/design/wire-protocol.md
- tests/pyplot/test_figure_decoration_compat.py
- python/xy/_validate.py
- tests/pyplot/test_categorical_gallery_regressions.py
- tests/test_static_client_security.py
- tests/test_svg_text_metrics.py
- tests/pyplot/test_table_gallery_compat.py
- tests/pyplot/test_autoscale_margin_bar_regressions.py
- js/src/00_header.ts
- tests/pyplot/test_multiline_chrome_layout.py
- tests/test_svg_export.py
- tests/pyplot/test_layout_text_parity_fixes.py
- tests/pyplot/test_frame_geometry.py
- tests/test_png_export.py
- tests/pyplot/test_annotation_box_text_fidelity.py
- tests/pyplot/test_figure_state.py
- tests/pyplot/test_tick_side_rendering.py
- python/xy/pyplot/_state.py
- python/xy/_textblock.py
- python/xy/pyplot/_mathtext.py
- tests/pyplot/test_line_gallery_semantics.py
- tests/pyplot/test_gallery_layout_api_regressions.py
- python/xy/pyplot/_grid.py
- python/xy/_svg.py
- js/src/50_chartview.ts
- python/xy/pyplot/_axes.py
3 participants
Summary
This is the consolidated mainline roll-up for the compatibility work previously stacked across #337, #339, #340, #351, #355, and #356. PRs #336, #338, and #341 are already on
main; the remaining gallery, authored-style, axes-helper, and Y-axis-title fixes now land together through this PR.Pie and annotations (#337)
annotate(zorder=...), sharpconnectionstyle="angle"elbows, and connectors that leave the axes when their target is in bounds;Quiver and streamplot (#339)
angles="uv"from data-transformangles="xy", including inverted axes;Layout, text, and tables (#340)
loc="bottom"layout;Authored styles (#351)
Axes helpers (#355)
label_outer()GridSpec semantics;margins()getter, validation, and autoscale-state behavior;Browser Y-axis titles (#356)
Why
The previous paths contained several shortcuts that were individually plausible but did not compose: pie wedges exposed triangulation seams, quiver used nominal geometry rather than the live transform, streamplot flattened trajectory identity, single-line chrome measurement clipped multiline text, mosaic labels lost their spans, and tables were emitted in data coordinates. The roll-up keeps the technically correct behavior from all three branches while resolving the shared protocol, tick, locator, and layout code once against current
main.Visual comparisons
These are review-only artifacts from immutable evidence commits; none is part of this PR diff.
Pie and donut labels
Quiver gallery
Streamplot gallery
Multiline categorical ticks
Bar labels and MathText
Mosaic geometry
Axes-relative table
Multiline top-aligned text box
Current integration state
mainat55b0ca81d10d8cd27d2374dfa095f745aa86a778;e7f31cdb78fdd846382ddc930e32299f53b470cd;mainis contained in the consolidated head history;Verification
python/xy/static/standalone.jsbundle is not committed; no Chrome or Playwright process was launched locally;git diff --checkpassed;spec/matplotlib/compat.md,spec/matplotlib/compat-changelog.md,spec/matplotlib/shim-todo.md,docs/styling/chrome-slots.md, andspec/api/export.mdare absent from the main-relative diff;pr-assets/multiline-layout-gallery/remains untracked and untouched;CHANGELOG.mdcontains one terse summary of the consolidated fixes;Fresh GitHub Actions on this exact mainline head are the final integrated full-suite/browser/native gate.
Remaining approximations
Summary by CodeRabbit