Fix PDF path collection culling for hexbin offsets - #32000
Conversation
Sorry, something went wrong.
| # Optimization: Fast path for markers with centers inside canvas. | ||
| # 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: |
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| facecolors, edgecolors, linewidths, linestyles, | ||
| antialiaseds, urls, offset_position, hatchcolors=hatchcolors): | ||
|
|
||
| # Optimization: Fast path for markers with centers inside canvas. |
There was a problem hiding this comment.
Do we have any benchmarks on what we are giving up performance-wise here?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
yuzie007
left a comment
There was a problem hiding this comment.
Thank you very much @tacaswell for your kind review, and I am so sorry for my late response.
Motivated by your comments, I have added one new commit, which I hope to solve the concerns. Could you review once again when convenient for you and check if what I wrote makes sense or maybe I misunderstand something?
Sorry, something went wrong.
| facecolors, edgecolors, linewidths, linestyles, | ||
| antialiaseds, urls, offset_position, hatchcolors=hatchcolors): | ||
|
|
||
| # Optimization: Fast path for markers with centers inside canvas. |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| # Optimization: Fast path for markers with centers inside canvas. | ||
| # 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: |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
PR summary
This PR fixes a PDF backend regression where visible hexbin cells can be incorrectly skipped in Matplotlib 3.11.0.
Closes #31999.
PR #30746 added path collection culling to the PDF backend to avoid bloated PDFs when many colored scatter markers are fully outside the canvas. That optimization works for scatter, where transformed offsets correspond to marker centers in canvas coordinates. However, hexbin uses a repeated hexagon path with offsets transformed by AffineDeltaTransform(self.transData). Those transformed offsets are displacements, not absolute canvas-space marker
centers.
The PDF backend was culling collections by checking the transformed offset plus an approximate marker extent. For hexbin, this can classify visible cells as off-canvas, especially when offsets are negative, so the PDF output loses part of the plot while raster output renders correctly.
This PR keeps the optimization from #30746, but changes the culling check to use the actual transformed reusable path bounds translated by each offset. This preserves PDF size improvements for truly off-canvas scatter markers while correctly rendering hexbin and other collections whose transformed offsets are not absolute marker centers. This fix make the example in #31999 work as expected.
A PDF regression test for hexbin with negative offsets, compared against an equivalent shifted reference.
AI Disclosure
Agentic AI was employed to identify the origin of the issue, make the original fix, add the original test, and write the original issue and the PR description. The AI suggestions were reviewed and polished by human.
PR checklist