Fix new older monocot traits for edge cases #77
Conversation
|
Warning Rate Limit Exceeded@eberrigan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 28 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates bring significant improvements to geometry handling and intersection computations in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 74.58% 74.62% +0.04%
==========================================
Files 13 13
Lines 1326 1344 +18
==========================================
+ Hits 989 1003 +14
- Misses 337 341 +4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Out of diff range and nitpick comments (1)
tests/test_points.py (1)
Line range hint
512-512: Duplicate function definition fortest_associate_lateral_to_primary_valid_input. Consider removing or renaming one of the implementations to avoid confusion and potential override issues.
tests/test_points.py
Outdated
|
|
||
| def test_extract_from_unsupported_type(): | ||
| with pytest.raises(NameError): | ||
| extract_points_from_geometry(Polygon([(0, 0), (1, 1), (1, 0)])) # Polygon is unsupported |
There was a problem hiding this comment.
The test attempts to use an unsupported Polygon type, which is not defined in the imports.
- 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.
| 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. |
| from sleap_roots.points import ( | ||
| extract_points_from_geometry, | ||
| get_line_equation_from_points, | ||
| ) |
There was a problem hiding this comment.
Unused imports detected.
- from shapely import box, LineString, normalize, Polygon
+ from shapely import LineStringRemove 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.
| 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, | |
| ) |
| 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( | ||
| "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 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).") |
There was a problem hiding this comment.
📝 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.
| @@ -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).") | |||
There was a problem hiding this comment.
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.
| 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).") |
sleap_roots/points.py
Outdated
| 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__}") |
There was a problem hiding this comment.
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.
| 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 test_extract_from_unsupported_type(): | ||
| with pytest.raises(NameError): | ||
| extract_points_from_geometry( | ||
| Polygon([(0, 0), (1, 1), (1, 0)]) | ||
| ) # Polygon is unsupported |
There was a problem hiding this comment.
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.
| 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. |
eberrigan
deleted the
elizabeth/Fix-new-older-monocot-traits-for-edge-cases
branch
MultiPointobject butLineString, a single point or a collection of geometry objectsSummary by CodeRabbit
New Features
Bug Fixes
Tests