Fix PDF path collection culling for hexbin offsets - #32000
Conversation
1cec52b to
3e5af6f
Compare
| # 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?
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.
| 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?
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.
yuzie007
left a comment
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?
| 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.
| # 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.
3 participants
PR summary
This PR fixes a PDF backend regression where visible
hexbincells 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
scattermarkers are fully outside the canvas. That optimization works forscatter, where transformed offsets correspond to marker centers in canvas coordinates. However,hexbinuses a repeated hexagon path with offsets transformed byAffineDeltaTransform(self.transData). Those transformed offsets are displacements, not absolute canvas-space markercenters.
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
scattermarkers while correctly renderinghexbinand 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
hexbinwith 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
Documentation complies with general and docstring guidelines