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 C++ Column API and Python API for pairwise_linestring_intersection #862

Merged
merged 23 commits into from
Feb 27, 2023
Merged

Add C++ Column API and Python API for pairwise_linestring_intersection #862

merged 23 commits into from
Feb 27, 2023

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Dec 21, 2022 •
edited
Loading

Description

This PR adds column/python API for pairwise_linestring_intersection. Closes #648

This PR is also the first attempt of #261 , establishing geometry_column_view that inherits cudf::column and adds additional information to identify the underlying nested levels. Note that it cannot directly inherit from cudf::lists::list_column_view, because a point array isn't a List<T> array.

Also note that the type codes defined in libcuspatial and cuspatial-python may be different, so there is a type code mapping after computing the result from libcuspaital.

Finally, since List<Union> is currently unsupported in libcudf, the python API returns the geometries in two parts: offset + GeoColumn.

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 requested review from a team as code owners December 21, 2022 00:58
@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 Dec 21, 2022
@isVoid isVoid requested review from harrism and thomcom and removed request for trxcllnt and cwharris December 21, 2022 01:00
@isVoid isVoid added feature request New feature or request non-breaking Non-breaking change labels Dec 21, 2022
@harrism harrism added the 2 - In Progress Currenty a work in progress label Jan 23, 2023
harrism
harrism requested changes Jan 24, 2023
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 probably could have been two PRs (C++ and Python). Please update the copyright in all added/changed files to 2023.

cpp/include/cuspatial/column/geometry_column_view.hpp Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/multilinestring_range.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/traits.hpp Outdated Show resolved Hide resolved
cpp/src/column/geometry_column_view.cu Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/_lib/types.pyx Show resolved Hide resolved
python/cuspatial/cuspatial/core/binops/intersection.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binops/intersection.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binops/intersection.py Outdated Show resolved Hide resolved
@isVoid isVoid changed the base branch from branch-23.02 to branch-22.04 February 6, 2023 17:24
@isVoid isVoid requested a review from a team as a code owner February 6, 2023 17:24
@isVoid isVoid changed the base branch from branch-22.04 to branch-23.04 February 6, 2023 17:24
@ajschmidt8 ajschmidt8 removed the request for review from a team February 6, 2023 17:35
@isVoid isVoid requested a review from harrism February 7, 2023 00:22
harrism
harrism requested changes Feb 14, 2023
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 great. A few small comments.

cpp/include/cuspatial/column/geometry_column_view.hpp Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/multilinestring_range.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/multilinestring_range.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/multilinestring_range.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/ranges/multilinestring_range.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/linestring_intersection.hpp Outdated Show resolved Hide resolved
cpp/src/column/geometry_column_view.cu Outdated Show resolved Hide resolved
cpp/src/spatial/linestring_intersection.cu Outdated Show resolved Hide resolved
cpp/src/utility/multi_geometry_dispatch.hpp Show resolved Hide resolved
cpp/src/utility/multi_geometry_dispatch.hpp Outdated Show resolved Hide resolved
@isVoid isVoid requested a review from harrism February 14, 2023 20:05
@isVoid isVoid added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Feb 14, 2023
@isVoid isVoid added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review Ready for review by team labels Feb 16, 2023
@isVoid
Copy link
Contributor Author

isVoid commented Feb 16, 2023

Although CI is passing, @thomcom finds a memory error when running all the tests locally which I can also repro. Holding until I figure out what's going on.

thomcom
thomcom requested changes Feb 17, 2023
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.

Just following up that I get a critical bad_access error when I run this will the full suite of tests. It is clearly not happening with CI but it happens on @isVoid and my local machines so he's looking into it.

@isVoid isVoid mentioned this pull request Feb 18, 2023
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Feb 27, 2023
@isVoid
Fix OB bug in linestring_intersection_intermediates.remove_if Funct…
bb6788d 
…ion (#945)

This PR fixes a bug mentioned by @thomcom at #862 (review). When computing the lookup id for each group to find the number of geometry removed in previous groups, the previous computation is flawed and resulted in OB access.

Besides, this PR also adds `CUDA_TRY(cudaGetLastError())` in several kernel calls in the intersection code path.

This PR also refined the documentation for `remove_if` and related helper function.

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

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

URL: #945
@isVoid isVoid added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Feb 27, 2023
@isVoid
Copy link
Contributor Author

isVoid commented Feb 27, 2023

/merge

@rapids-bot rapids-bot bot merged commit 8fbb208 into rapidsai:branch-23.04 Feb 27, 2023
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

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge 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 Python Related to Python code
Projects
cuSpatial
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] linestring-linestring intersection (pairwise)
4 participants
@isVoid @ajschmidt8 @thomcom @harrism