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

Reorganize cuSpatial headers #1097

Merged
merged 22 commits into from
May 2, 2023

Conversation

harrism
Copy link
Member

@harrism harrism commented Apr 26, 2023

Description

Fixes #1080 based on the discussion in #1087

  • Consolidate each major set of features within common headers, e.g. distance.hpp/.cuh, intersection.hpp/.cuh.
  • Consolidate point-in-polygon APIs / headers and DRY up implementation
  • Rename some headers/APIS that were inconsistent across the header-only and column-based APIs, e.g. points_in_spatial_window vs. points_in_range.
  • Update Cython where needed to account for above changes.

This PR does NOT yet reorganize source code.

Update: Compilation Time Results

Compilation time is not changed appreciably by this PR (<0.5% faster).

This test was performed by disabling sccache, erasing everything in the build directory, configuring cmake (including building tests and benchmarks), and then running time ninja. My machine has a "AMD Ryzen 7 3700X 8-Core Processor", and I'm using Ninja's default parallelism.

Disabling sccache when using the cuSpatial devcontainer

(rapids) coder ➜ ~/cuspatial/ $ cd cpp/build/release
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ rm -rf ./*
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ unset RUSTC_WRAPPER
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ unset CMAKE_C_COMPILER_LAUNCHER
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ unset CMAKE_CXX_COMPILER_LAUNCHER
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ unset CMAKE_CUDA_COMPILER_LAUNCHER
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ cmake ~/cuspatial/cpp -GNinja -DBUILD_TESTS=ON -DBUILD_BENCHMARKS=ON
(rapids) coder ➜ ~/cuspatial/cpp/build/release $ time ninja

This branch:

[201/201] Linking CXX executable gtests/LINESTRING_INTERSECTION_TEST_EXP

real    10m42.845s
user    122m28.077s
sys     6m7.935s

branch-23.06:

[202/202] Linking CXX executable gtests/LINESTRING_INTERSECTION_TEST_EXP

real    10m45.573s
user    122m52.896s
sys     6m9.357s

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@harrism harrism added feature request New feature or request breaking Breaking change labels Apr 26, 2023
@harrism harrism requested a review from a team as a code owner April 26, 2023 06:11
@harrism harrism self-assigned this Apr 26, 2023
@harrism harrism requested review from a team as code owners April 26, 2023 06:11
@harrism harrism requested review from trxcllnt, thomcom and isVoid April 26, 2023 06:11
@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 Apr 26, 2023
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Only small comments below, I think we should merge this asap so that you don't have to deal with conflicts from other C++ changes.

1 more comment on cython pxd bindings ("_lib/cpp/") - their file structure should exactly match hpp files. But I can create a new issue to track that: #1101

cpp/include/cuspatial/bounding_boxes.hpp Outdated Show resolved Hide resolved
} // namespace detail

template <typename IdInputIt, typename PointInputIt, typename BoundingBoxOutputIt, typename T>
BoundingBoxOutputIt point_bounding_boxes(IdInputIt ids_first,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of bounding_box I think this works out pretty nicely. For other functions (say distance), should we include all implementation in one file too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think combining implementations only makes sense if it reduces complexity and repetition. If it causes large files that are harder to navigate then I wouldn't do it. I've seen both situations in cuSpatial, so I think we can handle it on a case-by-case basis.

@isVoid
Copy link
Contributor

isVoid commented Apr 27, 2023

Can you also provide compile time of this PR against branch-23.06?

@harrism
Copy link
Member Author

harrism commented Apr 27, 2023

Can you also provide compile time of this PR against branch-23.06?

Added to description.

@harrism
Copy link
Member Author

harrism commented May 2, 2023

/merge

@rapids-bot rapids-bot bot merged commit 2509fce into rapidsai:branch-23.06 May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change cmake Related to CMake code or build configuration feature request New feature or request libcuspatial Relates to the cuSpatial C++ library Python Related to Python code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Improve C++ header file organization
4 participants