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.

Sign up for GitHub

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

Jump to bottom

Updated binpred architecture #1009

Merged
merged 36 commits into from
Apr 4, 2023
Merged

Updated binpred architecture #1009

merged 36 commits into from
Apr 4, 2023

Conversation

thomcom
Copy link
Contributor

@thomcom thomcom commented Mar 23, 2023 •
edited

Description

The last couple of PRs were very brittle w.r.t updating their functionality. This PR eliminates some of that brittleness by adding object inheritance for every combination of features for every binary predicate. Class choice is handled by type dispatching on the input columns.

The operation stack (_preprocess, _op, and _postprocess) is now chained to make debugging easier. Instead of calling the operations in sequence, they call one another.

Refactoring the existing modules to use the new architecture was trivial. I'm still interested in modifying the architecture so that the set operations that determine the final result are identifiable, and possibly composable.

Checklist

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

@github-actions github-actions bot added the Python Related to Python code label Mar 23, 2023
@thomcom thomcom mentioned this pull request Mar 27, 2023
Closed
@thomcom thomcom added the non-breaking Non-breaking change label Mar 27, 2023
@thomcom thomcom marked this pull request as ready for review March 27, 2023 22:05
@thomcom thomcom requested a review from a team as a code owner March 27, 2023 22:05
@thomcom thomcom requested a review from trxcllnt March 27, 2023 22:05
@github-actions github-actions bot added the Python Related to Python code label Mar 27, 2023
@thomcom thomcom requested a review from harrism March 27, 2023 22:05
@thomcom
Copy link
Contributor Author

thomcom commented Mar 27, 2023

I'd love to get this approved ASAP as my intersection work will all depend on it.

@thomcom thomcom requested review from isVoid and removed request for trxcllnt March 27, 2023 23:57
@thomcom thomcom mentioned this pull request Mar 28, 2023
Merged
3 tasks
isVoid
isVoid reviewed Mar 29, 2023
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Cool, pretty nice cleanup from current code.

python/cuspatial/cuspatial/core/binpreds/binpred_dispatch.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/_column/geocolumn.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/utils/binpred_utils.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/feature_crosses.py Outdated Show resolved Hide resolved
harrism
harrism reviewed Mar 30, 2023
View reviewed changes
Copy link
Member

@harrism harrism left a comment •
edited

Choose a reason for hiding this comment

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

A couple of design / naming considerations only, for now. Overall I like the direction, just think one layer could be collapsed.

python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/binpred_interface.py Outdated Show resolved Hide resolved
python/cuspatial/cuspatial/core/binpreds/feature_contains.py Outdated Show resolved Hide resolved
harrism
harrism approved these changes Apr 3, 2023
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Using double dispatch is a big improvement. Thanks for simplifying the class hierarchy.

python/cuspatial/cuspatial/core/binpreds/feature_equals.py Outdated Show resolved Hide resolved
@thomcom
Copy link
Contributor Author

thomcom commented Apr 4, 2023

/merge

@rapids-bot rapids-bot bot merged commit 7668fdc into rapidsai:branch-23.04 Apr 4, 2023
@thomcom thomcom mentioned this pull request Apr 4, 2023
Closed
rapids-bot bot pushed a commit that referenced this pull request Apr 6, 2023
@thomcom
Intersection only predicates (#1016)
405699f 
Closes #1015
Depends on #1009 

This PR implements `intersects` and all of the feature combinations that depend exclusively on intersects, as listed in #1015.

Authors:
  - H. Thomson Comer (https://github.com/thomcom)

Approvers:
  - Michael Wang (https://github.com/isVoid)

URL: #1016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trxcllnt trxcllnt trxcllnt left review comments

@harrism harrism harrism approved these changes

@isVoid isVoid isVoid approved these changes

Assignees

@thomcom thomcom

Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to Python code
Projects
cuSpatial
Status: Done
Milestone
DE-9IM
Development

Successfully merging this pull request may close these issues.

[FEA]: Refactor binpred architecture for better extensibility
4 participants
@thomcom @trxcllnt @harrism @isVoid