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

[BUG]: Polygon data layout expectations are unclear and inconsistent #962

Closed
harrism opened this issue Mar 1, 2023 · 0 comments · Fixed by #973
Closed

[BUG]: Polygon data layout expectations are unclear and inconsistent #962

harrism opened this issue Mar 1, 2023 · 0 comments · Fixed by #973
Assignees
Labels
bug Something isn't working libcuspatial Relates to the cuSpatial C++ library

Comments

@harrism
Copy link
Member

harrism commented Mar 1, 2023

Version

23.02

On which installation method(s) does this occur?

Rapids-Compose

Describe the issue

cuSpatial C++ algorithms that operate on polygons have different expectations of polygon layout. For example, some expect the number of vertices to be 3 * num_rings, some 4 * num_rings, and some 4 * num_polygons (which is a bug).

If we are implementing the GeoArrow specification, we need to rigorously enforce the well-defined portions of that specification. Note that we should not introspect device-side data, but we can validate data / offset array size consistency.

We should carefully and consistency use CUSPATIAL_EXPECTS in our implementations and test our expectations consistently. We should also document the format we expect. We should provide utilities for testing polygon expectations and use them across all algorithms that take polygon data.

Where it makes sense, all of the above can be applied to other geometry types as well.

Other/Misc.

See also geoarrow/geoarrow#35

@harrism harrism added bug Something isn't working libcuspatial Relates to the cuSpatial C++ library labels Mar 1, 2023
@harrism harrism self-assigned this Mar 1, 2023
@rapids-bot rapids-bot bot closed this as completed in #973 Mar 9, 2023
rapids-bot bot pushed a commit that referenced this issue Mar 9, 2023
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.

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: #973
trxcllnt pushed a commit to trxcllnt/cuspatial that referenced this issue Mar 15, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcuspatial Relates to the cuSpatial C++ library
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant