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

Add pairwise_linestring_intersection_with_duplicates API #813

Merged
merged 28 commits into from
Dec 13, 2022
Merged

Add pairwise_linestring_intersection_with_duplicates API #813

merged 28 commits into from
Dec 13, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Nov 22, 2022 •
edited
Loading

Description

This PR adds internal function: pairwise_linestring_intersection_with_duplicates API. closes #785,

The API returns two structs: linestring_intersection_intermediates<vec_2d> and linestring_intersection_intermediates<segment>. This struct is important for following steps when removing duplicate geometries. Because the geometries and underlying look-back ids needs to be updated in tandem.

Notice I didn't go too hard on the test cases - result from intersection with duplicates is often hard to hand craft. I would rely on future integrated tests with end-to-end intersection to test the correctness of this function.

Checklist

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

@isVoid isVoid added feature request New feature or request non-breaking Non-breaking change labels Nov 22, 2022
@isVoid isVoid added this to the DE-9IM milestone Nov 22, 2022
@isVoid isVoid requested review from thomcom and harrism November 22, 2022 23:03
@isVoid isVoid self-assigned this Nov 22, 2022
@isVoid isVoid requested review from a team as code owners November 22, 2022 23:03
@isVoid isVoid requested a review from zhangjianting November 22, 2022 23:03
@github-actions github-actions bot added cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library labels Nov 22, 2022
@isVoid isVoid changed the base branch from branch-22.12 to branch-23.02 November 22, 2022 23:05
robertmaynard
robertmaynard approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Approving CMake changes

@isVoid
Merge branch 'branch-23.02' of https://github.com/rapidsai/cuspatial …
bb5e0d4 
…into feature/intersection_with_duplicates_pr
@isVoid isVoid requested a review from harrism November 29, 2022 01:13
thomcom
thomcom requested changes Dec 5, 2022
View reviewed changes
cpp/tests/experimental/spatial/linestring_intersection_with_duplicates_test.cu Show resolved Hide resolved
cpp/tests/experimental/spatial/linestring_intersection_with_duplicates_test.cu Outdated Show resolved Hide resolved
cpp/tests/experimental/spatial/linestring_intersection_with_duplicates_test.cu Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/range.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/range.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Outdated Show resolved Hide resolved
@isVoid
Merge branch 'branch-23.02' of https://github.com/rapidsai/cuspatial …
2482d25 
…into feature/intersection_with_duplicates_pr
@isVoid isVoid requested a review from thomcom December 5, 2022 23:49
harrism
harrism requested changes Dec 7, 2022
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Looks good. This is pretty complicated, so I tried to provide a few ideas for simplification / DRY.

cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_intersection_with_duplicates.cuh Show resolved Hide resolved
cpp/include/doxygen_groups.h Outdated Show resolved Hide resolved
cpp/tests/experimental/spatial/linestring_intersection_with_duplicates_test.cu Outdated Show resolved Hide resolved
@isVoid isVoid requested a review from harrism December 9, 2022 19:21
This was referenced Dec 10, 2022
Merged
Merged
harrism
harrism approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

I have one naming request but otherwise approve. Love the enumerated_range stuff!

cpp/include/cuspatial/experimental/detail/ranges/enumerate_range.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/detail/utility/zero_data.cuh Outdated Show resolved Hide resolved
@thomcom
Copy link
Contributor

thomcom commented Dec 13, 2022

The size of a GeoSeries and GeoDataFrame changed slightly with this PR, causing CI to fail. I'm rebuilding my rapids-compose now to try and reproduce, so far unable to.

@isVoid
Copy link
Contributor Author

isVoid commented Dec 13, 2022

rerun tests

@isVoid
Copy link
Contributor Author

isVoid commented Dec 13, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit c6d4b75 into rapidsai:branch-23.02 Dec 13, 2022
rapids-bot bot pushed a commit that referenced this pull request Dec 14, 2022
@isVoid
Adds remove_if Function to linestring_intersection_intermediates (#…
3d814f5 
…851)

#813 adds a struct to hold the intermediate result from `linestring_intersection_with_duplicates`. After discovering the duplicates from the result, we need a way to remove these geometries from the struct, as well as updating the offsets and look-back ids. This PR adds a `remove_if` function to the struct, which models after `thrust::remove_if` that accepts a range of flags, and removes the geometries in the intermediates where the flag is set to 1.

Closes #853 
Depends on #813. Contributes to #765.

Authors:
  - Michael Wang (https://github.com/isVoid)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - H. Thomson Comer (https://github.com/thomcom)

URL: #851
rapids-bot bot pushed a commit that referenced this pull request Dec 16, 2022
@isVoid
Add Header Only API pairwise_linestring_intersection (#852)
f945e67 
This PR adds header only API `pairwise_linestring_intersection`. Closes #765 .

Depends on #813 #818 #819 #851

Authors:
  - Michael Wang (https://github.com/isVoid)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - H. Thomson Comer (https://github.com/thomcom)

URL: #852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism approved these changes

@thomcom thomcom thomcom approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

@zhangjianting zhangjianting Awaiting requested review from zhangjianting zhangjianting is a code owner automatically assigned from rapidsai/cuspatial-cpp-codeowners

Assignees

@isVoid isVoid

Labels
cmake Related to CMake code or build configuration feature request New feature or request libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change
Projects
cuSpatial
Status: Done
Milestone
DE-9IM
Development

Successfully merging this pull request may close these issues.

Header-only Implementation of intersection_with_duplicates
4 participants
@isVoid @thomcom @robertmaynard @harrism