Match Matplotlib best-legend placement [mpl compatibility]#274
Match Matplotlib best-legend placement [mpl compatibility]#274sselvakumaran wants to merge 2 commits into
Conversation
`legend(loc="best")` scored its candidate corners against a linear guess at the legend's size: `0.10 + 0.07 * rows` tall by `0.12 + 0.03 * max_label_len` wide. On the pdsh births figure that is a 0.30 x 0.17 box against a real 0.097 x 0.083 -- 3x too wide, 2x too tall. The inflated upper-right candidate reached back to x=0.70 and down to y=0.83, swallowing the late-summer peak (21 sampled vertices), while the inflated upper-left candidate stopped at x=0.30 and stayed empty. So upper left won a corner Matplotlib gives to upper right at badness 0. Score the box the exporters actually lay out instead. `_legend_layout` already derives it from the legend font size and the real label extents, so `_legend_footprint` reuses that rather than adding a second estimator; when #248's DejaVu glyph advances reach this stack the scoring inherits them for free. Three further departures from `Legend._find_best_position` came out of comparing against it directly: - Candidates were scored against the *raw data extent*, not the displayed view. The engine pads an autoranged view, so the corner a mark reaches on screen is not the corner it reaches in data space. Resolve `best` after the figure is built and ask it for its real ranges. - A 0.02 tie band over *mean fractional* occupancy flattened decisive gaps into ties. Matplotlib compares raw integer counts: it stops at the first empty candidate and prefers the lower location code otherwise. Count vertices, sum over series so a long series outweighs a short one, and keep the first minimum. - `borderaxespad` never reached scoring, so every candidate sat flush against the plot edge where Matplotlib insets the container on all four sides. Containment is now strict and unclipped, mirroring `Bbox.count_contains`; a point outside the view is outside every candidate rather than pinned to an edge. Measured against matplotlib 3.11.1 on the ten shapes in `tests/pyplot/test_best_legend_placement.py`: 7/10 match, from 6/10. Three shapes are fixed and two regressed -- those two matched by accident, because an oversized box happened to disqualify the same corner Matplotlib rejects for a real reason. The residual is dominated by xy's plot box being larger than Matplotlib's axes rect (564x428 against 496x369.6 at 640x480); substituting Matplotlib's rect lifts parity to 9/10, which `test_residual_divergence_is_attributable_to_the_plot_rect` pins. Placement is deliberately not scored against a geometry the renderers do not use. `test_center_right_legend_loc_reaches_spec` asserted the shim's own answer, not Matplotlib's: it expected center right where matplotlib 3.11.1 scores center left 20 against center right 36. Renamed and corrected. Separately, legend placement resolved `loc` by substring-matching "left"/"right" and "upper"/"lower", so a string matching neither half -- a typo, or an unresolved literal `"best"` -- silently produced a *centered* legend. One canonical table (`_validate.LEGEND_LOCATIONS`) now backs both the public `legend()` component and the shared static geometry, and anything outside it raises. This is a core-surface change: `xy.legend(loc=...)` outside pyplot now rejects an unrecognized location instead of centering it. Valid names keep their exact geometry, which `test_every_valid_loc_places_the_box_where_it_used_to` asserts against the plot box.
TEMPORARY: review evidence only, to be removed before merge (as in 3bd48f2, b8fb1d9, c90ad45). Referenced from the PR body by raw.githubusercontent.com URL. Three-up of the pdsh births figure -- matplotlib 3.11.1 reference, xy at this PR's base (#254 head 12d0cb9), xy with this PR. All three render the real `examples/pdsh/data/births.csv` through pandas' `.plot(ax=...)`, which passes `loc="best"` to both engines. Both xy panels came from one tree with one native core (dylib sha256 d79858dd12c2d4b0..., `src/` tree 69e331f, unmodified), toggling only the Python under test, so the only variable is this PR. The xy panels are downscaled from savefig's 2400x800 to the reference's 1200x400.
|
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
|
Summary
legend(loc="best")against the measured legend box from_legend_layout, not a linear guess from row count and label lengthbestafter the figure is built, against the displayed view rather than the raw data extentborderaxespadinto scoring, so candidates are inset from the plot edge asoffsetbox._get_anchored_bboxinsets themlocthrough one canonical table instead of substring-matchingleft/right, so an unresolved"best"can no longer silently centre the legendThis is stacked on #254 (
agent/legend-layout-gallery-compat).Root cause
_best_legend_locestimated the legend's footprint arithmetically:On
examples/pdsh/pdsh_04_09_text_and_annotation.ipynbcell 23 (realexamples/pdsh/data/births.csv, pandas passingloc="best"to both engines) that is a 0.30 x 0.17 candidate against a real 0.097 x 0.083 — 3x too wide and 2x too tall. The inflated upper-right candidate reached back to x=0.70 and down to y=0.83, swallowing the late-summer peak (21 sampled vertices); the inflated upper-left candidate stopped at x=0.30 and stayed empty. Upper left won a corner Matplotlib gives to upper right at badness 0.Comparing directly against
Legend._find_best_positionsurfaced three more departures, each independently able to pick the wrong corner:sin(x)over0..20in upper right where Matplotlib says lower left.borderaxespadnever reached scoringContainment is now strict and unclipped, mirroring
Bbox.count_contains— a point outside the view is outside every candidate instead of being pinned onto the nearest edge and counted there.Reusing #254's measurement rather than adding an estimator
_legend_layout(python/xy/_svg.py) already derives the real box from the legend font size and label extents, and is the geometry the SVG and raster exporters lay out with._legend_footprintcalls it. Nothing measures the legend twice, so when #248's0308c98DejaVu glyph advances reach this stack the scoring inherits them for free — checked by simulation, and this shape set stays at 7/10.Base choice. #254, not #248 or main. The scoring consumes two things only #254 provides: a font-relative measured box (
_legend_font_size,text_h, font-relative pads) andborderaxespadresolved to pixels on the wire (border_pad). #248 alone has neither. Nothing here rebases or force-pushes #248 or #254.Where the fix lives, and the one core change
The placement fix is entirely in the pyplot shim (
python/xy/pyplot/_axes.py) — no non-pyplot chart changes placement."best"is a shim concept and is still resolved before the wire.The substring-match guard is a core change, and it changes non-pyplot behaviour:
xy.legend(loc=...)now raisesValueErroron anything outside Matplotlib's ten anchored location names, where it previously accepted any string and centred the legend for anything containing neitherleftnorright."best"is rejected there too — placement is not computed at that layer. The shared static geometry (_legend_layout) refuses the same values as a second line of defence.This is a behaviour change for a caller who was passing a bogus
locand getting a centred legend. It is not a rendering change for anyone else: all ten valid names keep their exact geometry, asserted against the plot box intest_every_valid_loc_places_the_box_where_it_used_to. Nothing in the repo passed an invalid legendloc(theloc="top"/"bottom"uses areax.table, not legends), and the full suite is unchanged.Validation
Measured parity, matplotlib 3.11.1
Read out of
Legend._find_best_positionper figure, with its per-candidate badness. The three-up below is the reported figure; the table istests/pyplot/test_best_legend_placement.py.Honest accounting: net +1, not +3. Three shapes are fixed and two regressed. The two that regressed matched by accident — an oversized candidate box happened to disqualify the same corner Matplotlib rejects for a real reason. I am not claiming this generalises beyond the shapes measured.
What the residual three actually are
All three turn on one or two vertices, and two of them are not the scoring model. The measured box is pixel-accurate against Matplotlib — a one-row legend is 25.41 px here against Matplotlib's 25.4477 px — but xy lays out a 564x428 plot box where Matplotlib's default axes rect is 496x369.6 (640x480 figure), so the same box is ~12% narrower and ~14% shorter as a fraction, and knife-edge candidates fall the other way.
Substituting Matplotlib's axes rect as the denominator lifts parity 7/10 → 9/10, which
test_residual_divergence_is_attributable_to_the_plot_rectpins as executable evidence. I deliberately did not adopt that: it would place legends by a rect the exporters never use (a corner Matplotlib deems clear may not be clear at xy's actual size), it would hardcode Matplotlib's default subplot params and break undersubplots_adjust/add_axes, and it would need undoing once the frame-geometry work converges the two. It resolves for free then.The last one,
data_fills_axes, is a genuine tie: Matplotlib scores upper left and lower right both at 2 of 400 vertices and breaks it by location code. The shim's slightly different box separates the tie and takes the other member.Remaining departures from Matplotlib's badness, all recorded in
spec/api/styling.md: text and patch extents are counted as a single anchor point rather than a bbox overlap (and skipped when the coordinate is a date string); there is no segment-crossing term; occupancy is scored from at most 4096 strided vertices per series, weighted back up (§28).A test that was asserting the shim, not Matplotlib
test_center_right_legend_loc_reaches_specexpectedcenter right. matplotlib 3.11.1 scores that figure center left 20 against center right 36 — a decisive win for the left band, not a tie. The old 0.02 tie band over mean fractional occupancy is what flattened it. Renamed and corrected totest_center_band_legend_loc_reaches_spec.Suites
tests/pyplot: 574 passed, 76 failed — the same 76 failing on this PR's base, file for file (allFileNotFoundErroron the absentpython/xy/static/bundles). Zero regressions, zero newly-failing.tests/: 1289 passed, 125 failed, 5 collection errors — again identical to base, file for file. The corelegend(loc=...)validation breaks nothing.tests/pyplot/test_best_legend_placement.py; 15 of them fail on base.ruff check/ruff format --check(0.15.8, the pinned version) clean;pre-commit run --all-filesall hooks pass.ty check: 16 diagnostics, byte-identical to base — none new.Not verified
npm/nodeare unavailable here andpython/xy/static/does not exist, sonode js/build.mjscannot run. The browser client's_positionLegendstill substring-matchesloc; it is now unreachable with a bad value because core rejects it at construction, so I left the TS alone rather than ship an unbuildable edit. If a reviewer wants the client hardened symmetrically, that needs a tree with the toolchain.Three-up: Matplotlib reference / before / after
Committed separately as temporary review evidence, to be dropped before merge (as in 3bd48f2, b8fb1d9, c90ad45).
Both xy panels came from one tree with one native core, toggling only the Python under test, so this PR is the only variable. This PR touches no
src/. xy panels are downscaled from savefig's 2400x800 to the reference's 1200x400.Provenance was verified rather than assumed, because a Python-only before/after is exactly the shape that can silently measure the wrong tree:
xy.__file__resolves inside this worktree, not the shared.venv(whose_xy.pthpoints at the main checkout)._dyld_image_count/_dyld_get_image_name) reports exactly onelibxy_coreimage mapped, at this worktree'starget/release/libxy_core.dylib, sha256d79858dd12c2d4b0…— asked of the loader, not inferred fromXY_NATIVE_LIB.src/tree69e331f4at HEAD and unmodified in the working tree, so the native core is provably constant across both panels — the correct control for a Python-only change (asserting the two dylibs differ would be the confound here, not the control).ABI_VERSION = 40) and a byte-identical ABI-40 copy placed outside the worktree, which trips the path assertion rather than the ABI net.The other visible differences in the xy panels are not this PR's and are not claimed as fixed: the x tick labels (
2012/Mar 2012against Matplotlib's centred month names), the savefig 2x scale (#206), and the wider plot rect discussed above.