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

Refactor Hausdorff distance to header-only API #538

Merged
merged 112 commits into from
Jun 23, 2022

Conversation

harrism
Copy link
Member

@harrism harrism commented May 25, 2022

Adds header-only API cuspatial::directed_hausdorff_distance and refactors the existing API on top of the new API, and updates tests.

Part of https://github.com/rapidsai/cuspatial/milestone/1

harrism and others added 30 commits January 19, 2022 15:19
Co-authored-by: Jake Hemstad <[email protected]>
Co-authored-by: Paul Taylor <[email protected]>
@harrism harrism changed the base branch from branch-22.08 to branch-22.06 June 21, 2022 07:50
@harrism harrism changed the base branch from branch-22.06 to branch-22.08 June 21, 2022 07:50
@harrism harrism changed the base branch from branch-22.08 to branch-22.06 June 21, 2022 07:51
@harrism harrism changed the base branch from branch-22.06 to branch-22.08 June 21, 2022 07:51
@harrism harrism added the 3 - Ready for Review Ready for review by team label Jun 21, 2022
@harrism harrism requested review from vyasr and isVoid and removed request for thomcom and zhangjianting June 21, 2022 07:58
@harrism
Copy link
Member Author

harrism commented Jun 21, 2022

There is a problem with the commit history on this PR but I'm investigating. The changes shown are only changes from this PR though, so it can be reviewed.

I discovered that this is because I have merged in individual commits from feature branches targetting branch-22.06 before the associated PRs were squash-merged into 22.06. Since this PR now targets 22.08, those commits still show up in the feature branch history. However, the changes are already squash-merged into 22.08, so reviewers only see the code changes from this PR.

Even though the PR's history is not clean, when it is squash-merged into 22.08, it won't matter. Therefore I'm inclined to leave it alone.

* all threads have run to completion, all "maximums of the minumum distances" (aka, directed
* Hausdorff distances) reside in the output.
*
* @tparam T type of coordinate, either float or double.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I approved too soon. The docs for template parameter here should be added.

@harrism harrism removed the request for review from vyasr June 23, 2022 00:46
@harrism
Copy link
Member Author

harrism commented Jun 23, 2022

@gpucibot merge

@harrism
Copy link
Member Author

harrism commented Jun 23, 2022

@gpucibot merge

Copy link
Contributor

@cwharris cwharris 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

@rapids-bot rapids-bot bot merged commit 60b57f0 into rapidsai:branch-22.08 Jun 23, 2022
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Jun 23, 2022
We can use `rapids_cpm_libcudacxx` directly without this extra function. The only value it adds is setting the `LIBCUDACXX_INCLUDE_DIR` variable, which we don't use. See also rapidsai/cuspatial#538 (comment)

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #11138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants