NotificationsYou must be signed in to change notification settings
Fork8.1k
Star22.1k

Convert path extension to pybind11#27087

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

QuLogic merged 7 commits intomatplotlib:mainfromQuLogic:path-pybind11

Mar 8, 2024

Merged

Convert path extension to pybind11#27087

QuLogic merged 7 commits intomatplotlib:mainfromQuLogic:path-pybind11

Mar 8, 2024

Conversation

Copy link

Member

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 tolink the related issue
new and changed code istested
[n/a]Plotting related features are demonstrated in anexample
[n/a]New Features andAPI Changes are noted with adirective and release note
[n/a] Documentation complies withgeneral anddocstring guidelines

QuLogic added topic: path handling status: waiting for other PR Maintenance labels

Oct 14, 2023

QuLogic added this to thev3.9.0 milestone

Oct 14, 2023

QuLogic mentioned this pull request

Oct 14, 2023

[MNT]: Pybind11 transition plan#23846

Closed

13 tasks

github-actionsbot added the status: needs rebase label

Oct 15, 2023

QuLogic force-pushed thepath-pybind11 branch fromadd390d to7a4f06bCompare

October 20, 2023 05:28

github-actionsbot removed the status: needs rebase label

Oct 20, 2023

QuLogic force-pushed thepath-pybind11 branch 2 times, most recently from13aa334 tobf16ee0Compare

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

Copy link

Member

tacaswell commentedOct 26, 2023

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

Copy link

MemberAuthor

QuLogic commentedOct 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.

QuLogic added the status: waiting for other PR label

Oct 26, 2023

Copy link

Member

story645 commentedOct 27, 2023•
edited
Loading

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.

Copy link

Member

ianthomas23 commentedOct 27, 2023

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

Copy link

Member

oscargus commentedNov 13, 2023

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

Optimize Affine2D transforms#22191

Open

github-actionsbot added the status: needs rebase label

Jan 4, 2024

Copy link

Member

ianthomas23 commentedJan 4, 2024

@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 force-pushed thepath-pybind11 branch frombf16ee0 to25fbfe7Compare

January 6, 2024 11:28

github-actionsbot removed the status: needs rebase label

Jan 6, 2024

Copy link

MemberAuthor

QuLogic commentedJan 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 removed the status: waiting for other PR label

Jan 6, 2024

QuLogic force-pushed thepath-pybind11 branch 2 times, most recently from6a2f2b4 tocd0e236Compare

January 12, 2024 11:25

github-actionsbot added the topic: mpl_toolkit label

Jan 12, 2024

Copy link

Member

ianthomas23 commentedJan 12, 2024

@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

Convert Agg extension to pybind11#27011

Merged

1 task

ianthomas23 approved these changes

Jan 15, 2024

View reviewed changes

Copy link

Member

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 OutdatedShow resolvedHide 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

ianthomas23Jan 15, 2024

Choose a reason for hiding this comment

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

First use ofstd::optional in mpl, nice!

src/_path_wrapper.cpp OutdatedShow resolvedHide resolved

src/_path_wrapper.cpp

		{
		bool result;

		PyArrayObject array = (PyArrayObject )PyArray_CheckFromAny(

Copy link

Member

ianthomas23Jan 15, 2024

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 usepybind11 for this, similar toimage_resample, and then we could remove theimport_array lower down. But I'm happy to put this off to a future PR.

Copy link

MemberAuthor

QuLogicJan 16, 2024

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 othernumpy::array_view in this file though, so I think removing just this one won't mean removingimport_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

ianthomas23Jan 16, 2024

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.hShow resolvedHide resolved

QuLogic force-pushed thepath-pybind11 branch fromcd0e236 to7176f0aCompare

January 16, 2024 01:22

github-actionsbot removed the topic: mpl_toolkit label

Jan 16, 2024

Copy link

Member

ianthomas23 commentedJan 16, 2024

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

QuLogic force-pushed thepath-pybind11 branch 2 times, most recently fromacb4947 todbc6ceaCompare

February 23, 2024 09:44

Copy link

MemberAuthor

QuLogic commentedFeb 23, 2024

I've found thatpybind11 can do the copy topy::array for you, so I've made that small change to remove the manualmemcpy. It did need a cast forPolygon, as the otherwise it tries to make an array containingXY records (which aren't exposed).