| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
|
@tacaswell would you mind checking this? |
Sorry, something went wrong.
|
This looks reasonable, but had a few comments. Good use of a context manager. Could you please also add a test as well? |
Sorry, something went wrong.
|
Could you compare this approach to #18932 and why you think this is a better path @hukaidong ? |
Sorry, something went wrong.
Thanks, this does make sense. I'll append a test soon. |
Sorry, something went wrong.
I see quite amount of refraction tries to reduce the amount of method provided in artists class, which is not relevant to this topic. So I put them apart. For the work refractoring do_3d_projection, it saves space from storing offset with a cost re-computing projection transform twice. One in do_3d_projection in order to compute the over-all z order in the Axes#draw method. The other in Collection#Draw to get the projection. So, I'd like to say storing an extra offset and swap during drawing time will be cheaper. |
Sorry, something went wrong.
|
@tacaswell I'm not sure if this is the best way to test the issue but I've added a test case there. Also a null guard is set in context manager so we won't have to worry about AttributeError. |
Sorry, something went wrong.
And also, in case, I've checked main branch and new test case failed there as excepted. |
Sorry, something went wrong.
|
@tacaswell Looks I'm not able to do the merge. Who should I call for the merging? |
Sorry, something went wrong.
|
@hukaidong just FYI, we do require two approving reviews from core contributors before merging (for changes to library code) which is why @tacaswell did not previously merge this when he first approved it |
Sorry, something went wrong.
Awesome! Thanks for letting me know. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
This PR is targeted to resolve issue #23155 by maintain offset order for indexing purpose. And z-ordered offset will only be applied during rendering peroid.
PR Checklist
Documentation and Tests
Release Notes