FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Convert path extension to pybind11 by QuLogic · Pull Request #27087 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Convert path extension to pybind11 - #27087

Merged
QuLogic merged 7 commits into
matplotlib:mainfrom
QuLogic:path-pybind11
Mar 8, 2024
ConversationCommits (7)ChecksFiles changed
Merged

Convert path extension to pybind11#27087
QuLogic merged 7 commits into
matplotlib:mainfrom
QuLogic:path-pybind11

Conversation

QuLogic commented Oct 14, 2023

Copy link
Copy Markdown
Member

PR summary

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

PR checklist

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

tacaswell commented Oct 26, 2023

Copy link
Copy Markdown
Member

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

QuLogic commented Oct 26, 2023

Copy link
Copy Markdown
Member Author

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 commented Oct 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Oct 27, 2023

Copy link
Copy Markdown
Member

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

oscargus commented Nov 13, 2023

Copy link
Copy Markdown
Member

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 mentioned this pull request Dec 20, 2023
Open

ianthomas23 commented Jan 4, 2024

Copy link
Copy Markdown
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 commented Jan 6, 2024

Copy link
Copy Markdown
Member Author

@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 force-pushed the path-pybind11 branch 2 times, most recently from 6a2f2b4 to cd0e236 Compare January 12, 2024 11:25

ianthomas23 commented Jan 12, 2024

Copy link
Copy Markdown
Member

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

QuLogic mentioned this pull request Jan 13, 2024
Merged
1 task done
ianthomas23 approved these changes Jan 15, 2024
View reviewed changes

ianthomas23 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just a few comments and style issues.

Comment thread src/_path_wrapper.cpp Outdated
Comment thread src/_path_wrapper.cpp Outdated
Comment thread 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)

ianthomas23 Jan 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread src/_path_wrapper.cpp Outdated
Comment thread src/_path_wrapper.cpp
{
bool result;

PyArrayObject *array = (PyArrayObject *)PyArray_CheckFromAny(

ianthomas23 Jan 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

QuLogic Jan 16, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ianthomas23 Jan 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure.

Comment thread src/py_converters_11.h

ianthomas23 commented Jan 16, 2024

Copy link
Copy Markdown
Member

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

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

QuLogic commented Feb 23, 2024

Copy link
Copy Markdown
Member Author

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 commented Mar 6, 2024
View reviewed changes
Comment thread src/_path_wrapper.cpp Outdated
tacaswell approved these changes Mar 6, 2024
View reviewed changes

tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

QuLogic commented Mar 8, 2024

Copy link
Copy Markdown
Member Author

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

QuLogic merged commit 35c1dd2 into matplotlib:main Mar 8, 2024
QuLogic deleted the path-pybind11 branch March 8, 2024 02:45
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ianthomas23 ianthomas23 approved these changes
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.

5 participants

Back | FazBrowse Home | New Git URL