Skip to content

Fill patches in add_patch and flatten their real geometry - #381

Merged
Alek99 merged 13 commits into
reflex-dev:mainfrom
michael-denyer:fix/add-patch-fill
Jul 30, 2026
Merged

Fill patches in add_patch and flatten their real geometry#381
Alek99 merged 13 commits into
reflex-dev:mainfrom
michael-denyer:fix/add-patch-fill

Conversation

@michael-denyer

@michael-denyer michael-denyer commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Axes.add_patch draws every patch as edge-only line segments. Its docstring admits the outline approximation, but the gap is wider than that, and I found two further problems while fixing the fill.

Measured against Matplotlib 3.11.1:

Patch xy before Correct
Rectangle(facecolor="tab:blue") hollow filled
Rectangle((1,2), 3, 4, angle=30) x spans 1.000..4.000, angle dropped x spans -1.000..3.598
Circle(radius=1) area 3.2509 3.1416
Ellipse(width=2, height=1, angle=20) area 3.2509 1.5708

The rotation and curve errors have the same root cause as the missing fill. add_patch reconstructs geometry by hand from get_x/get_y/get_width/get_height, which has nowhere to put angle, and falls back to raw path.vertices, which are Bezier control points rather than the curve. The ellipse is out by a factor of two because nothing applies the patch transform.

What changed

Geometry now comes from patch.get_path().to_polygons(patch.get_patch_transform()), the same flattening Matplotlib's own renderers use. It applies the transform and flattens curves, so one call replaces the hand-rolled branching and fixes all three symptoms at once.

Each ring then fills through the recipe Axes.fill already uses, kernels.polygon_triangles into a triangle_mesh mark with _joined_fill=True. No new mark and no Rust change.

Extraction and paint are now separate. _patch_outline returns data-space rings, _patch_fill_color returns the face color or None, and add_patch runs one fill pass and one outline pass over them. Conflating extraction with a single edge-only emit is what let three distinct bugs share one line of code.

A filled patch emits more than one mark, so add_patch returns a Patch handle that carries the outline entries and sweeps them in remove() and set_zorder(). This follows Wedge in _artists.py. Without it the change would regress remove(), which used to clear a patch completely and would otherwise leave a ghost outline behind.

Outlines now take the patch's own get_linewidth() instead of a fixed 1.0.

Preserved behaviour

Patches reporting fill=False, a "none" face color, or a fully transparent one stay edge-only. StepPatch still routes to stairs. Unsupported objects still raise TypeError. The axes color cycle is never advanced, because the fill only ever uses the patch's own face color. A degenerate ring such as a zero-height Rectangle has no triangulation, so it draws its edge and skips the fill rather than raising.

Evidence

tests/pyplot goes from 1174 to 1183 passing, 3 skipped, and make check passes with 3443 passing. The ty advisory count is 77 both before and after.

Nine tests added, five of which fail against main:

FAILED test_added_rectangle_patch_fills_with_its_own_face_color
FAILED test_added_rotated_rectangle_keeps_its_rotation
FAILED test_added_ellipse_flattens_its_curve_under_the_patch_transform
FAILED test_added_concave_polygon_patch_triangulates_its_true_area
FAILED test_removing_a_filled_patch_takes_its_outline_with_it

The other four are regression guards that pass either way. The color-cycle guard is one of them, so a plain before-and-after run proves nothing about it. Swapping resolve_color(face) for self._next_patch_color() makes it fail with + #ff7f0e, which is the mistake worth guarding given how close _next_patch_color sits to this code.

Rendered output, not just entry structure. Exporting a rotated Rectangle, a concave Polygon, and an Ellipse at 100 dpi and counting pixels by color, using the polygon as the scale reference:

rot_rect  18808 px, true area 12.0000, predicted 18616, 1.03% error
concave   15513 px, true area 10.0000, predicted 15513, 0.00% error
ellipse    2413 px, true area  1.5708, predicted  2437, 0.98% error

Area alone cannot prove rotation, since an unrotated 3x4 rectangle also covers 12. Measuring the rectangle's x extent against the polygon's known 4-unit span gives 4.588 data units. Rotated is 4.598 and unrotated would be 3.000. In SVG the concave shape arrives as a single 5-vertex <polygon> rather than three triangles, which confirms _joined_fill suppresses internal seams through to the exported file.

Two related bugs left alone

Axes.fill orphans its own edge mark the same way, and filled stairs orphans its hatch mark. Both predate this change, and Patch now gives them a precedent to copy. Fixing them here would span concerns the contributor guide asks to keep apart.

Separately, add_patch ignores get_label() entirely, before and after this change, so a Rectangle(label="region") used as a legend proxy never appears. That is a feature gap rather than a regression.

Docs

spec/matplotlib/compat-changelog.md gets a dated entry, CHANGELOG.md a Fixed entry, and spec/matplotlib/compat.md one line, since curve flattening through to_polygons belongs in its list of accepted visual approximations. spec/matplotlib/shim-todo.md needs nothing. Line 396 already reads "wider add_patch / add_collection mappings for common Matplotlib objects", which stays true.

No benchmarks. This is a correctness fix and makes no performance claim.

Summary by CodeRabbit

  • Bug Fixes
    • Improved patch rendering: fills are applied only when enabled and appropriate; fill=False, "none", or fully transparent faces stay outline-only.
    • Enhanced patch geometry handling (including curved shapes), with more robust vertex deduplication to avoid ring collapse from non-finite values.
    • Patch outlines now respect the patch linewidth, scaled for the output DPI.
  • New Features
    • Axes.add_patch now returns a composite patch handle so remove()/set_zorder() affect the full patch.
  • Compatibility
    • Matplotlib-like nested-ring “hole” behavior: holes are not filled.
  • Tests
    • Added compat tests for non-finite rectangle geometry edge cases.

add_patch renders every patch as edge-only line segments: no fill, angle
dropped, and curved patches measured from raw Bezier control points. The
returned handle also has to own the outline so remove() clears the whole
patch, and the fill must never touch the axes color cycle. These tests pin
that behaviour and fail against the current implementation.
Extract data-space rings with Path.to_polygons(patch.get_patch_transform()),
matplotlib's own flattening call, then fill each ring with the patch's own
face color using the triangle-mesh recipe Axes.fill already uses. Rotation
and curvature now survive: Rectangle(angle=30) keeps its rotation and
Ellipse(width=2, height=1) covers pi*a*b rather than its control-point hull.

Unfilled patches, a "none" face color, and a fully transparent one stay
edge-only, degenerate rings draw their edge without raising, and the patch's
own face color means the axes color cycle is never advanced.

A patch now emits several marks, so add_patch returns a Patch handle that
carries the outline entries as companions and sweeps them in remove() and
set_zorder(), following the Wedge precedent in _artists.py.
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Axes.add_patch now preserves transformed patch geometry, renders filled and unfilled patches appropriately, handles degenerate and nested-ring shapes, and returns a composite handle managing fill and outline entries. Tests and compatibility documentation cover the updated behavior.

Changes

Patch rendering and lifecycle

Layer / File(s) Summary
Transformed patch geometry and rendering
python/xy/pyplot/_axes.py
Axes.add_patch derives transformed rings, triangulates eligible fills, emits DPI-scaled outlines, and handles curves, rotation, holes, disjoint rings, and degenerate shapes.
Composite patch handle lifecycle
python/xy/pyplot/_artists.py, python/xy/pyplot/_axes.py, tests/pyplot/test_launch_compat.py
The returned Patch handle owns fill and outline entries and applies removal and z-order changes across the complete patch.
Patch geometry and compatibility validation
tests/pyplot/test_launch_compat.py
Tests cover fill meshes, transformed geometry, outline styling, ring behavior, lifecycle behavior, color-cycle handling, deduplication, and unsupported inputs.
Compatibility and changelog updates
CHANGELOG.md, spec/matplotlib/...
Documentation records patch flattening, fill and hole behavior, DPI-scaled outlines, degenerate rings, and composite handles.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant AxesAddPatch
  participant PatchOutline
  participant AxesEntries
  participant Patch
  Caller->>AxesAddPatch: add_patch
  AxesAddPatch->>PatchOutline: derive transformed rings and fill state
  PatchOutline-->>AxesAddPatch: normalized rings
  AxesAddPatch->>AxesEntries: create triangle_mesh and segments entries
  AxesAddPatch-->>Caller: return Patch handle
  Caller->>Patch: remove()
  Patch->>AxesEntries: remove owned entries
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 24.24% 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 change: add_patch now fills patches and uses flattened real geometry.
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 unit tests (beta)
  • Create PR with unit tests

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

@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: 2

🤖 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 5629-5630: Update the patch outline width handling near edge and
width extraction to convert linewidth from points to pixels using
self._point_scale(). Apply the same conversion to all outline segment entries
created below, and add a DPI-sensitive regression test confirming consistent
rendered stroke widths.
- Around line 5633-5653: Update the fill handling around the ring loop and
triangle generation so compound paths preserve interior holes instead of
triangulating each ring independently. Ensure PathPatch holes and Wedge annuli
honor the path’s fill rule while retaining the existing outline rendering, and
add regression coverage for both hole-preserving cases.
🪄 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: 19a87fe1-4eea-4575-a28c-6380c29d99a6

📥 Commits

Reviewing files that changed from the base of the PR and between bd1d36e and 9701e6d.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • python/xy/pyplot/_artists.py
  • python/xy/pyplot/_axes.py
  • spec/matplotlib/compat-changelog.md
  • spec/matplotlib/compat.md
  • tests/pyplot/test_launch_compat.py

Comment thread python/xy/pyplot/_axes.py Outdated
Comment thread python/xy/pyplot/_axes.py
Three defects surviving the fill work. The outline width goes to the mark in
Matplotlib points where it wants pixels, so the stroke ignores figure DPI. A
compound path with nested rings paints its hole solid, 116 for a true 84. A
full-circle Wedge loses its fill entirely, because Matplotlib repeats a vertex
in that path and the triangulator's rejection is swallowed.

Two of the six are guards rather than repros: an annular sector and a pair of
disjoint rings must keep filling, so they pass before and after.
Three fixes that have to land together.

Outline width now multiplies by _point_scale(), the shim's points-to-pixels
conversion used at 39 other sites. A 5 pt stroke measures 5 px at 72 dpi and
13 px at 200 dpi, matching ax.plot.

_patch_outline drops consecutive duplicate vertices, not just a trailing one.
Matplotlib repeats a vertex inside its full-circle paths, so a Wedge annulus
reached polygon_triangles carrying a duplicate, was rejected, and the fill
vanished into the except ValueError. Rings keep their closing vertex, so the
outline pass still draws the closing edge.

Nested rings mean holes, and polygon_triangles takes one simple polygon, so
add_patch now abstains: it draws the outlines and skips the fill rather than
painting the hole solid. Containment is tested with kernels.polygon_select,
the native even-odd ray cast the lasso already uses. Disjoint rings still fill
both, and an annular sector still fills because Matplotlib returns it as one
ring. Without the dedupe this abstention would be untested, and without the
abstention the dedupe would fill an annulus as two solid discs.
@michael-denyer

Copy link
Copy Markdown
Contributor Author

Both findings tested rather than taken on trust. One is right as stated, one is right about the defect and wrong about the example, and chasing the second turned up a third bug neither of us had. All three are fixed in d7b441d.

Stroke units, confirmed

Right. Thickness in pixels of a 5 pt stroke, measured through the centre column of a rendered PNG:

                before                     after
line   dpi= 72  [5]      dpi=200 [13]      unchanged
patch  dpi= 72  [5, 5]   dpi=200 [5, 5]    dpi=72 [5, 5]   dpi=200 [13, 13]

ax.plot scaled, the patch outline did not move at all. Now multiplied by self._point_scale() and it tracks ax.plot exactly.

One correction to the framing. Released add_patch hardcoded width to 1.0, so it ignored DPI and dropped linewidth entirely. This PR's earlier commit read the right number in the wrong unit. It is now right in both, and the changelog says so.

Hole topology, right about the defect, wrong about the example

Wedge(..., width=...) was not a good case to cite. An annular sector comes back from to_polygons as a single ring that traces the outer arc and returns along the inner one, so it already filled correctly:

Wedge(0, 90, width=0.4)     1 mesh, area 0.5022, true 0.5027   correct before and after

Compound paths are the real case, and there the finding holds:

PathPatch square-with-hole  2 meshes, areas 100.0 and 16.0      paints 116 for a true 84

Not implementing hole support here. kernels.polygon_triangles takes one simple polygon and rejects anything else at the kernel boundary, so a hole needs either a bridged seam in Python, which _joined_fill would show through because a bridge is not a shared triangle edge, or a new kernel entry point. That is a maintainer's design call, not a review response.

Abstaining instead. When rings nest, the patch draws its outline and skips the fill, which is exactly the released behaviour, so nothing regresses and no known-wrong pixels get painted. Containment is the test, not ring count, so disjoint rings still fill. The check reuses kernels.polygon_select, the even-odd cast already shipped for lasso selection, rather than hand-rolled ray casting.

                            before                    after
square with square hole     2 meshes, 100.0 and 16.0  0 meshes, 2 outlines
disjoint squares            fills both                fills both
annular sector              1 mesh, 0.5022            unchanged

Third bug, found while testing the second

A full annulus emitted no fill at all, rather than the solid sector predicted:

Wedge(0, 360, width=0.4)    2 outlines, 0 meshes

Root cause. Matplotlib repeats a vertex inside its full-circle paths, so 33 vertices reached the triangulator carrying 32 unique points, and polygon_triangles rejects duplicates outright. The except ValueError around it then discarded the failure in silence.

Circle(1)              ring 0: 16 unique -> 14 triangles
Wedge 0-360 width .4   ring 0: 33 pts, 32 unique -> REJECTED
                       ring 1: 33 pts, 32 unique -> REJECTED

Rings now drop every consecutive duplicate, not just a trailing one, and both annulus rings triangulate. They still end up outline-only, but now because the nesting check abstains rather than because an exception vanished. The regression test asserts the ring vertex count for that reason, since meshes == [] alone cannot tell the two causes apart.

Verification

tests/pyplot 1183 to 1189 passing, 3 skipped. make check passes, 8 checks, 3449 passing. The ty advisory count is 77, identical to base, so the PR still adds none.

Six tests added, four failing against the previous head:

FAILED test_patch_outline_width_converts_points_to_pixels
FAILED test_patch_outline_stroke_thickens_with_dpi_in_the_png
FAILED test_patch_with_nested_rings_abstains_from_filling_the_hole
FAILED test_full_annulus_draws_both_outlines_without_raising

The two that pass either way guard the abstention against over-reaching, so passing before and after is the signal they exist for.

Known limitation, now documented

Nested rings never fill. A compound PathPatch with a hole and a full-circle Wedge with a width draw outlines only. Recorded in the compat changelog, the accepted-approximations line in compat.md, the docstring, and CHANGELOG.md.

Unrelated, spotted while here

Axes.fill passes its edge linewidth to the mark unscaled at _plot_types.py:1692, the same points-versus-pixels bug found here. It also orphans its edge mark on remove(), as does filled stairs with its hatch mark. All predate this PR and are left alone.

@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: 2

🤖 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`:
- Line 1113: Update the consecutive-vertex filtering expression in the
ring-processing logic to remove vertices only when they are exactly identical to
the preceding vertex, replacing the tolerance-based np.isclose comparison while
preserving the first vertex and existing ring output behavior.

In `@tests/pyplot/test_launch_compat.py`:
- Around line 298-309: Update
test_full_annulus_draws_both_outlines_without_raising to stop asserting that
each edge has exactly 32 vertices. Instead, assert the duplicate-removal
invariant by verifying each emitted outline has no duplicate endpoint, while
retaining the existing checks that meshes are empty and two edges are produced.
🪄 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: 91a35fcb-c836-4577-a48f-863425b9f3dd

📥 Commits

Reviewing files that changed from the base of the PR and between 9701e6d and d7b441d.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • python/xy/pyplot/_axes.py
  • spec/matplotlib/compat-changelog.md
  • spec/matplotlib/compat.md
  • tests/pyplot/test_launch_compat.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • spec/matplotlib/compat.md
  • CHANGELOG.md
  • spec/matplotlib/compat-changelog.md

Comment thread python/xy/pyplot/_axes.py Outdated
Comment thread tests/pyplot/test_launch_compat.py Outdated
…ssertion

The magnitude-relative duplicate test erases a patch drawn at genomic or
epoch-millisecond coordinates: a Rectangle at x=1e9 collapses from five
vertices to three and fills zero area, because 1e-5 of 1e9 is 10,000 data
units. Two new tests pin that and a genuinely tiny 1e-9 edge surviving.

A third pins the full-circle Wedge filling. Matplotlib's repeated vertex is
not bit-exact, so deduplicating only exact matches leaves it in place and the
triangulator rejects the whole disc. That test fails against an exact-equality
dedupe and against the state before any dedupe.

The full-annulus test no longer asserts a vertex count of 32, which hardcoded
Matplotlib's current tessellation. It asserts the invariant instead, that no
outline segment is a duplicate-length stub, measured against the ring's own
span so it stays scale-free. Comparing against zero would not do: a repeat
is floating-point noise at 2e-16, not an exact coincidence.
np.isclose is relative to coordinate magnitude, so at x=1e9 it treats two
vertices 10,000 data units apart as one and the patch disappears. Deduplicate
against an absolute tolerance derived from the ring's own bounding-box
diagonal instead.

Measured against Matplotlib 3.11.1, repeats sit at 9e-17 of the span while
the smallest real edge across the shapes we flatten sits at 2e-3 of it. The
threshold takes 1e-12 of the span, ten thousand times above the noise and a
billion times below the smallest real edge. Exact equality would not work
here: Matplotlib's repeated vertex differs in the last bits, so an exact test
catches nothing and the annulus loses its fill again.

The comparison is negated so a non-finite gap or span keeps its vertex. The
triangulator then rejects the ring on its own terms rather than us silently
emptying it.
@michael-denyer

Copy link
Copy Markdown
Contributor Author

Both fixed in 9e6490e and e79b1da. The first finding was right and the damage is worse than the example suggested, but the suggested remedy would have silently undone an earlier fix, so the implementation differs.

Tolerance-based deletion, confirmed

Worse than reported. np.isclose scales to coordinate magnitude, so at x = 1e9 it treats anything within 10,000 data units as one vertex. Through the real code path, not a simulation:

Rectangle(x=1e9, width=5000, height=10)
  before   outline verts [3]   mesh area     0.0
  after    outline verts [5]   mesh area 50000.0

The patch did not degrade, it disappeared. Genomic coordinates, epoch milliseconds and financial ticks all sit in that range.

Why the suggested fix is not the one applied

Deduplicating only exactly identical consecutive vertices catches nothing here. Matplotlib's repeat is floating-point noise, not a coincidence:

annulus ring0: exact_dupes=0  isclose_dupes=1
annulus ring1: exact_dupes=0  isclose_dupes=1

with exact-only dedupe:
  ring 0: 33 pts -> REJECTED (polygon must be finite, simple, and non-degenerate)
  ring 1: 33 pts -> REJECTED

That is the full-annulus bug from the previous round coming straight back, and it would come back silently.

What was applied

An absolute tolerance of 1e-12 times the ring's own bounding-box diagonal. Scaling to the ring rather than to coordinate magnitude satisfies both cases, and the threshold is not a judgement call. Gap between consecutive vertices as a fraction of ring span:

                    smallest      next smallest
annulus ring0       8.660e-17       6.957e-02
annulus ring1       8.660e-17       6.957e-02
circle              1.400e-01       1.400e-01
rect at x=1e9       2.000e-03       2.000e-03

Every repeat is noise at ~1e-16 of the span. The smallest real edge across every shape we flatten is 2e-3. Fourteen orders of magnitude apart, and the threshold sits in the middle. Both populations are recorded in the docstring so the constant stays re-derivable.

Multiplying by span rather than dividing the gaps means a zero-span ring needs no special case; the tolerance becomes zero and the test degrades to exact equality. The comparison is negated so a non-finite gap keeps its vertex and the ring is rejected by the triangulator on its own terms rather than being silently emptied.

After:

rect at x=1e9    rings=[5]       area 50000.0000   true 50000.0
full disc Wedge  rings=[33]      area     3.1414   true     3.1416
annulus          rings=[33, 33]  abstains, nested rings
tiny 1e-9 edge   rings=[6]       area     1.0000   true     1.0000

The pinned tessellation count

Right, and removed. One correction worth recording. The obvious replacement, asserting no zero-length segments, is a dead assertion: it passes against the pre-dedupe commit, because a matplotlib repeat is 2.4e-16 apart rather than coincident, so the stub segment has nonzero length. The test now asserts the shortest segment as a fraction of the outline's bounding diagonal exceeds 1e-6, which is scale-free, pins no number we do not control, and fails against the pre-dedupe state as intended.

Verification

tests/pyplot 1189 to 1192 passing, 3 skipped. make check passes, 8 checks, 3452 passing. ty still 77 diagnostics, unchanged from base. The three earlier fixes re-verified intact: patch stroke [5,5] at 72 dpi and [13,13] at 200, annular sector fills at 0.5022, square-with-hole abstains.

Three tests added and one rewritten, failing against the previous head on the collapse itself:

FAILED test_patch_at_genomic_coordinates_keeps_every_vertex
FAILED test_tightly_spaced_but_distinct_vertices_survive

test_full_disc_wedge_fills_despite_its_repeated_vertex guards against the exact-equality remedy specifically, so it passes at head by construction. Mutation-tested rather than trusted: it fails against exact-only dedupe and against pre-dedupe head, both with len(meshes) == 0.

No changelog entry for either. Both were introduced inside this unreleased PR and never shipped, so relative to released xy a patch at 1e9 coordinates worked before and works now.

@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: 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 `@python/xy/pyplot/_axes.py`:
- Around line 1123-1127: Update the ring filtering logic around the span
calculation to return the original ring immediately when span is non-finite.
Preserve the existing gap-based vertex filtering only for finite spans,
including the current behavior for non-finite gaps.
🪄 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: 26a6b03f-0574-41bd-aca5-a15edd0811c8

📥 Commits

Reviewing files that changed from the base of the PR and between d7b441d and e79b1da.

📒 Files selected for processing (2)
  • python/xy/pyplot/_axes.py
  • tests/pyplot/test_launch_compat.py

Comment thread python/xy/pyplot/_axes.py Outdated
An infinite coordinate makes the span infinite, so span * 1e-12 is infinite
too, every finite gap reads as a duplicate, and the ring collapses to one
vertex. NaN took the opposite path and worked by accident, because every
comparison against NaN is false.

Return the ring untouched when the span is not finite, which covers both and
lets the triangulator reject the geometry on its own terms. With that guard
in front, a finite span implies finite gaps, so the negated comparison the
NaN case needed is gone and the test is a plain greater-than again.

np.hypot is overflow-safe, so a ring at 1e200 still deduplicates normally;
infinity only reaches here when the caller's geometry already holds it.
@michael-denyer

Copy link
Copy Markdown
Contributor Author

Right, fixed in 2d66155. Reproduced by calling _without_repeats directly:

                 before          after
finite           5 -> 5          5 -> 5
NaN              5 -> 5          5 -> 5
+inf             5 -> 1          5 -> 5
-inf and +inf    5 -> 1          5 -> 5
1e200            5 -> 5          5 -> 5

An infinite span makes span * 1e-12 infinite, so every finite gap reads as a duplicate and the ring collapses to one vertex. NaN survived because comparisons against NaN are false, which is what the old comment described, so the comment was documenting half the behaviour. Early return on a non-finite span, as suggested, and the comment now says what the code does.

A genuine overflow does not reach this. np.hypot is overflow-safe, so a ring at 1e200 keeps a finite span. Infinity only arrives when the caller's geometry already contains it.

With the guard in place the negated comparison went back to a plain gaps > tolerance. The negation existed only to let NaN keep its vertex, and that job moved to the guard. It is now provably redundant rather than merely unused, since a non-finite coordinate propagates through max or min and forces a non-finite span, so a finite span implies finite gaps.

On the test

The obvious input does not reach the guard. Polygon([[0,0],[inf,0],[1,1],[0,1]]) never gets there, because matplotlib's to_polygons strips the non-finite vertex first:

inf raw rings: [[1.0, 1.0], [1.0, 1.0], [0.0, 1.0], [0.0, 0.0], [1.0, 1.0]]

Rectangle((0, 0), inf, 1.0) does reach it with all five vertices intact, so the test uses that and exercises the guard end to end through add_patch with a real matplotlib patch. Parametrised over inf, -inf and nan; both infinities fail against the previous head with the ring collapsed to a single vertex, and NaN passes either way, which is why it stays in the pair.

Verification

tests/pyplot 1192 to 1195 passing, 3 skipped. make check passes, 8 checks, 3455 passing. ty still 77 diagnostics, unchanged from base across all seven commits. Earlier rounds re-verified intact: the 1e9 rectangle keeps area 50000.0, the full disc fills at 3.1414, the annulus abstains, patch stroke scales 5 px at 72 dpi to 13 px at 200.

No changelog entry. This one was introduced by the span tolerance in e79b1da earlier in this PR and never shipped.

@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: 1

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)

5692-5693: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Avoid default-tolerance np.allclose for endpoint removal.

This bypasses _without_repeats’s ring-relative threshold. NumPy’s default tolerances can treat valid endpoints as equal at large coordinates or for tiny geometry, changing fill area and outline shape. Use exact equality or the same finite span * 1e-12 criterion, and add a near-but-distinct endpoint regression.

Suggested fix
-                if len(xv) > 2 and np.allclose((xv[0], yv[0]), (xv[-1], yv[-1])):
+                if len(xv) > 2 and xv[0] == xv[-1] and yv[0] == yv[-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/_axes.py` around lines 5692 - 5693, Replace the
default-tolerance np.allclose endpoint check in the polygon cleanup path with
exact coordinate equality or the established finite span * 1e-12 ring-relative
criterion used by _without_repeats. Preserve removal only for genuinely repeated
endpoints, and add a regression covering near-but-distinct endpoints so valid
geometry remains 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.

Inline comments:
In `@tests/pyplot/test_launch_compat.py`:
- Around line 365-377: Update the explanatory comment in
test_patch_with_a_non_finite_coordinate_keeps_its_vertices to state that
_without_repeats returns immediately for every non-finite span, preserving the
rectangle’s four vertices for inf, -inf, and nan. Remove the outdated claim that
infinite and NaN spans take opposite branches or that the infinite case empties
the ring.

---

Outside diff comments:
In `@python/xy/pyplot/_axes.py`:
- Around line 5692-5693: Replace the default-tolerance np.allclose endpoint
check in the polygon cleanup path with exact coordinate equality or the
established finite span * 1e-12 ring-relative criterion used by
_without_repeats. Preserve removal only for genuinely repeated endpoints, and
add a regression covering near-but-distinct endpoints so valid geometry remains
unchanged.
🪄 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: c6d6caf2-2d79-43d1-9ca7-d40dc1392ecf

📥 Commits

Reviewing files that changed from the base of the PR and between e79b1da and 2d66155.

📒 Files selected for processing (2)
  • python/xy/pyplot/_axes.py
  • tests/pyplot/test_launch_compat.py

Comment thread tests/pyplot/test_launch_compat.py
The comment claimed inf and NaN take opposite branches. They did before the
guard landed; now both hit the same early return, so it described the bug
rather than the fix and implied coverage of two paths where there is one.

The three cases still earn their place for a different reason: they are the
inputs that yield a span no tolerance can come from, and one predicate has to
catch all three. Narrowing it to isnan fails both infinities, narrowing it to
isinf fails the NaN.

Also disambiguate 'the test needs a tolerance' in _without_repeats, which
reads as a unit test rather than the duplicate comparison it means.
@michael-denyer

Copy link
Copy Markdown
Contributor Author

Right, fixed in 8701e42. The comment described the code the guard replaced. Both inf and nan now take the same early return, so "opposite branches" was false and the comment was justifying the parametrisation with a reason that no longer existed.

Rewritten to what is actually true:

# The tolerance is a fraction of the ring's span, and none of these three
# yield a usable one. They share a single guard, so what this pins is that
# its predicate catches all three: an isnan check would let the infinities
# through, an isinf check would let the NaN through.

Kept rather than deleted. The test name says the vertices survive but not why they would otherwise be lost, and "the tolerance is derived from the span" is not visible from the test body.

The new comment's claim is verified rather than asserted, by narrowing the guard in both directions:

guard narrowed to  if np.isnan(span):   -> FAILED [inf], FAILED [-inf]
guard narrowed to  if np.isinf(span):   -> FAILED [nan]

All three cases earn their place.

Audit of the rest

Checked every comment, docstring and doc line added across the eight commits, since a guard that changes behaviour can strand prose written against the previous version.

One more found, a wording ambiguity rather than staleness. _without_repeats said "the test needs a tolerance", which reads as a unit test in a function whose behaviour is pinned by a test file, when it means the duplicate comparison. Changed to "the comparison".

Nothing else was stale. Each remaining comment states a measured number or a mechanism that still reproduces: the 1.000..4.000 unrotated span, 3.2509 for control-point ellipse area, 116 against a true 84 for the hole, the annular sector tracing a single ring, the swallowed rejection on the annulus, 1e-5 of 1e9 being 10,000 data units, the non-bit-exact repeat defeating exact-equality dedupe, and np.isclose's 1e-8 floor swallowing a 1e-9 edge. _shortest_relative_edge's stated bounds of 1e-16 and 1e-3 still bracket the measured 8.66e-17 and 6.96e-2.

One line re-checked specifically. compat-changelog.md names a full-circle Wedge with a width as a hole case. The withdrawn prediction was that such a wedge renders as a solid disc; the text claims it abstains, which still measures true at rings=[33, 33] area=0.0000. Left as written.

Verification

No behaviour change and the numbers confirm it. tests/pyplot 1195 passed, 3 skipped, identical. make check passes, 8 checks, 3455 passing, identical. ty still 77, unchanged from base across all eight commits.

@codspeed-hq

codspeed-hq Bot commented Jul 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 103 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing michael-denyer:fix/add-patch-fill (cf4f9a6) with main (bd1d36e)

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.

@Alek99

Alek99 commented Jul 30, 2026

Copy link
Copy Markdown
Member

Thanks for the contribution! There are couple tweaks im going to push but overall looks pretty good

Alek99 added 2 commits July 30, 2026 12:11
…scale

A hidden patch still draws its outline, because set_visible reaches only the
first mark the patch produced; set_alpha, set_color, and set_transform share
the gap. A filled patch whose edge paints nothing, matplotlib's default,
emits an invisible segments mark per ring anyway. And to_polygons flattens in
data units, so Circle(radius=1) arrives as sixteen segments overshooting the
true radius by 2.5% while the identical circle at radius=1000 arrives smooth;
area assertions cannot see this because the overshoot between on-curve points
cancels the chord deficit.

Also pinned: a fill dropped for a reason the user should hear about, a ring
past the triangulator's 10,000-vertex cap or self-intersecting, warns instead
of going quietly hollow; a genuinely degenerate ring stays quiet; a patch
whose every mark would be invisible still leaves one entry for the handle to
stand on; and the output-scale round trip does not cost a small patch at a
large offset its area.

Existing tests that used the always-emitted outline as their observation
surface now ask for one explicitly with edgecolor=.
…scale

Four review fixes on the add_patch rework.

The Artist base gains a _companion_entries hook, and set_visible, set_alpha,
set_color, and set_transform now run over it, so a handle standing for
several marks moves all of them. Patch feeds its outline entries through the
hook; remove and set_zorder already did this by hand. set_color paints face
and edge alike, matching matplotlib's Patch.set_color.

_patch_edge_color mirrors _patch_fill_color: an edge that paints nothing,
which is matplotlib's default on a filled patch, emits no outline mark. That
was a fully transparent segments mark per ring, a third of the exported
payload for a figure of default rectangles. A patch that drew no body still
emits its outline so the handle has an entry to stand on; a width of zero
strokes no pixels, as in matplotlib.

_refine_at_pixel_scale re-flattens the path as though the patch filled the
figure, then undoes the scale. to_polygons subdivides until flat in the
coordinates it is handed, so flattening straight through the patch transform
took its tessellation from the numeric magnitude of the data: Circle(radius=1)
was a visible 16-gon overshooting the true radius by 2.5% while the same
circle at radius=1000 was smooth. The scale is anchored at the patch's own
corner because the round trip costs relative precision, which a small patch
at a large offset has little of to spare. Curved patches now carry ~10x the
triangles they did; straight-edged patches are unchanged. Applying the affine
to the control points before rebuilding the path is exact, and avoids the
matplotlib import the shim forbids.

The fill loop filters non-finite vertices first, as Axes.fill does, skips
true degenerates silently, and warns with the reason for anything else the
triangulator rejects: its 10,000-vertex cap and self-intersection both fell
into a bare except ValueError that read as a deliberate hollow style. The
closing-vertex test now uses the ring-relative tolerance _without_repeats
established rather than magnitude-relative np.allclose.

Docs no longer claim the flattening matches matplotlib's renderers; it
resolves at the figure's pixel size before the view is known, which is a
different mechanism with the same visual result.

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 6 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread python/xy/pyplot/_axes.py
Comment thread python/xy/pyplot/_axes.py
Comment thread python/xy/pyplot/_axes.py
Comment thread python/xy/pyplot/_axes.py Outdated
The curves these comments describe — Circle, Ellipse, Wedge, Arc paths — are
CURVE4 segments, and the repo writes Bézier with its accent everywhere else.

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 4 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread CHANGELOG.md Outdated
Alek99 added 2 commits July 30, 2026 13:00
Two cubic review findings confirmed and fixed, one documented, one already
settled in the thread.

Rectangle(..., transform=Affine2D().rotate_deg(45)) silently dropped its
rotation, because flattening went through get_patch_transform alone.
matplotlib's Patch.get_transform composes the artist transform in, so when
one is set and composes, that is what the flattening uses. xy's own transform
objects will not compose inside matplotlib: transData is the identity and is
accepted as the no-op it is, a data-space affine applies to the flattened
rings afterwards, and transAxes/transFigure raise NotImplementedError with
the same message _transform_points gives every other data artist, since
baked fractions go silently stale on the next limit change.

_rings_are_nested tested one vertex, so rings that merely overlap read as
nested and the whole patch went hollow. Nested now means every vertex of one
ring inside another. Overlapping rings fill ring-by-ring, their union, where
matplotlib's even-odd rule would leave the intersection unpainted; painting
what each ring alone would have painted errs on the visible side. The Wedge
annulus still abstains, since its inner ring is wholly inside its outer.

The tessellation-staleness finding is real and documented instead of fixed:
geometry is built when the patch is added, so a later figure resize, DPI
change, or deep zoom reuses the tessellation chosen then. Re-flattening at
materialization is an architecture change, not a review fix. The hole-fill
finding was already deliberately deferred in the review thread, with the
abstention documented in compat.md.
The hyphen crept in as a compound modifier; the term is written open in the
graphics literature and in this repo's own prior usage.
@Alek99
Alek99 merged commit d6c0ef1 into reflex-dev:main Jul 30, 2026
24 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