Convert remaining code to pybind11#28856
Merged
greglucas merged 8 commits intomatplotlib:mainfrom Oct 8, 2024
Merged
Conversation
0688bb3 to
1a9b601
Compare
13 tasks
ksunden
approved these changes
Sep 30, 2024
Member
ksunden
left a comment
There was a problem hiding this comment.
I would like to get this in for 3.10, so if we could get a second review, that would be great
anntzer
reviewed
Oct 1, 2024
anntzer
reviewed
Oct 1, 2024
This was previously checked by using an `array_view<type, ndim>`, but moving to pybind11 we won't have that until cast to `unchecked`.
They need only be the same number of dimensions, as sometimes code does `np.atleast_3d(array)` on something empty, which inserts the 0-dimension in a spot that messes with the expected trailing shape.
Now that everything else is using pybind11, this works without issue.
Since we're using pybind11 everywhere, it should be fine now to access it in any header, and putting the type caster there is clearer. We don't need the weird macro checks to conditionally define them either.
Co-authored-by: Antony Lee <[email protected]>
1a9b601 to
37206c2
Compare
Member
|
This looks like some of the grayscale have picked up a off-by-one rounding issue. Seeing this on other PRs as well. |
greglucas
approved these changes
Oct 8, 2024
Contributor
greglucas
left a comment
There was a problem hiding this comment.
I am not an expert on the pybind11 changes, but I did peruse this and it all looks reasonable to a non-expert. I reran the failing 313t tests and those now pass. I believe the other failures in Azure are unrelated and failing on other tests too.
I believe that it will be easier to review incremental updates after this is in if there is anything else to follow up on.
Great work on this @QuLogic! This was no small feat.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR summary
Now that we are building with pybind11 in all extensions, we can drop/convert several items that were used across extensions. Namely, everything in
py_adaptors(which was shared between_pathand_backend_agg) can now get a type caster, all the remainingarray_viewcan be moved topy::array_t, and theClipPathconverter issue from #27011 has been fixed.Additionally, I moved the pybind11 type casters from
py_converters_11.hto the files that define the related types. This keeps things a bit more consolidated, and avoids the weird#ifdefmacro magic that was needed before.PR checklist