Fix multipolygon geometry iterator. #1402
Conversation
| << "Part Offsets:\n\t{" << part_offsets << "}\n" | ||
| << "Ring Offsets: \n\t{" << ring_offsets << "}\n" | ||
| << "Coordinates: \n\t{" << coordinates << "}\n"; | ||
| auto print_vector = [&](auto const& vec) { |
There was a problem hiding this comment.
Without these changes, I was seeing errors that no operator<< overload existed for thrust::host_vector.
There was a problem hiding this comment.
Did thrust remove this overload from host_vector?
There was a problem hiding this comment.
I wasn't aware that such an overload ever existed. I don't see that implemented: https://github.com/search?q=repo%3ANVIDIA%2Fcccl%20operator%3C%3C&type=code
| CUSPATIAL_HOST_DEVICE auto geometry_offset_begin() { return _geometry_begin; } | ||
|
|
||
| /// Return the iterator to the one past the last geometry offset in the range. | ||
| CUSPATIAL_HOST_DEVICE auto geometry_offset_end() { return _part_end; } | ||
| CUSPATIAL_HOST_DEVICE auto geometry_offset_end() { return _geometry_end; } |
There was a problem hiding this comment.
This is the core bug: the thrust::copy call using the geometry offsets was getting the wrong size because the wrong iterator was being returned.
This only became visible in the one test case where a multipolygon contained more than one polygon.
| @@ -1069,7 +1069,7 @@ class MultipolygonRangeOneTest : public MultipolygonRangeTestBase<T> { | |||
| void test_geometry_offsets_it() | |||
| { | |||
| rmm::device_uvector<std::size_t> d_offsets = this->copy_geometry_offsets(); | |||
| auto expected = make_device_vector<std::size_t>({0, 1}); | |||
| auto expected = make_device_vector<std::size_t>({0, 2}); | |||
There was a problem hiding this comment.
The geometry offsets are {0, 2}. This test silently passed because the part iterators are {0, 1, 2} and somehow thrust::copy was silently copying only the first two values. CCCL's current behavior (without the patch) of erroring is appropriate, but I mistakenly thought it was a CCCL bug instead of a cuSpatial bug at that time.
|
Thanks for the reviews. There are some failing tests but I think they are related to #1400 and not this PR. I pinged @mroeschke to look into that PR, and I'll merge the upstream into this PR once that goes in. Then we can remove the CCCL patches from cuDF and rapids-cmake. |
|
/merge |
While upgrading CCCL, we ran into a test failure in cuSpatial. We added a patch to revert some changes from CCCL but the root cause was a bug in cuSpatial. I have fixed that bug here: rapidsai/cuspatial#1402 Once that PR is merged, we can remove this CCCL patch. See also: - rapids-cmake patch removal: rapidsai/rapids-cmake#640 - Original rapids-cmake patch: rapidsai/rapids-cmake#511 - CCCL epic to remove RAPIDS patches: NVIDIA/cccl#1939 Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Robert Maynard (https://github.com/robertmaynard) URL: #16207
While upgrading CCCL, we ran into a test failure in cuSpatial. We added a patch to revert some changes from CCCL but the root cause was a bug in cuSpatial. I have fixed that bug here: rapidsai/cuspatial#1402 Once that PR is merged, we can remove this CCCL patch. See also: - #511 - NVIDIA/cccl#1939 Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Robert Maynard (https://github.com/robertmaynard) URL: #640
Description
This PR fixes a bug in the multipolygon geometry iterator. The geometry iterator was returning part iterators by mistake, which masked an issue that we were patching out in rapids-cmake's CCCL. See rapidsai/rapids-cmake#511.
With this fix, we can remove that patch from rapids-cmake's CCCL: rapidsai/rapids-cmake#640
Checklist