Skip to content

Improve pyplot legend and dash parity - #42

Merged
FarhanAliRaza merged 5 commits into
mainfrom
mpl-shim
Jul 15, 2026
Merged

Improve pyplot legend and dash parity#42
FarhanAliRaza merged 5 commits into
mainfrom
mpl-shim

Conversation

@FarhanAliRaza

Copy link
Copy Markdown
Contributor

No description provided.

@Alek99 Alek99 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff (Python shim, payload/exporters, JS client) and dug into the CI failures.

CI status — not this PR's fault

Both failing checks (Test test_pyplot_build_tracks_declarative[10000-60-1.6] and CodSpeed test_first_payload_hexbin_core_2d) are pre-existing regressions from #37 — they fail identically on main (5a58834, eb0c3f7, and still on c36e908). Fixes are in #46. I verified locally that this branch merged with #46 passes all 558 pyplot/plot-family tests plus both previously-failing gates. Once #46 lands, update the branch and CI should go green. One thing to watch: python/xy/static/*.js are committed build artifacts and git will happily automerge them — after rebasing, re-run node js/build.mjs (for the current merge I verified the automerge result matches a fresh rebuild exactly).

What I verified works

  • _mpl_dash scaling is right: -- at linewidth=2 ships [10.28, 4.44] = (3.7, 1.6) × lw × dpi/72, matching Matplotlib's scale_dashes + point sizing; line width point-scaling is consistent between the trace and the frozen legend item (no double scaling).
  • loc parsing (independent h/v anchors with "center" fallback) is consistent across JS, SVG, and raster, and the single-transform fix in _legendBox is correct — the old code's two transform: declarations would indeed clobber each other.
  • _best_legend_loc stays bounded (512 samples/entry) and the ±0.02 tie band correctly preserves Matplotlib's candidate preference order.
  • Second-legend payload shape (extra_legends with explicit items) renders in all three renderers and survives primary-legend suppression; good test coverage for both.

Minor findings

  1. Axes.add_artist (_axes.py): artist._parent = host is reassigned after the Legend already froze _options and _items against the constructing axes in __init__ — the assignment has no effect. If a Legend(ax1, ...) is added to ax2 with a different dpi/rcParams state, the swatch scaling silently comes from ax1. Either drop the dead assignment or recompute options on attach.
  2. JS _legendBox: only the primary legend registers as this._legend, so extra legend boxes never get their max-height refreshed by _resize — a window resize can leave a tall second legend overflowing the plot. Consider tracking all boxes.
  3. JS line swatch: it.style?.width || 1.5 treats an explicit lw=0 as unset and draws a 1.5px handle; ?? 1.5 (plus letting 0 render nothing) is closer to mpl.
  4. Legend.__init__: zip(handles, labels, strict=False) silently truncates a handles/labels length mismatch — Matplotlib warns. A warnings.warn here would match shim policy of not deciding silently. Handles without an _entry (e.g. Text) are also silently dropped.
  5. Nit: in _best_legend_loc the final return "upper right" is unreachable — best is the min of scores, so the first candidate within the band always returns.

None of these block merging; 1 and 2 are worth a follow-up. Nice work on keeping the three renderers' legend geometry in lockstep — the shared item shape makes the parity easy to trust.

- Legend now (re)freezes options and swatch scaling via _attach() when
  added to an axes, so a legend constructed against one axes but
  attached to another picks up the host's dpi/rcParams (review #1).
- Legend.__init__ warns on handles/labels length mismatch and on
  handles without a legend entry instead of dropping them silently
  (review #4).
- JS tracks every legend box (not just the primary) so _resize
  refreshes max-height on extra legends too (review #2).
- JS line swatch uses ?? so an explicit lw=0 draws nothing instead of
  falling back to 1.5px (review #3).
- Drop unreachable return in _best_legend_loc (review #5).
@codspeed-hq

codspeed-hq Bot commented Jul 15, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 80 untouched benchmarks
⏩ 1 skipped benchmark1


Comparing mpl-shim (d52daf5) with main (b8aaa71)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

@FarhanAliRaza
FarhanAliRaza merged commit bba11a0 into main Jul 15, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants