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

Move experimental headers into main include/cuspatial directory #1081

Merged

Conversation

harrism
Copy link
Member

@harrism harrism commented Apr 18, 2023

Description

Fixes #1083.

  • Consolidates all headers into include/cuspatial/. This works without conflicts because header-only API headers have the .cuh extension while column-based API headers have the .cpp extension.
  • Consolidates and reorganizes tests. Header-only and column-based tests are stored together.
  • Updates documentation to remove references to experimental and also renames the REFACTORING_GUIDE.md to HEADER_ONLY_API_GUIDE.md and updates it.

Checklist

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

@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Apr 18, 2023
@harrism harrism added improvement Improvement / enhancement to an existing function breaking Breaking change and removed libcuspatial Relates to the cuSpatial C++ library labels Apr 18, 2023
@harrism harrism self-assigned this Apr 18, 2023
@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Apr 18, 2023
@harrism harrism changed the title Improve cuSpatial header organization Move experimental headers into main include/cuspatial directory Apr 19, 2023
@github-actions github-actions bot added the cmake Related to CMake code or build configuration label Apr 19, 2023
@harrism harrism marked this pull request as ready for review April 19, 2023 06:35
@harrism harrism requested review from a team as code owners April 19, 2023 06:35
@harrism harrism added this to the header-only C++ API milestone Apr 19, 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.

🔥

@@ -52,7 +55,7 @@ Testing header-only APIs requires CUDA compilation so should be done in `.cu` fi

## Base Fixture

All libcuspatial unit tests should make use of a GTest
All libcuspatial unit tests should make use of a GTest
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here we should encourage the use of cuspatial::test::BaseFixture?
https://github.com/rapidsai/cuspatial/blob/branch-23.06/cpp/include/cuspatial_test/base_fixture.hpp

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 that should be a separate PR.

@@ -65,7 +68,7 @@ Example:

## Typed Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

We also support value-parameterized test now:

class BaseFixtureWithParam : public RMMResourceMixin,

Copy link
Member Author

Choose a reason for hiding this comment

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

Separate PR (same as above).

@harrism
Copy link
Member Author

harrism commented Apr 26, 2023

/merge

@rapids-bot rapids-bot bot merged commit 5f91e68 into rapidsai:branch-23.06 Apr 26, 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 improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

Move "experimental" headers into main cuspatial include directory
3 participants