Skip to content

Convert the orient predicate to AdaptivePredicates + minor predicate changes #275

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

Merged
merged 3 commits into from
Apr 17, 2025

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Mar 4, 2025

Spun out from #259
Replaces #272 (which was a bad branch name)

Parent: #271 (TG geometry and algorithm implementations for GEOS and PROJ)
Children: #273 (clipping changes and passthrough) -> #274 (trees)

  • Add AdaptivePredicates as the exact backend for orient. At some point we should switch to an ecosystem wide (DelaunayTriangulation + GeometryOps + ...) representation for predicate kernels (Exact, Adaptive, Auto (choose adaptive if available and exact if not), and Fast).
  • Fix touches for multi geometries

https://github.com/JuliaGeometry/AdaptivePredicates.jl is a library that implements adaptive predicates - substantially faster, BUT harder to construct, than exact predicates. Adaptive predicates are valid through the range of coordinates that geometries usually have, but we may need exact predicates if they are not.

@asinghvi17 asinghvi17 changed the title Convert the orient predicate to AdaptivePredicates Convert the orient predicate to AdaptivePredicates + minor predicate changes Mar 4, 2025
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch 2 times, most recently from b52ef6c to f2d90a8 Compare March 4, 2025 15:38
Copy link
Member Author

asinghvi17 commented Mar 4, 2025

@rafaqz
Copy link
Member

rafaqz commented Mar 7, 2025

Graphite looks very good, I should start using too. And this looks good to me, accurate is good, we can make it faster later.

@asinghvi17
Copy link
Member Author

AdaptivePredicates is at least 10x if not 40x faster than exact predicates :D

@rafaqz
Copy link
Member

rafaqz commented Mar 8, 2025

Yeah, I'm referring to the missing fast returns

@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from f2d90a8 to 974a162 Compare March 9, 2025 02:45
@asinghvi17 asinghvi17 mentioned this pull request Mar 9, 2025
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from 974a162 to 5b0c4a1 Compare March 11, 2025 03:24
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from 5b0c4a1 to c36826d Compare April 3, 2025 20:09
asinghvi17 added a commit that referenced this pull request Apr 3, 2025
This was factored out of the "dev branch" #259 and contains the subset of changes that apply to GeometryOpsCore, for easier review.

Child PRs: #271 (TGGeometry) -> #275 (AdaptivePredicates) -> #273 (clipping algorithm type) -> #274 (trees)

- Use [StableTasks.jl](https://github.com/JuliaFolds2/StableTasks.jl) in apply and applyreduce - its type-stable tasks save us some allocations!
- Remove `Base.@assume_effects` on the low level functions, which caused issues on Julia v1.11 and was probably incorrect anyway
- Add an algorithm interface with an abstract supertype `Algorithm{M <: Manifold}`, as discussed in #247.  Also adds an abstract Operator supertype and some discussion in code comments, but no implementation or interface surface there yet.
- Split out `types.jl` into a directory `types` with a bunch of files in it, for ease of readability / docs / use.
- (out of context change): refactor CI a bit for cleanliness.


TODOs for later (not this PR):
- [ ] Add a `format` method that takes in an incompletely specified algorithm and some geometry as input, and returns a completely specified algorithm.  What does this mean?  Imagine I call `GO.intersection(FosterHormannClipping(), geom1, geom2)`.  That `FosterHormannClipping()` should get expanded to `FosterHormannClipping(AutoAlgorithm(), AutoAccelerator())`.  Then, `format` will take `format(alg, args...)` and:
  - get the `crstrait` of the two geometries, scan for incompatibilities, assign the correct manifold to the algorithm (maybe warn or emit debug info)
  - if no geometries available, get the manifold via `best_manifold(::Algorithm)`.
  - maybe inflate the accelerator by checking `npoint` and later preparations to see what's most efficient, maybe not - depends on what we want!
@asinghvi17 asinghvi17 force-pushed the as/tg branch 2 times, most recently from bb1ded5 to 80ef751 Compare April 3, 2025 20:16
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch 2 times, most recently from 62187d6 to e29badb Compare April 4, 2025 02:24
@asinghvi17 asinghvi17 changed the base branch from as/tg to graphite-base/275 April 14, 2025 22:26
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from e29badb to 23080c1 Compare April 16, 2025 20:38
@asinghvi17 asinghvi17 changed the base branch from graphite-base/275 to as/tg April 16, 2025 20:38
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch 2 times, most recently from 9e50abe to 7dac5d7 Compare April 16, 2025 21:32
@asinghvi17 asinghvi17 changed the base branch from as/tg to graphite-base/275 April 16, 2025 22:10
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from 7dac5d7 to 87920f7 Compare April 16, 2025 22:11
@graphite-app graphite-app bot changed the base branch from graphite-base/275 to main April 16, 2025 22:11
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from 87920f7 to 7b5c4ef Compare April 16, 2025 22:11
https://github.com/JuliaGeometry/AdaptivePredicates.jl is a library that implements adaptive predicates - substantially faster, BUT harder to construct, than exact predicates.  Adaptive predicates are valid through the range of coordinates that geometries usually have, but we may need exact predicates if they are not.
This should increase speed although we will have to benchmark.  Still, at least it's benchmarkable now.
Only one of the geoms in g2 MUST touch the geom in g1, all others MUST be disjoint or touching.

This could potentially be accelerated by building an STRtree if the query width is long enough, or we may also need some tree interface / preparation here.  E.g. Canada has a lot of islands that a tree approach could get rid of.
@asinghvi17 asinghvi17 force-pushed the as/adaptivepredicates branch from 7b5c4ef to 97015f9 Compare April 16, 2025 22:38
@asinghvi17 asinghvi17 requested a review from rafaqz April 16, 2025 22:39
Copy link
Member

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

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

LGTM

@asinghvi17 asinghvi17 merged commit df222e8 into main Apr 17, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants