Skip to content
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

Use quadtree for .contains_properly #910

Conversation

thomcom
Copy link
Contributor

@thomcom thomcom commented Feb 8, 2023

Description

Closes #889
Closes #896

This PR makes a number of changes to the binpred system, including adding boundary exclusion for .contains_properly to the quadtree line-crossings algorithm.

I've closed and then reopened this PR due to benchmark results that showed that using pairwise and column-limited forms of point-in-polygon don't offer any performance improvement but would just require more complicated code paths.

Checklist

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

@thomcom thomcom added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 8, 2023
@thomcom thomcom added this to the DE-9IM milestone Feb 8, 2023
@thomcom thomcom requested review from a team as code owners February 8, 2023 18:13
@thomcom thomcom self-assigned this Feb 8, 2023
@thomcom thomcom requested review from harrism and jrhemstad February 8, 2023 18:13
@github-actions github-actions bot added libcuspatial Relates to the cuSpatial C++ library Python Related to Python code labels Feb 8, 2023
@thomcom thomcom requested a review from isVoid February 10, 2023 19:41
@thomcom thomcom requested review from trxcllnt and removed request for jrhemstad February 10, 2023 20:11
cpp/src/utility/point_in_polygon.cuh Outdated Show resolved Hide resolved
cpp/src/utility/point_in_polygon.cuh Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/contains.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/geoseries.py Outdated Show resolved Hide resolved
cpp/src/join/quadtree_point_in_polygon.cu Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/contains.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/geoseries.py Outdated Show resolved Hide resolved
thomcom and others added 2 commits February 22, 2023 10:05
Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/utils/join_utils.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
cpp/src/utility/point_in_polygon.cuh Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpreds.py Outdated Show resolved Hide resolved
Comment on lines +246 to +248
# If there are 31 or fewer polygons in the input, the result
# is a dataframe with one row per point and one column per
# polygon.
Copy link
Member

@harrism harrism Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option would just be to change this output format before you consume it here. For the small-polygon-count version, just convert the bitmask into the same format output as the quadtree (all pairs): one row per intersection, containing a polygon index and a point index.

Then contains_properly only needs to convert the results if the user requests pairwise instead of all-pairs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it is really easy to change the output format with a cudf.stack() call, so I'm investigating this approach to reduce code-paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've finished that refactor, it makes solving the discrete math problem at the end harder but I'm ok with it for the current iteration of the architecture.

@thomcom
Copy link
Contributor Author

thomcom commented Mar 16, 2023

Please approve, this is ready for merge. I realized that the fundamental reason I've been grinding my teeth on this at all is because I keep trying to add small exceptions or refactorings to an architecture that isn't designed for it yet. I need to merge geom_equals and contains_properly before I sit down for a few days and redesign the architecture based on the lessons that have been learned building the first two basic predicates.

@thomcom
Copy link
Contributor Author

thomcom commented Mar 18, 2023

/merge

@rapids-bot rapids-bot bot merged commit f700030 into rapidsai:branch-23.04 Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code
Projects
Status: Done
4 participants