Skip to content
This repository was archived by the owner on Jul 28, 2025. It is now read-only.

Make read_polygon_shapefile place read-coordinates in the right order. - #627

Closed
thomcom wants to merge 2 commits into
rapidsai:branch-22.10from
thomcom:bug-polygons-wound-clockwise
Closed

Make read_polygon_shapefile place read-coordinates in the right order.#627
thomcom wants to merge 2 commits into
rapidsai:branch-22.10from
thomcom:bug-polygons-wound-clockwise

Conversation

@thomcom

@thomcom thomcom commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

read_polygon_shapefile inserts the coordinates in the opposite order. This PR fixes that issue. I also wrote a pip test in polygon_shapefile_reader_test.cpp, thinking that would prove that the ordering mattered.

However, the pip test works in both cases - it does not test for pip based on the "left hand rule" of point-to-linesegment projection. I'm looking at writing a different test to demonstrate the winding.

@thomcom
thomcom changed the base branch from branch-22.08 to branch-22.10 August 3, 2022 22:40
@github-actions github-actions Bot added the libcuspatial Relates to the cuSpatial C++ library label Aug 3, 2022

auto random_walk_func = [segment_length](auto const& prev, auto const& rad) {
return cartesian_2d<T>{prev.x + segment_length * rad.x, prev.y + segment_length * rad.y};
return cartesian_2d<T>({prev.x + segment_length * rad.x, prev.y + segment_length * rad.y});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't include this change in this PR. I don't agree that this is the right way to fix this issue, and we haven't even root-caused the issue yet. I have been discussing it with @trxcllnt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Understood!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll be sure to remove it before this is finalized.

Comment thread cpp/tests/io/shp/polygon_shapefile_reader_test.cpp Outdated

// append points in reverse order
for (cudf::size_type i = num_vertices - 1; i >= 0; i--) {
for (cudf::size_type i = 0; i < num_vertices; ++i) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I interpreted @zhangjianting 's comment here: #609 (comment) to mean that clockwise vs. counterclockwise is a matter of choice since the OGC and ESRI shapefile formats are opposites. In which case why not either leave this as-is, or provide a parameter to specify which is expected?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Providing a parameter might be the way to go here, just in case. In this context, GeoPandas is used to read a shapefile and read_polygon_shapefile is used to read a shapefile. While both are valid technically, the coordinates are wound in the opposite direction for the result that is produced by read_polygon_shapefile which is why I want to introduce this change.

Co-authored-by: Mark Harris <mharris@nvidia.com>
@thomcom thomcom closed this Aug 4, 2022
@thomcom

thomcom commented Aug 4, 2022

Copy link
Copy Markdown
Contributor Author

Abandoned in favor of #609

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

libcuspatial Relates to the cuSpatial C++ library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants