Add Point Linestring Distance #573
Conversation
github-actions
bot
added
cmake
harrism
added
feature request
|
This PR has been labeled |
|
Probably should retarget to 22.10? |
…into feature/header_only_point_linestring_distance
…t_linestring_distance Fix Thrust includes.
|
rerun tests (Seems like the rest of the CI is blocked by the fixes included in this PR) |
…b.com:isVoid/cuspatial into feature/header_only_point_linestring_distance
|
The last test failure was results from the incorrect grid-stride loop guard condition. The last point of the last linestring point array should not be accessed since each iteration handles one line-segment. Thus the guard condition should be: idx < std::distance(linestring_points_first, thrust::prev(linestring_points_last));The skip condition in the loop is also missing a check on OB iterator. And should skip iteration to next grid-stride instead of just returning. (The length of each linestring does not correlate with grid-stride). if (offsets_iter != linestring_offsets_last and *offsets_iter - 1 == idx) { continue; } |
cpp/include/cuspatial/experimental/detail/point_linestring_distance.cuh
Outdated
Show resolved
Hide resolved
cpp/include/cuspatial/experimental/detail/point_linestring_distance.cuh
Outdated
Show resolved
Hide resolved
cpp/include/cuspatial/experimental/detail/point_linestring_distance.cuh
Outdated
Show resolved
Hide resolved
cpp/include/cuspatial/experimental/detail/point_linestring_distance.cuh
Outdated
Show resolved
Hide resolved
| * pair | ||
| * @param linestring_points_last end of the range of points of the linestring element of each pair | ||
| * @param distances_first beginning the output range of distances | ||
| * @param stream The CUDA stream to use for device memory operations and kernel launches. |
There was a problem hiding this comment.
These docs aren't the same as the ones in detail/point_linestring_distance.cuh, but they are close. I slightly prefer the ones in detail. I don't like seeing the duplication, is there a good solution to it? They'll diverge and need constant attention.
There was a problem hiding this comment.
We can use @copydoc to copy all documentation contents from one to other. But I haven't tested if we can override specifics secions/parameters after doing that. Will try it out.
There was a problem hiding this comment.
but you can't remove.
Do we want different doc contents for the internal function v.s. the user API? IMO the internal function doc should describe how the function works to help developer understand the logics, and user API docs should focus on usage, parameter constraints, exceptions etc..
There was a problem hiding this comment.
Personally, having started this conversation, I prefer a single doc. Everything being developed in C++ RAPIDS, detail, experimental, and public namespaces, is targeted at power users aka advanced developers. I don't see much of a distinction between developer and user. Plus maintenance is much more complicated to maintain parallel docsets that are almost the same but differ in obscure ways. I'm not dogmatic about this though, if you think more refined documentation is appropriate I won't stop you.
There was a problem hiding this comment.
In fact, since the internal function will not be rendered on doxygen website, @copydoc will essentailly have no effect. Eventually the function's docstring goes with the function it decorates. While reducing duplication is a good thing, keeping track of remote referencing could be even a harder task to maintain without the help of doxygen.
|
@thomcom I believe I have added the geoarrow point array format and offset array format support per your review above.
The answer is positive. One last thing that's missing from making this fully geoarrow compliant is supporting multi-linestring and multi-point. Since that's a pretty big change to the code, after a second thought, I'm planning to submit it as a separate PR. Other than this I believe there's an open question for docstrings. If you agree, can we punt the question to later PR and proceed with this one? |
|
@gpucibot merge |
|
Thanks to all reviewers, a draft follow up PR is up as promised: #660 |
…ython bindings (#660) This PR adds multi-point and multi-linestring support to `point_linestring_distance`. The c++ API **always** supports multi-variant of the geometry type, since any single-geometry type can be wrapped with a `counting_iteartor` for its underlying geometry offset. The column API accepts a `std::optional` for the geometry offset inputs. If not provided, they are considered single geometry by default. A `multigeom_dispatcher` is used to generalize the runtime to compile optional information. This PR also builds python bindings for `point_linestring_distance`. First, the cython bindings is created with a backported `optional` module from cython 3.0 to support the above `std::optional` API. The cython APIs also made use of the dynamic typing property of python to allow geometry offsets to be optional arguments. The python API is the first examplar of a computing API accepting GeoSeries as input. For simplicity, we assume `GeoSeries` only contains single type geometry and there is no mixing of `points` and `multipoints`. Contributes to #231 Follow up to #573 Authors: - Michael Wang (https://github.com/isVoid) - Bradley Dice (https://github.com/bdice) Approvers: - Mark Harris (https://github.com/harrism) - H. Thomson Comer (https://github.com/thomcom) URL: #660
This PR adds point to linestring distance.
Contributes to #231