Skip to content

Fix PDF path collection culling for hexbin offsets #32000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yuzie007 wants to merge 3 commits into
base: main
Choose a base branch
from
+45 −31
Open

Fix PDF path collection culling for hexbin offsets #32000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb520f4
Fix to respect actual positions of offset paths
yuzie007 Jul 4, 2026
3e5af6f
Add `test_hexbin_negative_offsets`
yuzie007 Jul 5, 2026
8ffabe7
Avoid dictionary look-up for a single path
yuzie007 Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 18 additions & 31 deletions lib/matplotlib/backends/backend_pdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2011,19 +2011,20 @@ def draw_path_collection(self, gc, master_transform, paths, all_transforms,
name = self.file.pathCollectionObject(
gc, path, transform, padding, filled, stroked)
path_codes.append(name)
# Compute the extent of each marker path to enable per-marker
# bounds checking. This allows us to skip markers that are
# completely outside the visible canvas while preserving markers
# that are partially visible.
if len(path.vertices):
bbox = path.get_extents(transform)
# Store half-width and half-height for efficient bounds checking
path_extents.append((bbox.width / 2, bbox.height / 2))
else:
path_extents.append((0, 0))
# Compute each transformed path's exact bounds for per-marker
# canvas checks. Offsets are not necessarily full canvas-space
# centers, e.g. for collections using AffineDeltaTransform, so
# cull based on the final path bounds translated by the offset.
path_extents.append(path.get_extents(transform).frozen())

# Create a mapping from path_id to extent for efficient lookup
path_extent_map = dict(zip(path_codes, path_extents))
# for cases with multiple paths
if len(path_codes) == 1:
single_path_extent = path_extents[0]
path_extent_map = None
else:
single_path_extent = None
path_extent_map = dict(zip(path_codes, path_extents))

canvas_width = self.file.width * 72
canvas_height = self.file.height * 72
Expand All @@ -2036,26 +2037,12 @@ def draw_path_collection(self, gc, master_transform, paths, all_transforms,
facecolors, edgecolors, linewidths, linestyles,
antialiaseds, urls, offset_position, hatchcolors=hatchcolors):

# Optimization: Fast path for markers with centers inside canvas.

@tacaswell tacaswell Jul 10, 2026 •
edited by QuLogic
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have any benchmarks on what we are giving up performance-wise here?

@yuzie007 yuzie007 Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree on your concern and would like to suggest an alternative approach as follows.

If I understand correctly, the "optimization" here essentially meant to avoid the look-up of the path_extent_map dictionary.
We can actually avoid this look-up in a different viewpoint.
For most use cases of Collection via scatter and hexbin, the paths list has actually only a single element.
In such a case, we do not have to make a dictionary and do not need to look it up.

In the newly added commit, I implement the idea above. This should retain the original "optimization" in the sense of avoiding the dictionary look-up.

# This avoids the dictionary lookup for the common case where
# markers are visible, improving performance for large scatter plots.
if 0 <= xo <= canvas_width and 0 <= yo <= canvas_height:

@tacaswell tacaswell Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to look at offset_trans condition the fast path on that as well (my suspicion is that this is Identity by default and that is the only safe case).

Alternatively, would it make sense to apply the offset_trans when doing the check?

@yuzie007 yuzie007 Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as I understood from RendererBase._iter_collection, x0 and y0 are the coordinates already after application of offset_trans.
In most typical use cases of scatter and hexbin, offset_trans would be transData.

It is anyway probably wrong to refer only to the offset values without seeing the reference path extent, so I feel it may be better to revise this part substantially.

# Marker center is inside canvas - definitely render it
self.check_gc(gc0, rgbFace)
dx, dy = xo - lastx, yo - lasty
output(1, 0, 0, 1, dx, dy, Op.concat_matrix, path_id,
Op.use_xobject)
lastx, lasty = xo, yo
continue

# Marker center is outside canvas - check if partially visible.
# Skip markers completely outside visible canvas bounds to reduce
# PDF file size. Use per-marker extents to handle large markers
# correctly: only skip if the marker's bounding box doesn't
# intersect the canvas at all.
extent_x, extent_y = path_extent_map[path_id]
if not (-extent_x <= xo <= canvas_width + extent_x
and -extent_y <= yo <= canvas_height + extent_y):
# Skip markers completely outside the canvas to reduce PDF size.
# Use the translated path bounds, not the offset alone: the offset
# need not be the marker center in canvas coordinates.
bbox = path_extent_map[path_id] if path_extent_map else single_path_extent
if (bbox.x1 + xo < 0 or bbox.x0 + xo > canvas_width
or bbox.y1 + yo < 0 or bbox.y0 + yo > canvas_height):
continue

self.check_gc(gc0, rgbFace)
Expand Down
27 changes: 27 additions & 0 deletions lib/matplotlib/tests/test_backend_pdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,3 +732,30 @@ def test_scatter_polar(fig_test, fig_ref):
ax_ref = fig_ref.subplots(subplot_kw={'projection': 'polar'})
ax_ref.scatter(theta, r, c=c, s=50)
ax_ref.set_ylim(0, 2)


@check_figures_equal(extensions=["pdf"], tol=1.0)
def test_hexbin_negative_offsets(fig_test, fig_ref):
"""
Test path collection culling with non-canvas-space offsets.

Hexbin uses a repeated polygon path with offsets transformed by
AffineDeltaTransform. The transformed offsets are displacements, not the
final canvas-space marker centers, so PDF backend culling must account for
the transformed path bounds as well.
"""
rng = np.random.default_rng(19680801)
x = rng.normal(size=10_000)
y = rng.normal(size=10_000)

ax_test = fig_test.subplots()
ax_test.hexbin(x, y, gridsize=25, edgecolors="none")
ax_test.set_xlim(-2.0, 2.0)
ax_test.set_ylim(-2.0, 2.0)
ax_test.set_axis_off()

ax_ref = fig_ref.subplots()
ax_ref.hexbin(x + 2.0, y + 2.0, gridsize=25, edgecolors="none")
ax_ref.set_xlim(0.0, 4.0)
ax_ref.set_ylim(0.0, 4.0)
ax_ref.set_axis_off()
Loading