Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Change {FigureCanvasAgg,RendererAgg}.buffer_rgba to return a memoryview.#11735
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The `buffer_rgba` method now allows direct access to the renderer'sunderlying buffer (as a `(m, n, 4)`-shape memoryview) rather thancopying the data to a new bytestring. This is consistent with thebehavior on Py2, where a buffer object was returned.While this is technically a backwards-incompatible change, memoryviewsare in fact quite compatible with bytes for most uses, and I'd arguethat the bigger compatibility break was the change from no-copy in Py2to copy in Py3 (after all that's the main point of the method...).
pelson left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice to remove some of the old backend_agg code here, especially as it is being replaced by built-in memoryviews.
I haven't looked at the impact of downstream users oftostring_argb etc., I assume that PIL happily consumesnumpy.ndarray.tobytes and memoryviews, and that PIL will be the primary target for most direct users of these methods.
| # grab the pixel buffer and dump it into a numpy array | ||
| X=np.array(fig.canvas.renderer._renderer) | ||
| X=np.array(fig.canvas.renderer.buffer_rgba()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wowzers. Good catch.
| deftostring_argb(self): | ||
| returnself._renderer.tostring_argb() | ||
| returnnp.asarray(self._renderer).take([3,0,1,2],axis=2).tobytes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you take a look at uses of this andtostring_rgb on things like StackOverflow to confirm that the type change would continue to work as expected in those use-cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here there is no change to the return type, it was a bytes, it's still a bytes.
There's a change in the return type ofbuffer_rgba() but that's sort of the whole point of the PR (and it goes back to the old Py2 behavior, and is also clearly better for performance).
timhoffm commentedDec 25, 2018
Not sure about the squashing/merging policy. I'd say the changes are unrelated and can go in as separate PRs. However, I can only "squash and merge" or "create a merge commit". Naturally I'd use "rebase and merge", but that's not enabled for the repo. |
QuLogic commentedDec 26, 2018
Why would you want to rebase and merge? It's the same as just merging, but more annoying. |
timhoffm commentedDec 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@QuLogic Not quite sure , if it would really be working the way I intended. But with a rebase the merge could be fast forward, so that the revision history stays more linear. |
Uh oh!
There was an error while loading.Please reload this page.
The
buffer_rgbamethod now allows direct access to the renderer'sunderlying buffer (as a
(m, n, 4)-shape memoryview) rather thancopying the data to a new bytestring. This is consistent with the
behavior on Py2, where a buffer object was returned.
While this is technically a backwards-incompatible change, memoryviews
are in fact quite compatible with bytes for most uses, and I'd argue
that the bigger compatibility break was the change from no-copy in Py2
to copy in Py3 (after all that's the main point of the method...).
See also discussion in#11726.
Added second commit: reimplement tostring_rgba, tostring_rgb using numpy to access the buffer. Note that this was one of the suggestions of the original removal of PyCXX (#3646) :-)
PR Summary
PR Checklist