Fix artist state and marker compatibility [mpl compatibility] - #338
Conversation
|
Warning Review limit reached
Next review available in: 45 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdds bounded Matplotlib authored-marker support across rendering backends and legends. It also revises subplot activation, image data ownership, static layout cache invalidation, and prop-cycle compatibility behavior. ChangesAuthored scatter markers
Pyplot state and image lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PyplotMarkerNormalization
participant ScatterTrace
participant ChartView
participant SVGAndRasterRenderers
PyplotMarkerNormalization->>ScatterTrace: encode marker_path or marker_glyph
ScatterTrace->>ChartView: provide authored marker style and CPU channels
ChartView->>ChartView: queue and draw authored overlay markers and legends
ScatterTrace->>SVGAndRasterRenderers: provide authored marker style
SVGAndRasterRenderers->>SVGAndRasterRenderers: render glyphs or contours
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/pyplot/_artists.py (1)
617-629: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
set_data(x, y, z)leaves a stale extent behind.
_set_axes_image_data()rebuilds the image with the oldstate["kwargs"], andget_extent()returns the cachedentry["extent"]before looking atkwargs["x"/"y"]. That means the 3-arg path ignores the new coordinates for positioning. Update the extent handling here and add a regression test forset_data(x, y, z).🤖 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/_artists.py` around lines 617 - 629, Update the three-argument path in Artist.set_data so the cached entry extent is recalculated or invalidated after replacing kwargs["x"] and kwargs["y"], ensuring _set_axes_image_data and get_extent use the new coordinates. Add a regression test covering set_data(x, y, z) and verifying the image extent reflects the supplied x and y values.
🤖 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 3919-3923: The density-tier sample overlay is skipped by both
rendering paths because _drawPoints rejects authoredMarker objects and
_drawAuthoredScatterMarkers excludes density traces. Update
js/src/50_chartview.ts:3919-3923 to allow sample-overlay objects through
_drawPoints, or update js/src/51_annotations.ts:192-197 to resolve each density
trace’s current overlay using the same lodSampleForView selection as
_drawDensitySample and draw it; ensure the chosen path renders authored
density-tier markers without silently dropping them.
In `@js/src/51_annotations.ts`:
- Around line 203-222: Update the authored-marker rendering path around
_authoredScatterRgba and the per-point loop to honor per-point opacity,
artist_alpha, stroke_width, and symbol style channels instead of only
trace-level values. Retain CPU-readable copies of these style-channel columns
when authoredMarker is enabled, mirroring g._cpu.size, or perform a
once-per-frame styleBuf readback using the existing _filterScatterRows pattern,
then use those values for each point while preserving scalar fallbacks.
- Around line 169-190: Update the continuous-color branch of
_authoredScatterRgba so buildLutData is not called for every point; reuse a LUT
cached per colormap, consistent with this._lutCache used by the GPU path. Ensure
each trace’s colormap is built at most once and retain the existing slot
calculation and fallback behavior.
In `@python/xy/_raster.py`:
- Around line 1554-1628: Update _emit_authored_scatter to resolve a per-marker
stroke color using _trace_paint_rgba(t, "stroke", n, color, read), matching the
ordinary scatter path. Use that resolved color when stroking filled marker_path
contours, while preserving fill as the stroke color for unfilled/line-style
markers and retaining existing width and fill behavior.
In `@python/xy/pyplot/_axes.py`:
- Around line 5836-5865: Update endpoint-marker placement in _chart_children for
`@hline` and `@vline` to use the finalized axis domains selected by _build_chart,
including _fully_sticky_domain for sticky axes, instead of independently calling
_auto_domain. Reuse the established resolved-domain path or defer marker
construction until domain resolution completes, while preserving the existing
span_start/span_end calculations.
In `@python/xy/pyplot/_mplfig.py`:
- Around line 215-238: Update the existing-subplot branch of activate_subplot to
handle sharex and sharey through the same axis-sharing logic as add_subplot,
rather than passing them to existing.set(). Remove these sharing kwargs before
calling existing.set() and wire the requested shared axes using the established
subplot-sharing mechanism; preserve normal kwargs handling.
- Around line 1503-1509: Mark every axes created or returned by subplot_mosaic
as claimed by setting _subplot_claimed = True in its axes-construction loop,
matching make_axes_grid and the existing claim invariant. Update the
subplot_mosaic path around _axes_at so each mosaic cell is protected from later
subplot reuse.
---
Outside diff comments:
In `@python/xy/pyplot/_artists.py`:
- Around line 617-629: Update the three-argument path in Artist.set_data so the
cached entry extent is recalculated or invalidated after replacing kwargs["x"]
and kwargs["y"], ensuring _set_axes_image_data and get_extent use the new
coordinates. Add a regression test covering set_data(x, y, z) and verifying the
image extent reflects the supplied x and y values.
🪄 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: d8770b0d-63dc-4136-83a5-84e3d975b436
📒 Files selected for processing (24)
js/src/50_chartview.tsjs/src/51_annotations.tsjs/src/55_marks.tspython/xy/_fontmetrics.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/components.pypython/xy/marks.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_markers.pypython/xy/pyplot/_mathtext.pypython/xy/pyplot/_mplfig.pyscripts/gen_font.pyspec/api/styling.mdspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdsrc/font.rstests/pyplot/test_artist_mutations.pytests/pyplot/test_figure_state.pytests/pyplot/test_frame_geometry.pytests/pyplot/test_marker_fidelity.pytests/pyplot/test_pyplot_state_management.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)
js/src/45_lod.ts (1)
906-943: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale
_cpuStyle/_cpuStrokecan leak across replies with different point counts.
d.colorMode/d.sizeModeare explicitly reset (= 0) before their conditional branches (lines 870, 891), so a reply without a channel this time correctly disables the old cached data.d._cpuStyleandd._cpuStrokehave no equivalent reset: if a laterlodApplyDrillcall for the same traceddoesn't satisfy theifcondition (e.g., the encoder omits an all-default per-point channel, or the stroke mode changes away from"direct_rgba"), the previous array stays attached tod.The CPU-side consumer,
_drawAuthoredScatterMarkersinjs/src/51_annotations.ts, only guards with a length check (g._cpuStyle.length >= styleOffset + 4,g._cpuStroke.length >= (sourceIndex + 1) * 4), not a presence/version flag. If the new window's point count is smaller than (or equal to) the stale array's, that length check passes and silently applies opacity/artist-alpha/stroke-width/symbol/stroke-color values from an entirely different reply/window to the current points.🐛 Proposed fix: reset the CPU caches when the channel disappears
copy("opacity", 0); copy("artist_alpha", 1); copy("stroke_width", 2, view.dpr); copy("symbol", 3); d._cpuStyle = values; if (!d.styleBuf) d.styleBuf = gl.createBuffer(); d.styleBuf._fcType = gl.FLOAT; gl.bindBuffer(gl.ARRAY_BUFFER, d.styleBuf); gl.bufferData(gl.ARRAY_BUFFER, values, gl.STATIC_DRAW); + } else { + d._cpuStyle = undefined; } if (upd.stroke && upd.stroke.mode === "direct_rgba") { const values = view._asU8(buffers[upd.stroke.buf]); d._cpuStroke = values; if (!d.strokeBuf) d.strokeBuf = gl.createBuffer(); d.strokeBuf._fcType = gl.UNSIGNED_BYTE; gl.bindBuffer(gl.ARRAY_BUFFER, d.strokeBuf); gl.bufferData(gl.ARRAY_BUFFER, values, gl.STATIC_DRAW); + } else { + d._cpuStroke = undefined; }🤖 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/45_lod.ts` around lines 906 - 943, Reset d._cpuStyle before the style-channel conditional and d._cpuStroke before the direct_rgba stroke conditional in lodApplyDrill, so each reply clears cached CPU data when its corresponding channel is absent or changes mode. Preserve the existing assignments when channels are present, ensuring _drawAuthoredScatterMarkers cannot consume arrays from a previous window.
🤖 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 `@js/src/45_lod.ts`:
- Around line 906-943: Reset d._cpuStyle before the style-channel conditional
and d._cpuStroke before the direct_rgba stroke conditional in lodApplyDrill, so
each reply clears cached CPU data when its corresponding channel is absent or
changes mode. Preserve the existing assignments when channels are present,
ensuring _drawAuthoredScatterMarkers cannot consume arrays from a previous
window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62595e58-e033-41c6-b216-909df9b78208
📒 Files selected for processing (13)
js/src/45_lod.tsjs/src/50_chartview.tsjs/src/51_annotations.tspython/xy/_raster.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_mplfig.pyspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdtests/pyplot/test_artist_mutations.pytests/pyplot/test_marker_fidelity.pytests/pyplot/test_pyplot_state_management.pytests/test_static_client_security.py
🚧 Files skipped from review as they are similar to previous changes (7)
- spec/matplotlib/compat-changelog.md
- python/xy/pyplot/_artists.py
- spec/matplotlib/compat.md
- python/xy/_raster.py
- js/src/50_chartview.ts
- python/xy/pyplot/_mplfig.py
- python/xy/pyplot/_axes.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/pyplot/test_rc_color_export_contracts.py`:
- Around line 51-65: Update the test around the pyplot setup and assertions to
restore the global matplotlib state after plt.style.use("dark_background"),
including when an assertion fails. Use the repository’s existing reset fixture
if available; otherwise wrap the test body in try/finally and restore the prior
rcParams/theme state in finally.
🪄 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: ddd6a5ee-a5cb-470b-9664-e8238eac9e78
📒 Files selected for processing (4)
python/xy/pyplot/_rc.pyspec/matplotlib/compat-changelog.mdspec/matplotlib/compat.mdtests/pyplot/test_rc_color_export_contracts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/matplotlib/compat.md
…ion-cycle-fixes # Conflicts: # js/src/50_chartview.ts # python/xy/_raster.py # python/xy/_svg.py # python/xy/pyplot/_axes.py
3 participants
Summary
This groups related Matplotlib/PDSH artist-state fixes:
plt.axes()creates a fresh current axes whileplt.subplot()retains Matplotlib-style reuse;set_data()still reprocesses the new data;s=areas;Native XY scatter keeps its existing fixed-swatch legend behavior; the size-preservation opt-in is confined to the Matplotlib shim.
Why
The previous state layer reused axes too broadly and retained image input by reference. Marker conversion also collapsed authored marker paths to generic circles, while legend derivation copied only trace style and discarded the existing scalar size channel. Static exports could additionally retain single-panel cached geometry after switching to an overlay layout.
Visual comparisons
Review-only images from immutable evidence commit
a842688; none is part of this PR diff.Authored markers
Explicit scatter legend sizes
PDSH-style image ownership
Verification
git diff --checkpassed.Per the low-resource verification policy, no local browser, native build, or full suite was run. GitHub CI remains the final integrated gate.
Summary by CodeRabbit