Fix pie wedges and annotation connectors [mpl compatibility] - #337
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
📝 WalkthroughWalkthroughChangesAxis protocol and rendering
Line marker symbols
Pyplot chart rendering
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 9
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/_rc.py (1)
183-192: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
grid.alphais only checked for non-negativity.It lands verbatim in
grid_opacityon the axis style (_rc_axis_style/_rc_minor_axis_style) and reaches all three renderers as an opacity, sorcParams["grid.alpha"] = 5ships an out-of-range value. Elsewhere the shim validates alphas as a closed range (legend framealpha must be between 0 and 1).🛡️ Proposed fix
"grid.linewidth", - "grid.alpha", }: value = float(value) if value < 0: raise ValueError(f"{key} must be non-negative") + if key == "grid.alpha": + value = float(value) + if not 0.0 <= value <= 1.0: + raise ValueError(f"{key} must be between 0 and 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 183 - 192, Update validation for the grid.alpha branch in the rc parameter handling so values must be within the closed range 0 through 1, while retaining non-negative validation for the other size, width, and linewidth keys. Use the existing alpha validation pattern and error style, and ensure the constrained value passed through _rc_axis_style and _rc_minor_axis_style remains in range.
🧹 Nitpick comments (5)
tests/pyplot/test_axis_tick_gallery_compat.py (1)
61-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the y minor tier stayed untouched.
The test's purpose is that the proxy targets only its own dimension; for the new minor tier it currently only checks x.
xaxis.grid(which="minor")must leavey'sminor_styletransparent, which is exactly the routinggrid()'s newtiersloop could get wrong.💚 Proposed addition
ax.xaxis.grid(which="minor", color="0.9") 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 test around the xaxis.grid minor-tier assertion to also verify that ax._axis_props("y")["minor_style"]["grid_color"] remains transparent. Keep the existing x minor-style and y major-style assertions, confirming the grid call only updates the targeted dimension.python/xy/pyplot/_ticker.py (2)
325-330: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReturn sorted ticks like the sibling locators.
With multi-entry
subs, negative decades emit descending values (-10, -20, -50), sotick_valuesis non-monotonic. Renderers are order-insensitive, butLogLocator.tick_valuessorts andAxes.get_*ticks(minor=True)passes this through unsorted, so the public tick list can come back non-monotonic on a symlog axis.♻️ Proposed fix
- return np.asarray(ticks, dtype=float) + return np.sort(np.asarray(ticks, dtype=float))🤖 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 325 - 330, Update the tick construction in the locator method containing the shown `ticks` comprehension to return the generated values in ascending order. Preserve the existing decade/substitution logic, including the zero-decade special case, and sort the final array before returning it so public symlog tick values are monotonic.
341-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
symthreshis accepted but never used.It is stored and settable via
set_params, yettick_valuesnever reads it, soAsinhLocator(symthresh=...)is silently ignored rather than symmetrizing the range as Matplotlib does. Either implement it or drop it from the surface so the omission isn't silent.Also applies to: 363-364
🤖 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 341 - 347, Implement symthresh handling in AsinhLocator.tick_values, using the stored self.symthresh value to symmetrize the computed tick range when the threshold condition is met, matching Matplotlib behavior; ensure values updated through set_params are honored. Do not leave symthresh as an unused constructor or configuration parameter.js/src/50_chartview.ts (1)
4961-5002: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor ticks/grid are primary-axis only; secondary axes silently drop theirs.
minorTicksis computed forxAxis/yAxisonly, and theextraXAxes/extraYAxestick loops below have no minor pass. The Python side does publishminor_tick_valuesfory2(_build_chartcalls_apply_tickers("y2", ...)), so a twinned log axis ships minors that no renderer draws —_raster.render_rasterlikewise only callsminor_axis_ticks(xa)/(ya).All three renderers agree, so nothing diverges; the gap is just undocumented. Worth recording in the spec (or extending the loops) so twin-axis minor ticks aren't a silent omission.
As per coding guidelines, "Record every decimation and tier decision in the specification; decisions must never be silent" and "Keep the entire
spec/directory current with every relevant code, configuration, build, and release change".Also applies to: 5090-5102, 5114-5126
🤖 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/50_chartview.ts` around lines 4961 - 5002, Document in the relevant spec that minor ticks and minor grid lines are intentionally rendered only for the primary xAxis and yAxis, while extraXAxes and extraYAxes do not receive a minor-tick rendering pass. Record that secondary-axis minor_tick_values, including y2 values produced by _apply_tickers, are currently omitted consistently across renderers, including _raster.render_raster; do not change rendering behavior unless extending all corresponding axis loops.Source: Coding guidelines
python/xy/pyplot/_axes.py (1)
654-661: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMirror the major-locator guards on
set_minor_locator.The major path rejects non-Locator objects and no-ops on pandas units-registry tickers; the minor path stores anything. A pandas
TimeSeries_DateLocatorinstalled as a minor locator now reaches_apply_tickers, which only checkshasattr(tick_values), and publishes unit-registry ordinals asminor_tick_valueson a native ms axis.♻️ Proposed parity fix
def set_minor_locator(self, locator: Any) -> None: + if self._is_units_registry_ticker(locator): + return + if not hasattr(locator, "tick_values"): + raise TypeError("set_minor_locator() requires a Locator with tick_values()") host, key = self._ticker_slot() host._tickers[(key, "minor_locator")] = locator self.axes._invalidate()🤖 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 654 - 661, Update set_minor_locator to mirror set_major_locator’s validation and pandas units-registry guard: reject non-Locator values and no-op for registry-managed pandas tickers before storing the locator. Preserve the existing ticker storage and invalidation behavior for valid minor locators, preventing TimeSeries_DateLocator values from reaching _apply_tickers.
🤖 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/00_header.ts`:
- Around line 31-33: Update the bump comment above PROTOCOL to reference an
older v8 client instead of v7, keeping the rest of the comment unchanged.
In `@js/src/40_gl.ts`:
- Line 87: Update the mode == 3 branches in xyAxisCoord() and xyViewCoord() to
return the existing -1e30 off-screen sentinel for non-positive values, matching
mode == 1; remove the uintBitsToFloat NaN fallback while preserving the
logarithm calculation for positive values.
In `@js/src/51_annotations.ts`:
- Around line 304-323: Balance the per-annotation ctx.save() in the annotation
rendering loop by calling ctx.restore() before every continue in the band/rule
branches, including the branches around lines 329, 332, and 345–347. Ensure
annotations that apply the connector-target clip restore that state before being
skipped, while preserving the existing rendering behavior for processed
annotations.
In `@python/xy/config.py`:
- Around line 22-24: Update the predecessor-version reference in the comment
above PROTOCOL_VERSION from v7 to v8, keeping the rest of the protocol-version
explanation unchanged.
In `@python/xy/pyplot/_axes.py`:
- Around line 539-542: Update the symlog minor locator configuration in the axes
ticker setup so a missing subs value uses automatic subdivisions rather than
SymmetricalLogLocator’s degenerate (1.0,) default. Preserve explicitly
configured spec.get("subs") values, and align the behavior with the sibling
LogLocator handling so minor ticks remain distinct from major decades.
- Around line 3313-3317: Move z-order ordering out of the existing-entry-only
branch in annotate and into the entry-addition or draw path used by the host
axis. Ensure host._entries is reordered by each entry’s _zorder whenever artists
are added or rendered, so entries added after annotate(zorder=...) are
positioned correctly while preserving existing annotation z-order behavior.
- Around line 3555-3557: Validate log-scale bounds before storing raw extents in
_aspect_bounds, covering both autoscale(tight=True) and image-extent assignment
paths. Reuse the positivity and finiteness validation expected by
_aspect_coordinates() so invalid bounds raise at assignment time rather than
later during get_position() box-aspect layout.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 618-633: In the candidate/triangle clipping loop, add a cheap
axis-aligned bounding-box rejection before calling _clip_segment_to_triangle:
skip pairs whose segment bounds do not overlap the triangle bounds. Precompute
each triangle’s bounds once outside the candidate loop, preserve all existing
clipping and output behavior for overlapping pairs, and avoid introducing NumPy
allocations for the rejection.
In `@tests/pyplot/test_gallery_hist_errorbar_compat.py`:
- Around line 328-331: Update the cap color assertion in the loop over
horizontal and vertical caps so it validates the actual stored color without
relying on the hard-coded default from kwargs.get("stroke", "red"). Either
assert the entry’s existing color field directly or explicitly populate stroke
in the cap kwargs if that is the intended contract.
---
Outside diff comments:
In `@python/xy/pyplot/_rc.py`:
- Around line 183-192: Update validation for the grid.alpha branch in the rc
parameter handling so values must be within the closed range 0 through 1, while
retaining non-negative validation for the other size, width, and linewidth keys.
Use the existing alpha validation pattern and error style, and ensure the
constrained value passed through _rc_axis_style and _rc_minor_axis_style remains
in range.
---
Nitpick comments:
In `@js/src/50_chartview.ts`:
- Around line 4961-5002: Document in the relevant spec that minor ticks and
minor grid lines are intentionally rendered only for the primary xAxis and
yAxis, while extraXAxes and extraYAxes do not receive a minor-tick rendering
pass. Record that secondary-axis minor_tick_values, including y2 values produced
by _apply_tickers, are currently omitted consistently across renderers,
including _raster.render_raster; do not change rendering behavior unless
extending all corresponding axis loops.
In `@python/xy/pyplot/_axes.py`:
- Around line 654-661: Update set_minor_locator to mirror set_major_locator’s
validation and pandas units-registry guard: reject non-Locator values and no-op
for registry-managed pandas tickers before storing the locator. Preserve the
existing ticker storage and invalidation behavior for valid minor locators,
preventing TimeSeries_DateLocator values from reaching _apply_tickers.
In `@python/xy/pyplot/_ticker.py`:
- Around line 325-330: Update the tick construction in the locator method
containing the shown `ticks` comprehension to return the generated values in
ascending order. Preserve the existing decade/substitution logic, including the
zero-decade special case, and sort the final array before returning it so public
symlog tick values are monotonic.
- Around line 341-347: Implement symthresh handling in AsinhLocator.tick_values,
using the stored self.symthresh value to symmetrize the computed tick range when
the threshold condition is met, matching Matplotlib behavior; ensure values
updated through set_params are honored. Do not leave symthresh as an unused
constructor or configuration parameter.
In `@tests/pyplot/test_axis_tick_gallery_compat.py`:
- Around line 61-63: Extend the test around the xaxis.grid minor-tier assertion
to also verify that ax._axis_props("y")["minor_style"]["grid_color"] remains
transparent. Keep the existing x minor-style and y major-style assertions,
confirming the grid call only updates the targeted dimension.
🪄 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: 55e0be04-7c7e-4678-9eb5-4eda352ce974
📒 Files selected for processing (39)
docs/charts/scatter.mddocs/styling/mark-styles.mdjs/src/00_header.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/51_annotations.tspython/xy/_arrowgeom.pypython/xy/_figure.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_validate.pypython/xy/components.pypython/xy/config.pypython/xy/interaction.pypython/xy/marks.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pypython/xy/pyplot/_rc.pypython/xy/pyplot/_ticker.pypython/xy/pyplot/_translate.pyspec/api/styling.mdspec/design/renderer-architecture.mdspec/design/wire-protocol.mdsrc/raster.rstests/pyplot/test_axes_charts.pytests/pyplot/test_axes_layout.pytests/pyplot/test_axis_tick_gallery_compat.pytests/pyplot/test_gallery_hist_errorbar_compat.pytests/pyplot/test_gallery_text_pie_compat.pytests/pyplot/test_grid_legend_contracts.pytests/pyplot/test_log_scale_gallery_compat.pytests/pyplot/test_marker_fidelity.pytests/pyplot/test_nonlinear_scale_gallery_compat.pytests/pyplot/test_p3_option_contracts.pytests/pyplot/test_pie_annotation_grouped_repair.pytests/test_ui_issue_regressions.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
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/_rc.py (1)
183-192: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
grid.alphais only checked for non-negativity.It lands verbatim in
grid_opacityon the axis style (_rc_axis_style/_rc_minor_axis_style) and reaches all three renderers as an opacity, sorcParams["grid.alpha"] = 5ships an out-of-range value. Elsewhere the shim validates alphas as a closed range (legend framealpha must be between 0 and 1).🛡️ Proposed fix
"grid.linewidth", - "grid.alpha", }: value = float(value) if value < 0: raise ValueError(f"{key} must be non-negative") + if key == "grid.alpha": + value = float(value) + if not 0.0 <= value <= 1.0: + raise ValueError(f"{key} must be between 0 and 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 183 - 192, Update validation for the grid.alpha branch in the rc parameter handling so values must be within the closed range 0 through 1, while retaining non-negative validation for the other size, width, and linewidth keys. Use the existing alpha validation pattern and error style, and ensure the constrained value passed through _rc_axis_style and _rc_minor_axis_style remains in range.
🧹 Nitpick comments (5)
tests/pyplot/test_axis_tick_gallery_compat.py (1)
61-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the y minor tier stayed untouched.
The test's purpose is that the proxy targets only its own dimension; for the new minor tier it currently only checks x.
xaxis.grid(which="minor")must leavey'sminor_styletransparent, which is exactly the routinggrid()'s newtiersloop could get wrong.💚 Proposed addition
ax.xaxis.grid(which="minor", color="0.9") 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 test around the xaxis.grid minor-tier assertion to also verify that ax._axis_props("y")["minor_style"]["grid_color"] remains transparent. Keep the existing x minor-style and y major-style assertions, confirming the grid call only updates the targeted dimension.python/xy/pyplot/_ticker.py (2)
325-330: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReturn sorted ticks like the sibling locators.
With multi-entry
subs, negative decades emit descending values (-10, -20, -50), sotick_valuesis non-monotonic. Renderers are order-insensitive, butLogLocator.tick_valuessorts andAxes.get_*ticks(minor=True)passes this through unsorted, so the public tick list can come back non-monotonic on a symlog axis.♻️ Proposed fix
- return np.asarray(ticks, dtype=float) + return np.sort(np.asarray(ticks, dtype=float))🤖 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 325 - 330, Update the tick construction in the locator method containing the shown `ticks` comprehension to return the generated values in ascending order. Preserve the existing decade/substitution logic, including the zero-decade special case, and sort the final array before returning it so public symlog tick values are monotonic.
341-347: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
symthreshis accepted but never used.It is stored and settable via
set_params, yettick_valuesnever reads it, soAsinhLocator(symthresh=...)is silently ignored rather than symmetrizing the range as Matplotlib does. Either implement it or drop it from the surface so the omission isn't silent.Also applies to: 363-364
🤖 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 341 - 347, Implement symthresh handling in AsinhLocator.tick_values, using the stored self.symthresh value to symmetrize the computed tick range when the threshold condition is met, matching Matplotlib behavior; ensure values updated through set_params are honored. Do not leave symthresh as an unused constructor or configuration parameter.js/src/50_chartview.ts (1)
4961-5002: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor ticks/grid are primary-axis only; secondary axes silently drop theirs.
minorTicksis computed forxAxis/yAxisonly, and theextraXAxes/extraYAxestick loops below have no minor pass. The Python side does publishminor_tick_valuesfory2(_build_chartcalls_apply_tickers("y2", ...)), so a twinned log axis ships minors that no renderer draws —_raster.render_rasterlikewise only callsminor_axis_ticks(xa)/(ya).All three renderers agree, so nothing diverges; the gap is just undocumented. Worth recording in the spec (or extending the loops) so twin-axis minor ticks aren't a silent omission.
As per coding guidelines, "Record every decimation and tier decision in the specification; decisions must never be silent" and "Keep the entire
spec/directory current with every relevant code, configuration, build, and release change".Also applies to: 5090-5102, 5114-5126
🤖 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/50_chartview.ts` around lines 4961 - 5002, Document in the relevant spec that minor ticks and minor grid lines are intentionally rendered only for the primary xAxis and yAxis, while extraXAxes and extraYAxes do not receive a minor-tick rendering pass. Record that secondary-axis minor_tick_values, including y2 values produced by _apply_tickers, are currently omitted consistently across renderers, including _raster.render_raster; do not change rendering behavior unless extending all corresponding axis loops.Source: Coding guidelines
python/xy/pyplot/_axes.py (1)
654-661: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMirror the major-locator guards on
set_minor_locator.The major path rejects non-Locator objects and no-ops on pandas units-registry tickers; the minor path stores anything. A pandas
TimeSeries_DateLocatorinstalled as a minor locator now reaches_apply_tickers, which only checkshasattr(tick_values), and publishes unit-registry ordinals asminor_tick_valueson a native ms axis.♻️ Proposed parity fix
def set_minor_locator(self, locator: Any) -> None: + if self._is_units_registry_ticker(locator): + return + if not hasattr(locator, "tick_values"): + raise TypeError("set_minor_locator() requires a Locator with tick_values()") host, key = self._ticker_slot() host._tickers[(key, "minor_locator")] = locator self.axes._invalidate()🤖 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 654 - 661, Update set_minor_locator to mirror set_major_locator’s validation and pandas units-registry guard: reject non-Locator values and no-op for registry-managed pandas tickers before storing the locator. Preserve the existing ticker storage and invalidation behavior for valid minor locators, preventing TimeSeries_DateLocator values from reaching _apply_tickers.
🤖 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/00_header.ts`:
- Around line 31-33: Update the bump comment above PROTOCOL to reference an
older v8 client instead of v7, keeping the rest of the comment unchanged.
In `@js/src/40_gl.ts`:
- Line 87: Update the mode == 3 branches in xyAxisCoord() and xyViewCoord() to
return the existing -1e30 off-screen sentinel for non-positive values, matching
mode == 1; remove the uintBitsToFloat NaN fallback while preserving the
logarithm calculation for positive values.
In `@js/src/51_annotations.ts`:
- Around line 304-323: Balance the per-annotation ctx.save() in the annotation
rendering loop by calling ctx.restore() before every continue in the band/rule
branches, including the branches around lines 329, 332, and 345–347. Ensure
annotations that apply the connector-target clip restore that state before being
skipped, while preserving the existing rendering behavior for processed
annotations.
In `@python/xy/config.py`:
- Around line 22-24: Update the predecessor-version reference in the comment
above PROTOCOL_VERSION from v7 to v8, keeping the rest of the protocol-version
explanation unchanged.
In `@python/xy/pyplot/_axes.py`:
- Around line 539-542: Update the symlog minor locator configuration in the axes
ticker setup so a missing subs value uses automatic subdivisions rather than
SymmetricalLogLocator’s degenerate (1.0,) default. Preserve explicitly
configured spec.get("subs") values, and align the behavior with the sibling
LogLocator handling so minor ticks remain distinct from major decades.
- Around line 3313-3317: Move z-order ordering out of the existing-entry-only
branch in annotate and into the entry-addition or draw path used by the host
axis. Ensure host._entries is reordered by each entry’s _zorder whenever artists
are added or rendered, so entries added after annotate(zorder=...) are
positioned correctly while preserving existing annotation z-order behavior.
- Around line 3555-3557: Validate log-scale bounds before storing raw extents in
_aspect_bounds, covering both autoscale(tight=True) and image-extent assignment
paths. Reuse the positivity and finiteness validation expected by
_aspect_coordinates() so invalid bounds raise at assignment time rather than
later during get_position() box-aspect layout.
In `@python/xy/pyplot/_plot_types.py`:
- Around line 618-633: In the candidate/triangle clipping loop, add a cheap
axis-aligned bounding-box rejection before calling _clip_segment_to_triangle:
skip pairs whose segment bounds do not overlap the triangle bounds. Precompute
each triangle’s bounds once outside the candidate loop, preserve all existing
clipping and output behavior for overlapping pairs, and avoid introducing NumPy
allocations for the rejection.
In `@tests/pyplot/test_gallery_hist_errorbar_compat.py`:
- Around line 328-331: Update the cap color assertion in the loop over
horizontal and vertical caps so it validates the actual stored color without
relying on the hard-coded default from kwargs.get("stroke", "red"). Either
assert the entry’s existing color field directly or explicitly populate stroke
in the cap kwargs if that is the intended contract.
---
Outside diff comments:
In `@python/xy/pyplot/_rc.py`:
- Around line 183-192: Update validation for the grid.alpha branch in the rc
parameter handling so values must be within the closed range 0 through 1, while
retaining non-negative validation for the other size, width, and linewidth keys.
Use the existing alpha validation pattern and error style, and ensure the
constrained value passed through _rc_axis_style and _rc_minor_axis_style remains
in range.
---
Nitpick comments:
In `@js/src/50_chartview.ts`:
- Around line 4961-5002: Document in the relevant spec that minor ticks and
minor grid lines are intentionally rendered only for the primary xAxis and
yAxis, while extraXAxes and extraYAxes do not receive a minor-tick rendering
pass. Record that secondary-axis minor_tick_values, including y2 values produced
by _apply_tickers, are currently omitted consistently across renderers,
including _raster.render_raster; do not change rendering behavior unless
extending all corresponding axis loops.
In `@python/xy/pyplot/_axes.py`:
- Around line 654-661: Update set_minor_locator to mirror set_major_locator’s
validation and pandas units-registry guard: reject non-Locator values and no-op
for registry-managed pandas tickers before storing the locator. Preserve the
existing ticker storage and invalidation behavior for valid minor locators,
preventing TimeSeries_DateLocator values from reaching _apply_tickers.
In `@python/xy/pyplot/_ticker.py`:
- Around line 325-330: Update the tick construction in the locator method
containing the shown `ticks` comprehension to return the generated values in
ascending order. Preserve the existing decade/substitution logic, including the
zero-decade special case, and sort the final array before returning it so public
symlog tick values are monotonic.
- Around line 341-347: Implement symthresh handling in AsinhLocator.tick_values,
using the stored self.symthresh value to symmetrize the computed tick range when
the threshold condition is met, matching Matplotlib behavior; ensure values
updated through set_params are honored. Do not leave symthresh as an unused
constructor or configuration parameter.
In `@tests/pyplot/test_axis_tick_gallery_compat.py`:
- Around line 61-63: Extend the test around the xaxis.grid minor-tier assertion
to also verify that ax._axis_props("y")["minor_style"]["grid_color"] remains
transparent. Keep the existing x minor-style and y major-style assertions,
confirming the grid call only updates the targeted dimension.
🪄 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: 55e0be04-7c7e-4678-9eb5-4eda352ce974
📒 Files selected for processing (39)
docs/charts/scatter.mddocs/styling/mark-styles.mdjs/src/00_header.tsjs/src/40_gl.tsjs/src/50_chartview.tsjs/src/51_annotations.tspython/xy/_arrowgeom.pypython/xy/_figure.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_validate.pypython/xy/components.pypython/xy/config.pypython/xy/interaction.pypython/xy/marks.pypython/xy/pyplot/__init__.pypython/xy/pyplot/_artists.pypython/xy/pyplot/_axes.pypython/xy/pyplot/_mplfig.pypython/xy/pyplot/_plot_types.pypython/xy/pyplot/_rc.pypython/xy/pyplot/_ticker.pypython/xy/pyplot/_translate.pyspec/api/styling.mdspec/design/renderer-architecture.mdspec/design/wire-protocol.mdsrc/raster.rstests/pyplot/test_axes_charts.pytests/pyplot/test_axes_layout.pytests/pyplot/test_axis_tick_gallery_compat.pytests/pyplot/test_gallery_hist_errorbar_compat.pytests/pyplot/test_gallery_text_pie_compat.pytests/pyplot/test_grid_legend_contracts.pytests/pyplot/test_log_scale_gallery_compat.pytests/pyplot/test_marker_fidelity.pytests/pyplot/test_nonlinear_scale_gallery_compat.pytests/pyplot/test_p3_option_contracts.pytests/pyplot/test_pie_annotation_grouped_repair.pytests/test_ui_issue_regressions.py
🛑 Comments failed to post (8)
js/src/00_header.ts (1)
31-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale predecessor-version reference in the bump comment.
Comment says "an older v7 client" but this bumps v8→v9, so the stale-client reference should say "v8 client" (matching the pattern in
spec/design/wire-protocol.md's own changelog, e.g. "v8 adds ... an older v7 client would accept ...").🤖 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/00_header.ts` around lines 31 - 33, Update the bump comment above PROTOCOL to reference an older v8 client instead of v7, keeping the rest of the comment unchanged.js/src/40_gl.ts (1)
87-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
WebGL2 GLSL ES 3.00 NaN gl_Position vertex clipping behavior undefined💡 Result:
In WebGL2 and OpenGL ES 3.00, the behavior of gl_Position when it contains NaN (Not-a-Number) or is left uninitialized is considered undefined [1][2][3]. When gl_Position is undefined—such as when a vertex shader fails to write to it—or when it contains invalid floating-point values like NaN, the implementation is not required to handle the data in any specific way [1][4][5]. Because primitive assembly, clipping, and rasterization depend on valid clip-space coordinates, providing NaN typically leads to unpredictable rendering results [1][6][7]. Common outcomes include the triangle or primitive being discarded, rendered at incorrect positions, or causing visual artifacts, as the clipping stage cannot perform geometric tests against invalid coordinates [1][6][8]. Key aspects of this behavior include: 1. Undefined values: According to the OpenGL ES Shading Language Specification, if a result is undefined, the system may behave as if it had been assigned a random value [1]. Implementations are not required to catch these errors and may, but are not obligated to, detect the use of undefined values and behave accordingly (e.g., triggering a trap) [1]. 2. NaN support: The OpenGL ES specification permits, but does not require, implementations to support NaN in floating-point computations [6][4][5]. Consequently, an expression that evaluates to NaN on one GPU might behave differently on another [6][8]. 3. Clipping dependencies: The fixed-function clipping stage assumes gl_Position contains valid homogeneous coordinates [3][7]. If NaN is present, the mathematical comparisons required for clipping (e.g., checking if the vertex is within the view volume) become non-deterministic or fail entirely [6][7]. 4. Best practices: It is strongly discouraged to rely on NaN or uninitialized gl_Position values for flow control or discarding primitives [6][8]. Instead, developers should use well-defined techniques to discard primitives, such as placing them outside the view frustum (e.g., setting gl_Position to a value like vec4(0, 0, 0, -1)) [8], or using discard in a fragment shader if applicable.
Citations:
- 1: https://registry.khronos.org/OpenGL/specs/es/3.0/GLSL_ES_Specification_3.00.pdf
- 2: https://lists.freedesktop.org/archives/mesa-dev/2014-September/067410.html
- 3: https://docs.vulkan.org/glsl/latest/chapters/builtins.html
- 4: https://registry.khronos.org/OpenGL/specs/es/3.0/es_spec_3.0.pdf
- 5: https://developer.nvidia.com/docs/drive/drive-os/archives/6.0.3/linux/sdk/api_reference/es_spec_3.0.3.pdf
- 6: https://stackoverflow.com/questions/37532640/making-a-nan-on-purpose-in-webgl
- 7: https://wikis.khronos.org/opengl/Vertex_Post-Processing
- 8: https://stackoverflow.com/questions/70955373/opengl-es-well-defined-way-to-discard-primitive-in-vertex-shader
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the referenced file and nearby logic. wc -l js/src/40_gl.ts sed -n '1,160p' js/src/40_gl.ts # Find the surrounding mode handling and any gl_Position assignments. rg -n "mode == 1|mode == 3|gl_Position|uintBitsToFloat|log\\(" js/src/40_gl.ts js/src -g '*.ts'Repository: reflex-dev/xy
Length of output: 11793
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect how the log modes are used and documented. rg -n "u_xmode|u_ymode|mode == 3|mode == 1|xyViewValue|xyAxisCoord|log scale|log" js/src/40_gl.ts js/src -g '*.ts' -g '*.js' # Read the smaller relevant sections around the helpers and any mode documentation. sed -n '70,120p' js/src/40_gl.ts sed -n '650,720p' js/src/40_gl.ts sed -n '740,900p' js/src/40_gl.tsRepository: reflex-dev/xy
Length of output: 28591
Use the off-screen sentinel for masked log values
Themode == 3branch should use the same-1e30sentinel asmode == 1in bothxyAxisCoord()andxyViewCoord().uintBitsToFloat(0x7fc00000u)can feed NaN intogl_Position, and clip behavior is undefined there. The probe only proves the mode reaches the renderer, not that non-positive values are consistently discarded.🤖 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` at line 87, Update the mode == 3 branches in xyAxisCoord() and xyViewCoord() to return the existing -1e30 off-screen sentinel for non-positive values, matching mode == 1; remove the uintBitsToFloat NaN fallback while preserving the logarithm calculation for positive values.js/src/51_annotations.ts (1)
304-323: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Per-annotation
ctx.save()is not balanced on thecontinuepaths.
ctx.save()now runs per annotation (Line 305) with the matchingctx.restore()only at Line 398, but the band/rule branches stillcontinue(Lines 329, 332, 345–347). Each skipped annotation leaks a saved state and — when the connector-target clip was applied at Lines 320–322 — leaves the plot-rect clip active for every later annotation and for whatever draws on this context afterwards. That was harmless before because the save/clip lived outside the loop.🐛 Restore before skipping
- if (!Number.isFinite(a) || !Number.isFinite(b)) continue; + if (!Number.isFinite(a) || !Number.isFinite(b)) { ctx.restore(); continue; } const lo = Math.max(vertical ? p.x : p.y, Math.min(a, b)); const hi = Math.min(vertical ? p.x + p.w : p.y + p.h, Math.max(a, b)); - if (hi <= lo) continue; + if (hi <= lo) { ctx.restore(); continue; }- if (!Number.isFinite(pos)) continue; - if (vertical && (pos < p.x - 1 || pos > p.x + p.w + 1)) continue; - if (!vertical && (pos < p.y - 1 || pos > p.y + p.h + 1)) continue; + if (!Number.isFinite(pos)) { ctx.restore(); continue; } + if (vertical && (pos < p.x - 1 || pos > p.x + p.w + 1)) { ctx.restore(); continue; } + if (!vertical && (pos < p.y - 1 || pos > p.y + p.h + 1)) { ctx.restore(); continue; }Alternatively, extract the per-annotation body into a method and use
returnso the save/restore pair stays local.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (!Number.isFinite(a) || !Number.isFinite(b)) { ctx.restore(); continue; } const lo = Math.max(vertical ? p.x : p.y, Math.min(a, b)); const hi = Math.min(vertical ? p.x + p.w : p.y + p.h, Math.max(a, b)); if (hi <= lo) { ctx.restore(); continue; } if (!Number.isFinite(pos)) { ctx.restore(); continue; } if (vertical && (pos < p.x - 1 || pos > p.x + p.w + 1)) { ctx.restore(); continue; } if (!vertical && (pos < p.y - 1 || pos > p.y + p.h + 1)) { ctx.restore(); continue; }🤖 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/51_annotations.ts` around lines 304 - 323, Balance the per-annotation ctx.save() in the annotation rendering loop by calling ctx.restore() before every continue in the band/rule branches, including the branches around lines 329, 332, and 345–347. Ensure annotations that apply the connector-target clip restore that state before being skipped, while preserving the existing rendering behavior for processed annotations.python/xy/config.py (1)
22-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale predecessor-version reference, mirrors js/src/00_header.ts.
Same "v7 client" vs. expected "v8 client" issue as
js/src/00_header.ts.🤖 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/config.py` around lines 22 - 24, Update the predecessor-version reference in the comment above PROTOCOL_VERSION from v7 to v8, keeping the rest of the protocol-version explanation unchanged.python/xy/pyplot/_axes.py (3)
539-542: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Symlog minor locator collapses onto the major decades.
SymmetricalLogLocator.__init__mapssubs=None→(1.0,)(_ticker.pyline 285), so passingspec.get("subs")(normallyNone) builds a minor locator that returns exactly the major decade set._apply_tickersthen drops minors coinciding with majors, leaving symlog axes with no minor ticks or minor grid at all. Matplotlib's minorSymmetricalLogLocatordefaults tosubs=np.arange(1.0, base).Note the sibling
LogLocatorin this same change treatssubs=Noneas "automatic subdivisions"; the two locators now disagree on that sentinel.🐛 Proposed fix (either pass explicit subs here, or align the locator default)
"minor_locator": SymmetricalLogLocator( **locator_options, - subs=spec.get("subs"), + subs=spec.get("subs") or tuple(float(s) for s in np.arange(2.0, spec["base"])), ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."minor_locator": SymmetricalLogLocator( **locator_options, subs=spec.get("subs") or tuple(float(s) for s in np.arange(2.0, spec["base"])), ),🤖 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 539 - 542, Update the symlog minor locator configuration in the axes ticker setup so a missing subs value uses automatic subdivisions rather than SymmetricalLogLocator’s degenerate (1.0,) default. Preserve explicitly configured spec.get("subs") values, and align the behavior with the sibling LogLocator handling so minor ticks remain distinct from major decades.
3313-3317: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Which tests/galleries exercise annotate(zorder=...) and in what call order? rg -nP -C6 'annotate\([^)]*zorder' --type=pyRepository: reflex-dev/xy
Length of output: 151
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant implementation section with line numbers. sed -n '3260,3335p' python/xy/pyplot/_axes.py # Find annotate call sites, especially any with zorder. rg -n -C 4 'annotate\(' python/xy/pyplotRepository: reflex-dev/xy
Length of output: 8973
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file structure first, then inspect annotate-related symbols. ast-grep outline python/xy/pyplot/_axes.py --view expanded || true rg -n -C 3 '_zorder|annotation_entries|host\._entries\.sort|def annotate|class .*Axes' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 18141
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant implementation slice with line numbers. nl -ba python/xy/pyplot/_axes.py | sed -n '3285,3325p' # Search for documentation or tests mentioning zorder on annotate. rg -n -C 3 'annotate\(.*zorder|zorder=.*annotate|_zorder|annotation_entries' .Repository: reflex-dev/xy
Length of output: 191
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find all places that mutate host._entries or apply z-order sorting. rg -n -C 3 'host\._entries|_entries\.append|_entries\.extend|_entries\.insert|_entries\.sort|_zorder' python/xy/pyplot/_axes.py # Show nearby context around the annotation code path. nl -ba python/xy/pyplot/_axes.py | sed -n '3190,3335p'Repository: reflex-dev/xy
Length of output: 5242
annotate(zorder=...)reorders only existing entries
Becausehost._entriesis sorted only insideannotate, any artist added afterward keeps its append position even if its zorder should place it before the annotation. If this is meant to mirror Matplotlib, z-order needs to be applied when entries are added or at draw time.🤖 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 3313 - 3317, Move z-order ordering out of the existing-entry-only branch in annotate and into the entry-addition or draw path used by the host axis. Ensure host._entries is reordered by each entry’s _zorder whenever artists are added or rendered, so entries added after annotate(zorder=...) are positioned correctly while preserving existing annotation z-order behavior.
3555-3557: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Any path writing a log-axis domain without the set_*lim positivity guard? rg -nP -C3 '\["domain"\]\s*=' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 7233
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## outline: _axes.py\n' ast-grep outline python/xy/pyplot/_axes.py --view expanded | sed -n '1,220p' printf '\n## lines around _aspect_coordinates and get_position\n' sed -n '3530,3605p' python/xy/pyplot/_axes.py printf '\n## lines around _build_chart call sites\n' sed -n '6910,6975p' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 18626
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## more callers and domain setters\n' sed -n '6975,7145p' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 9663
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## grep for get_position usage\n' rg -n "get_position\(" python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 548
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## any tests or specs around log aspect / domain guards\n' rg -n "log aspect|log.*domain|aspect" python/xy -g '*test*' -g '*.py'Repository: reflex-dev/xy
Length of output: 11613
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## _figure.py log-domain validation\n' sed -n '240,380p' python/xy/_figure.py printf '\n## aspect setup and current-bounds capture\n' sed -n '4630,4675p' python/xy/pyplot/_axes.py printf '\n## image / extent path around log handling\n' sed -n '2670,2765p' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 13507
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## explicit domain writes in _axes.py that may skip validation\n' sed -n '1790,1835p' python/xy/pyplot/_axes.py sed -n '2960,2995p' python/xy/pyplot/_axes.py sed -n '4318,4655p' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 17978
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## image domain handling in _plot_types.py\n' sed -n '4040,4120p' python/xy/pyplot/_plot_types.py printf '\n## extent handling in imshow / image helpers\n' rg -n "_image_extent|extent|log_domain" python/xy/pyplot/_axes.py python/xy/pyplot/_plot_types.pyRepository: reflex-dev/xy
Length of output: 13626
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '4040,4120p' python/xy/pyplot/_plot_types.pyRepository: reflex-dev/xy
Length of output: 3731
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## image extent helpers\n' rg -n "_image_extent|_mpl_domain|extent" python/xy/pyplot/_axes.py python/xy/pyplot/_plot_types.py | sed -n '1,120p'Repository: reflex-dev/xy
Length of output: 9747
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## imshow implementation around extent/domain writes\n' sed -n '2670,2775p' python/xy/pyplot/_axes.py printf '\n## any direct use of _mpl_domain in image code\n' rg -n "_mpl_domain|log_domain|image.*domain|extent" python/xy/pyplot/_axes.py python/xy/pyplot/_plot_types.pyRepository: reflex-dev/xy
Length of output: 14858
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## _image_extent helper and nearby image logic\n' rg -n "_image_extent|_mpl_domain|log_domain" python/xy/pyplot/_axes.py python/xy/pyplot/_plot_types.py sed -n '3850,3915p' python/xy/pyplot/_axes.py sed -n '4048,4118p' python/xy/pyplot/_plot_types.pyRepository: reflex-dev/xy
Length of output: 7314
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "_aspect_bounds\s*=" python/xy/pyplot/_axes.py python/xy/pyplot/_artists.py python/xy/pyplot/_plot_types.pyRepository: reflex-dev/xy
Length of output: 886
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## entry value / auto-domain logic\n' sed -n '3710,4035p' python/xy/pyplot/_axes.py printf '\n## autoscale/tight-domain callers around the domain write\n' sed -n '4308,4345p' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 18699
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## axis() and square/equal handling\n' sed -n '4047,4125p' python/xy/pyplot/_axes.py printf '\n## set_aspect() current-bounds path\n' sed -n '4159,4205p' python/xy/pyplot/_axes.pyRepository: reflex-dev/xy
Length of output: 5715
Guard
_aspect_boundsbefore box-aspect layout_aspect_coordinates()will raise on nonpositive log bounds, so any log axis that stores raw extents into_aspect_boundscan fail later inget_position()instead of at assignment time.autoscale(tight=True)and image extents are the risky paths here.🤖 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 3555 - 3557, Validate log-scale bounds before storing raw extents in _aspect_bounds, covering both autoscale(tight=True) and image-extent assignment paths. Reuse the positivity and finiteness validation expected by _aspect_coordinates() so invalid bounds raise at assignment time rather than later during get_position() box-aspect layout.tests/pyplot/test_gallery_hist_errorbar_compat.py (1)
328-331: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 331's stroke check is tautological.
The cap entries never carry a
strokekey, socap["kwargs"].get("stroke", "red")always yields the hard-coded"red"default and the middle comparison can never fail — it would pass even ifecolorwere ignored. Assert only what the entry actually stores (or addstroketo the cap kwargs if caps are meant to expose it).💚 Tighten the assertion
- assert cap["kwargs"]["color"] == cap["kwargs"].get("stroke", "red") == "red" + assert cap["kwargs"]["color"] == "red" + assert "stroke" not in cap["kwargs"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for cap in (horizontal, vertical): assert cap["_mpl_line_marker_path_points"] == 10.0 assert cap["_mpl_line_marker_stroke_points"] == plt.rcParams["lines.markeredgewidth"] assert cap["kwargs"]["color"] == "red" assert "stroke" not in cap["kwargs"]🤖 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_gallery_hist_errorbar_compat.py` around lines 328 - 331, Update the cap color assertion in the loop over horizontal and vertical caps so it validates the actual stored color without relying on the hard-coded default from kwargs.get("stroke", "red"). Either assert the entry’s existing color field directly or explicitly populate stroke in the cap kwargs if that is the intended contract.
2 participants
Summary
This groups the pie-wedge and annotation-connector fixes needed by the selected Matplotlib pie gallery:
annotate(zorder=...);connectionstyle="angle"as a sharp elbow whileangle3remains quadratic;Visual comparison
This image is stored on immutable review-only evidence commit
683199b; it is not part of this PR's diff orpr-assets.The before rendering exposes pie-triangulation seams and reduces explicit wedge legend handles to line swatches. The after rendering uses clean joined sectors and patch swatches like Matplotlib.
Exact gallery result
The exact adapted
pie_and_polar_charts/pie_and_donut_labels.pynow emits both figures:On main, the exact run emitted only the first figure and then stopped at
annotate(zorder=0)with an unsupported-keyword error. After the fix, the donut connectors and labels remain within the figure bounds and closely match the Matplotlib reference.Verification
After rebasing onto current main:
git diff --check: passedBefore the resource-limit adjustment, the same change also passed:
tests/pyplot: 799 passed, 65 skipped, 1 deselectedNo additional Chrome/Playwright runs were launched after the resource constraint.
Summary by CodeRabbit