Consistently validate polygon inputs for GeoArrow offset format #973

harrism · 2023-03-07T05:34:08Z

Description

Fixes #962. Discussion: #972.

Adds a new C++ macro to validate polygon offset array sizes. Updates all C++ APIs that accepts polygons to use this macro for consistent validation and requirements. All C++ functions that accept polygon data should accept and enforce GeoArrow offset format ("N + 1 offsets").

Updates C++ tests to correctly pass offsets.

Updates Python implementation to no longer slice off final offsets.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

… validate polygon inputs.

isVoid

I think this is good in general - my point-polygon distance can also benefit from this macro. Would you like to wait till the closedness discussion to close (no-pun intended) until we merge this PR?

cpp/include/cuspatial/detail/utility/validation.hpp

harrism · 2023-03-08T03:33:24Z

I think this is good in general - my point-polygon distance can also benefit from this macro. Would you like to wait till the closedness discussion to close (no-pun intended) until we merge this PR?

I plan to update this based on that discussion, yes. Especially the change to the arithmetic for empty lists.

robertmaynard

Approving CMake changes

harrism · 2023-03-08T22:00:53Z

I think this is good in general - my point-polygon distance can also benefit from this macro. Would you like to wait till the closedness discussion to close (no-pun intended) until we merge this PR?

I plan to update this based on that discussion, yes. Especially the change to the arithmetic for empty lists.

Updated

cpp/src/spatial/point_in_polygon.cu

cpp/src/spatial/polygon_bounding_box.cu

cpp/tests/spatial/point_in_polygon_test.cpp

Co-authored-by: H. Thomson Comer <thomcom@gmail.com>

cpp/include/cuspatial/experimental/detail/point_in_polygon.cuh

isVoid · 2023-03-08T22:57:32Z

cpp/src/spatial/pairwise_point_in_polygon.cu

-                    "Each ring must have at least four vertices");
-
-  CUSPATIAL_EXPECTS(test_points_x.size() == poly_offsets.size(),
+  CUSPATIAL_EXPECTS(test_points_x.size() == std::max(poly_offsets.size() - 1, 0),


Since offsets must be greater or equal to 1, poly_offsets.size() - 1 >= 0. No need to wrap with std::max.

This needs to pass when an empty test_points column is passed, regardless of whether the polygon offsets are malformed. If an empty offsets array is also passed, this will be 0 == -1, and this will throw. Hence the need for std::max. Even though an empty polygon offsets column is not technically correct, I think we should consider it to mean "zero polygons" for this case where there are also zero test points.

Ok... Note that in header only API you added the test macro, if the offset is malformed the API will throw anyway.

If I remove std::max this test fails:

cuspatial/cpp/tests/spatial/pairwise_point_in_polygon_test.cpp

Line 43 in e9a84b9

TYPED_TEST(PairwisePointInPolygonTest, Empty)

If I leave it in, it passes. I think we want this case to not throw.

harrism · 2023-03-09T01:04:21Z

/merge

Closes #958. Replaces usage of GDAL `contains` in the quadtree point-in-polygon tests with a call to a different cuSpatial point-in-polygon function. This allows us to remove all current dependencies on GDAL in cuSpatial. Depends on #973. This PR currently updates some expectations to make tests pass that are fixed in that PR. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Wang (https://github.com/isVoid) - H. Thomson Comer (https://github.com/thomcom) - Paul Taylor (https://github.com/trxcllnt) - Jordan Jacobelli (https://github.com/jjacobelli) URL: #974

…dsai#973) Fixes rapidsai#962. Discussion: rapidsai#972. Adds a new C++ macro to validate polygon offset array sizes. Updates all C++ APIs that accepts polygons to use this macro for consistent validation and requirements. All C++ functions that accept polygon data should accept and enforce GeoArrow offset format ("N + 1 offsets"). Updates C++ tests to correctly pass offsets. Updates Python implementation to no longer slice off final offsets. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Robert Maynard (https://github.com/robertmaynard) - H. Thomson Comer (https://github.com/thomcom) - Michael Wang (https://github.com/isVoid) URL: rapidsai#973

Closes rapidsai#958. Replaces usage of GDAL `contains` in the quadtree point-in-polygon tests with a call to a different cuSpatial point-in-polygon function. This allows us to remove all current dependencies on GDAL in cuSpatial. Depends on rapidsai#973. This PR currently updates some expectations to make tests pass that are fixed in that PR. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Wang (https://github.com/isVoid) - H. Thomson Comer (https://github.com/thomcom) - Paul Taylor (https://github.com/trxcllnt) - Jordan Jacobelli (https://github.com/jjacobelli) URL: rapidsai#974

…cts (#976) - Adds `multipolygon_range`, `multipolygon_ref`, `polygon_ref` as non-owning objects for geoarrow compliant polygon types. - Adds `pairwise_point_polygon_distance` to compute the shortest distances between two columns of multipoints and multipolygons. - Refactors `is_point_in_polygon` with geometry object input. Dependent on #973 since geometry objects requires geoarrow input. closes #703 Authors: - Michael Wang (https://github.com/isVoid) Approvers: - Mark Harris (https://github.com/harrism) - H. Thomson Comer (https://github.com/thomcom) URL: #976

Add polygon validation C++ macro and update functions to consistently…

ac0b752

… validate polygon inputs.

github-actions bot added libcuspatial Relates to the cuSpatial C++ library Python Related to Python code labels Mar 7, 2023

Remove debug prints

7e31dfb

harrism self-assigned this Mar 7, 2023

harrism added bug Something isn't working 3 - Ready for Review Ready for review by team breaking Breaking change labels Mar 7, 2023

harrism marked this pull request as ready for review March 7, 2023 05:40

harrism requested review from a team as code owners March 7, 2023 05:40

harrism requested review from isVoid, zhangjianting, thomcom and trxcllnt and removed request for zhangjianting March 7, 2023 05:40

harrism mentioned this pull request Mar 7, 2023

Remove GDAL dependency in quadtree spatial join tests. #974

Merged

3 tasks

Copyright

285132c

harrism removed the 3 - Ready for Review Ready for review by team label Mar 7, 2023

isVoid reviewed Mar 8, 2023

View reviewed changes

cpp/include/cuspatial/detail/utility/validation.hpp Outdated Show resolved Hide resolved

harrism added 2 commits March 8, 2023 22:35

Update validation rules based on discussion.

c4864d0

Change bounding_boxes .cu tests to .cpp

40215f8

harrism requested a review from a team as a code owner March 8, 2023 11:41

github-actions bot added the cmake Related to CMake code or build configuration label Mar 8, 2023

robertmaynard approved these changes Mar 8, 2023

View reviewed changes

thomcom approved these changes Mar 8, 2023

View reviewed changes

cpp/src/spatial/point_in_polygon.cu Show resolved Hide resolved

cpp/src/spatial/polygon_bounding_box.cu Show resolved Hide resolved

cpp/tests/spatial/point_in_polygon_test.cpp Outdated Show resolved Hide resolved

harrism and others added 2 commits March 9, 2023 09:23

Update cpp/tests/spatial/point_in_polygon_test.cpp

e3c3644

Co-authored-by: H. Thomson Comer <thomcom@gmail.com>

style

74b0bfe

thomcom mentioned this pull request Mar 8, 2023

[FEA]: point_in_polygon doesn't use GeoArrow style offsets #829

Closed

isVoid approved these changes Mar 8, 2023

View reviewed changes

isVoid linked an issue Mar 8, 2023 that may be closed by this pull request

[FEA]: point_in_polygon doesn't use GeoArrow style offsets #829

Closed

isVoid mentioned this pull request Mar 8, 2023

Add Header-Only point_polygon_distance, add non-owning polygon objects #976

Merged

3 tasks

harrism added 2 commits March 9, 2023 10:28

Move to inner scope.

3eff195

Oops

e9a84b9

harrism mentioned this pull request Mar 8, 2023

Use Quadtree, Pairwise, and Brute-Force PIP methods when appropriate with .contains_properly and other pip-only binary predicates. #981

Closed

3 tasks

Merge branch 'branch-23.04' into fea-polygon-validation

8e7654f

rapids-bot bot merged commit 25466ed into rapidsai:branch-23.04 Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistently validate polygon inputs for GeoArrow offset format #973

Consistently validate polygon inputs for GeoArrow offset format #973

harrism commented Mar 7, 2023

isVoid left a comment

harrism commented Mar 8, 2023

robertmaynard left a comment

harrism commented Mar 8, 2023

isVoid Mar 8, 2023

harrism Mar 8, 2023 •

edited

Loading

isVoid Mar 8, 2023

harrism Mar 9, 2023

harrism commented Mar 9, 2023

Consistently validate polygon inputs for GeoArrow offset format #973

Consistently validate polygon inputs for GeoArrow offset format #973

Conversation

harrism commented Mar 7, 2023

Description

Checklist

isVoid left a comment

Choose a reason for hiding this comment

harrism commented Mar 8, 2023

robertmaynard left a comment

Choose a reason for hiding this comment

harrism commented Mar 8, 2023

isVoid Mar 8, 2023

Choose a reason for hiding this comment

harrism Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

isVoid Mar 8, 2023

Choose a reason for hiding this comment

harrism Mar 9, 2023

Choose a reason for hiding this comment

harrism commented Mar 9, 2023

harrism Mar 8, 2023 •

edited

Loading