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

Add find_duplicate_points Internal API #815

Merged
merged 8 commits into from
Nov 23, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Nov 22, 2022

Description

This PR adds combine_duplicate_points API. Given a multipoints range, this API sets a stencil range for each of the duplicate points in the multipoint range.

closes #814

A second introduction in this PR is make_multipoint_array, a test utility to create an owning object with pure initializer_list syntax for convenience construction. Example usage:

using P = vec_2d<float>;
auto multipoints = make_multipoint_array({
   {P{0.0, 1.0}, P{5.0, 7.0}},
   {},
   {P{3.0, 3.0}}
});

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 November 22, 2022 23:13
@isVoid isVoid self-assigned this Nov 22, 2022
@isVoid isVoid added the 3 - Ready for Review Ready for review by team label Nov 22, 2022
@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 added this to the DE-9IM milestone Nov 22, 2022
@isVoid isVoid requested review from harrism and thomcom and removed request for vyasr and jrhemstad November 22, 2022 23:14
@isVoid isVoid added the feature request New feature or request label Nov 22, 2022
@isVoid isVoid added the non-breaking Non-breaking change label Nov 22, 2022
@harrism harrism changed the title Adds combine_duplicate_points Internal API Add combine_duplicate_points Internal API Nov 22, 2022
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. My only concern is with the naming.


/**
* @internal
* @brief For each multipoint, computes duplicate points and stores as stencil.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "stores as stencil"? Do you mean an array of flags indicating which points are duplicates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc updated as a result of addressing: #815 (comment)


template <typename VecIterator>
template <typename IndexType>
CUSPATIAL_HOST_DEVICE auto multipoint_ref<VecIterator>::operator[](IndexType i)
Copy link
Member

Choose a reason for hiding this comment

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

Just thinking about host / device here. Is this really intended to work on the host? What if it is called from host when the iterator points to device memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can work on host, if all vectors are host vectors. Although I don't really see a use case at the moment. Up till now I mostly rely on developer's prudence to not access device memory from host.

cpp/tests/CMakeLists.txt Outdated Show resolved Hide resolved
@isVoid isVoid changed the title Add combine_duplicate_points Internal API Add find_duplicate_points Internal API Nov 23, 2022
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

@harrism
Copy link
Member

harrism commented Nov 23, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1589e08 into rapidsai:branch-22.12 Nov 23, 2022
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 feature request New feature or request libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Create internal API to remove duplicate points in a multipoint
3 participants