Convert path extension to pybind11 #27087
Conversation
add390d to
7a4f06b
Compare
13aa334 to
bf16ee0
Compare
|
Given the scope of these changes, what is the best way to review these? |
|
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. |
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. |
|
I should have some time available in a week or two to help review pybind11/C++ changes, if help is required? |
|
@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. |
6a2f2b4 to
cd0e236
Compare
|
@QuLogic Reviewing this is on my todo list, hopefully within the next week. Feel free to ping me if I forget. |
| 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) |
There was a problem hiding this comment.
First use of std::optional in mpl, nice!
|
|
||
| static PyObject *Py_is_sorted_and_has_non_nan(PyObject *self, PyObject *obj) | ||
| static bool | ||
| Py_is_sorted_and_has_non_nan(py::object obj) | ||
| { | ||
| bool result; | ||
|
|
||
| PyArrayObject *array = (PyArrayObject *)PyArray_CheckFromAny( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
cd0e236 to
7176f0a
Compare
|
This looks good to me, just needs a second review now. |
acb4947 to
dbc6cea
Compare
|
I've found that pybind11 can do the copy to |
|
OK, looks like Cygwin is also happy, so I'll merge. |
PR summary
Another part of #23846, but waiting for some other refactoring PRs to be ready.
PR checklist