Implement all operations that require only equality. #839

thomcom · 2022-12-02T22:54:44Z

Description

This PR implements the set of operations listed in #838.
Closes #838
Closes #832
Depends on #834

Checklist

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

… instead of __init__.

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

…rations

harrism

Looking for opportunities to be less repetitive, and to make the documentation more consistent (internally and with GeoPandas).

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

harrism · 2022-12-13T04:44:28Z

python/cuspatial/cuspatial/utils/column_utils.py

+    if contains_only_points(lhs) and contains_only_points(rhs):
+        return True
+    elif contains_only_multipoints(lhs) and contains_only_multipoints(rhs):
+        return True
+    elif contains_only_linestrings(lhs) and contains_only_linestrings(rhs):
+        return True
+    elif contains_only_polygons(lhs) and contains_only_polygons(rhs):
+        return True
+    else:
+        return False


Seems this would be more efficient and simpler if you first confirm that each side has a single geometry type, and then compare the geometry type. As it is, contains_only_* re-confirms that there is only a single geometry type each time. If you can also provide a way to just get a value representing the geometry types contained in a GS, then you only have to check that one of them has only a single geometry type, and then compare the types codes of both sides.

Suggested change

if contains_only_points(lhs) and contains_only_points(rhs):

return True

elif contains_only_multipoints(lhs) and contains_only_multipoints(rhs):

return True

elif contains_only_linestrings(lhs) and contains_only_linestrings(rhs):

return True

elif contains_only_polygons(lhs) and contains_only_polygons(rhs):

return True

else:

return False

if contains_single_geometry_type(lhs):

return geometry_types(lhs) == geometry_types(rhs)

else:

return False

I don't think we have a geometry_types function yet, what does it do?

It's hypothetical. The idea is to return something that enables uniquely identifying all the geometry types contained in the series, so they can be compared. E.g. a bitmask.

We can probably depend on pandas ExtensionDtype and make a GeometryDtype and assign to dtype. We can assign more meta data to GeometryDtype, for example, the underlying mixed geometry type. We can model GeometryDtype to UnionType.

Should this be a separate effort? We can create an issue from this thread if so.

This is a cool idea I will look at.

harrism · 2022-12-13T04:49:16Z

python/cuspatial/cuspatial/core/geoseries.py

+        Geometries cross if they have some but not all interior points in
+        common, have the same dimension, and the intersection of the
+        interiors of the geometries has the dimension of the geometries
+        themselves minus one.


So if we have two polygons that overlap, do they also cross? Confused by the "dimension of the geometries themselves minus 1".

How about a line that crosses a polygon. The intersection has 1D geometry, right? But that is not one less than the dimension of both the geometries -- the line is 1D and the polygon is 2D.

In A Small Set of Formal Topological Relationships Suitable for End User Interaction (SSFTR), $cross$ is defined as

$$ < \lambda_1, cross \lambda_2> \iff dim(\lambda_1^o \cap \lambda_2^o) = (max(dim(\lambda_1^o), dim(\lambda_2^o))-2 \land (\lambda_1 \cap \lambda_2 \neq \lambda_1) \land (\lambda_1 \cap \lambda_2 \neq \lambda_2) $$

Two polygons that overlap I think share an interior dimension, that is $$dim(\lambda_1^o \cap \lambda_2^o) = 2$$

In order for two polygons to cross they have to share an overlap that is just a line, and it can't only be in the exterior. This is essentially impossible for two areas, so we may not see cases of polygon crossing.

cross looks like it applies primarily to linestrings, as the dimensionality of the interior overlap will always be 0 for linestring.cross(linestring) and 1 for linestring.cross(polygon) or polygon.cross(linestring).

Looking again at SSFTR I see that cross is applied when $max(dim(\lambda_1^o), dim(\lambda_2^o)) -1 = \dim(\lambda_1^o \cap \lambda_2^o)$ and overlap is applied when the dimension of the interior intersection equals the dimension of the interior.

python/cuspatial/cuspatial/core/geoseries.py

harrism · 2022-12-13T04:55:39Z

python/cuspatial/cuspatial/core/geoseries.py

+        """
+        return CoversBinpred(self, other, align)()
+
+    def intersects(self, other, align=True):


After looking at all these predicates on GeoSeries, I realized we really should just start from a copy of the documentation from GeoPandas GeoSeries here. e.g. for intersects:

Returns a Series of dtype('bool') with value True for each aligned geometry that intersects other.

An object is said to intersect other if its boundary and interior intersects in any way with those of the other.

The operation works on a 1-to-1 row-wise manner:
../../../_images/binary_op-01.svg

I'm pretty much using GeoPandas as my source, but trying not to just copy everything outright. What do you suggest?

Just make sure every operation has consistent docs in terms of what is documented, what is defined, and what terminology and mathematics are used in the definitions.

harrism · 2022-12-13T04:58:09Z

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

+        if contains_only_points(lhs):
+            result = lhs.points.xy.equals(rhs.points.xy)
+        elif contains_only_linestrings(lhs):
+            result = lhs.lines.xy.equals(rhs.lines.xy)
+        elif contains_only_polygons(lhs):
+            result = lhs.polygons.xy.equals(rhs.polygons.xy)
+        elif contains_only_multipoints(lhs):
+            result = lhs.multipoints.xy.equals(rhs.multipoints.xy)


Is there a way to just get the geometry as the single type that the geoseries contains, rather than doing this multi-step if/else every time?

Suggested change

if contains_only_points(lhs):

result = lhs.points.xy.equals(rhs.points.xy)

elif contains_only_linestrings(lhs):

result = lhs.lines.xy.equals(rhs.lines.xy)

elif contains_only_polygons(lhs):

result = lhs.polygons.xy.equals(rhs.polygons.xy)

elif contains_only_multipoints(lhs):

result = lhs.multipoints.xy.equals(rhs.multipoints.xy)

if contains_single_geometry_type(lhs):

result = lhs.geometry.as_type(lhs.type).xy.equals(rhs.geometry.as_type(lhs.type).xy)

We haven't implemented that yet. It is basically just remapping the input_types buffer of the GeoColumn.

Seems like it would be very valuable.

It will come in handy for fixing the bug in equals I identified that caused me to close this PR: I was using .equals instead of .geom_equals. 😮‍💨

Co-authored-by: Mark Harris <mharris@nvidia.com>

…spatial into feature/all_equals_operations

Co-authored-by: Mark Harris <mharris@nvidia.com>

isVoid

Just some minor comments.

isVoid · 2022-12-14T08:32:01Z

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

+        Returns
+        -------
+        GeoSeries
+            The left-hand-side of the internal binary operation, may be
+            reordered.
+        GeoSeries
+            The right-hand-side of the internal binary operation, may be
+            reordered.


Missing documentation for the aligned index

isVoid · 2022-12-14T08:43:05Z

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

+        if has_same_geometry(self.lhs, self.rhs) and contains_only_points(
+            self.lhs
+        ):
+            return cudf.Series([False] * len(self.lhs))


Any chance we can avoid computing _op and early return this?

Hmm good idea I'll consider that in the abstraction design. My first impulse is to add a bool to the BinaryPredicate class called done. _op and postprocess will only call if not done.

My second impulse is to not perform this calculation in postprocess but just do it in _op instead.

I will probably do another pass of the inheritance design in this PR.

I'm also considering duck typing _op and replacing it with the correct predicate function if preprocess deigns. Alternatively I could allow specification of an alternate _op that would be used if defined.

isVoid · 2022-12-14T08:54:05Z

python/cuspatial/cuspatial/utils/column_utils.py

+    if contains_only_points(lhs) and contains_only_points(rhs):
+        return True
+    elif contains_only_multipoints(lhs) and contains_only_multipoints(rhs):
+        return True
+    elif contains_only_linestrings(lhs) and contains_only_linestrings(rhs):
+        return True
+    elif contains_only_polygons(lhs) and contains_only_polygons(rhs):
+        return True
+    else:
+        return False


We can probably depend on pandas ExtensionDtype and make a GeometryDtype and assign to dtype. We can assign more meta data to GeometryDtype, for example, the underlying mixed geometry type. We can model GeometryDtype to UnionType.

thomcom · 2022-12-14T22:44:59Z

I found a number of issues with my equals implementation and I'm closing this for now.

thomcom · 2022-12-15T16:22:12Z

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

+
+        Notes
+        -----
+        Should only be called once.


Suggested change

Should only be called once.

Should only be called once.

Inputs `lhs` and `rhs` may be index aligned or may not be, depending

on the argument to `BinaryPredicate.__init__(...align)`.

thomcom added 9 commits December 1, 2022 14:23

Start refactoring binops to handle various ops and inputs.

67a2a6a

Abstraction working with binops that depend on pip.

3038253

Clean it up in GeoSeries.

d7a842b

Correct abstraction for pip_only tests.

3301b95

Support the seven extra pip tests.

f6688d7

Support all supportable pip calls.

77b6858

Remove unused abstraction.

c69d2e6

Clean up binops.intersects docs.

de923c5

Pass all equals tests except one.

67b0df6

github-actions bot added the Python Related to Python code label Dec 2, 2022

thomcom added 3 commits December 5, 2022 09:31

Merge

b4ea5e5

Finish abstracting pip based ops.

305f937

Pass all multi tests.

707217b

thomcom self-assigned this Dec 6, 2022

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

thomcom marked this pull request as ready for review December 6, 2022 17:05

thomcom requested a review from a team as a code owner December 6, 2022 17:05

thomcom requested review from harrism and isVoid December 6, 2022 17:05

thomcom mentioned this pull request Dec 6, 2022

Use quadtree-based point-in-polygon for .contains binops. #845

Closed

3 tasks

thomcom added 8 commits December 7, 2022 11:59

Add equals only binops.

376833a

Rename binops to binpreds.

f0d0c46

Cleaning up based on review.

441a585

Cleaned up unneeded future material.

e02a669

Resolve has_same_dimension naming issue.

6444a4c

Fix postprocess docs.

29870d4

Factor out op string argument and move logic to the __call__ function…

c2cc722

… instead of __init__.

Fix bug with complete overlap

18631e3

thomcom and others added 7 commits December 9, 2022 09:39

Merge new pip only binpreds refactor into equals.

a1ab8a7

Pass all equals only tests with new factoring.

023247e

Fix contains issue.

3ddf7c4

Update python/cuspatial/cuspatial/core/binpreds/binpreds.py

82f40ec

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

Handle review comments from @isVoid.

b71f34f

Remove more equals code.

5338a73

Merge branch 'feature/all_pip_operations' into feature/all_equals_ope…

af5ae69

…rations

harrism requested changes Dec 13, 2022

View reviewed changes

thomcom and others added 5 commits December 13, 2022 16:31

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

34b3fbf

Co-authored-by: Mark Harris <mharris@nvidia.com>

Handle review comments from @harrism

7be5a73

Merge branch 'feature/all_equals_operations' of github.com:thomcom/cu…

230b0b7

…spatial into feature/all_equals_operations

Merge branch 'branch-23.02' into feature/all_equals_operations

e2a3616

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

04cd58d

Co-authored-by: Mark Harris <mharris@nvidia.com>

isVoid reviewed Dec 14, 2022

View reviewed changes

Replace operation with predicate

f749ca0

thomcom closed this Dec 14, 2022

thomcom commented Dec 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement all operations that require only equality. #839

Implement all operations that require only equality. #839

thomcom commented Dec 2, 2022 •

edited

Loading

harrism left a comment

harrism Dec 13, 2022

thomcom Dec 13, 2022

harrism Dec 14, 2022

isVoid Dec 14, 2022

harrism Dec 14, 2022

thomcom Dec 15, 2022

harrism Dec 13, 2022

thomcom Dec 15, 2022 •

edited

Loading

thomcom Dec 15, 2022 •

edited

Loading

thomcom Dec 15, 2022

harrism Dec 13, 2022

thomcom Dec 13, 2022

harrism Dec 14, 2022

harrism Dec 13, 2022

thomcom Dec 13, 2022

harrism Dec 14, 2022

thomcom Dec 15, 2022

isVoid left a comment

isVoid Dec 14, 2022

isVoid Dec 14, 2022

thomcom Dec 15, 2022

thomcom Dec 15, 2022

isVoid Dec 14, 2022

thomcom commented Dec 14, 2022

thomcom Dec 15, 2022 •

edited

Loading

Implement all operations that require only equality. #839

Implement all operations that require only equality. #839

Conversation

thomcom commented Dec 2, 2022 • edited Loading

Description

Checklist

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

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 Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

thomcom Dec 15, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomcom commented Dec 14, 2022

thomcom Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

thomcom commented Dec 2, 2022 •

edited

Loading

thomcom Dec 15, 2022 •

edited

Loading

thomcom Dec 15, 2022 •

edited

Loading

thomcom Dec 15, 2022 •

edited

Loading