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

Fix multipolygon geometry iterator. #1402

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Fix multipolygon geometry iterator. #1402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9d3028
Fix multipolygon geometry iterator.
bdice Jul 7, 2024
1a0ad73
Merge branch 'branch-24.08' into fix-multipolygon-geometry-iterator
trxcllnt Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cpp/include/cuspatial/range/multipolygon_range.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, NVIDIA CORPORATION.
* Copyright (c) 2023-2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -115,10 +115,10 @@ class multipolygon_range {
CUSPATIAL_HOST_DEVICE auto point_end();

/// Return the iterator to the first geometry offset in the range.
CUSPATIAL_HOST_DEVICE auto geometry_offset_begin() { return _part_begin; }
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; }
Comment on lines +118 to +121
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


/// Return the iterator to the first part offset in the range.
CUSPATIAL_HOST_DEVICE auto part_offset_begin() { return _part_begin; }
Expand Down
24 changes: 20 additions & 4 deletions cpp/include/cuspatial_test/vector_factories.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,26 @@ class multipolygon_array {
{
auto [geometry_offsets, part_offsets, ring_offsets, coordinates] = arr.to_host();

return os << "Geometry Offsets:\n\t{" << geometry_offsets << "}\n"
<< "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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without these changes, I was seeing errors that no operator<< overload existed for thrust::host_vector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did thrust remove this overload from host_vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (auto it = vec.begin(); it != vec.end(); it++) {
os << *it;
if (std::next(it) != vec.end()) { os << ", "; }
}
};

os << "Geometry Offsets:\n\t{";
print_vector(geometry_offsets);
os << "}\n";
os << "Part Offsets:\n\t{";
print_vector(part_offsets);
os << "}\n";
os << "Ring Offsets: \n\t{";
print_vector(ring_offsets);
os << "}\n";
os << "Coordinates: \n\t{";
print_vector(coordinates);
os << "}\n";
return os;
}

protected:
Expand Down
4 changes: 2 additions & 2 deletions cpp/tests/range/multipolygon_range_test.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, NVIDIA CORPORATION.
* Copyright (c) 2023-2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -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});
Copy link
Contributor Author

@bdice bdice Jul 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


CUSPATIAL_EXPECT_VECTORS_EQUIVALENT(d_offsets, expected);
}
Expand Down
Loading