Handle unsupported geometry types #91
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several enhancements across multiple files, focusing on improved logging and error handling. In Changes
Possibly related PRs
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 76.16% 77.44% +1.28%
==========================================
Files 13 13
Lines 1397 1419 +22
==========================================
+ Hits 1064 1099 +35
+ Misses 333 320 -13 ☔ 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: 3
🧹 Outside diff range and nitpick comments (3)
sleap_roots/convhull.py (2)
391-398: Good addition of intersection validationThe additional checks for intersection existence and emptiness improve robustness. However, consider consolidating the logging and return statements for better maintainability.
- if intersection and not intersection.is_empty: - logging.debug("Intersection points found between the convex hull and the line.") - intersect_points = extract_points_from_geometry(intersection) - if not intersect_points: # Ensure it's not an empty list - return np.nan, np.nan - else: - logging.debug("No intersection points found between the convex hull and the line.") - return np.nan, np.nan + if not intersection or intersection.is_empty: + logging.debug("No intersection points found between the convex hull and the line.") + return np.nan, np.nan + + logging.debug("Intersection points found between the convex hull and the line.") + intersect_points = extract_points_from_geometry(intersection) + if not intersect_points: + logging.debug("No valid intersection points after extraction.") + return np.nan, np.nan
34-34: Add tests for error handling pathsSeveral error handling paths with logging statements are not covered by tests. Consider adding test cases for:
- Invalid input with infinite values in
get_convhull- Insufficient valid points in
get_chull_intersection_vectors- Points not on convex hull in
get_chull_intersection_vectors- Failed line equation calculation in
get_chull_intersection_vectorsWould you like me to help generate test cases for these scenarios?
Also applies to: 496-496, 525-525, 535-535
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: sleap_roots/convhull.py#L34
Added line #L34 was not covered by teststests/test_points.py (1)
822-833: Enhance test coverage for MultiLineString handling.The parameterized test provides good coverage for MultiLineString scenarios, but consider adding more test cases:
- Empty MultiLineString
- MultiLineString with empty coordinates
- MultiLineString with invalid coordinates (e.g., NaN values)
Apply this diff to add more test cases:
@pytest.mark.parametrize( "geometry, expected", [ (MultiLineString([[(0, 0), (1, 1)], [(2, 2), (3, 3)]]), []), (GeometryCollection([Point(1, 2), LineString([(0, 0), (1, 1)]), MultiLineString([[(0, 0), (1, 1)], [(2, 2), (3, 3)]])]), [np.array([1, 2]), np.array([0, 0]), np.array([1, 1])]), + (MultiLineString([]), []), # Empty MultiLineString + (MultiLineString([[()]]), []), # MultiLineString with empty coordinates + (MultiLineString([[(np.nan, np.nan), (1, 1)]]), []), # MultiLineString with invalid coordinates ], )🧰 Tools
🪛 Ruff
830-830: Redefinition of unused
test_extract_from_multilinestringfrom line 782(F811)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (72)
tests/data/rice_10do_pipeline_output/308A7B1CRW/1.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/10.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/11.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/12.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/13.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/14.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/15.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/16.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/17.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/18.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/19.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/2.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/20.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/21.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/22.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/23.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/24.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/25.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/26.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/27.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/28.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/29.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/3.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/30.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/31.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/32.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/33.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/34.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/35.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/36.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/37.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/38.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/39.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/4.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/40.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/41.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/42.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/43.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/44.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/45.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/46.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/47.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/48.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/49.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/5.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/50.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/51.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/52.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/53.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/54.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/55.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/56.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/57.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/58.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/59.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/6.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/60.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/61.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/62.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/63.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/64.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/65.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/66.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/67.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/68.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/69.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/7.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/70.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/71.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/72.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/8.jpgis excluded by!**/*.jpgtests/data/rice_10do_pipeline_output/308A7B1CRW/9.jpgis excluded by!**/*.jpg
📒 Files selected for processing (5)
- sleap_roots/convhull.py (7 hunks)
- sleap_roots/points.py (3 hunks)
- tests/data/rice_10do_pipeline_output/scan_7859150.model_221208_113552.multi_instance.n=574.root_crown.slp (1 hunks)
- tests/test_points.py (3 hunks)
- tests/test_trait_pipelines.py (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/data/rice_10do_pipeline_output/scan_7859150.model_221208_113552.multi_instance.n=574.root_crown.slp
🧰 Additional context used
🪛 GitHub Check: codecov/patch
sleap_roots/convhull.py
[warning] 34-34: sleap_roots/convhull.py#L34
Added line #L34 was not covered by tests
[warning] 496-496: sleap_roots/convhull.py#L496
Added line #L496 was not covered by tests
[warning] 525-525: sleap_roots/convhull.py#L525
Added line #L525 was not covered by tests
[warning] 535-535: sleap_roots/convhull.py#L535
Added line #L535 was not covered by tests
🪛 Ruff
tests/test_points.py
830-830: Redefinition of unused
test_extract_from_multilinestringfrom line 782(F811)
tests/test_trait_pipelines.py
233-233: Yoda condition detected
(SIM300)
237-237: Yoda condition detected
(SIM300)
🔇 Additional comments (9)
tests/test_trait_pipelines.py (4)
185-190: LGTM: Test updates align with PR objectives.The updated assertions correctly reflect the expected behavior when handling split geometries, ensuring tests pass when MultiLineString cases occur.
200-200: LGTM: Shape assertion correctly updated.The shape assertion has been properly adjusted to account for the additional test case while maintaining the expected number of computed traits.
225-238: LGTM: Robust handling of NaN values in assertions.The addition of
.dropna()calls appropriately handles cases where NaN values are expected due to unsupported geometry types, while still ensuring valid values are within the correct ranges.Note: The Yoda conditions (e.g.,
0 <= value) flagged by static analysis are acceptable in test assertions as they prevent accidental assignments.🧰 Tools
🪛 Ruff
233-233: Yoda condition detected
(SIM300)
237-237: Yoda condition detected
(SIM300)
Line range hint
185-238: Verify test coverage for geometry edge cases.To ensure thorough testing of the new geometry handling, we should verify that the test data includes cases that would trigger NaN values.
sleap_roots/convhull.py (1)
4-4: LGTM: Good addition of logging moduleThe addition of the logging module aligns with the goal of replacing print statements with proper logging.
sleap_roots/points.py (3)
4-4: LGTM: Logging import added correctly.The addition of the logging module import aligns with the changes to improve error handling and logging capabilities.
35-36: LGTM: Added informative debug logging.The debug logging statements provide valuable context about the geometry type and content, which will help with troubleshooting issues related to unsupported geometry types.
49-51: Verify callers handle empty list return value.The function now returns an empty list instead of raising a TypeError for unsupported geometry types. While this aligns with the PR objectives for handling unsupported types gracefully, we should verify that all callers can handle this change in behavior.
Let's check the usage of this function:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the safety of returning an empty list:
Empty list return value is safely handled by all callers
The codebase shows proper handling of empty list returns:
In
convhull.py, empty list checks are explicitly performed:intersect_points = extract_points_from_geometry(intersection) if not intersect_points: # Ensure it's not an empty listTest cases explicitly verify empty list behavior:
test_extract_from_empty_multipoint()test_extract_from_empty_linestring()test_extract_from_empty_geometrycollection()test_extract_from_multilinestring()(explicitly tests unsupported type returning empty list)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to extract_points_from_geometry and their error handling # Search for function calls with context echo "Searching for function calls..." rg -A 5 "extract_points_from_geometry\(" # Search for potential error handling patterns echo -e "\nSearching for error handling patterns..." rg -B 5 -A 5 "try:.*extract_points_from_geometry"Length of output: 6040
tests/test_points.py (1)
835-844: LGTM! Good coverage of unsupported geometry types.The test cases effectively verify that the function returns an empty list for various unsupported input types, aligning with the PR's objective of gracefully handling unsupported geometry types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/test_points.py (1)
829-838: Consider adding more edge cases.While the test covers basic invalid inputs (string, None, integer), consider adding these cases:
- Empty string
- Empty list/tuple
- Invalid geometry object
@pytest.mark.parametrize( "unexpected_input, expected_output", [ ("24", []), (None, []), (5, []), + ("", []), + ([], []), + ((), []), + (object(), []), ], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/test_points.py (2 hunks)
🔇 Additional comments (3)
tests/test_points.py (3)
806-806: LGTM!The indentation fix improves code readability.
813-814: LGTM!Added blank lines improve code organization and readability by separating test functions.
815-827: Well-structured test cases for MultiLineString handling!The parameterized test cases effectively cover:
- Regular MultiLineString
- GeometryCollection containing MultiLineString
- Empty MultiLineString
This aligns well with the PR objective of handling unsupported geometry types.
This pull request is to address issue #88.
There are some cases in which
MultiLineStringis possible as a geometry type made from the intersection of a line made by nodes of the same index and the convex hull.The below example occurs when there are no nodes past r1. Then both r1 points are included in the line of the intersection with the convex hull and for some reason shapely make the intersection two line strings, instead of one (see how the point is repeated in each line).
all points:
r1 points:
Picture with repeated point in purple, r1 nodes in green, convex hull in black,
MultiStringLines in blue and yellow:Generally, if there are no nodes beyond the line used to calculate the convex hull division area or intersection angles, we should return NaN values. This approach maintains consistency with the use of NaN across the codebase and reflects that, with much of the plant missing, these quantities do not represent meaningful physical values.
get_chull_intersection_vectorsandget_chull_areas_via_intersection.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
MultiLineStringand infinite values.