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

Relocate Utility Files #560

Merged

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Jun 14, 2022

This PR moves several geometric utility functions to a separate header file so that they can be reused by other distance functions.

EDIT: per discussion below, this PR now also moves vec_2d.cuh to top level name space, traits.hpp and geometry_utilities are now under cuspatial/detail/utility and geometry_utilities are renamed to linestring.cuh.

@isVoid isVoid requested a review from a team as a code owner June 14, 2022 18:13
@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Jun 14, 2022
@@ -0,0 +1,96 @@
#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

The path "utility/geometry_utility..." is a bit redundant. Also, I fear that "geometry utilities" could grow quite large. All of these are linestring / segment utilities, so perhaps a more specific filename is in order?

Also these are all detail namespace so shouldn't detail be in the path?

e.g. include/cuspatial/detail/linestring_utilities.cuh

Looking at the utility folder, I think that vec_2d.cuh should be promoted to the top-level include directory (since it is not detail -- it's analogous to constants.hpp). Everything else in utility should move to detail. Thoughts?

Copy link
Contributor Author

@isVoid isVoid Jun 16, 2022

Choose a reason for hiding this comment

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

Great advice. In the other PR you mentioned we can move device_atomics.cuh to either within a utility folder or not. I think most methods here all fits under the utility category so should probably create a folder for that. Considering in the future when we move methods from experimental to include, the detail folder will be swarmed with many files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved as noted except device_atomics.cuh will be handled in the other PR.

@harrism harrism added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jun 15, 2022
@isVoid isVoid changed the title Move Several Geometric Utilities to geometric_utility.cuh Relocate Utility Files Jun 16, 2022
@isVoid isVoid requested a review from harrism June 16, 2022 01:08
@harrism
Copy link
Member

harrism commented Jun 16, 2022

pairwise linestring gtests now failing?

@isVoid
Copy link
Contributor Author

isVoid commented Jun 16, 2022

Pretty sure the rewrite endpoint_index_of_linestring helper is missing a parenthesis.

@harrism
Copy link
Member

harrism commented Jun 22, 2022

In the interest of time, I think it's safe to merge this with one approval

@harrism
Copy link
Member

harrism commented Jun 22, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 733263e into rapidsai:branch-22.08 Jun 22, 2022
isVoid added a commit to isVoid/cuspatial that referenced this pull request Jun 23, 2022
This PR moves several geometric utility functions to a separate header file so that they can be reused by other distance functions.

EDIT: per discussion below, this PR now also moves `vec_2d.cuh` to top level name space, `traits.hpp` and `geometry_utilities` are now under `cuspatial/detail/utility` and `geometry_utilities` are renamed to `linestring.cuh`.

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

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

URL: rapidsai#560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants