Skip to content

Reduce python interactions in cython part of code #58

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
Czaki merged 2 commits into from
Dec 21, 2024
Merged

Reduce python interactions in cython part of code #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf280f1
reduce python interactions in cython part of code
Czaki Dec 20, 2024
51dae01
improve tests
Czaki Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 89 additions & 13 deletions src/PartSegCore_compiled_backend/triangulate.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,46 @@ def triangulate_path_edge_py(path: Sequence[Sequence[float]], closed: bool=False
triangles,
)

def triangulate_path_edge_numpy(cnp.ndarray[cnp.float32_t, ndim=2] path, bool closed=False, float limit=3.0, bool bevel=False) -> tuple[np.ndarray, np.ndarray, np.ndarray]:
""" Triangulate path"""
cdef vector[Point] path_vector
cdef PathTriangulation result
cdef Point p1, p2
cdef cnp.ndarray[cnp.uint32_t, ndim=2] triangles
cdef cnp.ndarray[cnp.float32_t, ndim=2] offsets, centers
cdef size_t i, len_path

len_path = path.shape[0]

path_vector.reserve(len_path)
for i in range(len_path):
path_vector.push_back(Point(path[i, 0], path[i, 1]))
with cython.nogil:
result = triangulate_path_edge(path_vector, closed, limit, bevel)

triangles = np.empty((result.triangles.size(), 3), dtype=np.uint32)
centers = np.empty((result.centers.size(), 2), dtype=np.float32)
offsets = np.empty((result.offsets.size(), 2), dtype=np.float32)

for i in range(result.triangles.size()):
triangles[i, 0] = result.triangles[i].x
triangles[i, 1] = result.triangles[i].y
triangles[i, 2] = result.triangles[i].z

for i in range(result.centers.size()):
centers[i, 0] = result.centers[i].x
centers[i, 1] = result.centers[i].y

for i in range(result.offsets.size()):
offsets[i, 0] = result.offsets[i].x
offsets[i, 1] = result.offsets[i].y

return (
centers,
offsets,
triangles,
)

Comment on lines +391 to +430
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 20, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the code analysis, I can now provide the final response:

Add input validation for path length and shape
The C++ implementation already handles empty or single-point paths by returning a default triangulation, but this should be documented and handled explicitly in the Python wrapper. Add validation at the start of the function:

def triangulate_path_edge_numpy(path, closed=False, limit=3.0, bevel=False):
    """Triangulate path
    
    Args:
        path: numpy array of shape (N, 2) containing path points
        closed: whether the path forms a closed loop
        limit: angle limit for miter joins
        bevel: whether to use bevel joins
        
    Returns:
        Tuple of (centers, offsets, triangles) arrays
        
    Raises:
        ValueError: if path has less than 2 points or wrong shape
    """
    if path.shape[0] < 2:
        raise ValueError("Path must have at least 2 points")
    if path.shape[1] != 2:
        raise ValueError("Path must be a Nx2 array of points")
🔗 Analysis chain

Ensure robust handling of empty or invalid paths
If "path" is empty or has fewer than 2 points, this could cause unexpected behavior or an empty triangulation. Consider performing an early return or raising a ValueError when "path.shape[0]" is insufficient to produce meaningful triangles. Also, it may help to document the expected shape of "path" (N×2) in the docstring for clarity.

You can verify potential issues with empty "path" by searching for all calls to "triangulate_path_edge_numpy" to confirm that no zero-length arrays are used:

Let me search for more context about how this function is used.

Let me check the test file to understand how this function is tested and if there are any edge cases covered.

Let me check the C++ implementation to see if there's any validation in the underlying function.

Let me check the C++ header file for the implementation details.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for all calls to triangulate_path_edge_numpy
rg -A 2 'triangulate_path_edge_numpy'

Length of output: 420

Script:

#!/bin/bash
# Search for imports or references of triangulate_path_edge_numpy
rg -l "triangulate_path_edge_numpy"

# Search for test files related to triangulation
fd "test.*triangulat.*" 

# Look for any validation of path arguments
rg "path\.shape|len\(path\)" src/PartSegCore_compiled_backend/triangulate.pyx

Length of output: 284

Script:

#!/bin/bash
# Check the test file content
cat src/tests/test_triangulate.py

# Also check if there's any validation in the C++ implementation
rg -A 5 "triangulate_path_edge" src/PartSegCore_compiled_backend/triangulate.pyx

Length of output: 24047

Script:

#!/bin/bash
# Look for the C++ implementation of triangulate_path_edge
fd -e cpp -e hpp "triangulate"

Length of output: 93

Script:

#!/bin/bash
# Check the C++ header implementation
cat src/PartSegCore_compiled_backend/triangulation/triangulate.hpp

Length of output: 48891


def triangulate_polygon_with_edge_numpy_li(polygon_li: list[np.ndarray]) -> tuple[tuple[np.ndarray, np.ndarray], tuple[np.ndarray, np.ndarray, np.ndarray]]:
""" Triangulate polygon"""
Expand All @@ -396,18 +436,21 @@ def triangulate_polygon_with_edge_numpy_li(polygon_li: list[np.ndarray]) -> tupl
cdef Point p1, p2
cdef pair[vector[Triangle], vector[Point]] triangulation_result
cdef vector[PathTriangulation] edge_result
cdef cnp.ndarray[cnp.uint32_t, ndim=2] triangles
cdef cnp.ndarray[cnp.uint32_t, ndim=2] triangles, edge_triangles

cdef cnp.ndarray[cnp.float32_t, ndim=2] points, edge_offsets, edges_centers, polygon
cdef size_t i, j, len_path, edge_triangle_count, edge_center_count, edge_triangle_index, edge_center_index

polygon_vector_list.reserve(len(polygon_li))
for polygon in polygon_li:
polygon_vector.clear()
for i in range(len(polygon_li)):
polygon = polygon_li[i]

polygon_vector.reserve(polygon.shape[0])
polygon_vector.push_back(Point(polygon[0, 0], polygon[0, 1]))

for point in polygon[1:]:
for j in range(1, polygon.shape[0]):
p1 = polygon_vector.back()
p2 = Point(point[0], point[1])
p2 = Point(polygon[j, 0], polygon[j, 1])
if p1 != p2:
# prevent from adding polygon edge of width 0
polygon_vector.push_back(p2)
Expand All @@ -420,18 +463,51 @@ def triangulate_polygon_with_edge_numpy_li(polygon_li: list[np.ndarray]) -> tupl
with cython.nogil:
triangulation_result = triangulate_polygon_face(polygon_vector_list)

if triangulation_result.first.size() == 0:
triangles = np.zeros((0, 3), dtype=np.uint32)
else:
triangles = np.array([(triangle.x, triangle.y, triangle.z) for triangle in triangulation_result.first], dtype=np.uint32)

triangles = np.empty((triangulation_result.first.size(), 3), dtype=np.uint32)
for i in range(triangulation_result.first.size()):
triangles[i, 0] = triangulation_result.first[i].x
triangles[i, 1] = triangulation_result.first[i].y
triangles[i, 2] = triangulation_result.first[i].z

points =np.empty((triangulation_result.second.size(), 2), dtype=np.float32)
for i in range(triangulation_result.second.size()):
points[i, 0] = triangulation_result.second[i].x
points[i, 1] = triangulation_result.second[i].y

edge_triangle_count = 0
edge_center_count = 0
for i in range(edge_result.size()):
edge_triangle_count += edge_result[i].triangles.size()
edge_center_count += edge_result[i].centers.size()

edge_triangles = np.empty((edge_triangle_count, 3), dtype=np.uint32)
edge_offsets = np.empty((edge_center_count, 2), dtype=np.float32)
edges_centers = np.empty((edge_center_count, 2), dtype=np.float32)

edge_triangle_index = 0
edge_center_index = 0
for i in range(edge_result.size()):
for j in range(edge_result[i].triangles.size()):
edge_triangles[edge_triangle_index, 0] = edge_result[i].triangles[j].x
edge_triangles[edge_triangle_index, 1] = edge_result[i].triangles[j].y
edge_triangles[edge_triangle_index, 2] = edge_result[i].triangles[j].z
edge_triangle_index += 1

for j in range(edge_result[i].centers.size()):
edges_centers[edge_center_index, 0] = edge_result[i].centers[j].x
edges_centers[edge_center_index, 1] = edge_result[i].centers[j].y
edge_offsets[edge_center_index, 0] = edge_result[i].offsets[j].x
edge_offsets[edge_center_index, 1] = edge_result[i].offsets[j].y
edge_center_index += 1

return ((
triangles,
np.array([(point.x, point.y) for point in triangulation_result.second], dtype=np.float32)
points,
),
(
np.array([(point.x, point.y) for res in edge_result for point in res.centers], dtype=np.float32),
np.array([(offset.x, offset.y) for res in edge_result for offset in res.offsets], dtype=np.float32),
np.array([(triangle.x, triangle.y, triangle.z) for res in edge_result for triangle in res.triangles], dtype=np.uint32),
edges_centers,
edge_offsets,
edge_triangles,
)
)
165 changes: 85 additions & 80 deletions src/tests/test_triangulate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
segment_left_to_right_comparator,
triangle_convex_polygon,
triangulate_monotone_polygon_py,
triangulate_path_edge_numpy,
triangulate_path_edge_py,
triangulate_polygon_numpy,
triangulate_polygon_numpy_li,
Expand Down Expand Up @@ -559,88 +560,92 @@ def test_triangulate_monotone_polygon_py(polygon, expected):
assert triangulate_monotone_polygon_py(*polygon) == expected


PATH_DATA = [
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
True,
False,
10,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [6, 5, 4], [5, 6, 7], [8, 7, 6], [7, 8, 9]],
),
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
False,
False,
8,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [6, 5, 4], [5, 6, 7]],
),
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
True,
True,
14,
[
[2, 1, 0],
[3, 2, 0],
[2, 3, 4],
[5, 4, 3],
[6, 5, 3],
[5, 6, 7],
[8, 7, 6],
[9, 8, 6],
[8, 9, 10],
[11, 10, 9],
[12, 11, 9],
[11, 12, 13],
],
),
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
False,
True,
10,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [5, 4, 2], [4, 5, 6], [7, 6, 5], [8, 7, 5], [7, 8, 9]],
),
(
[[2, 10], [0, -5], [-2, 10], [-2, -10], [2, -10]],
True,
False,
15,
[
[2, 1, 0],
[1, 2, 3],
[1, 3, 4],
[5, 4, 3],
[6, 5, 3],
[5, 6, 7],
[8, 7, 6],
[7, 8, 9],
[7, 9, 10],
[11, 10, 9],
[10, 11, 12],
[13, 12, 11],
[12, 13, 14],
],
),
([[0, 0], [0, 10]], False, False, 4, [[2, 1, 0], [1, 2, 3]]),
([[0, 0], [0, 10], [0, 20]], False, False, 6, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5]]),
(
[[0, 0], [0, 2], [10, 1]],
True,
False,
9,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [6, 5, 4], [7, 6, 4], [6, 7, 8]],
),
([[0, 0], [10, 1], [9, 1.1]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [3, 5, 6]]),
([[9, 0.9], [10, 1], [0, 2]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [3, 5, 6]]),
([[0, 0], [-10, 1], [-9, 1.1]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [5, 4, 2], [4, 5, 6]]),
([[-9, 0.9], [-10, 1], [0, 2]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [5, 4, 2], [4, 5, 6]]),
]


@pytest.mark.parametrize(
('path', 'closed', 'bevel', 'expected', 'exp_triangles'),
[
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
True,
False,
10,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [6, 5, 4], [5, 6, 7], [8, 7, 6], [7, 8, 9]],
),
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
False,
False,
8,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [6, 5, 4], [5, 6, 7]],
),
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
True,
True,
14,
[
[2, 1, 0],
[3, 2, 0],
[2, 3, 4],
[5, 4, 3],
[6, 5, 3],
[5, 6, 7],
[8, 7, 6],
[9, 8, 6],
[8, 9, 10],
[11, 10, 9],
[12, 11, 9],
[11, 12, 13],
],
),
(
[[0, 0], [0, 10], [10, 10], [10, 0]],
False,
True,
10,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [5, 4, 2], [4, 5, 6], [7, 6, 5], [8, 7, 5], [7, 8, 9]],
),
(
[[2, 10], [0, -5], [-2, 10], [-2, -10], [2, -10]],
True,
False,
15,
[
[2, 1, 0],
[1, 2, 3],
[1, 3, 4],
[5, 4, 3],
[6, 5, 3],
[5, 6, 7],
[8, 7, 6],
[7, 8, 9],
[7, 9, 10],
[11, 10, 9],
[10, 11, 12],
[13, 12, 11],
[12, 13, 14],
],
),
([[0, 0], [0, 10]], False, False, 4, [[2, 1, 0], [1, 2, 3]]),
([[0, 0], [0, 10], [0, 20]], False, False, 6, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5]]),
(
[[0, 0], [0, 2], [10, 1]],
True,
False,
9,
[[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [6, 5, 4], [7, 6, 4], [6, 7, 8]],
),
([[0, 0], [10, 1], [9, 1.1]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [3, 5, 6]]),
([[9, 0.9], [10, 1], [0, 2]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [3, 4, 5], [3, 5, 6]]),
([[0, 0], [-10, 1], [-9, 1.1]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [5, 4, 2], [4, 5, 6]]),
([[-9, 0.9], [-10, 1], [0, 2]], False, False, 7, [[2, 1, 0], [1, 2, 3], [4, 3, 2], [5, 4, 2], [4, 5, 6]]),
],
PATH_DATA,
)
def test_triangulate_path_edge_py(path, closed, bevel, expected, exp_triangles):
centers, offsets, triangles = triangulate_path_edge_py(np.array(path, dtype='float32'), closed=closed, bevel=bevel)
@pytest.mark.parametrize('triangulate_fun', [triangulate_path_edge_py, triangulate_path_edge_numpy])
def test_triangulate_path_edge_py(path, closed, bevel, expected, exp_triangles, triangulate_fun):
centers, offsets, triangles = triangulate_fun(np.array(path, dtype='float32'), closed=closed, bevel=bevel)
assert centers.shape == offsets.shape
assert centers.shape[0] == expected
assert triangles.shape[0] == expected - 2
Expand All @@ -651,7 +656,7 @@ def test_triangulate_path_edge_py(path, closed, bevel, expected, exp_triangles):
@pytest.mark.parametrize(('polygon', 'expected'), TEST_POLYGONS)
def test_triangulate_polygon_with_edge_numpy_li(polygon, expected):
(triangles, points), (centers, offsets, edge_triangles) = triangulate_polygon_with_edge_numpy_li(
[np.array(polygon)]
[np.array(polygon, dtype=np.float32)]
)
triangles_ = _renumerate_triangles(polygon, points, triangles)
assert triangles_ == expected
Expand Down
Loading