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

[FEA] Integrate GeoSeries with read_polygon_shapefile - #609

Merged
rapids-bot[bot] merged 1 commit into
rapidsai:branch-22.10from
thomcom:fea-add-read-polygon-shapefile-integration
Sep 19, 2022
Merged

[FEA] Integrate GeoSeries with read_polygon_shapefile#609
rapids-bot[bot] merged 1 commit into
rapidsai:branch-22.10from
thomcom:fea-add-read-polygon-shapefile-integration

Conversation

@thomcom

@thomcom thomcom commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

This useful PR adds a GeoSeries constructor for the tuple returned by read_polygon_shapefile. Now users can say

geoseries = cuspatial.GeoSeries(cuspatial.read_polygon_shapefile('the_shapefile'))

and load the data directly into GeoArrow.

This PR touches many files because I add a reversed argument to python and C++ so that the user can swap back and forth if they desire. This was required because the original implementation was reverse-ordering the polygons, which does not match GeoPandas results when reading a shapefile. Now it does, and is tested in both configurations.

Closes #668.

@thomcom thomcom added feature request New feature or request 1 - On Deck To be worked on next non-breaking Non-breaking change labels Jul 29, 2022
@thomcom thomcom self-assigned this Jul 29, 2022
@github-actions github-actions Bot added the Python Related to Python code label Jul 29, 2022
@zhangjianting

Copy link
Copy Markdown
Contributor

There are very informative discussions on the ordering of polygon vertices: https://gis.stackexchange.com/questions/119150/order-of-polygon-vertices-in-general-gis-clockwise-or-counterclockwise
The OGC SFS and ESRI shapefile have different orderings. As long as the inner rings and the outer rings have different directions, PIP algorithms should still work. Also see
https://wrfranklin.org/Research/Short_Notes/pnpoly.html#Listing%20the%20Vertices

@zhangjianting

zhangjianting commented Aug 4, 2022

Copy link
Copy Markdown
Contributor

The Shapefile Reader API does have a problem in dealing with multi-polygons. The initial design was to break multi-polygons (as well as GeometryCollection) into multiple (single/regular) polygons (with or without holes). Yet the implementations was to consolidate the rings of multi-polygons, which essentially treats the first ring as the outer ring and the rest as the inner rings, i.e., a single/regular polygon with holes. Subsequently, it will break PIP algorithms. There are two solutions:

  1. Simply declaring not supporting multi-polygons or GeometryCollection.
  2. Modify the Shapefile reader and extend the code of all relevant operations to truly support multi-polygons.

@github-actions github-actions Bot added the libcuspatial Relates to the cuSpatial C++ library label Aug 4, 2022
@thomcom thomcom added 3 - Ready for Review Ready for review by team and removed 1 - On Deck To be worked on next labels Aug 4, 2022
@thomcom
thomcom marked this pull request as ready for review August 4, 2022 18:46
@thomcom
thomcom requested review from a team as code owners August 4, 2022 18:46

@isVoid isVoid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For discussion: would you think it's better to support factory functions like GeoColumn.from_shapefile and GeoSeries.from_shapefile? We can still support constructing the objects in constructor, but under the hood punt to the factory functions.

Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated
Comment thread cpp/tests/io/shp/polygon_shapefile_reader_test.cpp
Comment thread python/cuspatial/cuspatial/_lib/cpp/shapefile_reader.pxd Outdated

@harrism harrism left a comment

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 think handling winding order can be cleaner, made some suggestions.

Comment thread cpp/include/cuspatial/shapefile_reader.hpp Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated
Comment thread python/cuspatial/cuspatial/geometry/geocolumn.py Outdated
@harrism harrism added breaking Breaking change and removed non-breaking Non-breaking change labels Aug 24, 2022
Comment thread cpp/include/cuspatial/shapefile_reader.hpp Outdated
Comment thread cpp/include/cuspatial/shapefile_reader.hpp Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp
@zhangjianting

zhangjianting commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

There is a simple way to determine the orientation of a ring/curve, e.g., signed area.
https://en.wikipedia.org/wiki/Curve_orientation
Also https://stackoverflow.com/questions/1165647/how-to-determine-if-a-list-of-polygon-points-are-in-clockwise-order
It actually might be useful to return the area of rings for "advanced" users or applications that require computing areas of polygons. A Thrust segmented reduction (prefix-sum) will do.

@thomcom

thomcom commented Aug 25, 2022

Copy link
Copy Markdown
Contributor Author

Ah nice! Computing the handedness of the polygon is nothing more than the determinant of the matrix formed by the first three points. That handedness is guaranteed to continue through all of the rest of the points. It is tempting to implement this for completeness.

@thomcom

thomcom commented Aug 25, 2022

Copy link
Copy Markdown
Contributor Author

We should definitely have an area computation as well, and it looks quite simple. I'll file an issue for it.

@thomcom

thomcom commented Aug 29, 2022

Copy link
Copy Markdown
Contributor Author

Ready for your re-review @harrism @isVoid

Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated

@harrism harrism left a comment

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.

Nearly there. A couple of smelly C-style casts need to be fixed up.

Comment thread cpp/include/cuspatial/experimental/points_in_range.cuh Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp Outdated
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp
Comment thread cpp/src/io/shp/polygon_shapefile_reader.cpp
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py Outdated
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py Outdated
@thomcom
thomcom requested a review from harrism September 6, 2022 14:46
Comment thread cpp/include/cuspatial/shapefile_reader.hpp Outdated
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py Outdated
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py Outdated
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py Outdated
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py
@thomcom

thomcom commented Sep 7, 2022

Copy link
Copy Markdown
Contributor Author

@harrism @isVoid this is cleaner and readier for another round of review.

@thomcom
thomcom requested a review from isVoid September 7, 2022 23:59
Comment thread python/cuspatial/cuspatial/core/_column/geocolumn.py

@isVoid isVoid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing all comments. I think you should update the PR description from mentioning reversed to winding_order.

Comment thread cpp/include/cuspatial/shapefile_reader.hpp Outdated
Comment thread python/cuspatial/cuspatial/core/geoseries.py Outdated
@thomcom
thomcom requested a review from a team as a code owner September 14, 2022 16:24
@github-actions github-actions Bot added the conda Related to conda and conda configuration label Sep 14, 2022
@thomcom
thomcom force-pushed the fea-add-read-polygon-shapefile-integration branch from 157cf5b to 8ccc066 Compare September 14, 2022 17:41
@github-actions github-actions Bot removed the conda Related to conda and conda configuration label Sep 14, 2022
@thomcom thomcom added non-breaking Non-breaking change and removed breaking Breaking change labels Sep 19, 2022
@thomcom
thomcom force-pushed the fea-add-read-polygon-shapefile-integration branch from 8ccc066 to 19600a5 Compare September 19, 2022 17:48
@rapidsai rapidsai deleted a comment from review-notebook-app Bot Sep 19, 2022
@thomcom

thomcom commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit b38ea17 into rapidsai:branch-22.10 Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3 - Ready for Review Ready for review by team feature request New feature or request libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[FEA] Add offset column accessors

4 participants