Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert path extension to pybind11 #27087

Merged
merged 7 commits into from
Mar 8, 2024
Merged

Convert path extension to pybind11 #27087

merged 7 commits into from
Mar 8, 2024

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 14, 2023

PR summary

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

PR checklist

@QuLogic QuLogic added this to the v3.9.0 milestone Oct 14, 2023
@QuLogic QuLogic mentioned this pull request Oct 14, 2023
Open
13 tasks
@QuLogic QuLogic force-pushed the path-pybind11 branch 2 times, most recently from 13aa334 to bf16ee0 Compare October 24, 2023 23:39
@QuLogic QuLogic marked this pull request as ready for review October 24, 2023 23:43
@tacaswell
Copy link
Member

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

@QuLogic
Copy link
Member Author

QuLogic commented Oct 26, 2023

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
Copy link
Member

story645 commented Oct 27, 2023 •
edited

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
Copy link
Member

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

@oscargus
Copy link
Contributor

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.

@QuLogic QuLogic mentioned this pull request Dec 20, 2023
Open
@ianthomas23
Copy link
Member

@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
Copy link
Member Author

QuLogic commented Jan 6, 2024

@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.

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

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

@QuLogic QuLogic mentioned this pull request Jan 13, 2024
Draft
1 task
ianthomas23
ianthomas23 approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@ianthomas23 ianthomas23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments and style issues.

src/_path_wrapper.cpp Outdated Show resolved Hide resolved
src/_path_wrapper.cpp Outdated Show resolved Hide resolved
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/_path_wrapper.cpp Outdated Show resolved Hide resolved
src/_path_wrapper.cpp

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

src/py_converters_11.h Show resolved Hide resolved
@ianthomas23
Copy link
Member

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

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

QuLogic commented Feb 23, 2024

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).

QuLogic
QuLogic commented Mar 6, 2024
View reviewed changes
src/_path_wrapper.cpp Outdated Show resolved Hide resolved
tacaswell
tacaswell approved these changes Mar 6, 2024
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@QuLogic QuLogic added the CI: Run cygwin Run cygwin tests on a PR label Mar 8, 2024
@QuLogic
Copy link
Member Author

QuLogic commented Mar 8, 2024

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

@QuLogic QuLogic merged commit 35c1dd2 into matplotlib:main Mar 8, 2024
46 of 47 checks passed
@QuLogic QuLogic deleted the path-pybind11 branch March 8, 2024 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianthomas23 ianthomas23 ianthomas23 approved these changes

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
CI: Run cygwin Run cygwin tests on a PR Maintenance topic: path handling
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@QuLogic @tacaswell @story645 @ianthomas23 @oscargus