Skip to content

Fix log scale ticks, grids, and nonpositive handling [mpl compatibility] - #336

Merged
Alek99 merged 12 commits into
mainfrom
agent/gallery-log-scale-compat
Jul 27, 2026
Merged

Fix log scale ticks, grids, and nonpositive handling [mpl compatibility]#336
Alek99 merged 12 commits into
mainfrom
agent/gallery-log-scale-compat

Conversation

@sselvakumaran

@sselvakumaran sselvakumaran commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

This makes the selected Matplotlib log-scale gallery path technically correct across browser, SVG, and raster output:

  • emit separate major/minor tick payloads and styles instead of flattening both tiers;
  • match Matplotlib log subdivisions and minor-grid geometry;
  • support base-2 locator/label behavior;
  • distinguish nonpositive="mask" from nonpositive="clip", including WebGL;
  • preserve explicit base-2 labels through final axis materialization;
  • fix the errorbar-cap and constrained-suptitle geometry exposed by the same exact gallery example.

The renderer protocol advances to v9 because minor tick values/styles are now explicit wire data.

Source behavior

The shim now follows Matplotlib's LogLocator/formatter separation: major ticks own labels, minor ticks own subdivisions and optional grid lines, and nonpositive masking is not silently converted into clipping.

Visual comparison

The image is stored on immutable review-only evidence commit 3677799, not in this PR's diff or pr-assets.

Matplotlib / XY before / XY after

Before, XY emitted only major grids for the semilogy/loglog panels. After, the exact gallery output carries the same minor subdivisions and hierarchy as Matplotlib.

Exact gallery verification

The rebased branch reran the exact adapted scales/log_demo.py and produced all three figures:

  1. major/minor log grids;
  2. base-2 scaling and labels;
  3. mask versus clip nonpositive behavior.

The rebased outputs are byte-identical to the reviewed post-fix artifacts.

Verification

Current integration state:

  • current main base 9fdc236; head d4aa8db;
  • 49 focused branch tests passed, including 17 exact reviewer regressions;
  • 15/15 final tick/grid CI-regression tests passed after the last expectation update;
  • Ruff lint and format checks pass;
  • zero unresolved review threads;
  • no Matplotlib compatibility-ledger or review-asset changes in the net PR diff;
  • GitHub reports the PR mergeable and non-draft.

The exact scales/log_demo.py evidence above and the earlier 799-test pyplot run predate the final current-main merge; they are retained as review evidence rather than presented as fresh post-merge runs. No local Chrome/Playwright capture, full suite, or native build was rerun under the low-resource policy. Fresh GitHub Actions are the final integrated gate.

Summary by CodeRabbit

  • New Features
    • Added configurable minor axis ticks and independent minor grid styling across interactive, SVG, and raster charts.
    • Added clip or mask handling for non-positive values on logarithmic axes.
    • Added horizontal and vertical line marker shapes, including Matplotlib marker translations.
    • Expanded nonlinear scale support, tick formatting, aspect controls, and error-bar cap rendering.
  • Bug Fixes
    • Improved log-axis limits, tick expansion, grid behavior, constrained layouts, and marker consistency.
  • Documentation
    • Updated styling, marker, axis, and protocol documentation for the new capabilities.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Protocol v9 adds explicit minor tick values/styles and configurable log nonpositive handling. Pyplot gains nonlinear scales, tick expansion, log-aware aspect behavior, and new formatters. Browser, SVG, and raster renderers draw minor gridlines and ticks, while line markers, errorbar caps, layout, and compatibility tests are extended.

Changes

Axis scale, ticker, grid, and layout behavior

Layer / File(s) Summary
Pyplot scales, tickers, and layout state
python/xy/pyplot/_axes.py, python/xy/pyplot/_ticker.py, python/xy/pyplot/_rc.py
Adds nonlinear locators/formatters, log-aware transforms and aspect calculations, explicit tick-domain expansion, independent minor-grid state, and rc defaults for minor tick styling.
Axis configuration and protocol contract
python/xy/components.py, python/xy/_figure.py, python/xy/config.py, js/src/00_header.ts, spec/...
Carries minor tick values/styles and log nonpositive policies through axis configuration and protocol v9 specifications.
Live, SVG, and raster rendering
js/src/40_gl.ts, js/src/50_chartview.ts, python/xy/_svg.py, python/xy/_raster.py
Applies log masking or clipping and renders styled minor gridlines and tick marks across live canvas, SVG, and raster outputs.
Line markers and errorbar cap artists
python/xy/marks.py, python/xy/pyplot/_plot_types.py, python/xy/pyplot/_artists.py, src/raster.rs
Adds horizontal and vertical line symbols and renders errorbar caps as point-based overlay artists with container cleanup support.
Compatibility and regression coverage
tests/pyplot/*, tests/test_ui_issue_regressions.py, docs/*, spec/*
Covers nonlinear scales, minor grids, protocol behavior, marker fidelity, errorbar caps, tight-layout suptitles, and browser rendering.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • reflex-dev/xy#337: Shares the protocol v9 update and cross-renderer changes for log handling, minor ticks, and line symbols.
  • reflex-dev/xy#340: Updates overlapping JavaScript log-axis mode and coordinate handling.
  • reflex-dev/xy#341: Updates the shared protocol version constant from 8 to 9.

Suggested reviewers: alek99

Sequence Diagram(s)

sequenceDiagram
  participant Pyplot
  participant Figure
  participant AxisSpec
  participant Renderer
  Pyplot->>Figure: configure scales, ticks, styles, and nonpositive policy
  Figure->>AxisSpec: normalize and serialize axis options
  AxisSpec->>Renderer: send protocol v9 fields
  Renderer->>Renderer: map values and draw major/minor axis elements
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 24.83% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main changes: log-scale tick/grid behavior and nonpositive handling for Matplotlib compatibility.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/gallery-log-scale-compat

Comment @coderabbitai help to get the list of available commands.

@codspeed-hq

codspeed-hq Bot commented Jul 27, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 103 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing agent/gallery-log-scale-compat (bcc04cc) with main (63d52e4)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

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/_axes.py (1)

5787-5799: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Docstring is stale for the new minor tier. The body now accepts "minor", but the docstring still says which accepts "major"/"both".

📝 Proposed fix
-        ``visible=None`` toggles the current state; ``which`` accepts
-        ``"major"``/``"both"`` and ``axis`` restricts to ``"x"`` or ``"y"``.
+        ``visible=None`` toggles the current state; ``which`` accepts
+        ``"major"``/``"minor"``/``"both"`` and ``axis`` restricts to ``"x"``
+        or ``"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 5787 - 5799, Update the grid method
docstring to document that the which parameter accepts "major", "minor", or
"both", matching the validation in the grid implementation; leave the behavior
unchanged.
🧹 Nitpick comments (6)
python/xy/pyplot/_axes.py (2)

653-660: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider mirroring set_major_locator's validation. set_minor_locator accepts any object; _apply_tickers silently skips it when it lacks tick_values(), so a typo'd argument disappears instead of raising.

♻️ Optional parity fix
     def set_minor_locator(self, locator: Any) -> None:
+        if not hasattr(locator, "tick_values"):
+            raise TypeError("set_minor_locator() requires a Locator with tick_values()")
         host, key = self._ticker_slot()
🤖 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 653 - 660, Update set_minor_locator
to validate the supplied locator with the same validation used by
set_major_locator before storing it in host._tickers. Reject objects that do not
provide the required tick_values() behavior so invalid arguments raise
immediately rather than being skipped by _apply_tickers; preserve the existing
storage and invalidation flow for valid locators.

1242-1245: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

Per-_add domain recomputation can get quadratic. _expand_domain_to_ticks calls _auto_domain, which rescans every entry array; running it on each _add makes plotting N series O(N·data) once ticks were set before plotting. Deferring the expansion to _build_chart (where the other domain materialization happens) would avoid the repeated scan.

🤖 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 1242 - 1245, Remove the per-series
`_expand_domain_to_ticks` invocation from the `_add` path’s axis loop. Defer
tick-based domain expansion until `_build_chart`, alongside the existing domain
materialization, ensuring each applicable axis is expanded once after all series
are added and preserving the `_tick_expanded_domains` checks.
tests/pyplot/test_axis_tick_gallery_compat.py (1)

61-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider asserting the y minor tier stays untouched. The test name claims dimension isolation, but only the x minor style and the y major style are checked.

💚 Suggested addition
     assert ax._axis_props("x")["minor_style"]["grid_color"] == "rgb(230,230,230)"
+    assert ax._axis_props("y")["minor_style"]["grid_color"] == "transparent"
     assert ax._axis_props("y")["style"]["grid_color"] == "`#1f77b4`"
🤖 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_axis_tick_gallery_compat.py` around lines 61 - 63, Extend
the dimension-isolation assertions in the test to verify that the y-axis minor
tier remains unchanged after configuring the x-axis minor grid. Use the existing
ax._axis_props("y") structure and its minor_style grid_color value, while
preserving the current x-minor and y-major assertions.
python/xy/pyplot/_ticker.py (1)

513-516: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

_locs is stored but never read. set_locs exists so _apply_tickers can call it, but __call__ ignores the locations entirely — either use it (Matplotlib's LogitFormatter decides minor labeling from _locs) or drop the field and keep set_locs a no-op.

🤖 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/_ticker.py` around lines 513 - 516, Update the formatter
implementation around set_locs and __call__ so the stored _locs value affects
minor-label decisions consistently with Matplotlib’s LogitFormatter; otherwise
remove the unused _locs storage while preserving set_locs as a no-op for
_apply_tickers compatibility. Choose one approach and ensure the resulting
behavior no longer stores locations that are never read.
python/xy/pyplot/_rc.py (1)

181-190: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

grid.alpha only gets a non-negative check. Values above 1 flow into grid_opacity and out to the renderers as an out-of-range opacity. Matplotlib bounds this to [0, 1].

🤖 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/_rc.py` around lines 181 - 190, Update the validation branch
in the rc parameter handling around the “grid.alpha” key so alpha values must
remain within the inclusive [0, 1] range. Preserve the existing non-negative
validation for the other size, width, and grid parameters, and raise the same
ValueError style when grid.alpha exceeds 1.
python/xy/_figure.py (1)

1316-1320: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

minor_style skips the same normalization pass style gets.

style is recompiled via styles.compile_axis_style(...) right before shipping (Line 1320), but minor_style is shipped as a raw dict(opts["minor_style"]) copy (Lines 1316-1317) without going through the same normalizer. Both fields are populated identically at set_axis() time, so this is an inconsistency in the defensive-revalidation pattern this function already applies to style.

♻️ Proposed fix
-        if opts.get("minor_style"):
-            spec["minor_style"] = dict(opts["minor_style"])
-        if opts.get("format") is not None:
+        if opts.get("format") is not None:
             spec["format"] = opts["format"]
         style = styles.compile_axis_style(opts.get("style"), f"{axis_id} axis style")
         if style:
             spec["style"] = style
+        minor_style = styles.compile_axis_style(opts.get("minor_style"), f"{axis_id} minor axis style")
+        if minor_style:
+            spec["minor_style"] = minor_style
🤖 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 1316 - 1320, Update the minor_style
handling in the axis-specification flow to pass opts["minor_style"] through
styles.compile_axis_style(...) using the same normalization pattern and axis
context as style, rather than copying it directly; preserve the existing format
handling and style compilation.
🤖 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 3439-3450: Update the log-limit transformation in the bounds
conversion logic used by _aspect_coordinates so non-positive or non-finite
transformed values do not raise during read-only geometry queries. Clamp invalid
non-positive log limits to the smallest positive representable bound, or use the
unadjusted rectangle as the fallback, while preserving normal transformation for
valid limits and inverse-log behavior.
- Around line 5003-5014: Update the log-scale validation in the scale
configuration block to reject every base value less than or equal to 1, matching
LogLocator’s requirement that the base be greater than 1. Preserve the existing
finite-value validation and raise the current ValueError at set_xscale call
time.

In `@python/xy/pyplot/_plot_types.py`:
- Around line 2540-2559: Update the capsize conversion around resolved_capsize
to handle both yerr and xerr orientations, rather than gating conversion on yerr
alone. Use the active axis transform and current limits to convert the
point-based half-width into data units for each orientation, preserving
half-width semantics and supporting log or explicitly limited axes instead of
deriving scale from x_values and figure width.

In `@python/xy/pyplot/_rc.py`:
- Line 80: Resolve the unused grid.linestyle configuration by either wiring the
_DEFAULTS entry into the grid style builder used by grid rendering, or removing
grid.linestyle from _DEFAULTS; ensure the chosen approach keeps rcParams
behavior consistent with the supported grid(linestyle=...) path.

In `@spec/design/renderer-architecture.md`:
- Line 331: Update the wording in the renderer architecture documentation so the
`tick_values` description uses “labelled,” matching the surrounding section’s
spelling convention and existing “labels” terminology.

---

Outside diff comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 5787-5799: Update the grid method docstring to document that the
which parameter accepts "major", "minor", or "both", matching the validation in
the grid implementation; leave the behavior unchanged.

---

Nitpick comments:
In `@python/xy/_figure.py`:
- Around line 1316-1320: Update the minor_style handling in the
axis-specification flow to pass opts["minor_style"] through
styles.compile_axis_style(...) using the same normalization pattern and axis
context as style, rather than copying it directly; preserve the existing format
handling and style compilation.

In `@python/xy/pyplot/_axes.py`:
- Around line 653-660: Update set_minor_locator to validate the supplied locator
with the same validation used by set_major_locator before storing it in
host._tickers. Reject objects that do not provide the required tick_values()
behavior so invalid arguments raise immediately rather than being skipped by
_apply_tickers; preserve the existing storage and invalidation flow for valid
locators.
- Around line 1242-1245: Remove the per-series `_expand_domain_to_ticks`
invocation from the `_add` path’s axis loop. Defer tick-based domain expansion
until `_build_chart`, alongside the existing domain materialization, ensuring
each applicable axis is expanded once after all series are added and preserving
the `_tick_expanded_domains` checks.

In `@python/xy/pyplot/_rc.py`:
- Around line 181-190: Update the validation branch in the rc parameter handling
around the “grid.alpha” key so alpha values must remain within the inclusive [0,
1] range. Preserve the existing non-negative validation for the other size,
width, and grid parameters, and raise the same ValueError style when grid.alpha
exceeds 1.

In `@python/xy/pyplot/_ticker.py`:
- Around line 513-516: Update the formatter implementation around set_locs and
__call__ so the stored _locs value affects minor-label decisions consistently
with Matplotlib’s LogitFormatter; otherwise remove the unused _locs storage
while preserving set_locs as a no-op for _apply_tickers compatibility. Choose
one approach and ensure the resulting behavior no longer stores locations that
are never read.

In `@tests/pyplot/test_axis_tick_gallery_compat.py`:
- Around line 61-63: Extend the dimension-isolation assertions in the test to
verify that the y-axis minor tier remains unchanged after configuring the x-axis
minor grid. Use the existing ax._axis_props("y") structure and its minor_style
grid_color value, while preserving the current x-minor and y-major assertions.
🪄 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: 127e8dc9-9a0c-4906-960a-b4802de73a69

📥 Commits

Reviewing files that changed from the base of the PR and between 4c26d97 and f054550.

📒 Files selected for processing (22)
  • js/src/00_header.ts
  • js/src/40_gl.ts
  • js/src/50_chartview.ts
  • python/xy/_figure.py
  • python/xy/_raster.py
  • python/xy/_svg.py
  • python/xy/components.py
  • python/xy/config.py
  • python/xy/pyplot/_axes.py
  • python/xy/pyplot/_mplfig.py
  • python/xy/pyplot/_plot_types.py
  • python/xy/pyplot/_rc.py
  • python/xy/pyplot/_ticker.py
  • spec/api/styling.md
  • spec/design/renderer-architecture.md
  • spec/design/wire-protocol.md
  • tests/pyplot/test_axis_tick_gallery_compat.py
  • tests/pyplot/test_grid_legend_contracts.py
  • tests/pyplot/test_log_scale_gallery_compat.py
  • tests/pyplot/test_nonlinear_scale_gallery_compat.py
  • tests/pyplot/test_p3_option_contracts.py
  • tests/test_ui_issue_regressions.py

Comment thread python/xy/pyplot/_axes.py
Comment thread python/xy/pyplot/_axes.py
Comment thread python/xy/pyplot/_plot_types.py Outdated
Comment thread python/xy/pyplot/_rc.py
Comment thread spec/design/renderer-architecture.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
js/src/40_gl.ts (1)

84-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Use a NaN-safe mask sentinel here

mode == 3 now returns NaN for nonpositive values, but HEATMAP_FS discards with ordered comparisons (uv.x < 0.0 || uv.x > 1.0 || ...), so masked coordinates can slip through and sample with NaN UVs instead of being dropped. The existing mode == 1 path already uses a finite sentinel (-1e30); keep the mask path finite too, or switch the fragment discard to a NaN-safe range check.

🤖 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/40_gl.ts` around lines 84 - 104, Update the mode == 3 branches in
xyAxisCoord and xyViewCoord to return a finite sentinel for nonpositive values,
matching the existing mode == 1 behavior so HEATMAP_FS ordered comparisons
discard masked coordinates. Keep the positive-value logarithmic conversion and
other mode behavior unchanged.
🤖 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/40_gl.ts`:
- Around line 84-104: Update the mode == 3 branches in xyAxisCoord and
xyViewCoord to return a finite sentinel for nonpositive values, matching the
existing mode == 1 behavior so HEATMAP_FS ordered comparisons discard masked
coordinates. Keep the positive-value logarithmic conversion and other mode
behavior unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3db1fc2c-6e4e-4a68-b4a4-c9d4d74065b2

📥 Commits

Reviewing files that changed from the base of the PR and between f054550 and d4aa8db.

📒 Files selected for processing (22)
  • docs/charts/scatter.md
  • docs/styling/mark-styles.md
  • js/src/40_gl.ts
  • js/src/50_chartview.ts
  • python/xy/_raster.py
  • python/xy/_svg.py
  • python/xy/_validate.py
  • python/xy/interaction.py
  • python/xy/marks.py
  • python/xy/pyplot/_artists.py
  • python/xy/pyplot/_axes.py
  • python/xy/pyplot/_plot_types.py
  • python/xy/pyplot/_rc.py
  • python/xy/pyplot/_translate.py
  • spec/api/styling.md
  • spec/design/renderer-architecture.md
  • src/raster.rs
  • tests/pyplot/test_axes_layout.py
  • tests/pyplot/test_gallery_hist_errorbar_compat.py
  • tests/pyplot/test_grid_legend_contracts.py
  • tests/pyplot/test_marker_fidelity.py
  • tests/pyplot/test_nonlinear_scale_gallery_compat.py
🚧 Files skipped from review as they are similar to previous changes (6)
  • spec/design/renderer-architecture.md
  • spec/api/styling.md
  • python/xy/pyplot/_rc.py
  • js/src/50_chartview.ts
  • python/xy/_svg.py
  • python/xy/pyplot/_axes.py

…le-compat

# Conflicts:
#	js/src/00_header.ts
#	js/src/50_chartview.ts
#	python/xy/_raster.py
#	python/xy/_svg.py
#	python/xy/config.py
#	python/xy/pyplot/_axes.py
#	spec/design/wire-protocol.md
#	tests/pyplot/test_marker_fidelity.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/_plot_types.py (1)

3581-3593: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Use the active display transform for contour-label geometry.

Lines 3581–3593 map raw data with a positive linear scale, skipping log/symlog transforms and screen-Y inversion. Consequently rotation, collision avoidance, inline gaps, and manual snapping at Lines 3645–3648 are incorrect on nonlinear or inverted axes. Transform paths and manual queries through the active axis/display transform first.

Also applies to: 3645-3648

🤖 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 3581 - 3593, Update
contour-label geometry in the surrounding plotting method, including the manual
query logic near the referenced snapping code, to use the active axis/display
transform rather than the raw positive linear scale. Transform connected contour
paths and manual label positions through that transform so rotation, collision
avoidance, inline gaps, and snapping remain correct for log, symlog, and
inverted axes; preserve the existing level selection and path-connection
behavior.
python/xy/pyplot/_axes.py (1)

3110-3142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

_set_axes_image_data leaks axes-level aspect state on set_data().

This rebuilds the array via a temporary self.imshow(...) call and only cleans up the temp entry/artist afterward. But imshow() also mutates axes-wide state (self._aspect_equal, self._aspect_value, and especially self._aspect_bounds), and none of that is restored. _aspect_bounds in particular is unioned with the previous value rather than replaced:

if self._aspect_bounds is None:
    self._aspect_bounds = bounds
else:
    old = self._aspect_bounds
    self._aspect_bounds = (min(old[0], bounds[0]), max(old[1], bounds[1]), ...)

Since the persistent entry already contributed its old extent to _aspect_bounds when it was first created, calling image.set_data(new_z) with a differently shaped/extent array will accumulate the union of old and new extents into _aspect_bounds instead of replacing it — corrupting equal-aspect layout (get_position()/_build_chart() both consume _aspect_bounds) after a data update.

Save and restore _aspect_equal/_aspect_value/_aspect_bounds (and _aspect_adjustable) around the temporary imshow() call, then apply only the freshly computed bounds for this image.

🐛 Proposed fix
         replacement = self.imshow(z, state["cmap"], **state["kwargs"])
         prepared = replacement._entry
         self._remove_entry(prepared)
         self._unregister_artist(replacement)
+        replacement = self.imshow(z, state["cmap"], **state["kwargs"])
+        prepared = replacement._entry
+        self._remove_entry(prepared)
+        self._unregister_artist(replacement)
+        # imshow() above mutated axes-wide aspect tracking as a side effect;
+        # restore it and apply only this image's fresh extent.
+        self._aspect_equal = prior_aspect_equal
+        self._aspect_value = prior_aspect_value
+        self._aspect_bounds = prepared["extent"]

(with prior_aspect_equal, prior_aspect_value, prior_aspect_bounds = self._aspect_equal, self._aspect_value, self._aspect_bounds captured before the call)

🤖 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 3110 - 3142, Update
_set_axes_image_data to capture _aspect_equal, _aspect_value, _aspect_bounds,
and _aspect_adjustable before the temporary imshow call, then restore the aspect
flags and replace _aspect_bounds with the bounds freshly computed for the
updated image after cleanup. Ensure the prior axes-wide aspect state is not
accumulated or leaked while retaining the new image’s bounds.
🧹 Nitpick comments (1)
python/xy/pyplot/_axes.py (1)

5601-5682: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

minor=True in set_xticks/set_yticks stays a silent no-op even though this PR adds native minor-tick support.

The docstring still says "minor ticks are outside the native axis contract," but this PR introduces minor_tick_values, minor_locator/minor_formatter, and minor_style as first-class axis fields. A matplotlib script calling ax.set_xticks(positions, minor=True) to pin custom minor tick locations will now silently do nothing, even though the wire protocol can represent exactly that. Consider wiring minor=True through to minor_tick_values/a FixedLocator on the minor slot (or updating the docstring/no-op comment to make the limitation explicit and intentional).

🤖 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 5601 - 5682, Update set_xticks and
set_yticks so minor=True applies the supplied ticks to the axis’s native
minor-tick configuration, including minor_tick_values and the minor locator
slot, while preserving label and rotation handling as appropriate; remove the
obsolete silent no-op behavior and update both docstrings to describe the
supported minor-tick 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.

Outside diff comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 3110-3142: Update _set_axes_image_data to capture _aspect_equal,
_aspect_value, _aspect_bounds, and _aspect_adjustable before the temporary
imshow call, then restore the aspect flags and replace _aspect_bounds with the
bounds freshly computed for the updated image after cleanup. Ensure the prior
axes-wide aspect state is not accumulated or leaked while retaining the new
image’s bounds.

In `@python/xy/pyplot/_plot_types.py`:
- Around line 3581-3593: Update contour-label geometry in the surrounding
plotting method, including the manual query logic near the referenced snapping
code, to use the active axis/display transform rather than the raw positive
linear scale. Transform connected contour paths and manual label positions
through that transform so rotation, collision avoidance, inline gaps, and
snapping remain correct for log, symlog, and inverted axes; preserve the
existing level selection and path-connection behavior.

---

Nitpick comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 5601-5682: Update set_xticks and set_yticks so minor=True applies
the supplied ticks to the axis’s native minor-tick configuration, including
minor_tick_values and the minor locator slot, while preserving label and
rotation handling as appropriate; remove the obsolete silent no-op behavior and
update both docstrings to describe the supported minor-tick behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: cd931525-7d7f-49fa-8ed6-78adedce4ab4

📥 Commits

Reviewing files that changed from the base of the PR and between d4aa8db and bcc04cc.

📒 Files selected for processing (17)
  • js/src/00_header.ts
  • js/src/50_chartview.ts
  • python/xy/_raster.py
  • python/xy/_svg.py
  • python/xy/components.py
  • python/xy/config.py
  • python/xy/marks.py
  • python/xy/pyplot/_artists.py
  • python/xy/pyplot/_axes.py
  • python/xy/pyplot/_mplfig.py
  • python/xy/pyplot/_plot_types.py
  • python/xy/pyplot/_rc.py
  • spec/api/styling.md
  • spec/design/wire-protocol.md
  • tests/pyplot/test_axes_layout.py
  • tests/pyplot/test_marker_fidelity.py
  • tests/pyplot/test_p3_option_contracts.py
🚧 Files skipped from review as they are similar to previous changes (12)
  • python/xy/marks.py
  • tests/pyplot/test_axes_layout.py
  • tests/pyplot/test_p3_option_contracts.py
  • js/src/00_header.ts
  • python/xy/pyplot/_rc.py
  • python/xy/pyplot/_artists.py
  • tests/pyplot/test_marker_fidelity.py
  • spec/api/styling.md
  • python/xy/_raster.py
  • js/src/50_chartview.ts
  • python/xy/components.py
  • python/xy/_svg.py

@Alek99
Alek99 merged commit 1de1b38 into main Jul 27, 2026
38 of 40 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