Use pybind11 for tri module #24522

ianthomas23 · 2022-11-20T17:02:28Z

This is a pybind11 wrapping of the _tri module. It is taken from the pybind11 proof of concept (#23787) with some of @anntzer's extra comments addressed. Also see pybind transition plan #23846.

Worthy of note for future PRs that wrap C/C++ code using pybind11 is how I have chosen to deal with passing None in lieu of NumPy arrays from Python to C++ (i.e. when the array is optional). These are auto converted to arrays using the target dtype. So for float and bool arrays this is fine but it doesn't work for integer arrays

>>> np.array(None, dtype=int)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'

Hence passing from a Python Triangulation to a C++ one the various optional array arguments are passed as empty tuples rather than None. These appear on the C++ side as arrays with size==0 and ndim==1. Probably when not passing optional integers arrays I would prefer to stick to None, which appear as arrays with size==1 and ndim==0, but rather than mix the two approaches in the same function I have just used the tuple approach. All of this is on the internal C++ API behind the Python Triangulation so it doesn't affect user code either way.

oscargus

Seems to make sense to me, but I probably trust the passing tests more than I trust my knowledge in the area...

oscargus · 2022-11-28T10:05:01Z

src/tri/_tri_wrapper.cpp

+            py::arg("neighbors"),
+            py::arg("correct_triangle_orientations"),
+            "Create a new C++ Triangulation object.\n"
+            "This should not be called directly, instead use the python class\n"


Suggested change

"This should not be called directly, instead use the python class\n"

"This should not be called directly, use the python class\n"

(or drop the instead on the next line.)

oscargus · 2022-11-28T10:05:37Z

src/tri/_tri_wrapper.cpp

+            py::arg("triangulation"),
+            py::arg("z"),
+            "Create a new C++ TriContourGenerator object.\n"
+            "This should not be called directly, instead use the functions\n"


Suggested change

"This should not be called directly, instead use the functions\n"

"This should not be called directly, use the functions\n"

Same

oscargus · 2022-11-28T10:05:54Z

src/tri/_tri_wrapper.cpp

+        .def(py::init<Triangulation&>(),
+            py::arg("triangulation"),
+            "Create a new C++ TrapezoidMapTriFinder object.\n"
+            "This should not be called directly, instead use the python class\n"


Suggested change

"This should not be called directly, instead use the python class\n"

"This should not be called directly, use the python class\n"

oscargus · 2022-11-28T10:10:45Z

lib/matplotlib/tests/test_triangulation.py

        mpl._tri.Triangulation()

    with pytest.raises(
            ValueError, match=r'x and y must be 1D arrays of the same length'):
-        mpl._tri.Triangulation([], [1], [[]], None, None, None, False)
+        mpl._tri.Triangulation([], [1], [[]], (), (), (), False)


Because of lacking knowledge: will this cause problems for anyone possibly creating the Triangulation explicitly? Am I correct assuming that passing None is no longer feasible at this level?

In principle yes, but we do not expose this class out publicly (there is a Python-side Triangulation class in tri._triangulation that we re-export in tri/__init__.py).

I think it is OK both that we change this API without warning and that we have tests that touch private API.

I'm not entirely happy with this internal conversion of None to empty tuple for optional integer arrays but I cannot think of an alternative that isn't worse. In the long run we will use std::optional but that is C++17. I wouldn't want to use any C++17 until SciPy does, but that may not be far away (https://docs.scipy.org/doc/scipy/dev/toolchain.html#c-language-standards).

In the meantime we can use std::optional by shipping our own optional.h but I wouldn't want to do that as if there is a problem with some obscure compiler we will be on our own. Or we could write our own bespoke np.array | None pybind11 converter class but that is much more esoteric C++ than we currently have. So the least bad option is to stick with the None to tuple until we are OK with using C++17.

Note to self: need to add explicit internal API tests for set_mask as this also accepts an optional array.

fwiw scipy seems to aim to bump to c++17 in the next (1.10) release, see scipy/scipy#16589.

tacaswell · 2022-11-29T00:22:15Z

The appveyor failure is real:

 c:\users\appveyor\appdata\local\temp\1\pip-build-env-uvxxe3n3\overlay\lib\site-packages\pybind11\include\pybind11\detail/common.h(86): fatal error C1189: #error:  pybind11 2.10+ requires MSVC 2017 or newer

Currently we only require 2015, but bumping to 2017 seems resonable to me.

ianthomas23 · 2022-12-02T12:39:14Z

All checks are passing now that I have changed the appveyor image to Visual Studio 2017.

I've also had to preinstall pybind11 just before the python setup.py bdist_wheel appveyor stage. This won't be needed when we upgrade the wheel building stage to python -m build or equivalent.

oscargus · 2022-12-02T15:54:33Z

I added the Run cibuildwheel label just to confirm, and that seems to be passing as well.

QuLogic · 2022-12-03T07:32:24Z

I've also had to preinstall pybind11 just before the python setup.py bdist_wheel appveyor stage. This won't be needed when we upgrade the wheel building stage to python -m build or equivalent.

I think there's no need for this stage at all, and opened #24595 to remove it.

ianthomas23 · 2022-12-04T10:28:02Z

I've removed the appveyor wheel-building fix and rebased to pick up #24595.

src/tri/_tri.h

We need pybind11 and a C++ compiler because of matplotlib#24522

Prior to their conversion to pybind11 classes in matplotlib#24522, these types had `tp_flags = Py_TPFLAGS_DEFAULT`, meaning they did not have the `Py_TPFLAGS_BASETYPE` flag and could not be subtyped. As these are internal classes, I don't believe this was intentionally chnaged, so restore that flag. Additionally, since we require C++17 now, mark the C++ classes themselves as `final`. I believe this really only makes a difference if we have `virtual` methods (which we don't here), but that doesn't mean the compiler can't infer some other optimizations with this information.

Prior to their conversion to pybind11 classes in matplotlib#24522, these types had `tp_flags = Py_TPFLAGS_DEFAULT`, meaning they did not have the `Py_TPFLAGS_BASETYPE` flag and could not be subtyped. As these are internal classes, I don't believe this was intentionally changed, so restore that flag. Additionally, since we require C++17 now, mark the C++ classes themselves as `final`. I believe this really only makes a difference if we have `virtual` methods (which we don't here), but that doesn't mean the compiler can't infer some other optimizations with this information.

oscargus reviewed Nov 28, 2022

View reviewed changes

tacaswell added this to the v3.7.0 milestone Nov 28, 2022

ianthomas23 force-pushed the pybind11_tri_module branch 2 times, most recently from 2f8a3b0 to 7baffa4 Compare December 2, 2022 10:47

oscargus added the CI: Run cibuildwheel Run wheel building tests on a PR label Dec 2, 2022

tacaswell approved these changes Dec 3, 2022

View reviewed changes

github-actions bot added the status: needs rebase label Dec 4, 2022

ianthomas23 added 6 commits December 4, 2022 10:23

Use pybind11 for tri module

5e29a2a

Preinstall pybind11 on no-build-isolation builds

a4ffb29

Triangulation.set_mask fix and tests

1e93684

Review comments

286e48f

Use MSVC 2017 on appveyor

fbcf714

Preinstall pybind11 in CodeQL action

5360353

ianthomas23 force-pushed the pybind11_tri_module branch from ed981d0 to 5360353 Compare December 4, 2022 10:24

github-actions bot removed the status: needs rebase label Dec 4, 2022

oscargus reviewed Dec 6, 2022

View reviewed changes

src/tri/_tri.h Show resolved Hide resolved

oscargus approved these changes Dec 6, 2022

View reviewed changes

oscargus merged commit 046cd46 into matplotlib:main Dec 6, 2022

ianthomas23 deleted the pybind11_tri_module branch December 6, 2022 13:47

ianthomas23 mentioned this pull request Dec 6, 2022

[MNT]: Pybind11 transition plan #23846

Closed

13 tasks

timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Dec 19, 2022

Update environment.yml

02fd48e

We need pybind11 and a C++ compiler because of matplotlib#24522

timhoffm mentioned this pull request Dec 19, 2022

Update environment.yml #24780

Merged

timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Dec 20, 2022

Update environment.yml

f33b4d4

We need pybind11 and a C++ compiler because of matplotlib#24522

raphaelquast pushed a commit to raphaelquast/matplotlib that referenced this pull request Mar 16, 2023

Update environment.yml

94d2bf6

We need pybind11 and a C++ compiler because of matplotlib#24522

ksunden mentioned this pull request Dec 12, 2023

test_tripcolor (Issue #26864) plots with datetime on x-axis only , y-axis only, and both x- and y-axis #27491

Open

1 task

ksunden mentioned this pull request Dec 12, 2023

[Bug]: Units behavior of tri* methods #27499

Open

QuLogic mentioned this pull request Mar 9, 2024

Mark triangulation classes as final #27894

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pybind11 for tri module #24522

Use pybind11 for tri module #24522

ianthomas23 commented Nov 20, 2022

oscargus left a comment

oscargus Nov 28, 2022

oscargus Nov 28, 2022

oscargus Nov 28, 2022

oscargus Nov 28, 2022

tacaswell Nov 28, 2022

ianthomas23 Nov 29, 2022

anntzer Nov 29, 2022

tacaswell commented Nov 29, 2022

ianthomas23 commented Dec 2, 2022

oscargus commented Dec 2, 2022

QuLogic commented Dec 3, 2022

ianthomas23 commented Dec 4, 2022

	"This should not be called directly, instead use the python class\n"
	"This should not be called directly, use the python class\n"

Use pybind11 for tri module #24522

Use pybind11 for tri module #24522

Conversation

ianthomas23 commented Nov 20, 2022

oscargus left a comment

Choose a reason for hiding this comment

oscargus Nov 28, 2022

Choose a reason for hiding this comment

oscargus Nov 28, 2022

Choose a reason for hiding this comment

oscargus Nov 28, 2022

Choose a reason for hiding this comment

oscargus Nov 28, 2022

Choose a reason for hiding this comment

tacaswell Nov 28, 2022

Choose a reason for hiding this comment

ianthomas23 Nov 29, 2022

Choose a reason for hiding this comment

anntzer Nov 29, 2022

Choose a reason for hiding this comment

tacaswell commented Nov 29, 2022

ianthomas23 commented Dec 2, 2022

oscargus commented Dec 2, 2022

QuLogic commented Dec 3, 2022

ianthomas23 commented Dec 4, 2022