Binary Predicate Test Dispatching #1085

thomcom · 2023-04-19T19:01:31Z

Closes #1046
Closes #1036

This PR adds a new binary predicate test dispatch test system. The test dispatcher creates one test for each ordered pair of features from the set (point, linestring, polygon) and each predicate that feature tuple can be applied to:

contains
covers
crosses
disjoint
geom_equals
intersects
overlaps
touches
within

The combination of 9 predicates and 33 tests creates 297 tests that cover all possible combinations of simple features and their predicates.

While development is underway, the test dispatcher automatically xfails any test that fails or hasn't been implemented yet in order to pass CI.

The test dispatcher outputs diagnostic results during each test run. An output file test_binpred_test_dispatch.log is created containing all of the failing tests, including their name, a visual description of the feature tuple and relationship, the shapely objects used to create the test, and the runtime name of the test so it is easy for a developer (myself) to identify which test failed and rerun it. It also creates four .csv files during runtime that collect the results of each test pass or fail relative to which predicate is being run and which pair of features are being tested. These .csv files can be displayed using tests/binpred/summarize_binpred_test_dispatch_results.py, which will output a dataframe of each CSV file thusly:

(rapids) coder ➜ ~/cuspatial/python/cuspatial/cuspatial $ python tests/binpreds/summarize_binpred_test_dispatch_results.py 
     predicate  predicate_passes
0  geom_equals                20
1   intersects                11
2       covers                17
3      crosses                 6
4     disjoint                 9
5     overlaps                20
6       within                14
     predicate  predicate_fails
0     contains               33
1  geom_equals               13
2   intersects               22
3       covers               16
4      crosses               27
5     disjoint               24
6     overlaps               13
7      touches               33
8       within               19
                                             feature  feature_passes
0     (<ColumnType.POINT: 1>, <ColumnType.POINT: 1>)              14
1   (<ColumnType.POINT: 1>, <ColumnType.POLYGON: 4>)              22
2  (<ColumnType.LINESTRING: 3>, <ColumnType.LINES...              16
3  (<ColumnType.LINESTRING: 3>, <ColumnType.POLYG...              38
4  (<ColumnType.POINT: 1>, <ColumnType.LINESTRING...               7
                                             feature  feature_fails
0     (<ColumnType.POINT: 1>, <ColumnType.POINT: 1>)              4
1  (<ColumnType.POINT: 1>, <ColumnType.LINESTRING...             20
2   (<ColumnType.POINT: 1>, <ColumnType.POLYGON: 4>)             14
3  (<ColumnType.LINESTRING: 3>, <ColumnType.LINES...             20
4  (<ColumnType.LINESTRING: 3>, <ColumnType.POLYG...             52
5  (<ColumnType.POLYGON: 4>, <ColumnType.POLYGON:...             90

Without additional modifications, the test dispatcher produces 97 passing results and 200 xfailing results. In a shortly upcoming PR, updates to the basic predicates and binpred test architecture increase the number of passing results to 274, with 25 xfails. These PRs are separated for ease of review.

Checklist

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

harrism

One initial question about pytest machinery.

harrism · 2023-04-19T23:53:13Z

python/cuspatial/cuspatial/tests/binpreds/binpred_test_dispatch.py

+    (Polygon, Polygon): object_dispatch(polygon_polygon_dispatch_list),
+}
+
+


Can't you rely on the machinery of @pytest.mark.parameterize() to build up the Cartesian product of test combinations rather than using nested loops?

types = [Point, Linestring, Polygon] predicates = [disjoint, intersects, overlaps, contains, constains_properly, within, etc.] @pytest.mark.parametrize('LHS type', types) @pytest.mark.parametrize('RHS type', types) @pytest.mark.parametrize('predicate', predicates) @pytest.mark.parametrize('the data', the_data) def test(...

I started out just parameterizing the cases in the fashion that you describe. Because each of the feature combinations is predefined, I needed to have a separate test with separate parameterizations for exclusive set of features (point, linestring), (point, polygon), etc. Because I wanted logging and reporting on the test results, it was much easier to move that into a single test, and a single list of features that are used in the test. The primary contribution of this PR is the hand-coded list in features.

I remember the specific reason that I parameterize the tests according to the pattern in https://github.com/rapidsai/cuspatial/pull/1085/files#diff-7064c17cb647ac9ab6d1e6f5c4c1726f68fd5b20205dd85f1d34684d53fd3ea6R20-R35 instead of using the pattern that you described. The simple answer is because you cannot parameterize a pytest fixture, only native iterables. I am using the @pytest.mark.parameterize functionality, I've just moved the generation of the tests into the fixture instead of passing a list to the @pytest.mark.parameterize call.

python/cuspatial/cuspatial/tests/binpreds/test_binpred_test_dispatch.py

harrism · 2023-04-20T00:03:54Z

Closes #1036

That issue is about documenting all special cases. How does this PR close that?

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

thomcom · 2023-04-20T15:11:56Z

#1036 is "Document all simple and single geometric special cases #1036" which specifies two follow up issues, one for multiple geometries and polygons with rings, and another for true "special cases" as we discover them. This PR closes #1036 by including 34 specific tests that cover all geometric configurations of the feature combinations for (point, linestring, polygon), which is presented and executed from the features fixture in binpred_test_dispatch.py.

harrism · 2023-04-26T00:13:39Z

So you are replacing "document" with "test"?

isVoid

Really cool!

python/cuspatial/cuspatial/tests/binpreds/binpred_test_dispatch.py

isVoid · 2023-04-26T19:06:42Z

python/cuspatial/cuspatial/tests/binpreds/test_binpred_test_dispatch.py

+out_file = open("test_binpred_test_dispatch.log", "w")
+
+
+@xfail_on_exception  # TODO: Remove when all tests are passing


All tests are passing now ;)

They are but not in this PR, the full submission comes in 3 PRs: #1085, #1086, and #1064

isVoid · 2023-04-26T19:10:27Z

python/cuspatial/cuspatial/tests/binpreds/test_binpred_test_dispatch.py

+        expected = gpd_pred_fn(gpdrhs)
+        assert (got.values_host == expected.values).all()
+
+        # The test is complete, the rest is just logging.


I appreciate the logging to file part, which can prove to be useful when bulk debugging a new feature of hundreds of failures. But when this system is online in CI, it would also be helpful to have a "CI mode" that throws the result to stdout. What do you think?

Because there will be more binary predicate PRs to cover complex (multi-) geometry types, how would you feel about filing an issue for this feature? I'm certainly fine with implementing it but I'd rather not make it a dependency of #1064 that holds it up and needs to be merged.

That's totally fine with me

thomcom · 2023-04-26T19:42:44Z

python/cuspatial/cuspatial/tests/binpreds/binpred_test_dispatch.py

+    """,
+        point_polygon,
+        point_polygon,
+    ),


@harrism the "Documentation" is primarily here. I can move it to the binary predicates design document if you like, as well.

isVoid

Small suggestion. Not blocking to merge.

python/cuspatial/cuspatial/tests/binpreds/binpred_test_dispatch.py

thomcom · 2023-04-26T21:15:40Z

/merge

thomcom added 3 commits April 19, 2023 18:24

First commit of test dispatch branch.

86ad906

Enable xfail.

1a0072c

Binpred test dispatch documented and ready for review.

b2ea930

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 requested a review from a team as a code owner April 19, 2023 19:01

thomcom self-assigned this Apr 19, 2023

thomcom requested review from harrism and isVoid April 19, 2023 19:01

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

Fix bug in test list.

dfcd583

harrism reviewed Apr 20, 2023

View reviewed changes

Update python/cuspatial/cuspatial/tests/binpreds/test_binpred_test_di…

1aefaa2

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

Merge branch 'branch-23.06' into feature/test-dispatch

b636a71

harrism approved these changes Apr 26, 2023

View reviewed changes

Merge branch 'branch-23.06' into feature/test-dispatch

58982db

thomcom mentioned this pull request Apr 26, 2023

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

Merged

3 tasks

isVoid reviewed Apr 26, 2023

View reviewed changes

Add description to why every test returns a GeoSeries of length 3.

c9e3bf3

isVoid mentioned this pull request Apr 26, 2023

Allow "CI mode" for binary predicate dispatch system #1098

Open

thomcom commented Apr 26, 2023

View reviewed changes

isVoid approved these changes Apr 26, 2023

View reviewed changes

python/cuspatial/cuspatial/tests/binpreds/binpred_test_dispatch.py Show resolved Hide resolved

Merge branch 'branch-23.06' into feature/test-dispatch

2922d2c

rapids-bot bot merged commit 959cfdb into rapidsai:branch-23.06 Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary Predicate Test Dispatching #1085

Binary Predicate Test Dispatching #1085

thomcom commented Apr 19, 2023 •

edited

Loading

harrism left a comment

harrism Apr 19, 2023

thomcom Apr 20, 2023

thomcom Apr 20, 2023

harrism commented Apr 20, 2023

thomcom commented Apr 20, 2023

harrism commented Apr 26, 2023

isVoid left a comment

isVoid Apr 26, 2023

thomcom Apr 26, 2023

isVoid Apr 26, 2023

thomcom Apr 26, 2023

isVoid Apr 26, 2023

thomcom Apr 26, 2023

harrism Apr 26, 2023

isVoid left a comment

thomcom commented Apr 26, 2023

		(Polygon, Polygon): object_dispatch(polygon_polygon_dispatch_list),
		}

		out_file = open("test_binpred_test_dispatch.log", "w")


		@xfail_on_exception # TODO: Remove when all tests are passing

Binary Predicate Test Dispatching #1085

Binary Predicate Test Dispatching #1085

Conversation

thomcom commented Apr 19, 2023 • edited Loading

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

harrism commented Apr 20, 2023

thomcom commented Apr 20, 2023

harrism commented Apr 26, 2023

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

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

thomcom commented Apr 26, 2023

thomcom commented Apr 19, 2023 •

edited

Loading