Convert path extension to pybind11 #27087

QuLogic · 2023-10-14T05:14:33Z

PR summary

Another part of #23846, but waiting for some other refactoring PRs to be ready.

PR checklist

[n/a] "closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
[n/a] Plotting related features are demonstrated in an example
[n/a] New Features and API Changes are noted with a directive and release note
[n/a] Documentation complies with general and docstring guidelines

tacaswell · 2023-10-26T20:45:56Z

Given the scope of these changes, what is the best way to review these?

QuLogic · 2023-10-26T21:50:59Z

I think commit-by-commit is not bad, or maybe I should split out the first commit from the remaining ones, as those are all fairly small.

But I also think we maybe should wait for #26050, which I want to get back to reviewing, and I don't want to cause some undue rebasing for the contributor.

story645 · 2023-10-27T04:44:26Z

But I also think we maybe should wait for #26050, which I want to get back to reviewing, and I don't want to cause some undue rebasing for the contributor.

I kinda told him I'd finish that one up for him b/c he had to go do other things, so I'll probably be the one rebasing. I was trying to get in the xkcd PR first (but scratch that, it probably makes more sense to get sketch seed in and then update xkcd) since that would also cause a rebase and touches some of the same code paths.

ianthomas23 · 2023-10-27T09:50:10Z

I should have some time available in a week or two to help review pybind11/C++ changes, if help is required?

oscargus · 2023-11-13T16:10:23Z

Maybe we can consider #25645 as well (after #26050) before this? Although I can probably recreate it (rather than rebasing) if there are any issues.

ianthomas23 · 2024-01-04T09:13:38Z

@QuLogic Both this and the other pybind11 PR (#27011) are labelled as "waiting for other PR". Can you clarify what needs to be done before either of them become reviewable?

QuLogic · 2024-01-06T11:30:44Z

@ianthomas23 I was waiting for #26050 to make it easier for the new contributor, but as @story645 says she will take care of it, I've rebased this PR and fixed the conflicts.

ianthomas23 · 2024-01-12T11:57:12Z

@QuLogic Reviewing this is on my todo list, hopefully within the next week. Feel free to ping me if I forget.

ianthomas23

Just a few comments and style issues.

src/_path_wrapper.cpp

ianthomas23 · 2024-01-15T21:36:00Z

src/_path_wrapper.cpp

+static py::tuple
+Py_cleanup_path(mpl::PathIterator path, agg::trans_affine trans, bool remove_nans,
+                agg::rect_d clip_rect, e_snap_mode snap_mode, double stroke_width,
+                std::optional<bool> simplify, bool return_curves, SketchParams sketch)


First use of std::optional in mpl, nice!

src/_path_wrapper.cpp

ianthomas23 · 2024-01-15T21:52:36Z

src/_path_wrapper.cpp

 {
    bool result;

    PyArrayObject *array = (PyArrayObject *)PyArray_CheckFromAny(


I think it should be possible to use pybind11 for this, similar to image_resample, and then we could remove the import_array lower down. But I'm happy to put this off to a future PR.

I thought I had tried this, but I can't find a stash for it. There are still a few other numpy::array_view in this file though, so I think removing just this one won't mean removing import_array?

For those other calls, we still need to get Agg closer to pybind11, and then we can do the NumPy API removal, so I think I'll wait on this one.

src/py_converters_11.h

ianthomas23 · 2024-01-16T08:24:00Z

This looks good to me, just needs a second review now.

QuLogic · 2024-02-23T09:47:59Z

I've found that pybind11 can do the copy to py::array for you, so I've made that small change to remove the manual memcpy. It did need a cast for Polygon, as the otherwise it tries to make an array containing XY records (which aren't exposed).

src/_path_wrapper.cpp

tacaswell

The tests pass, but we should merge the cgywin PR first.

QuLogic · 2024-03-08T02:44:41Z

OK, looks like Cygwin is also happy, so I'll merge.

QuLogic added topic: path handling status: waiting for other PR Maintenance labels Oct 14, 2023

QuLogic added this to the v3.9.0 milestone Oct 14, 2023

QuLogic mentioned this pull request Oct 14, 2023

[MNT]: Pybind11 transition plan #23846

Closed

13 tasks

github-actions bot added the status: needs rebase label Oct 15, 2023

QuLogic force-pushed the path-pybind11 branch from add390d to 7a4f06b Compare October 20, 2023 05:28

github-actions bot removed the status: needs rebase label Oct 20, 2023

QuLogic force-pushed the path-pybind11 branch 2 times, most recently from 13aa334 to bf16ee0 Compare October 24, 2023 23:39

QuLogic removed the status: waiting for other PR label Oct 24, 2023

QuLogic marked this pull request as ready for review October 24, 2023 23:43

QuLogic added the status: waiting for other PR label Oct 26, 2023

QuLogic mentioned this pull request Dec 20, 2023

Optimize Affine2D transforms #22191

Open

github-actions bot added the status: needs rebase label Jan 4, 2024

QuLogic force-pushed the path-pybind11 branch from bf16ee0 to 25fbfe7 Compare January 6, 2024 11:28

github-actions bot removed the status: needs rebase label Jan 6, 2024

QuLogic removed the status: waiting for other PR label Jan 6, 2024

QuLogic force-pushed the path-pybind11 branch 2 times, most recently from 6a2f2b4 to cd0e236 Compare January 12, 2024 11:25

github-actions bot added the topic: mpl_toolkit label Jan 12, 2024

QuLogic mentioned this pull request Jan 13, 2024

Convert Agg extension to pybind11 #27011

Merged

1 task

ianthomas23 approved these changes Jan 15, 2024

View reviewed changes

QuLogic force-pushed the path-pybind11 branch from cd0e236 to 7176f0a Compare January 16, 2024 01:22

github-actions bot removed the topic: mpl_toolkit label Jan 16, 2024

QuLogic force-pushed the path-pybind11 branch 2 times, most recently from acb4947 to dbc6cea Compare February 23, 2024 09:44

QuLogic commented Mar 6, 2024

View reviewed changes

src/_path_wrapper.cpp Outdated Show resolved Hide resolved

tacaswell approved these changes Mar 6, 2024

View reviewed changes

QuLogic added 7 commits March 7, 2024 18:47

Convert path extension to pybind11

879779b

Add a pybind11 type caster for agg::rect_d

25fbbd3

Add a pybind11 type caster for agg::trans_affine

4a56e52

Add a pybind11 type caster for mpl::PathIterator

d32a372

Add a pybind11 type caster for e_snap_mode

925d619

Add a pybind11 type caster for SketchParams

22c2689

Optimize convert_polygon_vector a bit

a906be6

QuLogic force-pushed the path-pybind11 branch from dbc6cea to a906be6 Compare March 7, 2024 23:47

QuLogic added the CI: Run cygwin Run cygwin tests on a PR label Mar 8, 2024

QuLogic merged commit 35c1dd2 into matplotlib:main Mar 8, 2024

QuLogic deleted the path-pybind11 branch March 8, 2024 02:45

Uh oh!

Convert path extension to pybind11 #27087

Convert path extension to pybind11 #27087

Uh oh!

Conversation

QuLogic commented Oct 14, 2023

PR summary

PR checklist

Uh oh!

tacaswell commented Oct 26, 2023

Uh oh!

QuLogic commented Oct 26, 2023

Uh oh!

story645 commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianthomas23 commented Oct 27, 2023

Uh oh!

oscargus commented Nov 13, 2023

Uh oh!

ianthomas23 commented Jan 4, 2024

Uh oh!

QuLogic commented Jan 6, 2024

Uh oh!

ianthomas23 commented Jan 12, 2024

Uh oh!

ianthomas23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ianthomas23 Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ianthomas23 Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

QuLogic Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

ianthomas23 Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ianthomas23 commented Jan 16, 2024

Uh oh!

QuLogic commented Feb 23, 2024

Uh oh!

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

story645 commented Oct 27, 2023 •

edited

Loading