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.

Sign up for GitHub

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

Jump to bottom

Header-only refactoring of trajectory_distances_and_speeds #769

Merged
merged 28 commits into from
Nov 8, 2022
Merged

Header-only refactoring of trajectory_distances_and_speeds #769

merged 28 commits into from
Nov 8, 2022

Conversation

harrism
Copy link
Member

@harrism harrism commented Nov 2, 2022 •
edited
Loading

Description

Closes #563

In addition to refactoring to header-only API, this PR changes the implementation to use thrust_transform (two input version) instead of thrust::adjacent_difference. This simplifies the code a bit and eliminates the need for the duplicate_first_element_iterator, which was a bit confusing.

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 self-assigned this Nov 2, 2022
@github-actions github-actions bot added cmake Related to CMake code or build configuration libcuspatial Relates to the cuSpatial C++ library labels Nov 2, 2022
@harrism harrism added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Nov 2, 2022
@harrism harrism marked this pull request as ready for review November 3, 2022 04:59
@harrism harrism requested review from a team as code owners November 3, 2022 04:59
@harrism harrism requested review from isVoid, jrhemstad and thomcom and removed request for jrhemstad November 3, 2022 04:59
@harrism harrism mentioned this pull request Nov 3, 2022
Closed
3 tasks
isVoid
isVoid reviewed Nov 4, 2022
View reviewed changes
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.

Seems pretty solid to me. I have one question regarding reintroducing the hand derived tests.

@isVoid
Copy link
Contributor

isVoid commented Nov 7, 2022

rerun tests

isVoid
isVoid approved these changes Nov 8, 2022
View reviewed changes
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.

lgtm!

@harrism
Copy link
Member Author

harrism commented Nov 8, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0c2fe86 into rapidsai:branch-22.12 Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isVoid isVoid isVoid approved these changes

@thomcom thomcom Awaiting requested review from thomcom

Assignees

@harrism harrism

Labels
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
cuSpatial
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Refactor trajectory APIs to header-only API
2 participants
@harrism @isVoid