Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix new older monocot traits for edge cases #77

Merged
merged 5 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
24 changes: 16 additions & 8 deletions sleap_roots/convhull.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
from scipy.spatial import ConvexHull
from scipy.spatial.distance import pdist
from typing import Tuple, Optional, Union
from sleap_roots.points import get_line_equation_from_points
from sleap_roots.points import (
extract_points_from_geometry,
get_line_equation_from_points,
)
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused imports detected.

- from shapely import box, LineString, normalize, Polygon
+ from shapely import LineString

Remove the unused imports box, normalize, and Polygon to clean up the code and avoid potential confusion about their necessity.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
from sleap_roots.points import (
extract_points_from_geometry,
get_line_equation_from_points,
)
from sleap_roots.points import (
extract_points_from_geometry,
get_line_equation_from_points,
)

from shapely import box, LineString, normalize, Polygon


Expand Down Expand Up @@ -382,13 +385,9 @@
# Find the intersection between the hull perimeter and the extended line
intersection = extended_line.intersection(hull_perimeter)

# Add intersection points to both lists
# Compute the intersection points and add to lists
if not intersection.is_empty:
intersect_points = (
np.array([[point.x, point.y] for point in intersection.geoms])
if intersection.geom_type == "MultiPoint"
else np.array([[intersection.x, intersection.y]])
)
intersect_points = extract_points_from_geometry(intersection)
above_line.extend(intersect_points)
below_line.extend(intersect_points)

Expand Down Expand Up @@ -452,6 +451,12 @@
Raises:
ValueError: If pts does not have the expected shape.
"""
if r0_pts.ndim == 1 or rn_pts.ndim == 1 or pts.ndim == 2:
print(

Check warning on line 455 in sleap_roots/convhull.py

View check run for this annotation

Codecov / codecov/patch

sleap_roots/convhull.py#L455

Added line #L455 was not covered by tests
"Not enough instances or incorrect format to compute convex hull intersections."
)
return (np.array([[np.nan, np.nan]]), np.array([[np.nan, np.nan]]))

Check warning on line 458 in sleap_roots/convhull.py

View check run for this annotation

Codecov / codecov/patch

sleap_roots/convhull.py#L458

Added line #L458 was not covered by tests

# Check for valid pts input
if not isinstance(pts, np.ndarray) or pts.ndim != 3 or pts.shape[-1] != 2:
raise ValueError("pts must be a numpy array of shape (instances, nodes, 2).")
Comment on lines 451 to 462
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [454-468]

Ensure proper input validation.

- if r0_pts.ndim == 1 or rn_pts.ndim == 1 or pts.ndim == 2:
+ if r0_pts.ndim != 2 or rn_pts.ndim != 2 or pts.ndim != 3:

The condition for checking the dimensions of the input arrays should ensure that they meet the expected dimensions explicitly, rather than checking for what they should not be. This makes the validation more robust and understandable.

Expand All @@ -460,7 +465,7 @@
raise ValueError("rn_pts must be a numpy array of shape (instances, 2).")
# Ensure r0_pts is a numpy array of shape (instances, 2)
if not isinstance(r0_pts, np.ndarray) or r0_pts.ndim != 2 or r0_pts.shape[-1] != 2:
raise ValueError("r0_pts must be a numpy array of shape (instances, 2).")
raise ValueError(f"r0_pts must be a numpy array of shape (instances, 2).")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unnecessary f-string.

- raise ValueError(f"r0_pts must be a numpy array of shape (instances, 2).")
+ raise ValueError("r0_pts must be a numpy array of shape (instances, 2).")

The f-string in this line does not contain any placeholders, making it unnecessary. Replace it with a regular string to avoid confusion and potential errors in future modifications.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
raise ValueError(f"r0_pts must be a numpy array of shape (instances, 2).")
raise ValueError("r0_pts must be a numpy array of shape (instances, 2).")


# Flatten pts to 2D array and remove NaN values
flattened_pts = pts.reshape(-1, 2)
Expand All @@ -481,6 +486,9 @@

# Ensuring r0_pts does not contain NaN values
r0_pts_valid = r0_pts[~np.isnan(r0_pts).any(axis=1)]
# Expect two vectors in the end
if len(r0_pts_valid) < 2:
return (np.array([[np.nan, np.nan]]), np.array([[np.nan, np.nan]]))

# Get the vertices of the convex hull
hull_vertices = hull.points[hull.vertices]
Expand Down
43 changes: 42 additions & 1 deletion sleap_roots/points.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,52 @@
import numpy as np
from matplotlib import pyplot as plt
from matplotlib.lines import Line2D
from shapely.geometry import LineString
from shapely.geometry import Point, MultiPoint, LineString, GeometryCollection
from shapely.ops import nearest_points
from typing import List, Optional, Tuple


def extract_points_from_geometry(geometry):
"""Extracts coordinates as a list of numpy arrays from any given Shapely geometry object.

This function supports Point, MultiPoint, LineString, and GeometryCollection types.
It recursively extracts coordinates from complex geometries and aggregates them into a single list.
For unsupported geometry types, it returns an empty list.

Parameters:
- geometry (shapely.geometry.base.BaseGeometry): A Shapely geometry object from which to extract points.

Returns:
- List[np.ndarray]: A list of numpy arrays, where each array represents the coordinates of a point.
The list will be empty if the geometry type is unsupported or contains no coordinates.

Raises:
- TypeError: If the input is not a recognized Shapely geometry type.

Example:
>>> from shapely.geometry import Point, MultiPoint, LineString, GeometryCollection
>>> point = Point(1, 2)
>>> multipoint = MultiPoint([(1, 2), (3, 4)])
>>> linestring = LineString([(0, 0), (1, 1), (2, 2)])
>>> geom_col = GeometryCollection([point, multipoint, linestring])
>>> extract_points_from_geometry(geom_col)
[array([1, 2]), array([1, 2]), array([3, 4]), array([0, 0]), array([1, 1]), array([2, 2])]
"""
if isinstance(geometry, Point):
return [np.array([geometry.x, geometry.y])]
elif isinstance(geometry, MultiPoint):
return [np.array([point.x, point.y]) for point in geometry.geoms]
elif isinstance(geometry, LineString):
return [np.array([x, y]) for x, y in zip(*geometry.xy)]
elif isinstance(geometry, GeometryCollection):
points = []
for geom in geometry.geoms:
points.extend(extract_points_from_geometry(geom))
return points
else:
raise TypeError(f"Unsupported geometry type: {type(geometry).__name__}")

Check warning on line 49 in sleap_roots/points.py

View check run for this annotation

Codecov / codecov/patch

sleap_roots/points.py#L49

Added line #L49 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Implement robust error handling in geometry extraction.

- raise TypeError(f"Unsupported geometry type: {type(geometry).__name__}")
+ if not isinstance(geometry, (Point, MultiPoint, LineString, GeometryCollection)):
+     raise TypeError(f"Unsupported geometry type: {type(geometry).__name__}")

Modify the error handling to check explicitly for supported types before processing the geometry. This prevents the function from attempting to process an unsupported type, which currently would only raise an error after failing to match any conditions.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
def extract_points_from_geometry(geometry):
"""Extracts coordinates as a list of numpy arrays from any given Shapely geometry object.
This function supports Point, MultiPoint, LineString, and GeometryCollection types.
It recursively extracts coordinates from complex geometries and aggregates them into a single list.
For unsupported geometry types, it returns an empty list.
Parameters:
- geometry (shapely.geometry.base.BaseGeometry): A Shapely geometry object from which to extract points.
Returns:
- List[np.ndarray]: A list of numpy arrays, where each array represents the coordinates of a point.
The list will be empty if the geometry type is unsupported or contains no coordinates.
Raises:
- TypeError: If the input is not a recognized Shapely geometry type.
Example:
>>> from shapely.geometry import Point, MultiPoint, LineString, GeometryCollection
>>> point = Point(1, 2)
>>> multipoint = MultiPoint([(1, 2), (3, 4)])
>>> linestring = LineString([(0, 0), (1, 1), (2, 2)])
>>> geom_col = GeometryCollection([point, multipoint, linestring])
>>> extract_points_from_geometry(geom_col)
[array([1, 2]), array([1, 2]), array([3, 4]), array([0, 0]), array([1, 1]), array([2, 2])]
"""
if isinstance(geometry, Point):
return [np.array([geometry.x, geometry.y])]
elif isinstance(geometry, MultiPoint):
return [np.array([point.x, point.y]) for point in geometry.geoms]
elif isinstance(geometry, LineString):
return [np.array([x, y]) for x, y in zip(*geometry.xy)]
elif isinstance(geometry, GeometryCollection):
points = []
for geom in geometry.geoms:
points.extend(extract_points_from_geometry(geom))
return points
else:
raise TypeError(f"Unsupported geometry type: {type(geometry).__name__}")
def extract_points_from_geometry(geometry):
"""Extracts coordinates as a list of numpy arrays from any given Shapely geometry object.
This function supports Point, MultiPoint, LineString, and GeometryCollection types.
It recursively extracts coordinates from complex geometries and aggregates them into a single list.
For unsupported geometry types, it returns an empty list.
Parameters:
- geometry (shapely.geometry.base.BaseGeometry): A Shapely geometry object from which to extract points.
Returns:
- List[np.ndarray]: A list of numpy arrays, where each array represents the coordinates of a point.
The list will be empty if the geometry type is unsupported or contains no coordinates.
Raises:
- TypeError: If the input is not a recognized Shapely geometry type.
Example:
>>> from shapely.geometry import Point, MultiPoint, LineString, GeometryCollection
>>> point = Point(1, 2)
>>> multipoint = MultiPoint([(1, 2), (3, 4)])
>>> linestring = LineString([(0, 0), (1, 1), (2, 2)])
>>> geom_col = GeometryCollection([point, multipoint, linestring])
>>> extract_points_from_geometry(geom_col)
[array([1, 2]), array([1, 2]), array([3, 4]), array([0, 0]), array([1, 1]), array([2, 2])]
"""
if isinstance(geometry, Point):
return [np.array([geometry.x, geometry.y])]
elif isinstance(geometry, MultiPoint):
return [np.array([point.x, point.y]) for point in geometry.geoms]
elif isinstance(geometry, LineString):
return [np.array([x, y]) for x, y in zip(*geometry.xy)]
elif isinstance(geometry, GeometryCollection):
points = []
for geom in geometry.geoms:
points.extend(extract_points_from_geometry(geom))
return points
else:
if not isinstance(geometry, (Point, MultiPoint, LineString, GeometryCollection)):
raise TypeError(f"Unsupported geometry type: {type(geometry).__name__}")



def get_count(pts: np.ndarray):
"""Get number of roots.

Expand Down
1 change: 0 additions & 1 deletion tests/test_convhull.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,6 @@ def test_basic_functionality(pts_shape_3_6_2):
@pytest.mark.parametrize(
"invalid_input",
[
(np.array([1, 2]), np.array([3, 4]), np.array([[[1, 2], [3, 4]]]), None),
(np.array([[1, 2, 3]]), np.array([[3, 4]]), np.array([[[1, 2], [3, 4]]]), None),
# Add more invalid inputs as needed
],
Expand Down
61 changes: 59 additions & 2 deletions tests/test_points.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import numpy as np
import pytest
from shapely.geometry import LineString
from shapely.geometry import Point, MultiPoint, LineString, GeometryCollection
from sleap_roots import Series
from sleap_roots.lengths import get_max_length_pts
from sleap_roots.points import filter_plants_with_unexpected_ct, get_count, join_pts
from sleap_roots.points import (
extract_points_from_geometry,
filter_plants_with_unexpected_ct,
get_count,
join_pts,
)
from sleap_roots.points import (
get_all_pts_array,
get_nodes,
Expand Down Expand Up @@ -738,3 +743,55 @@ def test_filter_plants_with_unexpected_ct_incorrect_input_types():
expected_count = "not a float"
with pytest.raises(ValueError):
filter_plants_with_unexpected_ct(primary_pts, lateral_pts, expected_count)


def test_extract_from_point():
point = Point(1, 2)
expected = [np.array([1, 2])]
assert np.array_equal(extract_points_from_geometry(point), expected)


def test_extract_from_multipoint():
multipoint = MultiPoint([(1, 2), (3, 4)])
expected = [np.array([1, 2]), np.array([3, 4])]
results = extract_points_from_geometry(multipoint)
assert all(np.array_equal(result, exp) for result, exp in zip(results, expected))


def test_extract_from_linestring():
linestring = LineString([(0, 0), (1, 1), (2, 2)])
expected = [np.array([0, 0]), np.array([1, 1]), np.array([2, 2])]
results = extract_points_from_geometry(linestring)
assert all(np.array_equal(result, exp) for result, exp in zip(results, expected))


def test_extract_from_geometrycollection():
geom_collection = GeometryCollection([Point(1, 2), LineString([(0, 0), (1, 1)])])
expected = [np.array([1, 2]), np.array([0, 0]), np.array([1, 1])]
results = extract_points_from_geometry(geom_collection)
assert all(np.array_equal(result, exp) for result, exp in zip(results, expected))


def test_extract_from_empty_multipoint():
empty_multipoint = MultiPoint()
expected = []
assert extract_points_from_geometry(empty_multipoint) == expected


def test_extract_from_empty_linestring():
empty_linestring = LineString()
expected = []
assert extract_points_from_geometry(empty_linestring) == expected


def test_extract_from_unsupported_type():
with pytest.raises(NameError):
extract_points_from_geometry(
Polygon([(0, 0), (1, 1), (1, 0)])
) # Polygon is unsupported
Comment on lines +787 to +791
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle unsupported geometry type correctly.

-        extract_points_from_geometry(Polygon([(0, 0), (1, 1), (1, 0)]))  # Polygon is unsupported
+        # Polygon is unsupported, consider adding support or handling this case differently.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
def test_extract_from_unsupported_type():
with pytest.raises(NameError):
extract_points_from_geometry(
Polygon([(0, 0), (1, 1), (1, 0)])
) # Polygon is unsupported
def test_extract_from_unsupported_type():
with pytest.raises(NameError):
# Polygon is unsupported, consider adding support or handling this case differently.



def test_extract_from_empty_geometrycollection():
empty_geom_collection = GeometryCollection()
expected = []
assert extract_points_from_geometry(empty_geom_collection) == expected
Loading