Render the pyplot axes frame on the position it reports [mpl compatibility] - #272
Closed
sselvakumaran wants to merge 2 commits into
Closed
Render the pyplot axes frame on the position it reports [mpl compatibility]#272sselvakumaran wants to merge 2 commits into
sselvakumaran wants to merge 2 commits into
Conversation
The shim reported Matplotlib's figure.subplot.* rectangle from get_position() but never told the renderers about it, so they fell back to label-aware default margins: a default 640x480 figure drew its frame 13.8% too wide with its top edge 48px too high. get_position() was also not grid-aware, collapsing every panel of an n x m grid onto the 1x1 default rectangle. Resolve the axes rectangle once, in Figure._axes_rect(), and use it for both what scripts read and what the exporters place. Derive the chart padding from that rectangle, subtracting the gutters the renderers reserve outside it (title band, top-side x axis, secondary-y band) so a title still draws above the axes without moving it. The grid compositor adds the same gutters to the panel it allocates, which also lets the equal-aspect solve measure the real plot rect. Separately, alpha-composite absolutely placed panels instead of pasting them opaquely. A panel is wider than its gridspec cell because its tick labels live outside the plot rect, so each column erased the one to its left and a dense subplot grid rendered only its last column. Colorbar-bearing axes keep the label-aware margins: Matplotlib's colorbar() takes its strip out of the parent axes rectangle while the renderers reserve it outside the padding.
Three-up Matplotlib 3.11.1 / before / after renders of four figures: a single-axes line plot, a 2x3 sharex/sharey grid, cell 29 of examples/pdsh/pdsh_04_07_customizing_colorbars.ipynb (8x8 imshow over the real sklearn load_digits images), and cell 16 of examples/pdsh/pdsh_04_10_customizing_ticks.ipynb (5x5 imshow over the real sklearn fetch_olivetti_faces images, wspace=hspace=0). Temporary — for PR review only, to be removed before merge.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #206 (
agent/fix-pyplot-savefig-scale) and targets that branch — it changes the raster scale on the samepyplot/_grid.py/pyplot/_mplfig.pycode path, so this builds on it rather than racing it.Summary
Figure._axes_rect(), and use it for both what scripts read and what the exporters placeAxes.get_position()grid-aware: a panel reports its gridspec cell under the live SubplotParams, so ann x mgrid returnsn * mdistinct boxes instead of one repeated 1x1 defaultmatshowin a grid stops solving against a plot box 26 px taller than the one drawnDeliberately narrow: no autoscale, tick-spacing, colorbar-placement, or legend work (#240, #244, #246, #248/#254 own those).
Root causes
Two, both in the shim.
1. The shim never told the renderers which rectangle it had promised.
Axes.get_position()returnedfigure.subplot.*correctly, but_build_chartpassedpadding=None, so_svg.layout()(and the browser'sChartView._layout()) fell back to label-aware default margins —62/14/10/42px at ordinary export sizes. Those margins have nothing to do with the SubplotParams frame, so a default 640x480 figure drew its frame atx 0.0969..0.9781(w 0.8812, 13.8 % too wide) with its top edge aty 0.0208instead of0.1208. Same cause forget_position()itself:python/xy/pyplot/_axes.py:2595returnedself._figure_rect or (0.125, 0.11, 0.775, 0.77), and asubplots()panel has no_figure_rect, so all 64 panels of an 8x8 grid reported one identical box.2. Panel tiles were pasted opaquely. A free-form panel is deliberately larger than its gridspec cell — its tick labels and title live outside the plot rect — and
stitch_pngpasted each tile withcanvas[...] = tile, background included. Each column therefore erased the one to its left, and only the rightmost column of a dense grid survived.Worth stating precisely, because it narrows what reviewers need to check: the grid panels' plot rects were already sub-pixel correct before this PR. I measured them against
Axes.get_window_extent()and the 2x2 / 5x5 / 8x8 / ratio /subplots_adjustcases were all within 0.75 px on the base branch. C3 was entirely root cause 2, not a geometry error. The geometry errors were the single-axes cases (and, once panels are pinned,matshow/twinxpanels).Core boundary
No core changes. The diff is four files, all under
python/xy/pyplot/plus tests and spec:python/xy/_svg.py,_raster.py,js/src/, andsrc/*.rsare untouched, so no non-pyplot chart changes behavior. The shim now supplies an explicitpaddingon a path where it previously supplied none;paddingis an existing, already-documentedxy.chart()option and the renderers' handling of it is unchanged.Rendered-frame accuracy
Absolute plot rects from
_svg.layout()(the single resolver both static exporters use), against Matplotlib 3.11'sAxes.get_window_extent(). Max|delta|in px over(x0, y0, w, h)across all panels:add_axesrectsubplots_adjustwspace=hspace=0matshowtwinximshowstripsget_position()output is now byte-identical to Matplotlib 3.11 for 1x1, 2x2, 8x8 +subplots_adjust, 2x3 + ratios, and 5x5 + zero spacing (diffof both engines' bounds: identical, including Matplotlib's own0.10999999999999999bottom edge).Visual comparison
Columns are Matplotlib 3.11, the branch base (#206), and this PR.
PDSH impact
Global SSIM against the Matplotlib 3.11 render of the same cell, plus ink coverage (mpl / before / after):
imshowofload_digitsimshowoffetch_olivetti_facessharex/shareygridRendered from the real datasets (
sklearn.datasets.load_digits/fetch_olivetti_faces), not substitutes.Honest reading of that table:
Regression sweep
24 dual-engine figures (line/scatter/bar/barh/hist/fill_between/log/imshow/matshow/twinx/pie/errorbar/step/
axis("equal"), four figure sizes, six grid shapes,add_axes+ inset), scored by global SSIM against Matplotlib 3.11:Largest gains
imshow+0.371,matshow+0.347, 320x240 figure +0.205. The only negative movement in the whole sweep isadd_axes+ inset at -0.0014 — below the sampling noise of this metric, and its measured plot rects are 0 px off Matplotlib both before and after, so I read it as flat rather than a regression. Nothing else moved down.Known limitation (deliberate)
An axes carrying a colorbar keeps the old label-aware margins:
_frame_padding()returnsNonewhenself._colorbar is not None. Matplotlib'scolorbar()takes its strip out of the parent axes rectangle, while the renderers reserve it outside the padding; pinning the frame without reconciling that would push the colorbar off the canvas. Reconciling it is colorbar-placement work (#244/#246), andspec/matplotlib/compat.mdnow records the gap explicitly.Validation
2183 passed, 12 skippedacross the full suite (tests/), up from2168 passed— the 15 new tests intests/pyplot/test_frame_geometry.pytests/test_framing.py::test_javascript_{decodes_python_golden_frame_without_payload_copies,rejects_malformed_and_unaligned_frames}, bothFileNotFoundError: 'node'— no Node toolchain in this environment, unrelated to this change14 failed, 1 passedwith the three source files reverted, so they pin real behavior rather than snapshotting ittests/pyplot/test_axes_layout.pyupdated, both because the new geometry is more Matplotlib-correct:axis("equal")now yields(-0.31967741935483873, 1.3196774193548388), which is Matplotlib 3.11's own answer for that figure to 15 digits (previously±1.3348, solved against the wrong plot rect); andget_position().boundsnow carries Matplotlib's own0.11rounding so it needspytest.approxuv run --no-project --with pre-commit pre-commit run --all-files— ruff check, ruff format, docs-app codespell all passruff check .clean;ruff format --check .cleanty check python/xy/pyplot/—Found 14 diagnostics, identical count and locations before and after (all in pre-existing_colors.py/_artists.py/_axes.py:1283-1477code)python3 scripts/abi_smoke.py—121 checks passed<svg>elements, 64<image>marks, 0 full-panel background rects — the SVG path was already transparent and never had the clobbering bug, so it needed no changeNot verified
npmandnodeare not installed in this environment andpython/xy/static/has no bundles, sonode js/build.mjscannot run and the interactive client could not be exercised. No TypeScript was changed, andChartView._layout()was read to confirm it applies the same title/top-axis/secondary-y/colorbar reservations outsidepaddingthat_svg.layout()does — but that is code reading, not a render.Measurement provenance
This is a Python-only PR —
git diff --name-onlytouches nothing undersrc/, andgit rev-parse HEAD:srcis46eba622…on both this branch and its base. So the native core is a constant across the before/after panels, and both panels deliberately share one dylib (4301be03…) rather than two.That control is load-bearing here, because the obvious failure mode is not loud: the main checkout's prebuilt
libxy_core.dylib(4509266e…) reports ABI 37, and this tree's_native.pyalso expects 37, so a stray load of the wrong library passes the ABI gate silently instead of raisingImportError. I therefore verified in-process which image was actually mapped — asking dyld (_dyld_image_count/_dyld_get_image_name) for the process's Mach-O images and asserting exactly onelibxy_coreis mapped, that it resolves inside this worktree, and that its sha256 is4301be03…— rather than trustingXY_NATIVE_LIBto have won. The check has a working negative control: pointed at the main checkout's dylib it fails on the mapped-path assertion, and notably gets that far without anImportError, confirming the silent-swap mode was real for this tree.Every geometry, SSIM, and ink number above was re-measured with that assertion active and reproduced identically, as did the comparison sheet (sha256
2c03d918…). Renders ran through a worktree-local venv with no_xy.pth, withPYTHONPATHandXY_NATIVE_LIBboth pointed at this worktree;xy.__file__was asserted under it on every run.