Add `contains`predicate. #1086

thomcom · 2023-04-19T20:10:45Z

Closes #1077
Depends on #1022

This PR is a precursor to #1064.

Description

This PR creates a new .contains predicate that uses the sum of the result of the .contains_properly predicate and the .intersects predicate to compute the .contains relationship, boundary inclusive.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

isVoid

Attention span cutting off. Part 1 review. Will continue after some rest.

python/cuspatial/cuspatial/core/binpreds/binpred_interface.py

python/cuspatial/cuspatial/core/binpreds/complex_geometry_predicate.py

isVoid · 2023-04-26T21:51:55Z

python/cuspatial/cuspatial/core/binpreds/complex_geometry_predicate.py

+    def _convert_quadtree_result_from_part_to_polygon_indices(
+        self, lhs, point_result
+    ):
+        """Convert the result of a quadtree contains_properly call from


Does it matter if this internal function is used for contains_properly? It seems like it can be applied generically to all other predicates.

It is only used in contains_properly. I will move it to binpred_utils.py if another predicate depends on it.

python/cuspatial/cuspatial/core/binpreds/feature_contains_properly.py

harrism · 2023-04-26T23:56:30Z

Perhaps use a more descriptive name for this PR. After about 6 of these PRs, my eyes have started glazing over whenever I see "refactor" and "contains" in a PR title. And if anyone reads our changelog, they are going to wonder why the heck we refactor contains so frequently.

python/cuspatial/cuspatial/core/binpreds/contains_geometry_processor.py

python/cuspatial/cuspatial/core/binpreds/feature_contains_properly.py

isVoid · 2023-04-28T19:21:02Z

python/cuspatial/cuspatial/core/geoseries.py

+        result = pairwise_multipoint_equals_count(lhs, rhs)
+        return result
+
+    def _basic_intersects_pli(self, other):


pli? Please avoid unnecessary abbreviation.

It also seems to me that the other intersection should be called _basic_point_intersects since that computes point intersections while this is _basic_intersects.

pli: pairwise_linestring_intersection. This returns the unmodified results of the pairwise_linestring_intersection call, hence pli. I'll think about some renamings.

python/cuspatial/cuspatial/core/geoseries.py

isVoid · 2023-04-28T19:31:02Z

python/cuspatial/cuspatial/tests/binpreds/test_pip_only_binpreds.py

I think it would be nice to separate this file into test_[name_of_binpreds] for each tested binpreds.

This file only covers an extremely small range of within, overlaps, and intersects tests. I'm happy to write more tests in specific files once test dispatching with passing tests is merged in #1064.

python/cuspatial/cuspatial/utils/binpred_utils.py

isVoid · 2023-04-28T20:23:35Z

python/cuspatial/cuspatial/utils/binpred_utils.py

+    return result
+
+
+def _linestrings_is_degenerate(geoseries):


Can we remove this now that #1093 is merged?

Probably, I'd like to leave that for a future issue/PR.

Created an issue to track this #1113

…erly.py Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>

Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>

…l into feature/_basic_binpreds

harrism

Mostly docs, a few structural questions.

python/cuspatial/cuspatial/core/binpreds/contains_geometry_processor.py

python/cuspatial/cuspatial/core/binpreds/feature_contains_properly.py

python/cuspatial/cuspatial/core/geoseries.py

harrism · 2023-05-02T02:40:46Z

python/cuspatial/cuspatial/core/geoseries.py

+        The size of a multipoint is the number of points in its single point.
+        The size of a point is 1.
+        """
+        if contains_only_polygons(self):


Can't this be a lot simpler? Any geometry, regardless of complexity, stores a flat array of coordinates. The number of points is just the length of the xy points array, isn't it?

The reason for this logic is because sizes returns the size of each row of the GeoSeries. For example

x = cuspatial.GeoSeries([ LineString([(0, 0), (1, 1)]), LineString([(0, 0), (1, 1), (2, 2)]), LineString([(0, 0), (1, 1), (2, 2), (3, 3)]), ]) print(x.sizes)

0 2 1 3 2 4 dtype: int32

The .xy array in this case contains all of the points for all of the features in the GeoSeries:

x.lines.xy

0 0.0 1 0.0 2 1.0 3 1.0 4 0.0 5 0.0 6 1.0 7 1.0 8 2.0 9 2.0 10 0.0 11 0.0 12 1.0 13 1.0 14 2.0 15 2.0 16 3.0 17 3.0 dtype: float64

But has no information about the lengths of the individual geometries.

sum(x.sizes) == 9 and len(x.lines.xy) / 2 == 9. So why doesn't the latter serve the purpose?

For example, the sizes are used for each row of the GeoSeries to determine if the .contains (boundary inclusive) predicate has been met:

return contains + intersects >= rhs.sizes

Where contains is a cudf.Series of the number of points that satisfied .contains_properly for each row in the GeoSeries, and intersects is a cudf.Series of the number of points that satisfied .intersects for those same rows in the GeoSeries. So if I had a GeoSeries of a length-three polygon and a length 4 polygon, sizes would be

0 3 1 4 dtype: int32

and in the cast of a self-contains test contains would be

0 0 1 0 dtype: int32

and intersects would be

0 3 0 4 dtype: int32

so the predicate result of .contains is
`contains_properly + intersects >= rhs.sizes ==

0 True 1 True dtype: bool

Does that properly answer your question? I'm not using the sum of the sizes in the GeoSeries, but each individually.

python/cuspatial/cuspatial/core/geoseries.py

…omments.

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

…l into feature/_basic_binpreds

python/cuspatial/cuspatial/core/geoseries.py

isVoid

Just two small comments relating to documentation above.

harrism · 2023-05-03T20:56:31Z

Also please address my two remaining open comments.

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

…l into feature/_basic_binpreds

thomcom · 2023-05-04T23:13:46Z

/merge

thomcom added 4 commits April 19, 2023 19:08

Basic binpred first fork of big PR.

1766437

Forgot geoseries.py

1ff4589

Refactor contains and contains properly and pass all tests.

367a420

Remove unneeded predicate.

fbc1618

thomcom added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 19, 2023

thomcom self-assigned this Apr 19, 2023

thomcom requested a review from a team as a code owner April 19, 2023 20:10

thomcom requested review from trxcllnt and harrism April 19, 2023 20:10

github-actions bot added the Python Related to Python code label Apr 19, 2023

thomcom requested review from isVoid and removed request for trxcllnt April 19, 2023 20:18

thomcom added 10 commits April 19, 2023 20:37

Tweak docs.

873525a

Clean up docs in feature_contains.py

0d84603

Clean up contains_properly docs.

e4132b0

Clean up ContainsProperlyByIntersection.

8dfca13

Clean up WithinIntersectsPredicate.

a3d8b8a

Write docs for geoseries.sizes

008631e

Clean up docs and write docstrings for all _basic utility methods.

550fa9b

Document and improve binpred_utils.py

f45acda

Fix a bad test.

fc1ca25

Merge branch 'branch-23.06' into feature/_basic_binpreds

32c9e0a

This was referenced Apr 26, 2023

Implement and Test All non-multi-Feature Spatial Predicate Combinations #1064

Merged

Binary Predicate Test Dispatching #1085

Merged

isVoid reviewed Apr 26, 2023

View reviewed changes

thomcom changed the title ~~Refactor of contains and contains_properly.~~ Add contains function. Apr 27, 2023

Merge branch 'branch-23.06' into feature/_basic_binpreds

1c0a018

isVoid reviewed Apr 28, 2023

View reviewed changes

thomcom and others added 8 commits April 28, 2023 15:37

Update python/cuspatial/cuspatial/core/binpreds/feature_contains_prop…

9e65cb4

…erly.py Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>

Handle review comments.

8a1d234

Drop one useless inheritance.

c433949

Update python/cuspatial/cuspatial/core/geoseries.py

24e4d9a

Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>

Change error type of mixed geometry geoseries.sizes call.

a86fb2b

Merge branch 'feature/_basic_binpreds' of github.com:thomcom/cuspatia…

e9dd49e

…l into feature/_basic_binpreds

Remove local imports from _basic_predicates.

c813ba7

Get rid of intesrsects_through

c3d03cb

harrism reviewed May 2, 2023

View reviewed changes

thomcom and others added 7 commits May 2, 2023 15:21

Move _basic_predicates into a dedicated file. Handle various review c…

92c918c

…omments.

Update python/cuspatial/cuspatial/core/geoseries.py

86dd5b3

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Update python/cuspatial/cuspatial/core/geoseries.py

4f6f8a8

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Update python/cuspatial/cuspatial/core/geoseries.py

c14b4ca

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Merge branch 'feature/_basic_binpreds' of github.com:thomcom/cuspatia…

6e8f6ee

…l into feature/_basic_binpreds

Update contains predicate docs.

72ad8c9

Refactor use of quadtree and language.

6c6f097

thomcom mentioned this pull request May 2, 2023

Can we remove pythonic degenerate linestrings now that #1093 is merged? #1113

Open

harrism mentioned this pull request May 2, 2023

Optimization: Expose multipolygon_range::geometry_index_from_part_index to Python. #1114

Open

isVoid reviewed May 3, 2023

View reviewed changes

python/cuspatial/cuspatial/core/geoseries.py Outdated Show resolved Hide resolved

python/cuspatial/cuspatial/core/geoseries.py Outdated Show resolved Hide resolved

isVoid approved these changes May 3, 2023

View reviewed changes

harrism approved these changes May 3, 2023

View reviewed changes

thomcom and others added 5 commits May 4, 2023 10:59

Update python/cuspatial/cuspatial/core/geoseries.py

d628a44

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Handle @isVoid's final comments.

2eb4b32

Merge branch 'feature/_basic_binpreds' of github.com:thomcom/cuspatia…

81d85f5

…l into feature/_basic_binpreds

Merge branch 'branch-23.06' into feature/_basic_binpreds

14ec9d7

Style

9855137

rapids-bot bot merged commit 8b97a72 into rapidsai:branch-23.06 May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `contains`predicate. #1086

Add `contains`predicate. #1086

thomcom commented Apr 19, 2023 •

edited

Loading

isVoid left a comment

isVoid Apr 26, 2023

thomcom Apr 27, 2023

harrism commented Apr 26, 2023 •

edited

Loading

isVoid Apr 28, 2023

thomcom Apr 28, 2023

isVoid Apr 28, 2023

thomcom Apr 28, 2023

isVoid Apr 28, 2023

thomcom Apr 28, 2023

thomcom May 2, 2023

harrism left a comment

harrism May 2, 2023

thomcom May 2, 2023

harrism May 2, 2023

thomcom May 4, 2023 •

edited

Loading

isVoid left a comment

harrism commented May 3, 2023

thomcom commented May 4, 2023

Add containspredicate. #1086

Add containspredicate. #1086

Conversation

thomcom commented Apr 19, 2023 • edited Loading

Description

Checklist

isVoid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomcom May 4, 2023 • edited Loading

Choose a reason for hiding this comment

isVoid left a comment

Choose a reason for hiding this comment

harrism commented May 3, 2023

thomcom commented May 4, 2023

Add `contains`predicate. #1086

Add `contains`predicate. #1086

thomcom commented Apr 19, 2023 •

edited

Loading

harrism commented Apr 26, 2023 •

edited

Loading

thomcom May 4, 2023 •

edited

Loading