Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Use pybind11 for tri module#24522
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems to make sense to me, but I probably trust the passing tests more than I trust my knowledge in the area...
src/tri/_tri_wrapper.cpp Outdated
| 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| "This should not be called directly,insteaduse the python class\n" | |
| "This should not be called directly, use the python class\n" |
(or drop the instead on the next line.)
src/tri/_tri_wrapper.cpp Outdated
| py::arg("triangulation"), | ||
| py::arg("z"), | ||
| "Create a new C++ TriContourGenerator object.\n" | ||
| "This should not be called directly, instead use the functions\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| "This should not be called directly,insteaduse the functions\n" | |
| "This should not be called directly, use the functions\n" |
Same
src/tri/_tri_wrapper.cpp Outdated
| .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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| "This should not be called directly,insteaduse the python class\n" | |
| "This should not be called directly, use the python class\n" |
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In principle yes, but we do not expose this class out publicly (there is a Python-sideTriangulation class intri._triangulation that we re-export intri/__init__.py).
I think it is OK both that we change this API without warning and that we have tests that touch private API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not entirely happy with this internal conversion ofNone to empty tuple for optional integer arrays but I cannot think of an alternative that isn't worse. In the long run we will usestd::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 usestd::optional by shipping our ownoptional.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 bespokenp.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 theNone to tuple until we are OK with using C++17.
Note to self: need to add explicit internal API tests forset_mask as this also accepts an optional array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fwiw scipy seems to aim to bump to c++17 in the next (1.10) release, seescipy/scipy#16589.
The appveyor failure is real: Currently we only require 2015, but bumping to 2017 seems resonable to me. |
2f8a3b0 to7baffa4CompareAll checks are passing now that I have changed the appveyor image to Visual Studio 2017. I've also had to preinstall |
I added the Run cibuildwheel label just to confirm, and that seems to be passing as well. |
I think there's no need for this stage at all, and opened#24595 to remove it. |
ed981d0 to5360353CompareI've removed the appveyor wheel-building fix and rebased to pick up#24595. |
Uh oh!
There was an error while loading.Please reload this page.
We need pybind11 and a C++ compiler because ofmatplotlib#24522
We need pybind11 and a C++ compiler because ofmatplotlib#24522
We need pybind11 and a C++ compiler because ofmatplotlib#24522
Prior to their conversion to pybind11 classes inmatplotlib#24522, these typeshad `tp_flags = Py_TPFLAGS_DEFAULT`, meaning they did not have the`Py_TPFLAGS_BASETYPE` flag and could not be subtyped. As these areinternal classes, I don't believe this was intentionally chnaged, sorestore that flag.Additionally, since we require C++17 now, mark the C++ classesthemselves as `final`. I believe this really only makes a difference ifwe have `virtual` methods (which we don't here), but that doesn't meanthe compiler can't infer some other optimizations with this information.
Prior to their conversion to pybind11 classes inmatplotlib#24522, these typeshad `tp_flags = Py_TPFLAGS_DEFAULT`, meaning they did not have the`Py_TPFLAGS_BASETYPE` flag and could not be subtyped. As these areinternal classes, I don't believe this was intentionally changed, sorestore that flag.Additionally, since we require C++17 now, mark the C++ classesthemselves as `final`. I believe this really only makes a difference ifwe have `virtual` methods (which we don't here), but that doesn't meanthe compiler can't infer some other optimizations with this information.
This is a
pybind11wrapping of the_trimodule. It is taken from thepybind11proof of concept (#23787) with some of@anntzer's extra comments addressed. Also seepybindtransition plan#23846.Worthy of note for future PRs that wrap C/C++ code using
pybind11is how I have chosen to deal with passingNonein lieu of NumPy arrays from Python to C++ (i.e. when the array is optional). These are auto converted to arrays using the targetdtype. So for float and bool arrays this is fine but it doesn't work for integer arraysHence passing from a Python
Triangulationto a C++ one the various optional array arguments are passed as empty tuples rather thanNone. These appear on the C++ side as arrays withsize==0andndim==1. Probably when not passing optional integers arrays I would prefer to stick toNone, which appear as arrays withsize==1andndim==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 PythonTriangulationso it doesn't affect user code either way.