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

[thomcom]

Depends on #1085
Depends on #1086
Depends on #1022
Closes #1062
Closes #1046

Description

Tests and passes all simple feature combinations across nine binary predicates.

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]

This seems a bit conflicting to me. Negating contains and then contains_properly?

[thomcom]

For polygons a and b to overlap, it is necessary that neither polygon completely contain the other polygon, but that some of the points in a or b be contained by the other. https://postgis.net/docs/ST_Overlaps.html. Let me know if this answer is sufficient.

[isVoid]

To my understanding, the only difference between contains and contains_properly, are that if one point falls on the boundary of another, it does/does not counted as contains.

The first part of the statement:

neither polygon completely contain the other polygon

is correctly captured by ~(contains_lhs | contains_rhs).

However, this statement isn't:

that some of the points in a or b be contained by the other

contains_properly still tests if "all points of A is contained by B", what you need is a contains_any that test if any of A is contained by B.

[thomcom]

I think this is resolved as PolygonPolygonOverlaps now uses _basic_contains_properly_any, which returns true if any of B is contained by A, and the reverse.

[thomcom]

Thanks for your detailed and perceptive comments and additional test cases @isVoid I've handled your review.

[isVoid]

Minor comments, well done!

[isVoid]

Looking at this again, you don't need an additional layer of exception handler that rethrows all exceptions as ValueError.

[thomcom]

You're right - this handler just prevents potential issues with the logging appearing like test errors, so I've modified it to just raise e

[harrism]

Is this comment correct? This is contains, not covers. The code for the two looks the same. Is that correct?

[thomcom]

I think that in the context of LineString+LineString, .contains and .covers are equivalent. This comment persisting in feature_contains.py is not correct, thanks!

https://postgis.net/docs/ST_Covers.html
https://postgis.net/docs/ST_Contains.html

[thomcom]

import geopandas
from cuspatial.tests.binpreds.binpred_test_dispatch import (
    features,
    linestring_linestring_dispatch_list,
)
def sample_test_data(features, dispatch_list, size, lib=cuspatial):
    geometry_tuples = [features[key][1:3] for key in dispatch_list]
    geometries = [
        [lhs_geo for lhs_geo, _ in geometry_tuples],
        [rhs_geo for _, rhs_geo in geometry_tuples]
    ]
    lhs = lib.GeoSeries(list(geometries[0]))
    rhs = lib.GeoSeries(list(geometries[1]))
    if lib == geopandas:
        rng = np
    else:
        rng = cp
    rng.random.seed()
    lhs_picks = rng.random.randint(0, len(lhs), size)
    rhs_picks = rng.random.randint(0, len(rhs), size)
    return (
        lhs[lhs_picks].reset_index(drop=True),
        rhs[rhs_picks].reset_index(drop=True)
    )
lhs, rhs = sample_test_data(features, linestring_linestring_dispatch_list, 1000, geopandas)
(lhs.contains(rhs) == lhs.covers(rhs)).all()

True

[harrism]

I can't comment on non-changed lines, but please don't add documentation that will go stale quickly, e.g. "in this initial release". Also, the fact that it is implemented using only the equals predicate is an implementation detail, not a feature of the API. Public documentation should cover features and usage of the API, not implementation details.

[thomcom]

Thanks for the clarification, those docs and others definitely needed a cleanup.

[thomcom]

/merge

[harrism]

BTW, I think non-multi is a better name than "simple". A simple feature means something specific, right? OSGeo IsSimple means no self intersections or self tangents, etc.

[harrism]

Updated title so hopefully it will make it into our changelog with a very clear description of the feature. Please remember this for all PRs.