Header-only refactoring of derive_trajectories #628
Conversation
harrism
added
improvement
cpp/include/cuspatial/experimental/detail/derive_trajectories.cuh
Outdated
Show resolved
Hide resolved
cpp/include/cuspatial/experimental/detail/derive_trajectories.cuh
Outdated
Show resolved
Hide resolved
cpp/include/cuspatial/experimental/detail/derive_trajectories.cuh
Outdated
Show resolved
Hide resolved
cpp/include/cuspatial/experimental/detail/derive_trajectories.cuh
Outdated
Show resolved
Hide resolved
Yes, my idea is to eliminate it. It is still used by |
| /** | ||
| * @brief Derive trajectories from object ids, points, and timestamps. | ||
| * | ||
| * Groups the input object ids to determine unique trajectories, and reorders all input data to be |
There was a problem hiding this comment.
This reads like the input data will be reordered in-place.
| stream, | ||
| mr); | ||
|
|
||
| auto const num_points = std::distance(ids_first, ids_last); |
There was a problem hiding this comment.
I noticed here that you're computing the number of points based on the number of ids. I suggest that you error check here that the number of ids, points, and trajectories are all equal.
There was a problem hiding this comment.
That's not possible -- we only get a last iterator for ids. Assuming (not asserting) the iterator ranges are equal is standard practice in iterator-based APIs like this (and Thrust, and STL).
There was a problem hiding this comment.
Also, the beauty of an iterator-based API is flexibility / fusability. The caller is free to pass anything that looks like an iterator -- it might be, for example, a counting iterator, or some other dynamic iterator type that generates an open-ended sequence. There is no end. :)
| } | ||
| }; | ||
|
|
||
| void make_data(std::size_t num_trajectories, std::size_t max_trajectory_size) |
There was a problem hiding this comment.
| void make_data(std::size_t num_trajectories, std::size_t max_trajectory_size) | |
| void trajectory_test_data(std::size_t num_trajectories, std::size_t max_trajectory_size) |
| cudf::column(rmm::device_uvector<cudf::timestamp_ms>(size / 2, rmm::cuda_stream_default)); | ||
| EXPECT_THROW(cuspatial::derive_trajectories(id, xs, ys, ts, this->mr()), | ||
| cuspatial::logic_error); | ||
| } |
There was a problem hiding this comment.
Judging from these tests, I missed the spot above where you are in fact checking that all lengths are the same?
There was a problem hiding this comment.
We check in the column-based API, but we can't check in the header-only API.
There was a problem hiding this comment.
Columns have a defined size. Iterators don't.
Co-authored-by: H. Thomson Comer <[email protected]>
|
rerun tests |
|
@gpucibot merge |
Contributes to #563 Header-only refactoring of `cuspatial::trajectory_bounding_boxes` Note the commit list is messy because it was originally based off of #628 which was merged into 22.10. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Wang (https://github.com/isVoid) - Paul Taylor (https://github.com/trxcllnt) URL: #741
Contributes to #563 .