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.

Sign up for GitHub

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

Jump to bottom

Header-only polygon_bounding_boxes and linestring_bounding_boxes, make_geometry_id_iterator utility, and box<T> class. #820

Merged
merged 37 commits into from
Dec 14, 2022
Merged

Header-only polygon_bounding_boxes and linestring_bounding_boxes, make_geometry_id_iterator utility, and box<T> class. #820

merged 37 commits into from
Dec 14, 2022

Conversation

harrism
Copy link
Member

@harrism harrism commented Nov 24, 2022 •
edited
Loading

Description

  • Adds a new axis-aligned box type cuspatial::box<T> that has two vertices (default vec_2d<T>), but is generic to work with 3D vertices in the future.
  • Uses the new box type in all bounding box code.
  • New make_geometry_id_iterator iterator to eliminate current gather/scatter/scan sequence required to convert polygon offsets into unique polygon IDs for bounding box reduction keys. This fuses with the reduce_by_key so saves memory.
  • Header-only version of polygon_bounding_boxes
  • Header-only version of linestring_bounding_boxes

Closes #567
Closes #568

Checklist

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

@harrism harrism requested review from a team as code owners November 24, 2022 01:42
@github-actions github-actions bot added cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library Python Related to Python code labels Nov 24, 2022
@harrism harrism self-assigned this Nov 24, 2022
@harrism harrism added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Nov 24, 2022
@harrism harrism changed the base branch from branch-22.12 to branch-23.02 November 24, 2022 01:45
@harrism harrism changed the title tests Header-only polygon_bounding_boxes and linestring_bounding_boxes Nov 28, 2022
@harrism harrism changed the title Header-only polygon_bounding_boxes and linestring_bounding_boxes Header-only polygon_bounding_boxes Nov 28, 2022
isVoid
isVoid reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just questions about computing the keys on the fly and needs document.

cpp/include/cuspatial/experimental/polygon_bounding_boxes.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/polygon_bounding_boxes.cuh Outdated Show resolved Hide resolved
cpp/tests/experimental/spatial/polygon_bounding_boxes_test.cu Show resolved Hide resolved
@github-actions github-actions bot removed the Python Related to Python code label Nov 30, 2022
@harrism harrism added the 2 - In Progress Currenty a work in progress label Nov 30, 2022
@harrism harrism changed the title Header-only polygon_bounding_boxes Header-only polygon_bounding_boxes, make_geometry_id_iterator utility, and box<T> class. Nov 30, 2022
thomcom
thomcom requested changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@thomcom thomcom left a comment

Choose a reason for hiding this comment

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

Nice additions to iterator_factory.cuh I think I'll end up using some of them. I have a few questions about GeoArrow n+1 offset arrays and docs comments here and there.

cpp/tests/experimental/spatial/polygon_bounding_boxes_test.cu Outdated Show resolved Hide resolved
cpp/tests/experimental/spatial/linestring_bounding_boxes_test.cu Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/polygon_bounding_boxes.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/iterator_factory.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/iterator_factory.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/geometry/box.hpp Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_bounding_boxes.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/polygon_bounding_boxes.cuh Show resolved Hide resolved
@harrism harrism requested a review from thomcom December 13, 2022 04:12
isVoid
isVoid approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Still approving. I think the failure in the python tests are because the input offsets are length n not n+1, and that triggers the early return with all 0 results.

cpp/tests/experimental/spatial/point_bounding_boxes_test.cu Outdated Show resolved Hide resolved
trxcllnt
trxcllnt approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@trxcllnt trxcllnt left a comment

Choose a reason for hiding this comment

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

One small suggestion, but other than that LGTM 👍

@harrism harrism requested a review from a team as a code owner December 14, 2022 01:39
@github-actions github-actions bot added the Python Related to Python code label Dec 14, 2022
isVoid
isVoid approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Approving python

@harrism
Copy link
Member Author

harrism commented Dec 14, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 5ea5bbb into rapidsai:branch-23.02 Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhemstad jrhemstad jrhemstad left review comments

@isVoid isVoid isVoid approved these changes

@thomcom thomcom thomcom approved these changes

@trxcllnt trxcllnt trxcllnt approved these changes

Assignees

@harrism harrism

Labels
3 - Ready for Review Ready for review by team cmake Related to CMake code or build configuration improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code
Projects
cuSpatial
Status: Done
Milestone
No milestone
5 participants
@harrism @trxcllnt @thomcom @isVoid @jrhemstad