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

Use pybind11 for tri module by ianthomas23 · Pull Request #24522 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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.

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

Merged
oscargus merged 6 commits into from
Dec 6, 2022
Conversation 16 Commits 6 Checks 0 Files changed
Merged

Use pybind11 for tri module #24522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e29a2a
Use pybind11 for tri module
ianthomas23 Nov 20, 2022
a4ffb29
Preinstall pybind11 on no-build-isolation builds
ianthomas23 Nov 20, 2022
1e93684
Triangulation.set_mask fix and tests
ianthomas23 Nov 30, 2022
286e48f
Review comments
ianthomas23 Nov 30, 2022
fbcf714
Use MSVC 2017 on appveyor
ianthomas23 Dec 2, 2022
5360353
Preinstall pybind11 in CodeQL action
ianthomas23 Dec 2, 2022
File filter

Filter by extension

Filter by extension .cpp  (2) .h  (1) .py  (3) .toml  (1) .yml  (3) All 5 file types selected
Only manifest files
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
2 changes: 2 additions & 0 deletions .appveyor.yml
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ branches:

clone_depth: 50

image: Visual Studio 2017

environment:

global:
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/codeql-analysis.yml
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ jobs:
# https://github.com/jazzband/pip-tools/pull/1681
python -m pip install --upgrade \
certifi contourpy cycler fonttools kiwisolver importlib_resources \
numpy packaging pillow pyparsing python-dateutil setuptools-scm
numpy packaging pillow pyparsing python-dateutil setuptools-scm \
pybind11
echo "CODEQL_PYTHON=$(which python)" >> $GITHUB_ENV

- name: Initialize CodeQL
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/tests.yml
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ jobs:
-r requirements/testing/all.txt \
${{ matrix.extra-requirements }}

# Preinstall pybind11 on no-build-isolation builds.
if [[ "${{ matrix.name-suffix }}" == '(Minimum Versions)' ]]; then
python -m pip install 'pybind11>=2.6'
fi

# Install optional dependencies from PyPI.
# Sphinx is needed to run sphinxext tests
python -m pip install --upgrade sphinx
Expand Down
63 changes: 47 additions & 16 deletions lib/matplotlib/tests/test_triangulation.py
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,29 @@ def test_triangulation_init():
mtri.Triangulation(x, y, [[0, 1, -1]])


def test_triangulation_set_mask():
x = [-1, 0, 1, 0]
y = [0, -1, 0, 1]
triangles = [[0, 1, 2], [2, 3, 0]]
triang = mtri.Triangulation(x, y, triangles)

# Check neighbors, which forces creation of C++ triangulation
assert_array_equal(triang.neighbors, [[-1, -1, 1], [-1, -1, 0]])

# Set mask
triang.set_mask([False, True])
assert_array_equal(triang.mask, [False, True])

# Reset mask
triang.set_mask(None)
assert triang.mask is None

msg = r"mask array must have same length as triangles array"
for mask in ([False, True, False], [False], [True], False, True):
with pytest.raises(ValueError, match=msg):
triang.set_mask(mask)


def test_delaunay():
# No duplicate points, regular grid.
nx = 5
Expand Down Expand Up @@ -1166,55 +1189,62 @@ def test_internal_cpp_api():
# C++ Triangulation.
with pytest.raises(
TypeError,
match=r'function takes exactly 7 arguments \(0 given\)'):
match=r'__init__\(\): incompatible constructor arguments.'):
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)

oscargus Nov 28, 2022

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

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?

tacaswell Nov 28, 2022

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

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.

ianthomas23 Nov 29, 2022

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

anntzer Nov 29, 2022

Copy link
Copy Markdown
Contributor

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

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

x = [0, 1, 1]
y = [0, 0, 1]
with pytest.raises(
ValueError,
match=r'triangles must be a 2D array of shape \(\?,3\)'):
mpl._tri.Triangulation(x, y, [[0, 1]], None, None, None, False)
mpl._tri.Triangulation(x, y, [[0, 1]], (), (), (), False)

tris = [[0, 1, 2]]
with pytest.raises(
ValueError,
match=r'mask must be a 1D array with the same length as the '
r'triangles array'):
mpl._tri.Triangulation(x, y, tris, [0, 1], None, None, False)
mpl._tri.Triangulation(x, y, tris, [0, 1], (), (), False)

with pytest.raises(
ValueError, match=r'edges must be a 2D array with shape \(\?,2\)'):
mpl._tri.Triangulation(x, y, tris, None, [[1]], None, False)
mpl._tri.Triangulation(x, y, tris, (), [[1]], (), False)

with pytest.raises(
ValueError,
match=r'neighbors must be a 2D array with the same shape as the '
r'triangles array'):
mpl._tri.Triangulation(x, y, tris, None, None, [[-1]], False)
mpl._tri.Triangulation(x, y, tris, (), (), [[-1]], False)

triang = mpl._tri.Triangulation(x, y, tris, None, None, None, False)
triang = mpl._tri.Triangulation(x, y, tris, (), (), (), False)

with pytest.raises(
ValueError,
match=r'z array must have same length as triangulation x and y '
r'array'):
match=r'z must be a 1D array with the same length as the '
r'triangulation x and y arrays'):
triang.calculate_plane_coefficients([])

with pytest.raises(
ValueError,
match=r'mask must be a 1D array with the same length as the '
r'triangles array'):
triang.set_mask([0, 1])
for mask in ([0, 1], None):
with pytest.raises(
ValueError,
match=r'mask must be a 1D array with the same length as the '
r'triangles array'):
triang.set_mask(mask)

triang.set_mask([True])
assert_array_equal(triang.get_edges(), np.empty((0, 2)))

triang.set_mask(()) # Equivalent to Python Triangulation mask=None
assert_array_equal(triang.get_edges(), [[1, 0], [2, 0], [2, 1]])

# C++ TriContourGenerator.
with pytest.raises(
TypeError,
match=r'function takes exactly 2 arguments \(0 given\)'):
match=r'__init__\(\): incompatible constructor arguments.'):
mpl._tri.TriContourGenerator()

with pytest.raises(
Expand All @@ -1232,7 +1262,8 @@ def test_internal_cpp_api():

# C++ TrapezoidMapTriFinder.
with pytest.raises(
TypeError, match=r'function takes exactly 1 argument \(0 given\)'):
TypeError,
match=r'__init__\(\): incompatible constructor arguments.'):
mpl._tri.TrapezoidMapTriFinder()

trifinder = mpl._tri.TrapezoidMapTriFinder(triang)
Expand Down
11 changes: 8 additions & 3 deletions lib/matplotlib/tri/_triangulation.py
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,12 @@ def get_cpp_triangulation(self):
from matplotlib import _tri
if self._cpp_triangulation is None:
self._cpp_triangulation = _tri.Triangulation(
self.x, self.y, self.triangles, self.mask, self._edges,
self._neighbors, not self.is_delaunay)
# For unset arrays use empty tuple which has size of zero.
self.x, self.y, self.triangles,
self.mask if self.mask is not None else (),
self._edges if self._edges is not None else (),
self._neighbors if self._neighbors is not None else (),
not self.is_delaunay)
return self._cpp_triangulation

def get_masked_triangles(self):
Expand Down Expand Up @@ -229,7 +233,8 @@ def set_mask(self, mask):

# Set mask in C++ Triangulation.
if self._cpp_triangulation is not None:
self._cpp_triangulation.set_mask(self.mask)
self._cpp_triangulation.set_mask(
self.mask if self.mask is not None else ())

# Clear derived fields so they are recalculated when needed.
self._edges = None
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@ build-backend = "setuptools.build_meta"
requires = [
"certifi>=2020.06.20",
"oldest-supported-numpy",
"pybind11>=2.6",
"setuptools_scm>=7",
]
7 changes: 4 additions & 3 deletions setupext.py
Show comments
View file
Open in desktop
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import textwrap
import urllib.request

from pybind11.setup_helpers import Pybind11Extension
from setuptools import Distribution, Extension

_log = logging.getLogger(__name__)
Expand Down Expand Up @@ -459,12 +460,12 @@ def get_extensions(self):
add_libagg_flags(ext)
yield ext
# tri
ext = Extension(
ext = Pybind11Extension(
"matplotlib._tri", [
"src/tri/_tri.cpp",
"src/tri/_tri_wrapper.cpp",
])
add_numpy_flags(ext)
],
cxx_std=11)
yield ext
# ttconv
ext = Extension(
Expand Down
Loading
Back | FazBrowse Home | New Git URL