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

Refactor linestring_distance to header only API #526

Merged
merged 36 commits into from
Jun 1, 2022
Merged

Refactor linestring_distance to header only API #526

merged 36 commits into from
Jun 1, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented May 13, 2022 •
edited
Loading

This PR converts pairwise_linestring_distance into header only API.

@isVoid isVoid self-assigned this May 13, 2022
@github-actions github-actions bot added cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library labels May 13, 2022
@isVoid isVoid marked this pull request as ready for review May 16, 2022 21:21
@isVoid isVoid requested review from a team as code owners May 16, 2022 21:21
@isVoid isVoid requested review from harrism and zhangjianting May 16, 2022 21:21
@isVoid isVoid added feature request New feature or request non-breaking Non-breaking change labels May 16, 2022
harrism
harrism requested changes May 17, 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.

Something seems off -- the added code is the old version of the implementation. The removed code is the new version.

cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/tests/experimental/spatial/linestring_distance_test.cu Show resolved Hide resolved
cpp/include/cuspatial/experimental/linestring_distance.cuh Outdated Show resolved Hide resolved
@isVoid isVoid requested a review from harrism May 18, 2022 00:47
harrism
harrism requested changes May 18, 2022
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/utility/device_atomics.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/utility/device_atomics.cuh Outdated Show resolved Hide resolved
cpp/scripts/run-cmake-format.sh Outdated Show resolved Hide resolved
@harrism harrism added this to the header-only C++ API milestone May 25, 2022
@harrism harrism changed the title Convert linestring_distance to header only. Refactor linestring_distance to header only API May 26, 2022
@harrism harrism requested review from trxcllnt and removed request for zhangjianting May 26, 2022 04:21
@harrism
Copy link
Member

harrism commented May 26, 2022

rerun tests

vyasr
vyasr requested changes May 27, 2022
View reviewed changes
cpp/include/cuspatial/utility/device_atomics.cuh Show resolved Hide resolved
cpp/include/cuspatial/utility/device_atomics.cuh
namespace cuspatial {
namespace detail {

__device__ double atomicMin(double* addr, double val)
Copy link
Contributor

Choose a reason for hiding this comment

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

All four of these operators should probably be implemented as specializations of a single templated function. It'll need ~5 template parameters (the value type, the bit view type, the two conversion functions between value and view types, and the actual operation to perform e.g. std::max), which is kind of gross, but it'll significantly reduce duplication and the pain of adding new operators if you need other atomics in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Let's punt this to a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

#547

cpp/include/cuspatial/utility/traits.hpp Show resolved Hide resolved
cpp/tests/experimental/spatial/linestring_distance_test.cu Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
cpp/include/cuspatial/experimental/detail/linestring_distance.cuh Outdated Show resolved Hide resolved
@harrism harrism mentioned this pull request May 30, 2022
Merged
@harrism
Copy link
Member

harrism commented Jun 1, 2022

rerun tests

@harrism harrism mentioned this pull request Jun 1, 2022
Closed
harrism added 2 commits May 31, 2022 23:28
@harrism
Remove erroneous nested std::vector
9a15132
@harrism
Responds to review feedback
d242701 
* docs
* error messages
* std::size_t
* const
* variable name
@harrism harrism requested a review from vyasr June 1, 2022 06:49
vyasr
vyasr approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM

@vyasr
Copy link
Contributor

vyasr commented Jun 1, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 793a878 into rapidsai:branch-22.06 Jun 1, 2022
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

@vyasr vyasr vyasr approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

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
None yet
Milestone
header-only C++ API
Development

Successfully merging this pull request may close these issues.
3 participants
@isVoid @harrism @vyasr