-
Notifications
You must be signed in to change notification settings - Fork 154
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.
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
Create GeoSeries.contains_properly
method using point_in_polygon.
#749
Create GeoSeries.contains_properly
method using point_in_polygon.
#749
Conversation
…to produce the correct results.
…tial into feature/GeoSeries.contains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some more style requests, with an open question in the end. Great work!
expected = gpdlhs.contains(gpdrhs).values | ||
assert (got == expected).all() | ||
got = rhs.contains_properly(lhs).values_host | ||
expected = gpdrhs.contains(gpdlhs).values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, shouldn't you be using shapely.contains_properly()
for the expected result as we discussed in the meeting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interfaces are not the same. Using shapely.contains_properly(x, y)
is a method that takes two Shapely geometries and returns True or False. .contains
is a GeoSeries
method that operates on self
and other
. Refactoring these tests to use shapely
only is not comparing apples to oranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naively you can use a for-loop...
expected = pd.Series()
for lhs, rhs in zip(gpdlhs, gpdrhs):
expected = pd.concat([expected, [shapely.contains_properly(lhs, rhs)])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But comparing cuspatial.contains_properly
to geopandas.contains
is comparing apples to oranges.
BTW, does this support multipoint in polygon? |
Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>
Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>
Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>
Co-authored-by: Mark Harris <mharris@nvidia.com>
…tial into feature/GeoSeries.contains
GeoSeries.contains
method using point_in_polygon.GeoSeries.contains_properly
method using point_in_polygon.
yes, there are tests for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few other tiny things.
are properly contained within the corresponding polygon. Polygon A contains Point B | ||
properly if B intersects the interior of A but not the boundary (or exterior). | ||
|
||
Note that polygons must be closed: the first and last vertex of each |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in a Note
section as well?
Co-authored-by: Mark Harris <mharris@nvidia.com>
@gpucibot merge |
Closes #743
Closes #744
Description
This PR closes the above named issues relating to creating a .contains method and, more importantly, resolving boundary case inconsistency with
point_in_polygon
.As it stands the colinearity test I've added tois_point_in_polygon
doubles the runtime of brute-forcepoint_in_polygon
and has no visible effect on the runtime ofquadtree_point_in_polygon
.- Note I need to double check the above benchmark, having set this project down for the last few weeks.This depends on #750, please do not review the C++ code here until that PR is merged. Please do review the python code.
Benchmark
Benchmark results are in, looks like there's no measurable speed difference between 22.12 pre-boundary exclusion and our current implementation:
vs
branch-22.12
Still adding:
.contains
implementation in python..contains
docs when necessary..contains
ExamplesChecklist